From 85d587edec88234ca801ad4c58f6b5fd73cc14f0 Mon Sep 17 00:00:00 2001 From: Tobias Zwick Date: Thu, 17 Oct 2024 20:24:03 +0200 Subject: [PATCH] add fixme when deleting nodes that are part of a relation (fixes #5851) --- .../osm/edits/delete/DeletePoiNodeAction.kt | 25 ++++++++++++------- .../edits/delete/DeletePoiNodeActionTest.kt | 9 +++++++ 2 files changed, 25 insertions(+), 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 index 60a4adfbac..4b22b2e7b9 100644 --- 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/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeActionTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeActionTest.kt index 15350cf74e..74ae225369 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/data/osm/edits/delete/DeletePoiNodeActionTest.kt +++ b/app/src/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)))