add fixme when deleting nodes that are part of a relation (fixes #5851)

streetcomplete · Oct 17, 2024 · 85d587e · 85d587e
1 parent 899c7fb
commit 85d587e
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 9 deletions.
diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeAction.kt b/app/src/main/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeAction.kt
@@ -46,23 +46,30 @@ data class DeletePoiNodeAction(
             throw ConflictException("Element geometry changed substantially")
         }
 
-        val isInWayOrRelation =
-            mapDataRepository.getWaysForNode(currentNode.id).isNotEmpty()
-            || mapDataRepository.getRelationsForNode(currentNode.id).isNotEmpty()
+        val isInWay = mapDataRepository.getWaysForNode(currentNode.id).isNotEmpty()
+        val isInRelation = mapDataRepository.getRelationsForNode(currentNode.id).isNotEmpty()
 
-        return if (!isInWayOrRelation) {
-            // delete free-floating node
-            MapDataChanges(deletions = listOf(currentNode))
-        } else {
-            // if it is a vertex in a way or has a role in a relation: just clear the tags then
+        // if it is a vertex in a way or has a role in a relation: clear the tags
+        return if (isInWay || isInRelation) {
+            // for relations specifically, we want to add a marker, because maybe it should be
+            // deleted fully or at least deleted from the relation if it doesn't fulfill a role
             val emptyNode = currentNode.copy(
-                tags = emptyMap(),
+                tags =
+                    if (!isInRelation) emptyMap()
+                    else mapOf("fixme" to FIXME_DELETED_NODE_IN_RELATION_TEXT),
                 timestampEdited = nowAsEpochMilliseconds()
             )
             MapDataChanges(modifications = listOf(emptyNode))
         }
+        // otherwise, can safely delete the free-floating node
+        else {
+            MapDataChanges(deletions = listOf(currentNode))
+        }
     }
 
     override fun createReverted(idProvider: ElementIdProvider) =
         RevertDeletePoiNodeAction(originalNode)
 }
+
+private const val FIXME_DELETED_NODE_IN_RELATION_TEXT =
+    "object not found on a survey, check if it should be deleted or deleted from the relation"
diff --git a/.../test/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeActionTest.kt b/.../test/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeActionTest.kt
@@ -47,6 +47,15 @@ class DeletePoiNodeActionTest {
         assertTrue(data.modifications.single().tags.isEmpty())
     }
 
+    @Test fun `'delete' relation member`() {
+        on(repos.getRelationsForNode(1L)).thenReturn(listOf(mock()))
+        on(repos.getNode(e.id)).thenReturn(e)
+        val data = DeletePoiNodeAction(e).createUpdates(repos, provider)
+        assertTrue(data.deletions.isEmpty())
+        assertTrue(data.creations.isEmpty())
+        assertTrue(data.modifications.single().tags.containsKey("fixme"))
+    }
+
     @Test
     fun `moved element creates conflict`() {
         on(repos.getNode(e.id)).thenReturn(e.copy(position = p(1.0, 1.0)))