Fix me review check (new check) (#547)

* began fixMeReviewCheck * almost ready for qa and started documentation * tests complete checking coverage * fixes * fix files I shouldnt touch * update values and format. * updated config, no need for unknown characters * update documentation * format. * addressing comments
osmlab · Apr 21, 2021 · 2748de1 · 2748de1
1 parent dbcc2c7
commit 2748de1
Show file tree

Hide file tree

Showing 6 changed files with 342 additions and 0 deletions.
diff --git a/config/configuration.json b/config/configuration.json
@@ -286,6 +286,16 @@
       "difficulty": "EASY"
     }
   },
+  "FixMeReviewCheck": {
+    "fixMe.supported.values": ["verify","position","resurvey","Revisar: este punto fue creado por importación directa","continue", "name", "incomplete", "draw geometry and delete this point", "unfinished", "recheck"],
+    "min.highway.type": "tertiary",
+    "challenge": {
+      "description": "Tasks contain features with fixme/FIXME (most common fix me tags) that need to be addressed",
+      "blurb": "Features that need to be fixed.",
+      "instruction": "Open your favorite editor and assess the features that need to be fixed.",
+      "difficulty": "EASY"
+    }
+  },
   "FloatingEdgeCheck": {
     "highway.minimum": "service",
     "length": {

diff --git a/docs/available_checks.md b/docs/available_checks.md
@@ -80,6 +80,7 @@ This document is a list of tables with a description and link to documentation f
 | [ConflictingAreaTagCombination](checks/conflictingAreaTagCombination.md) | The purpose of this check is to identify Areas with invalid tag combinations. |
 | ConflictingTagCombinationCheck | This check verifies whether an atlas object has a conflicting tag combination or not. |
 | [ConstructionCheck](checks/constructionCheck.md) | The purpose of this check is to identify construction tags where the construction hasn't been checked on recently, or the expected finish date has been passed. |
+| [FixMeReviewCheck](checks/fixMeReviewCheck.md) | The purpose of this check is to flag features that contain the "fixme"/"FIXME" tags with along with a variety of other important tags. |
 | [HighwayMissingNameAndRefTagCheck](checks/highwayMissingNameAndRefTagCheck.md) | This check detects highways that are missing a name and ref tag. At least one of them is required. |
 | [HighwayToFerryTagCheck](checks/highwayToFerryTagCheck.md) | The purpose of this check is to identify all Edges with route=FERRY and highway=PATH (or higher). |
 | ImproperAndUnknownRoadNameCheck | This check flags improper road name values. |

diff --git a/docs/checks/fixMeReviewCheck.md b/docs/checks/fixMeReviewCheck.md
@@ -0,0 +1,33 @@
+# FixMeReviewCheck
+
+#### Description
+The purpose of this check is to flag features that contain the "fixme"/"FIXME" tags with along with a variety of other important tags. Other important tags that need to be used in unison with the fixme tag
+are: [WaterwayTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/WaterwayTag.java), [OneWayTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/oneway/OneWayTag.java), 
+[BuildingTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/BuildingTag.java), [HighwayTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/HighwayTag.java), 
+[NameTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/names/NameTag.java), [ReferenceTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/names/ReferenceTag.java),
+[PlaceTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/PlaceTag.java), and [SurfaceTag](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/tags/SurfaceTag.java). 
+
+The current supported values for the fixme tag are: "verify", "position", "resurvey", 
+"Revisar: este punto fue creado por importación directa", "continue", "name", 
+"incomplete", "draw geometry and delete this point", "unfinished", and "recheck".
+
+The flagged features' fixme tag must contain one of the values listed in the supported values list and the feature must contain at least one of the important tags.
+
+
+#### Configurables
+* ***minHighwayTag:*** Minimum highway tag value for object.
+* ***fixMeSupportedValues:*** Supported fixme tag values
+
+#### Live Examples
+
+1. [Way:177401829](https://www.openstreetmap.org/way/177401829) has "fixme" tag with priority value (name) along with important supplementary tag(s).
+2. [Way:328136807](https://www.openstreetmap.org/way/328136807) has "fixme" tag with priority value (continue) along with important supplementary tag(s).
+
+#### Code Review
+This check evaluates [Nodes](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Node.java),
+[Edges](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Edge.java),
+[Relations](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Relation.java),
+[Points](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Point.java),
+[Areas](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Area.java), and
+[Lines](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Line.java) 
+determining if they contain the fixme tag and priority fixme tag values along with other important tags.
diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheck.java
@@ -0,0 +1,160 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import org.openstreetmap.atlas.checks.base.BaseCheck;
+import org.openstreetmap.atlas.checks.flag.CheckFlag;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
+import org.openstreetmap.atlas.geography.atlas.items.Edge;
+import org.openstreetmap.atlas.geography.atlas.walker.OsmWayWalker;
+import org.openstreetmap.atlas.tags.BuildingTag;
+import org.openstreetmap.atlas.tags.HighwayTag;
+import org.openstreetmap.atlas.tags.PlaceTag;
+import org.openstreetmap.atlas.tags.SurfaceTag;
+import org.openstreetmap.atlas.tags.WaterwayTag;
+import org.openstreetmap.atlas.tags.names.NameTag;
+import org.openstreetmap.atlas.tags.names.ReferenceTag;
+import org.openstreetmap.atlas.tags.oneway.OneWayTag;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+
+/**
+ * This check flags features which contain the "fixme" or "FIXME" tag as well as other combinations
+ * of important tags.
+ *
+ * @author v-garei
+ */
+public class FixMeReviewCheck extends BaseCheck<Long>
+{
+    private static final long serialVersionUID = -2191776278923026805L;
+    private static final String HAS_FIXME_TAG = "Object {0, number, #} has 'fixme' tag and needs to be investigated.";
+    private static final List<String> FALLBACK_INSTRUCTIONS = Collections
+            .singletonList(HAS_FIXME_TAG);
+    private static final List<String> FIX_ME_SUPPLEMENTARY_TAGS = List.of(WaterwayTag.KEY,
+            OneWayTag.KEY, BuildingTag.KEY, HighwayTag.KEY, NameTag.KEY, ReferenceTag.KEY,
+            PlaceTag.KEY, SurfaceTag.KEY);
+    private static final List<String> FIX_ME_SUPPORTED_VALUES_DEFAULT = List.of("verify",
+            "position", "resurvey", "Revisar: este punto fue creado por importación directa",
+            "continue", "name", "incomplete", "draw geometry and delete this point", "unfinished",
+            "recheck");
+    private final List<String> fixMeSupportedValues;
+    private final HighwayTag minHighwayTag;
+    private static final String MIN_HIGHWAY_TAG_DEFAULT = "tertiary";
+    private static final String FIX_ME_LOWERCASE = "fixme";
+    private static final String FIX_ME_UPPERCASE = "FIXME";
+
+    /**
+     * instantiate config values
+     * 
+     * @param configuration
+     *            the JSON configuration for this check
+     */
+    public FixMeReviewCheck(final Configuration configuration)
+    {
+
+        super(configuration);
+        this.fixMeSupportedValues = this.configurationValue(configuration, "fixMe.supported.values",
+                FIX_ME_SUPPORTED_VALUES_DEFAULT);
+        this.minHighwayTag = Enum.valueOf(HighwayTag.class,
+                this.configurationValue(configuration, "min.highway.type", MIN_HIGHWAY_TAG_DEFAULT)
+                        .toUpperCase());
+    }
+
+    /**
+     * This function will validate if the supplied atlas object is valid for the check.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return {@code true} if this object should be checked
+     */
+    @Override
+    public boolean validCheckForObject(final AtlasObject object)
+    {
+        final Map<String, String> tags = object.getTags();
+        return !isFlagged(object.getOsmIdentifier())
+                && (tags.containsKey(FIX_ME_UPPERCASE) || tags.containsKey(FIX_ME_LOWERCASE));
+    }
+
+    /**
+     * This is the function that will check to see whether the object needs to be flagged.
+     *
+     * @param object
+     *            the atlas object supplied by the Atlas-Checks framework for evaluation
+     * @return an optional {@link CheckFlag} object that
+     */
+    @Override
+    protected Optional<CheckFlag> flag(final AtlasObject object)
+    {
+        markAsFlagged(object.getOsmIdentifier());
+        final Map<String, String> tags = object.getTags();
+
+        if (this.featureHasSupplementaryTags(tags) && this.featureHasPriorityFixMeValues(tags))
+        {
+            // If the object is an edge we only care about tertiary (configurable) and above.
+            if (object instanceof Edge)
+            {
+                if (((Edge) object).highwayTag().isMoreImportantThanOrEqualTo(this.minHighwayTag))
+                {
+                    return Optional
+                            .of(this.createFlag(new OsmWayWalker((Edge) object).collectEdges(),
+                                    this.getLocalizedInstruction(0, object.getOsmIdentifier())));
+                }
+                return Optional.empty();
+            }
+            return Optional.of(this.createFlag(object,
+                    this.getLocalizedInstruction(0, object.getOsmIdentifier())));
+
+        }
+        return Optional.empty();
+    }
+
+    @Override
+    protected List<String> getFallbackInstructions()
+    {
+        return FALLBACK_INSTRUCTIONS;
+    }
+
+    /**
+     * Function to determining if object's fixme tag contains priority values (configurable values)
+     *
+     * @param tags
+     *            object osm tags
+     * @return boolean if the object's fixme tag contains priority values
+     */
+    private boolean featureHasPriorityFixMeValues(final Map<String, String> tags)
+    {
+        for (final String priorityTagValue : this.fixMeSupportedValues)
+        {
+            if ((tags.containsKey(FIX_ME_UPPERCASE)
+                    && tags.get(FIX_ME_UPPERCASE).equals(priorityTagValue))
+                    || (tags.containsKey(FIX_ME_LOWERCASE)
+                            && tags.get(FIX_ME_LOWERCASE).equals(priorityTagValue)))
+            {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Function to determine if object tags contain flaggable priority tags (configurable)
+     *
+     * @param tags
+     *            object osm tags
+     * @return boolean if the object has flaggable priority tags.
+     */
+    private boolean featureHasSupplementaryTags(final Map<String, String> tags)
+    {
+        for (final String supplementaryTag : FIX_ME_SUPPLEMENTARY_TAGS)
+        {
+            if (tags.containsKey(supplementaryTag))
+            {
+                return true;
+            }
+        }
+        return false;
+    }
+
+}
diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheckTest.java
@@ -0,0 +1,53 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
+import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
+
+/**
+ * Tests for {@link FixMeReviewCheck}
+ *
+ * @author v-garei
+ */
+public class FixMeReviewCheckTest
+{
+
+    @Rule
+    public FixMeReviewCheckTestRule setup = new FixMeReviewCheckTestRule();
+
+    @Rule
+    public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();
+
+    private final FixMeReviewCheck check = new FixMeReviewCheck(
+            ConfigurationResolver.inlineConfiguration(
+                    "{\"FixMeReviewCheck\": {\"fixMe.supported.values\": [\"verify\", \"position\", \"resurvey\", \"continue\", \"name\", \"incomplete\", \"draw geometry and delete this point\", \"unfinished\", \"recheck\"], \"min.highway.type\": \"tertiary\"}}"));
+
+    @Test
+    public void nodeWithInvalidFixMe()
+    {
+        this.verifier.actual(this.setup.nodeWithInvalidFixMe(), this.check);
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void nodeWithValidFixMe()
+    {
+        this.verifier.actual(this.setup.nodeWithValidFixMe(), this.check);
+        this.verifier.verifyExpectedSize(1);
+    }
+
+    @Test
+    public void wayWithInvalidFixMe()
+    {
+        this.verifier.actual(this.setup.wayWithInvalidFixMe(), this.check);
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void wayWithValidFixMe()
+    {
+        this.verifier.actual(this.setup.wayWithValidFixMe(), this.check);
+        this.verifier.verifyExpectedSize(1);
+    }
+}
diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/tag/FixMeReviewCheckTestRule.java
@@ -0,0 +1,85 @@
+package org.openstreetmap.atlas.checks.validation.tag;
+
+import org.openstreetmap.atlas.geography.atlas.Atlas;
+import org.openstreetmap.atlas.utilities.testing.CoreTestRule;
+import org.openstreetmap.atlas.utilities.testing.TestAtlas;
+
+/**
+ * Tests for {@link FixMeReviewCheck}
+ *
+ * @author v-garei
+ */
+public class FixMeReviewCheckTestRule extends CoreTestRule
+{
+
+    private static final String node1 = "40.9108780, 29.4695635";
+
+    private static final String WAY1_NODE1 = "40.9130354, 29.4700719";
+    private static final String WAY1_NODE2 = "40.9123887, 29.4698597";
+    private static final String WAY2_NODE2 = "40.9118904, 29.4696993";
+    private static final String WAY3_NODE2 = "40.9082867, 29.4685152";
+
+    @TestAtlas(nodes = { @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = node1), tags = {
+            "fixme=not valid" }) })
+
+    private Atlas nodeWithInvalidFixMe;
+
+    @TestAtlas(nodes = { @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = node1), tags = {
+            "fixme=continue", "place=bees" }) })
+
+    private Atlas nodeWithValidFixMe;
+
+    @TestAtlas(nodes = { @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY1_NODE1)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY1_NODE2)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY2_NODE2)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY3_NODE2)) }, edges = {
+                    @TestAtlas.Edge(id = "3000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY1_NODE1),
+                            @TestAtlas.Loc(value = WAY1_NODE2) }, tags = { "FIXME=not valid",
+                                    "toll=yes" }),
+                    @TestAtlas.Edge(id = "4000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY1_NODE2),
+                            @TestAtlas.Loc(value = WAY2_NODE2) }, tags = "highway=motorway"),
+                    @TestAtlas.Edge(id = "5000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY2_NODE2),
+                            @TestAtlas.Loc(value = WAY3_NODE2) }, tags = { "highway=motorway",
+                                    "toll=yes" }) })
+    private Atlas wayWithInvalidFixMe;
+
+    @TestAtlas(nodes = { @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY1_NODE1)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY1_NODE2)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY2_NODE2)),
+            @TestAtlas.Node(coordinates = @TestAtlas.Loc(value = WAY3_NODE2)) }, edges = {
+                    @TestAtlas.Edge(id = "3000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY1_NODE1),
+                            @TestAtlas.Loc(value = WAY1_NODE2) }, tags = { "FIXME=name",
+                                    "highway=motorway", "toll=yes" }),
+                    @TestAtlas.Edge(id = "4000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY1_NODE2),
+                            @TestAtlas.Loc(value = WAY2_NODE2) }, tags = "highway=motorway"),
+                    @TestAtlas.Edge(id = "5000001", coordinates = {
+                            @TestAtlas.Loc(value = WAY2_NODE2),
+                            @TestAtlas.Loc(value = WAY3_NODE2) }, tags = { "highway=motorway",
+                                    "toll=yes" }) })
+    private Atlas wayWithValidFixMe;
+
+    public Atlas nodeWithInvalidFixMe()
+    {
+        return this.nodeWithInvalidFixMe;
+    }
+
+    public Atlas nodeWithValidFixMe()
+    {
+        return this.nodeWithValidFixMe;
+    }
+
+    public Atlas wayWithInvalidFixMe()
+    {
+        return this.wayWithInvalidFixMe;
+    }
+
+    public Atlas wayWithValidFixMe()
+    {
+        return this.wayWithValidFixMe;
+    }
+}