CheckShopType: Don't ask if there is a (mistagged) new shop already, …

…do ask also or other shop-like amenities (#3158)
streetcomplete · Aug 16, 2021 · a072702 · a072702
1 parent bfed57a
commit a072702
Show file tree

Hide file tree

Showing 4 changed files with 133 additions and 55 deletions.
diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/meta/OsmTaggings.kt b/app/src/main/java/de/westnordost/streetcomplete/data/meta/OsmTaggings.kt
@@ -57,3 +57,41 @@ val KEYS_THAT_SHOULD_NOT_BE_REMOVED_WHEN_SHOP_IS_REPLACED = listOf(
         "building", "heritage", "height", "architect", "ele"
     ).joinToString("|")})(:.*)?",
 ).map { it.toRegex() }
+
+/** ~ tenant of a normal retail shop area.
+ *  So,
+ *  - no larger or purpose-built things like malls, cinemas, theatres, car washes, fuel stations,
+ *    museums, galleries, zoos, aquariums, bowling alleys...
+ *  - no things that are usually not found in normal retail shop areas but in offices:
+ *    clinics, doctors, fitness centers, dental technicians...
+ *  - nothing that is rather located in an industrial estate like car repair and other types
+ *    of workshops (most craft=* other than those where people go to have something repaired or so)
+ *
+ *  It is possible to specify a prefix for the keys here, f.e. "disused", to find disused shops etc.
+ *  */
+fun isKindOfShopExpression(prefix: String? = null): String {
+    val p = if(prefix != null) "$prefix:" else ""
+    return ("""
+        ${p}shop and ${p}shop !~ no|vacant|mall
+        or ${p}tourism = information and ${p}information = office
+        or """ +
+        mapOf(
+            "amenity" to arrayOf(
+                "restaurant", "cafe", "ice_cream", "fast_food", "bar", "pub", "biergarten", "nightclub",
+                "bank", "bureau_de_change", "money_transfer", "post_office", "internet_cafe",
+                "pharmacy",
+                "driving_school",
+            ),
+            "leisure" to arrayOf(
+                "amusement_arcade", "adult_gaming_centre", "tanning_salon",
+            ),
+            "office" to arrayOf(
+                "insurance", "travel_agent", "tax_advisor", "estate_agent", "political_party",
+            ),
+            "craft" to arrayOf(
+                "shoemaker", "tailor", "photographer", "watchmaker", "optician",
+                "electronics_repair", "key_cutter",
+            )
+        ).map { p + it.key + " ~ " + it.value.joinToString("|") }.joinToString("\n  or ") + "\n"
+        ).trimIndent()
+}
diff --git a/app/src/main/java/de/westnordost/streetcomplete/ktx/Element.kt b/app/src/main/java/de/westnordost/streetcomplete/ktx/Element.kt
@@ -2,6 +2,7 @@ package de.westnordost.streetcomplete.ktx
 
 import de.westnordost.osmfeatures.GeometryType
 import de.westnordost.streetcomplete.data.elementfilter.toElementFilterExpression
+import de.westnordost.streetcomplete.data.meta.isKindOfShopExpression
 import de.westnordost.streetcomplete.data.osm.mapdata.*
 
 fun Element.copy(
@@ -62,36 +63,5 @@ private val IS_AREA_EXPR = """
 
 fun Element.isSomeKindOfShop(): Boolean = IS_SOME_KIND_OF_SHOP_EXPR.matches(this)
 
-/** ~ tenant of a normal retail shop area.
- *  So,
- *  - no larger or purpose-built things like malls, cinemas, theatres, car washes, fuel stations,
- *    museums, galleries, zoos, aquariums, bowling alleys...
- *  - no things that are usually not found in normal retail shop areas but in offices:
- *    clinics, doctors, fitness centers, dental technicians...
- *  - nothing that is rather located in an industrial estate like car repair and other types
- *    of workshops (most craft=* other than those where people go to have something repaired or so)
- *  */
-private val IS_SOME_KIND_OF_SHOP_EXPR = ("""
-    nodes, ways, relations with
-      shop and shop !~ no|vacant|mall
-      or tourism = information and information = office
-      or """ +
-    mapOf(
-        "amenity" to arrayOf(
-            "restaurant", "cafe", "ice_cream", "fast_food", "bar", "pub", "biergarten", "nightclub",
-            "bank", "bureau_de_change", "money_transfer", "post_office", "internet_cafe",
-            "pharmacy",
-            "driving_school",
-        ),
-        "leisure" to arrayOf(
-            "amusement_arcade", "adult_gaming_centre", "tanning_salon",
-        ),
-        "office" to arrayOf(
-            "insurance", "travel_agent", "tax_advisor", "estate_agent", "political_party",
-        ),
-        "craft" to arrayOf(
-            "shoemaker", "tailor", "photographer", "watchmaker", "optician",
-            "electronics_repair", "key_cutter",
-        )
-    ).map { it.key + " ~ " + it.value.joinToString("|") }.joinToString("\n  or ") + "\n"
-    ).trimIndent().toElementFilterExpression()
+private val IS_SOME_KIND_OF_SHOP_EXPR =
+    ("nodes, ways, relations with " + isKindOfShopExpression()).toElementFilterExpression()
diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/shop_type/CheckShopType.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/shop_type/CheckShopType.kt
@@ -1,31 +1,45 @@
 package de.westnordost.streetcomplete.quests.shop_type
 
 import de.westnordost.streetcomplete.R
-import de.westnordost.streetcomplete.data.meta.KEYS_THAT_SHOULD_NOT_BE_REMOVED_WHEN_SHOP_IS_REPLACED
-import de.westnordost.streetcomplete.data.meta.LAST_CHECK_DATE_KEYS
-import de.westnordost.streetcomplete.data.meta.SURVEY_MARK_KEY
-import de.westnordost.streetcomplete.data.meta.toCheckDateString
+import de.westnordost.streetcomplete.data.elementfilter.toElementFilterExpression
+import de.westnordost.streetcomplete.data.meta.*
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapChangesBuilder
-import de.westnordost.streetcomplete.data.osm.osmquests.OsmFilterQuestType
+import de.westnordost.streetcomplete.data.osm.mapdata.Element
+import de.westnordost.streetcomplete.data.osm.mapdata.MapDataWithGeometry
+import de.westnordost.streetcomplete.data.osm.osmquests.OsmElementQuestType
 import java.time.LocalDate
 
-class CheckShopType : OsmFilterQuestType<ShopTypeAnswer>() {
+class CheckShopType : OsmElementQuestType<ShopTypeAnswer> {
 
-    override val elementFilter = """
+    private val disusedShops by lazy { """
         nodes, ways, relations with (
-            shop = vacant
-            or disused:shop
-          ) and (
-            older today -1 years
-            or ${LAST_CHECK_DATE_KEYS.joinToString(" or ") { "$it < today -1 years" }}
-          )
-    """
+          shop = vacant
+          or ${isKindOfShopExpression("disused")}
+        ) and (
+          older today -1 years
+          or ${LAST_CHECK_DATE_KEYS.joinToString(" or ") { "$it < today -1 years" }}
+        )
+    """.toElementFilterExpression() }
+
+    /* elements tagged like "shop=ice_cream + disused:amenity=bank" should not appear as quests.
+     *  This is arguably a tagging mistake, but that mistake should not lead to all the tags of
+     *  this element being cleared when the quest is answered */
+    private val shops by lazy { """
+        nodes, ways, relations with ${isKindOfShopExpression()}
+    """.toElementFilterExpression() }
+
     override val commitMessage = "Check if vacant shop is still vacant"
-    override val wikiLink = "Tag:shop=vacant"
+    override val wikiLink = "Key:disused:"
     override val icon = R.drawable.ic_quest_check_shop
 
     override fun getTitle(tags: Map<String, String>) = R.string.quest_shop_vacant_type_title
 
+    override fun getApplicableElements(mapData: MapDataWithGeometry): Iterable<Element> =
+        mapData.filter { isApplicableTo(it) }
+
+    override fun isApplicableTo(element: Element): Boolean =
+        disusedShops.matches(element) && !shops.matches(element)
+
     override fun createForm() = ShopTypeForm()
 
     override fun applyAnswerTo(answer: ShopTypeAnswer, changes: StringMapChangesBuilder) {
@@ -38,14 +52,14 @@ class CheckShopType : OsmFilterQuestType<ShopTypeAnswer>() {
                 changes.addOrModify(SURVEY_MARK_KEY, LocalDate.now().toCheckDateString())
             }
             is ShopType -> {
-                changes.deleteIfExists("disused:shop")
                 if (!answer.tags.containsKey("shop")) {
                     changes.deleteIfExists("shop")
                 }
 
                 changes.deleteIfExists(SURVEY_MARK_KEY)
 
-                for ((key, value) in changes.getPreviousEntries()) {
+                for ((key, _) in changes.getPreviousEntries()) {
+                    // also deletes all "disused:" keys
                     val isOkToRemove =
                         KEYS_THAT_SHOULD_NOT_BE_REMOVED_WHEN_SHOP_IS_REPLACED.none { it.matches(key) }
                     if (isOkToRemove && !answer.tags.containsKey(key)) {
@@ -56,9 +70,8 @@ class CheckShopType : OsmFilterQuestType<ShopTypeAnswer>() {
                 for ((key, value) in answer.tags) {
                     changes.addOrModify(key, value)
                 }
-
-
             }
         }
     }
+
 }
diff --git a/app/src/test/java/de/westnordost/streetcomplete/quests/shop_type/CheckShopTypeTest.kt b/app/src/test/java/de/westnordost/streetcomplete/quests/shop_type/CheckShopTypeTest.kt
@@ -5,12 +5,51 @@ import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapEntryAd
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapEntryDelete
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapEntryModify
 import de.westnordost.streetcomplete.quests.verifyAnswer
+import de.westnordost.streetcomplete.testutils.node
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
 import org.junit.Test
 import java.time.LocalDate
 
 class CheckShopTypeTest {
     private val questType = CheckShopType()
 
+    @Test fun `is applicable to vacant shop`() {
+        assertTrue(questType.isApplicableTo(
+            node(tags = mapOf("shop" to "vacant"), timestamp = 100)
+        ))
+    }
+
+    @Test fun `is applicable to disused shop`() {
+        assertTrue(questType.isApplicableTo(
+            node(tags = mapOf("disused:shop" to "yes"), timestamp = 100)
+        ))
+    }
+
+    @Test fun `is applicable to disused specific amenity`() {
+        assertTrue(questType.isApplicableTo(
+            node(tags = mapOf("disused:amenity" to "bar"), timestamp = 100)
+        ))
+    }
+
+    @Test fun `is not applicable to other disused amenity`() {
+        assertFalse(questType.isApplicableTo(
+            node(tags = mapOf("disused:amenity" to "telephone"), timestamp = 100)
+        ))
+        assertFalse(questType.isApplicableTo(
+            node(tags = mapOf("disused:amenity" to "yes"), timestamp = 100)
+        ))
+    }
+
+    @Test fun `is not applicable to disused shop that is not disused after all, apparently`() {
+        assertFalse(questType.isApplicableTo(
+            node(
+                tags = mapOf("disused:shop" to "yes", "shop" to "something new already"),
+                timestamp = 100
+            )
+        ))
+    }
+
     @Test fun `apply shop vacant answer`() {
         questType.verifyAnswer(
             IsShopVacant,
@@ -70,12 +109,13 @@ class CheckShopTypeTest {
         )
     }
 
-    @Test fun `apply shop with tags answer removes disused-shop`() {
+    @Test fun `apply shop with tags answer removes disused-shop and amenity`() {
         questType.verifyAnswer(
-            mapOf("disused:shop" to "yes"),
+            mapOf("disused:shop" to "yes", "disused:amenity" to "yes"),
             ShopType(mapOf("a" to "b")),
             StringMapEntryAdd("a", "b"),
-            StringMapEntryDelete("disused:shop", "yes")
+            StringMapEntryDelete("disused:shop", "yes"),
+            StringMapEntryDelete("disused:amenity", "yes"),
         )
     }
 
@@ -95,4 +135,21 @@ class CheckShopTypeTest {
             StringMapEntryModify("shop", "vacant", "not vacant")
         )
     }
+
+    // see KEYS_THAT_SHOULD_NOT_BE_REMOVED_WHEN_SHOP_IS_REPLACED
+    @Test fun `apply shop with tags answer clears all tags except for a few ones`() {
+        questType.verifyAnswer(
+            mapOf(
+                "building" to "yes", // <- should not be cleared
+                "disused:amenity" to "yes",
+                "phone" to "123456",
+                "name" to "Juppiebude"
+            ),
+            ShopType(mapOf("shop" to "ice_cream")),
+            StringMapEntryAdd("shop", "ice_cream"),
+            StringMapEntryDelete("disused:amenity", "yes"),
+            StringMapEntryDelete("phone", "123456"),
+            StringMapEntryDelete("name", "Juppiebude"),
+        )
+    }
 }