fix(sync): handle sync of datalayer delete (#2416)

fix #2268

There is a tricky choice to do: the delete actually occurs in two
times, first the datalayer is hidden from the UI and set as "deleted"
(this can then be undone) then at next "save" it will totally removed.

When syncing, given we removed the "reset/undo" feature for now, and
because it was simpler, I decide to do both step in once.

When working on a proper "undo/redo", we may challenge this choice
again.

umap/static/umap/js/modules/caption.js

@@ -3,13 +3,14 @@ import * as Utils from './utils.js'
 
 const TEMPLATE = `
 <div class="umap-caption">
-  <hgroup>
+  <div class="header">
-    <h3>
     <i class="icon icon-16 icon-caption icon-block"></i>
-      <span class="map-name" data-ref="name"></span>
+    <hgroup>
-    </h3>
+      <h3><span class="map-name" data-ref="name"></span></h3>
       <h4 data-ref="author"></h4>
+      <h5 class="dates" data-ref="dates"></h5>
     </hgroup>
+  </div>
   <div class="umap-map-description text" data-ref="description"></div>
   <div class="datalayer-container" data-ref="datalayersContainer"></div>
   <div class="credits-container">
@@ -65,6 +66,17 @@ export default class Caption extends Utils.WithTemplate {
       // Create the legend when the panel is actually on the DOM
       this._umap.eachDataLayerReverse((datalayer) => datalayer.renderLegend())
     })
+    if (this._umap.properties.created_at) {
+      const created_at = translate('Created at {date}', {
+        date: new Date(this._umap.properties.created_at).toLocaleDateString(),
+      })
+      const modified_at = translate('Modified at {date}', {
+        date: new Date(this._umap.properties.modified_at).toLocaleDateString(),
+      })
+      this.elements.dates.innerHTML = `${created_at} - ${modified_at}`
+    } else {
+      this.elements.dates.hidden = true
+    }
   }
 
   addDataLayer(datalayer, parent) {

umap/static/umap/js/modules/data/layer.js

@@ -574,9 +574,12 @@ export class DataLayer extends ServerStored {
     })
   }
 
-  _delete() {
+  del(sync = true) {
-    this.isDeleted = true
     this.erase()
+    if (sync) {
+      this.isDeleted = true
+      this.sync.delete()
+    }
   }
 
   empty() {
@@ -819,7 +822,7 @@ export class DataLayer extends ServerStored {
         <i class="icon icon-24 icon-delete"></i>${translate('Delete')}
       </button>`)
     deleteButton.addEventListener('click', () => {
-      this._delete()
+      this.del()
       this._umap.editPanel.close()
     })
     advancedButtons.appendChild(deleteButton)
@@ -1147,10 +1150,14 @@ export class DataLayer extends ServerStored {
     if (this.createdOnServer) {
       await this._umap.server.post(this.getDeleteUrl())
     }
-    delete this._umap.datalayers[stamp(this)]
+    this.commitDelete()
     return true
   }
 
+  commitDelete() {
+    delete this._umap.datalayers[stamp(this)]
+  }
+
   getName() {
     return this.options.name || translate('Untitled layer')
   }
@@ -1221,7 +1228,7 @@ export class DataLayer extends ServerStored {
           this._umap.dialog
             .confirm(translate('Are you sure you want to delete this layer?'))
             .then(() => {
-              this._delete()
+              this.del()
             })
         },
         this

umap/static/umap/js/modules/sync/updaters.js

@@ -72,6 +72,14 @@ export class DataLayerUpdater extends BaseUpdater {
     }
     datalayer.render([key])
   }
+
+  delete({ metadata }) {
+    const datalayer = this.getDataLayerFromID(metadata.id)
+    if (datalayer) {
+      datalayer.del(false)
+      datalayer.commitDelete()
+    }
+  }
 }
 
 export class FeatureUpdater extends BaseUpdater {

umap/static/umap/map.css

@@ -694,8 +694,14 @@ a.umap-control-caption,
 .datalayer-name {
     cursor: pointer;
 }
-.umap-caption .umap-map-author {
+.umap-caption .dates {
-    padding-inline-start: 31px;
+    color: var(--color-mediumGray);
+}
+.umap-caption .header {
+    display: flex;
+}
+.umap-caption .header i.icon {
+    flex-shrink: 0;
 }
 .umap-browser .main-toolbox {
     padding-left: 4px; /* Align with toolbox below  */

umap/tests/base.py

@@ -127,6 +127,9 @@ class DataLayerFactory(factory.django.DjangoModelFactory):
     def _adjust_kwargs(cls, **kwargs):
         if "data" in kwargs:
             data = copy.deepcopy(kwargs.pop("data"))
+            data.setdefault("_umap_options", {})
+            if "name" in data["_umap_options"] and kwargs["name"] == cls.name:
+                kwargs["name"] = data["_umap_options"]["name"]
             if "settings" not in kwargs:
                 kwargs["settings"] = data.get("_umap_options", {})
         else:
@@ -135,7 +138,6 @@ class DataLayerFactory(factory.django.DjangoModelFactory):
                 **DataLayerFactory.settings._defaults,
                 **kwargs["settings"],
             }
-        data.setdefault("_umap_options", {})
         kwargs["settings"]["name"] = kwargs["name"]
         data["_umap_options"]["name"] = kwargs["name"]
         data.setdefault("type", "FeatureCollection")

umap/tests/integration/test_caption.py

@@ -25,6 +25,7 @@ def test_caption(live_server, page, map):
         panel.locator(".caption-item .off").get_by_text(non_loaded.name)
     ).to_be_visible()
     expect(panel.locator(".caption-item").get_by_text(hidden.name)).to_be_hidden()
+    expect(panel.get_by_text("Created at")).to_be_visible()
 
 
 def test_caption_should_display_owner_as_author(live_server, page, map):

umap/tests/integration/test_websocket_sync.py

@@ -420,6 +420,72 @@ def test_should_sync_datalayers(new_page, live_server, websocket_server, tilelay
     assert DataLayer.objects.count() == 2
 
 
+@pytest.mark.xdist_group(name="websockets")
+def test_should_sync_datalayers_delete(
+    new_page, live_server, websocket_server, tilelayer
+):
+    map = MapFactory(name="sync", edit_status=Map.ANONYMOUS)
+    map.settings["properties"]["syncEnabled"] = True
+    map.save()
+    data1 = {
+        "type": "FeatureCollection",
+        "features": [
+            {
+                "type": "Feature",
+                "properties": {
+                    "name": "Point 1",
+                },
+                "geometry": {"type": "Point", "coordinates": [0.065918, 48.385442]},
+            },
+        ],
+        "_umap_options": {
+            "name": "datalayer 1",
+        },
+    }
+    data2 = {
+        "type": "FeatureCollection",
+        "features": [
+            {
+                "type": "Feature",
+                "properties": {
+                    "name": "Point 2",
+                },
+                "geometry": {"type": "Point", "coordinates": [3.55957, 49.767074]},
+            },
+        ],
+        "_umap_options": {
+            "name": "datalayer 2",
+        },
+    }
+    DataLayerFactory(map=map, data=data1)
+    DataLayerFactory(map=map, data=data2)
+
+    # Create two tabs
+    peerA = new_page("Page A")
+    peerA.goto(f"{live_server.url}{map.get_absolute_url()}?edit")
+    peerB = new_page("Page B")
+    peerB.goto(f"{live_server.url}{map.get_absolute_url()}?edit")
+
+    peerA.get_by_role("button", name="Open browser").click()
+    expect(peerA.get_by_text("datalayer 1")).to_be_visible()
+    expect(peerA.get_by_text("datalayer 2")).to_be_visible()
+    peerB.get_by_role("button", name="Open browser").click()
+    expect(peerB.get_by_text("datalayer 1")).to_be_visible()
+    expect(peerB.get_by_text("datalayer 2")).to_be_visible()
+
+    # Delete "datalayer 2" in peerA
+    peerA.locator(".datalayer").get_by_role("button", name="Delete layer").first.click()
+    peerA.get_by_role("button", name="OK").click()
+    expect(peerA.get_by_text("datalayer 2")).to_be_hidden()
+    expect(peerB.get_by_text("datalayer 2")).to_be_hidden()
+
+    # Save delete to the server
+    with peerA.expect_response(re.compile(".*/datalayer/delete/.*")):
+        peerA.get_by_role("button", name="Save").click()
+    expect(peerA.get_by_text("datalayer 2")).to_be_hidden()
+    expect(peerB.get_by_text("datalayer 2")).to_be_hidden()
+
+
 @pytest.mark.xdist_group(name="websockets")
 def test_create_and_sync_map(
     new_page, live_server, websocket_server, tilelayer, login, user

umap/tests/test_datalayer.py

@@ -1,4 +1,3 @@
-import tempfile
 from pathlib import Path
 
 import pytest

umap/tests/test_datalayer_views.py

@@ -1,6 +1,8 @@
 import json
 from copy import deepcopy
+from datetime import datetime, timedelta
 from pathlib import Path
+from unittest import mock
 from uuid import uuid4
 
 import pytest
@@ -621,3 +623,17 @@ def test_optimistic_merge_conflicting_change_raises(
     modified_datalayer = DataLayer.objects.get(pk=datalayer.pk)
     merged_features = json.load(modified_datalayer.geojson)["features"]
     assert merged_features == client1_data["features"]
+
+
+def test_saving_datalayer_should_change_map_last_modified(
+    client, datalayer, map, post_data
+):
+    with mock.patch("django.utils.timezone.now") as mocked:
+        mocked.return_value = datetime.utcnow() - timedelta(days=8)
+        map.save()  # Change last_modified to past
+    old_modified_at = map.modified_at.date()
+    url = reverse("datalayer_update", args=(map.pk, datalayer.pk))
+    client.login(username=map.owner.username, password="123123")
+    response = client.post(url, post_data, follow=True)
+    assert response.status_code == 200
+    assert Map.objects.get(pk=map.pk).modified_at.date() != old_modified_at

umap/views.py

@@ -614,6 +614,13 @@ class MapDetailMixin(SessionMixin):
             "defaultLabelKeys": settings.UMAP_LABEL_KEYS,
         }
         created = bool(getattr(self, "object", None))
+        if created:
+            properties.update(
+                {
+                    "created_at": self.object.created_at,
+                    "modified_at": self.object.modified_at,
+                }
+            )
         if (created and self.object.owner) or (not created and not user.is_anonymous):
             edit_statuses = Map.EDIT_STATUS
             datalayer_statuses = DataLayer.EDIT_STATUS
@@ -1293,6 +1300,7 @@ class DataLayerUpdate(FormLessEditMixin, UpdateView):
 
     def form_valid(self, form):
         self.object = form.save()
+        self.object.map.save(update_fields=["modified_at"])
         data = {**self.object.metadata(self.request)}
         if self.request.session.get("needs_reload"):
             data["geojson"] = json.loads(self.object.geojson.read().decode())