Fix duplicated content during sync (#2388)

Here are the main fixes:

- mark a synched datalayer as loaded (so the peer does not try to get
data from the server)
- do not mark synched datalayers as dirty
- properly consume the lastKnownHLC, so to get an accurate list of
operations

fix #2219

Author: yohanboniface

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

@@ -88,7 +88,6 @@ export class DataLayer extends ServerStored {
 
     if (!this.createdOnServer) {
       if (this.showAtLoad()) this.show()
-      this.isDirty = true
     }
 
     // Only layers that are displayed on load must be hidden/shown
@@ -151,7 +150,6 @@ export class DataLayer extends ServerStored {
           for (const field of fields) {
             this.layer.onEdit(field, builder)
           }
-          this.redraw()
           this.show()
           break
         case 'remote-data':
@@ -592,7 +590,7 @@ export class DataLayer extends ServerStored {
     options.name = translate('Clone of {name}', { name: this.options.name })
     delete options.id
     const geojson = Utils.CopyJSON(this._geojson)
-    const datalayer = this._umap.createDataLayer(options)
+    const datalayer = this._umap.createDirtyDataLayer(options)
     datalayer.fromGeoJSON(geojson)
     return datalayer
   }
@@ -1066,7 +1064,7 @@ export class DataLayer extends ServerStored {
 
   setReferenceVersion({ response, sync }) {
     this._referenceVersion = response.headers.get('X-Datalayer-Version')
-    this.sync.update('_referenceVersion', this._referenceVersion)
+    if (sync) this.sync.update('_referenceVersion', this._referenceVersion)
   }
 
   async save() {

umap/static/umap/js/modules/importer.js

@@ -161,7 +161,7 @@ export default class Importer extends Utils.WithTemplate {
   get layer() {
     return (
       this._umap.datalayers[this.layerId] ||
-      this._umap.createDataLayer({ name: this.layerName })
+      this._umap.createDirtyDataLayer({ name: this.layerName })
     )
   }
 

umap/static/umap/js/modules/sync/engine.js

@@ -222,9 +222,13 @@ export class SyncEngine {
    * @param {string} payload.sender the uuid of the requesting peer
    * @param {string} payload.latestKnownHLC the latest known HLC of the requesting peer
    */
-  onListOperationsRequest({ sender, lastKnownHLC }) {
+  onListOperationsRequest({ sender, message }) {
+    debug(
+      `received operations request from peer ${sender} (since ${message.lastKnownHLC})`
+    )
+
     this.sendToPeer(sender, 'ListOperationsResponse', {
-      operations: this._operations.getOperationsSince(lastKnownHLC),
+      operations: this._operations.getOperationsSince(message.lastKnownHLC),
     })
   }
 
@@ -485,5 +489,5 @@ export class Operations {
 }
 
 function debug(...args) {
-  console.debug('SYNC ⇆', ...args)
+  console.debug('SYNC ⇆', ...args.map((x) => JSON.stringify(x)))
 }

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

@@ -54,7 +54,10 @@ export class MapUpdater extends BaseUpdater {
 export class DataLayerUpdater extends BaseUpdater {
   upsert({ value }) {
     // Upsert only happens when a new datalayer is created.
-    this._umap.createDataLayer(value, false)
+    const datalayer = this._umap.createDataLayer(value, false)
+    // Prevent the layer to get data from the server, as it will get it
+    // from the sync.
+    datalayer._loaded = true
   }
 
   update({ key, metadata, value }) {

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

@@ -324,14 +324,14 @@ export default class Umap extends ServerStored {
         dataUrl = decodeURIComponent(dataUrl)
         dataUrl = this.renderUrl(dataUrl)
         dataUrl = this.proxyUrl(dataUrl)
-        const datalayer = this.createDataLayer()
+        const datalayer = this.createDirtyDataLayer()
         await datalayer
           .importFromUrl(dataUrl, dataFormat)
           .then(() => datalayer.zoomTo())
       }
     } else if (data) {
       data = decodeURIComponent(data)
-      const datalayer = this.createDataLayer()
+      const datalayer = this.createDirtyDataLayer()
       await datalayer.importRaw(data, dataFormat).then(() => datalayer.zoomTo())
     }
   }
@@ -599,8 +599,14 @@ export default class Umap extends ServerStored {
     return datalayer
   }
 
+  createDirtyDataLayer(options) {
+    const datalayer = this.createDataLayer(options, true)
+    datalayer.isDirty = true
+    return datalayer
+  }
+
   newDataLayer() {
-    const datalayer = this.createDataLayer({})
+    const datalayer = this.createDirtyDataLayer({})
     datalayer.edit()
   }
 
@@ -1389,7 +1395,7 @@ export default class Umap extends ServerStored {
       fallback.show()
       return fallback
     }
-    return this.createDataLayer()
+    return this.createDirtyDataLayer()
   }
 
   findDataLayer(method, context) {
@@ -1553,7 +1559,7 @@ export default class Umap extends ServerStored {
     if (type === 'umap') {
       this.importUmapFile(file, 'umap')
     } else {
-      if (!layer) layer = this.createDataLayer({ name: file.name })
+      if (!layer) layer = this.createDirtyDataLayer({ name: file.name })
       layer.importFromFile(file, type)
     }
   }
@@ -1579,7 +1585,7 @@ export default class Umap extends ServerStored {
         delete geojson._storage
       }
       delete geojson._umap_options?.id // Never trust an id at this stage
-      const dataLayer = this.createDataLayer(geojson._umap_options)
+      const dataLayer = this.createDirtyDataLayer(geojson._umap_options)
       dataLayer.fromUmapGeoJSON(geojson)
     }
 

umap/tests/integration/test_websocket_sync.py

@@ -39,6 +39,9 @@ def test_websocket_connection_can_sync_markers(
     a_map_el.click(position={"x": 220, "y": 220})
     expect(a_marker_pane).to_have_count(1)
     expect(b_marker_pane).to_have_count(1)
+    # Peer B should not be in state dirty
+    expect(peerB.get_by_role("button", name="View")).to_be_visible()
+    expect(peerB.get_by_role("button", name="Cancel edits")).to_be_hidden()
     peerA.locator("body").type("Synced name")
     peerA.locator("body").press("Escape")
 
@@ -415,3 +418,69 @@ def test_should_sync_datalayers(new_page, live_server, websocket_server, tilelay
         peerA.get_by_role("button", name="Save").click()
 
     assert DataLayer.objects.count() == 2
+
+
+@pytest.mark.xdist_group(name="websockets")
+def test_create_and_sync_map(
+    new_page, live_server, websocket_server, tilelayer, login, user
+):
+    # Create a syncable map with peerA
+    peerA = login(user, prefix="Page A")
+    peerA.goto(f"{live_server.url}/en/map/new/")
+    with peerA.expect_response(re.compile("./map/create/.*")):
+        peerA.get_by_role("button", name="Save Draft").click()
+    peerA.get_by_role("link", name="Map advanced properties").click()
+    peerA.get_by_text("Real-time collaboration", exact=True).click()
+    peerA.get_by_text("Enable real-time").click()
+    peerA.get_by_role("link", name="Update permissions and editors").click()
+    peerA.locator('select[name="share_status"]').select_option(str(Map.PUBLIC))
+    with peerA.expect_response(re.compile("./update/settings/.*")):
+        peerA.get_by_role("button", name="Save").click()
+    expect(peerA.get_by_role("button", name="Cancel edits")).to_be_hidden()
+    # Quit edit mode
+    peerA.get_by_role("button", name="View").click()
+
+    # Open map and go to edit mode with peer B
+    peerB = new_page("Page B")
+    peerB.goto(peerA.url)
+    peerB.get_by_role("button", name="Edit").click()
+
+    # Create a marker from peerA
+    markersA = peerA.locator(".leaflet-marker-pane > div")
+    markersB = peerB.locator(".leaflet-marker-pane > div")
+    expect(markersA).to_have_count(0)
+    expect(markersB).to_have_count(0)
+
+    # Add a marker from peer A
+    peerA.get_by_role("button", name="Edit").click()
+    peerA.get_by_title("Draw a marker").click()
+    peerA.locator("#map").click(position={"x": 220, "y": 220})
+    expect(markersA).to_have_count(1)
+    expect(markersB).to_have_count(1)
+
+    # Save and quit edit mode again
+    with peerA.expect_response(re.compile("./datalayer/create/.*")):
+        peerA.get_by_role("button", name="Save").click()
+    peerA.get_by_role("button", name="View").click()
+    expect(markersA).to_have_count(1)
+    expect(markersB).to_have_count(1)
+    peerA.wait_for_timeout(500)
+    expect(markersA).to_have_count(1)
+    expect(markersB).to_have_count(1)
+
+    # Peer B should not be in state dirty
+    expect(peerB.get_by_role("button", name="View")).to_be_visible()
+    expect(peerB.get_by_role("button", name="Cancel edits")).to_be_hidden()
+
+    # Add a marker from peer B
+    peerB.get_by_title("Draw a marker").click()
+    peerB.locator("#map").click(position={"x": 200, "y": 200})
+    expect(markersB).to_have_count(2)
+    expect(markersA).to_have_count(1)
+    with peerB.expect_response(re.compile("./datalayer/update/.*")):
+        peerB.get_by_role("button", name="Save").click()
+    expect(markersB).to_have_count(2)
+    expect(markersA).to_have_count(1)
+    peerA.get_by_role("button", name="Edit").click()
+    expect(markersA).to_have_count(2)
+    expect(markersB).to_have_count(2)