don't remove check date when a tag it references is changed but updat…

…e it instead (mitigate #2861, #2883)
streetcomplete · May 15, 2021 · 239a09e · 239a09e
1 parent 9a2ed20
commit 239a09e
Show file tree

Hide file tree

Showing 5 changed files with 36 additions and 20 deletions.
diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/meta/ResurveyUtils.kt b/app/src/main/java/de/westnordost/streetcomplete/data/meta/ResurveyUtils.kt
@@ -33,11 +33,15 @@ fun String.toCheckDate(): LocalDate? {
  *  tag to today instead. */
 fun StringMapChangesBuilder.updateWithCheckDate(key: String, value: String) {
     val previousValue = getPreviousValue(key)
-    if (previousValue == value) {
-        updateCheckDateForKey(key)
-    } else {
+    if (previousValue != value) {
         addOrModify(key, value)
-        deleteCheckDatesForKey(key)
+    }
+    /* if the value is changed, set the check date only if it has been set before. Behavior
+    *  before v32.0 was to delete the check date. However, this destroys data that was
+    *  previously collected by another surveyor - we don't want to destroy other people's data
+    *  */
+    if (previousValue == value || hasCheckDateForKey(key)) {
+        updateCheckDateForKey(key)
     }
 }
 
@@ -50,6 +54,10 @@ fun StringMapChangesBuilder.updateCheckDateForKey(key: String) {
     }
 }
 
+/** Return whether a check date is set for the given key */
+fun StringMapChangesBuilder.hasCheckDateForKey(key: String): Boolean =
+    getLastCheckDateKeys(key).any { getPreviousValue(it) != null }
+
 /** Delete any check date keys for the given key */
 fun StringMapChangesBuilder.deleteCheckDatesForKey(key: String) {
     getLastCheckDateKeys(key).forEach { deleteIfExists(it) }

diff --git a/app/src/main/java/de/westnordost/streetcomplete/quests/bikeway/AddCycleway.kt b/app/src/main/java/de/westnordost/streetcomplete/quests/bikeway/AddCycleway.kt
@@ -5,12 +5,9 @@ import de.westnordost.streetcomplete.R
 import de.westnordost.streetcomplete.data.elementfilter.filters.RelativeDate
 import de.westnordost.streetcomplete.data.elementfilter.filters.TagOlderThan
 import de.westnordost.streetcomplete.data.elementfilter.toElementFilterExpression
-import de.westnordost.streetcomplete.data.meta.ANYTHING_UNPAVED
-import de.westnordost.streetcomplete.data.meta.MAXSPEED_TYPE_KEYS
+import de.westnordost.streetcomplete.data.meta.*
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapChangesBuilder
 import de.westnordost.streetcomplete.data.quest.NoCountriesExcept
-import de.westnordost.streetcomplete.data.meta.deleteCheckDatesForKey
-import de.westnordost.streetcomplete.data.meta.updateCheckDateForKey
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapEntryModify
 import de.westnordost.streetcomplete.data.osm.geometry.ElementPolylinesGeometry
 import de.westnordost.streetcomplete.data.osm.mapdata.Element
@@ -148,10 +145,8 @@ class AddCycleway : OsmElementQuestType<CyclewayAnswer> {
         val isNotActuallyChangingAnything = changes.getChanges().all { change ->
             change is StringMapEntryModify && change.value == change.valueBefore
         }
-        if (isNotActuallyChangingAnything) {
+        if (isNotActuallyChangingAnything || changes.hasCheckDateForKey("cycleway")) {
             changes.updateCheckDateForKey("cycleway")
-        } else {
-            changes.deleteCheckDatesForKey("cycleway")
         }
     }
 

diff --git a/...de/westnordost/streetcomplete/quests/recycling_material/AddRecyclingContainerMaterials.kt b/...de/westnordost/streetcomplete/quests/recycling_material/AddRecyclingContainerMaterials.kt
@@ -7,6 +7,7 @@ import de.westnordost.streetcomplete.data.elementfilter.filters.TagOlderThan
 import de.westnordost.streetcomplete.data.elementfilter.toElementFilterExpression
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapChangesBuilder
 import de.westnordost.streetcomplete.data.meta.deleteCheckDatesForKey
+import de.westnordost.streetcomplete.data.meta.hasCheckDateForKey
 import de.westnordost.streetcomplete.data.meta.updateCheckDateForKey
 import de.westnordost.streetcomplete.data.osm.edits.update_tags.StringMapEntryModify
 import de.westnordost.streetcomplete.data.osm.mapdata.Element
@@ -134,10 +135,8 @@ class AddRecyclingContainerMaterials : OsmElementQuestType<RecyclingContainerMat
         val isNotActuallyChangingAnything = changes.getChanges().all { change ->
             change is StringMapEntryModify && change.value == change.valueBefore
         }
-        if (isNotActuallyChangingAnything) {
+        if (isNotActuallyChangingAnything || changes.hasCheckDateForKey("recycling")) {
             changes.updateCheckDateForKey("recycling")
-        } else {
-            changes.deleteCheckDatesForKey("recycling")
         }
     }
 

diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/meta/ResurveyUtilsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/meta/ResurveyUtilsTest.kt
@@ -29,6 +29,20 @@ class ResurveyUtilsTest {
         ), changes)
     }
 
+    @Test fun hasCheckDateForKey() {
+        assertFalse(StringMapChangesBuilder(mapOf("key" to "value")).hasCheckDateForKey("key"))
+
+        assertFalse(StringMapChangesBuilder(mapOf(
+            "key" to "value",
+            "check_date:another_key" to "value"
+        )).hasCheckDateForKey("key"))
+
+        assertTrue(StringMapChangesBuilder(mapOf(
+            "key" to "value",
+            "check_date:another_key" to "value"
+        )).hasCheckDateForKey("another_key"))
+    }
+
     @Test fun `updateWithCheckDate modifies tag`() {
         val builder = StringMapChangesBuilder(mapOf("old key" to "old value"))
         builder.updateWithCheckDate("old key", "new value")
@@ -59,7 +73,7 @@ class ResurveyUtilsTest {
         ), changes)
     }
 
-    @Test fun `updateWithCheckDate removes old check dates on modifying key`() {
+    @Test fun `updateWithCheckDate modifies old check date on modifying key`() {
         val builder = StringMapChangesBuilder(mapOf(
             "key" to "old value",
             "key:check_date" to "2000-11-01",
@@ -74,16 +88,16 @@ class ResurveyUtilsTest {
 
         assertTrue(changes.containsExactlyInAnyOrder(listOf(
             StringMapEntryModify("key", "old value", "new value"),
+            StringMapEntryModify("check_date:key", "2000-11-02", LocalDate.now().toCheckDateString()),
             StringMapEntryDelete("key:check_date", "2000-11-01"),
-            StringMapEntryDelete("check_date:key", "2000-11-02"),
             StringMapEntryDelete("key:lastcheck", "2000-11-03"),
             StringMapEntryDelete("lastcheck:key", "2000-11-04"),
             StringMapEntryDelete("key:last_checked", "2000-11-05"),
             StringMapEntryDelete("last_checked:key", "2000-11-06")
         )))
     }
 
-    @Test fun `updateWithCheckDate removes old check dates on adding key`() {
+    @Test fun `updateWithCheckDate modifies old check dates on adding key`() {
         val builder = StringMapChangesBuilder(mapOf(
             "key:check_date" to "2000-11-01",
             "check_date:key" to "2000-11-02",
@@ -98,7 +112,7 @@ class ResurveyUtilsTest {
         assertTrue(changes.containsExactlyInAnyOrder(listOf(
             StringMapEntryAdd("key", "value"),
             StringMapEntryDelete("key:check_date", "2000-11-01"),
-            StringMapEntryDelete("check_date:key", "2000-11-02"),
+            StringMapEntryModify("check_date:key", "2000-11-02", LocalDate.now().toCheckDateString()),
             StringMapEntryDelete("key:lastcheck", "2000-11-03"),
             StringMapEntryDelete("lastcheck:key", "2000-11-04"),
             StringMapEntryDelete("key:last_checked", "2000-11-05"),

diff --git a/app/src/test/java/de/westnordost/streetcomplete/quests/AddRecyclingContainerMaterialsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/quests/AddRecyclingContainerMaterialsTest.kt
@@ -240,7 +240,7 @@ class AddRecyclingContainerMaterialsTest {
         )
     }
 
-    @Test fun `apply answer removes previous check dates`() {
+    @Test fun `apply answer updates previous check dates`() {
         questType.verifyAnswer(
             mapOf(
                 "recycling:paper" to "no",
@@ -254,7 +254,7 @@ class AddRecyclingContainerMaterialsTest {
             RecyclingMaterials(listOf(PAPER)),
             StringMapEntryModify("recycling:paper", "no", "yes"),
             StringMapEntryDelete("recycling:check_date", "2000-11-01"),
-            StringMapEntryDelete("check_date:recycling", "2000-11-02"),
+            StringMapEntryModify("check_date:recycling", "2000-11-02", LocalDate.now().toCheckDateString()),
             StringMapEntryDelete("recycling:lastcheck", "2000-11-03"),
             StringMapEntryDelete("lastcheck:recycling", "2000-11-04"),
             StringMapEntryDelete("recycling:last_checked", "2000-11-05"),