From 093ed061c15e6f3d02585c5468a29fcc432f9197 Mon Sep 17 00:00:00 2001
From: Yohan Boniface <yohanboniface@free.fr>
Date: Fri, 21 Mar 2025 16:17:31 +0100
Subject: [PATCH] wip: tests pass

Co-authored-by: David Larlet <david@larlet.fr>
---
 umap/static/umap/js/modules/data/features.js  |  2 -
 umap/static/umap/js/modules/data/layer.js     | 16 ++++----
 umap/static/umap/js/modules/form/fields.js    |  4 +-
 umap/static/umap/js/modules/importer.js       |  2 +-
 umap/static/umap/js/modules/permissions.js    | 16 ++++++++
 umap/static/umap/js/modules/sync/engine.js    | 39 ++++++++++++++-----
 umap/static/umap/js/modules/sync/undo.js      |  9 +++--
 umap/static/umap/js/modules/sync/updaters.js  | 34 ++++++++++++----
 umap/static/umap/js/modules/ui/bar.js         |  7 +---
 umap/static/umap/js/modules/umap.js           | 15 ++++---
 umap/static/umap/js/umap.controls.js          |  1 -
 umap/sync/payloads.py                         |  2 +-
 umap/tests/integration/test_edit_datalayer.py |  1 -
 .../integration/test_optimistic_merge.py      |  7 ++--
 umap/tests/integration/test_save.py           |  3 +-
 umap/tests/integration/test_undo_redo.py      |  4 +-
 16 files changed, 104 insertions(+), 58 deletions(-)

diff --git a/umap/static/umap/js/modules/data/features.js b/umap/static/umap/js/modules/data/features.js
index 8278d498..cd04dba3 100644
--- a/umap/static/umap/js/modules/data/features.js
+++ b/umap/static/umap/js/modules/data/features.js
@@ -108,13 +108,11 @@ class Feature {
     const oldGeometry = Utils.CopyJSON(this._geometry)
     this.fromLatLngs(this._getLatLngs())
     if (sync) {
-      console.log('sync geometry')
       this.sync.update('geometry', this.geometry, oldGeometry)
     }
   }
 
   fromLatLngs(latlngs) {
-    console.log('fromLatLngs', latlngs)
     this._geometry_bk = Utils.CopyJSON(this._geometry)
     this._geometry = this.convertLatLngs(latlngs)
   }
diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js
index 26a767ef..793e5c78 100644
--- a/umap/static/umap/js/modules/data/layer.js
+++ b/umap/static/umap/js/modules/data/layer.js
@@ -49,7 +49,6 @@ export class DataLayer extends ServerStored {
     this._leafletMap = leafletMap
     this.parentPane = this._leafletMap.getPane('overlayPane')
     this.pane = this._leafletMap.createPane(`datalayer${stamp(this)}`, this.parentPane)
-    this.pane.dataset.id = stamp(this)
     // FIXME: should be on layer
     this.renderer = L.svg({ pane: this.pane })
     this.defaultOptions = {
@@ -66,6 +65,7 @@ export class DataLayer extends ServerStored {
     data.id = data.id || crypto.randomUUID()
 
     this.setOptions(data)
+    this.pane.dataset.id = this.id
 
     if (!Utils.isObject(this.options.remoteData)) {
       this.options.remoteData = {}
@@ -366,9 +366,8 @@ export class DataLayer extends ServerStored {
   }
 
   connectToMap() {
-    const id = stamp(this)
-    if (!this._umap.datalayers[id]) {
-      this._umap.datalayers[id] = this
+    if (!this._umap.datalayers[this.id]) {
+      this._umap.datalayers[this.id] = this
     }
     if (!this._umap.datalayersIndex.includes(this)) {
       this._umap.datalayersIndex.push(this)
@@ -419,7 +418,6 @@ export class DataLayer extends ServerStored {
     const id = stamp(feature)
     if (sync !== false) {
       const oldValue = feature.toGeoJSON()
-      console.log('oldValue in removeFeature', oldValue)
       feature.sync.delete(oldValue)
     }
     this.hideFeature(feature)
@@ -460,13 +458,13 @@ export class DataLayer extends ServerStored {
       .sort(Utils.naturalSort)
   }
 
-  addData(geojson, sync) {
+  addData(geojson, sync, batch = true) {
     try {
       // Do not fail if remote data is somehow invalid,
       // otherwise the layer becomes uneditable.
-      this.sync.startBatch()
+      if (batch) this.sync.startBatch()
       const features = this.makeFeatures(geojson, sync)
-      this.sync.commitBatch()
+      if (batch) this.sync.commitBatch()
       return features
     } catch (err) {
       console.debug('Error with DataLayer', this.id)
@@ -1187,7 +1185,7 @@ export class DataLayer extends ServerStored {
   }
 
   commitDelete() {
-    delete this._umap.datalayers[stamp(this)]
+    delete this._umap.datalayers[this.id]
   }
 
   getName() {
diff --git a/umap/static/umap/js/modules/form/fields.js b/umap/static/umap/js/modules/form/fields.js
index 71abe87d..5a8be8d6 100644
--- a/umap/static/umap/js/modules/form/fields.js
+++ b/umap/static/umap/js/modules/form/fields.js
@@ -541,14 +541,14 @@ Fields.DataLayerSwitcher = class extends Fields.Select {
         !datalayer.isDataReadOnly() &&
         datalayer.isBrowsable()
       ) {
-        options.push([L.stamp(datalayer), datalayer.getName()])
+        options.push([datalayer.id, datalayer.getName()])
       }
     })
     return options
   }
 
   toHTML() {
-    return L.stamp(this.obj.datalayer)
+    return this.obj.datalayer.id
   }
 
   toJS() {
diff --git a/umap/static/umap/js/modules/importer.js b/umap/static/umap/js/modules/importer.js
index 83e09812..f2738f08 100644
--- a/umap/static/umap/js/modules/importer.js
+++ b/umap/static/umap/js/modules/importer.js
@@ -249,7 +249,7 @@ export default class Importer extends Utils.WithTemplate {
           tagName: 'option',
           parent: layerSelect,
           textContent: datalayer.options.name,
-          value: L.stamp(datalayer),
+          value: datalayer.id,
         })
       }
     })
diff --git a/umap/static/umap/js/modules/permissions.js b/umap/static/umap/js/modules/permissions.js
index ffaab4fa..d4eca1e0 100644
--- a/umap/static/umap/js/modules/permissions.js
+++ b/umap/static/umap/js/modules/permissions.js
@@ -13,6 +13,7 @@ export class MapPermissions extends ServerStored {
     this.setProperties(umap.properties.permissions)
     this._umap = umap
     this._isDirty = false
+    this.sync = umap.syncEngine.proxy(this)
   }
 
   setProperties(properties) {
@@ -28,6 +29,13 @@ export class MapPermissions extends ServerStored {
     )
   }
 
+  getSyncMetadata() {
+    return {
+      subject: 'mappermissions',
+      metadata: {},
+    }
+  }
+
   render() {
     this._umap.render(['properties.permissions'])
   }
@@ -259,6 +267,14 @@ export class DataLayerPermissions extends ServerStored {
     )
 
     this.datalayer = datalayer
+    this.sync = umap.syncEngine.proxy(this)
+  }
+
+  getSyncMetadata() {
+    return {
+      subject: 'datalayerpermissions',
+      metadata: { id: this.datalayer.id },
+    }
   }
 
   edit(container) {
diff --git a/umap/static/umap/js/modules/sync/engine.js b/umap/static/umap/js/modules/sync/engine.js
index 2e098efc..a12c01a5 100644
--- a/umap/static/umap/js/modules/sync/engine.js
+++ b/umap/static/umap/js/modules/sync/engine.js
@@ -2,7 +2,13 @@ import * as SaveManager from '../saving.js'
 import * as Utils from '../utils.js'
 import { HybridLogicalClock } from './hlc.js'
 import { UndoManager } from './undo.js'
-import { DataLayerUpdater, FeatureUpdater, MapUpdater } from './updaters.js'
+import {
+  DataLayerUpdater,
+  FeatureUpdater,
+  MapUpdater,
+  MapPermissionsUpdater,
+  DataLayerPermissionsUpdater,
+} from './updaters.js'
 import { WebSocketTransport } from './websocket.js'
 
 // Start reconnecting after 2 seconds, then double the delay each time
@@ -56,6 +62,8 @@ export class SyncEngine {
       map: new MapUpdater(umap),
       feature: new FeatureUpdater(umap),
       datalayer: new DataLayerUpdater(umap),
+      mappermissions: new MapPermissionsUpdater(umap),
+      datalayerpermissions: new DataLayerPermissionsUpdater(umap),
     }
     this.transport = undefined
     this._operations = new Operations()
@@ -129,17 +137,15 @@ export class SyncEngine {
     this._batch = []
   }
 
-  commitBatch() {
+  commitBatch(subject, metadata) {
     if (!this._batch.length) {
       this._batch = null
       return
     }
-    this._undoManager.add({
-      verb: 'batch',
-      operations: this._batch,
-    })
     const operations = this._batch.map((stage) => stage.operation)
-    this._send({ verb: 'batch', operations: operations, subject: 'batch' })
+    const operation = { verb: 'batch', operations, subject, metadata }
+    this._undoManager.add({ operation, stages: this._batch })
+    this._send(operation)
     this._batch = null
   }
 
@@ -204,6 +210,10 @@ export class SyncEngine {
 
   async save() {
     const needSave = new Map()
+    if (!this._umap.id) {
+      // There is no operation for fist map save
+      needSave.set(this._umap, [])
+    }
     for (const operation of this._operations.sorted()) {
       if (operation.dirty) {
         const updater = this._getUpdater(operation.subject)
@@ -215,7 +225,8 @@ export class SyncEngine {
       }
     }
     for (const [obj, operations] of needSave.entries()) {
-      await obj.save()
+      const ok = await obj.save()
+      if (!ok) break
       for (const operation of operations) {
         operation.dirty = false
       }
@@ -243,7 +254,11 @@ export class SyncEngine {
     }
   }
 
-  _getUpdater(subject, metadata) {
+  _getUpdater(subject, metadata, sync) {
+    // For now, prevent permissions to be synced, for security reasons
+    if (sync && (subject === 'mappermissions' || subject === 'datalayerpermissions')) {
+      return
+    }
     if (Object.keys(this.updaters).includes(subject)) {
       return this.updaters[subject]
     }
@@ -256,6 +271,10 @@ export class SyncEngine {
       return
     }
     const updater = this._getUpdater(operation.subject, operation.metadata)
+    if (!updater) {
+      debug('No updater for', operation)
+      return
+    }
     updater.applyMessage(operation)
   }
 
@@ -449,7 +468,7 @@ export class SyncEngine {
     const handler = {
       get(target, prop) {
         // Only proxy these methods
-        if (['upsert', 'update', 'delete'].includes(prop)) {
+        if (['upsert', 'update', 'delete', 'commitBatch'].includes(prop)) {
           const { subject, metadata } = object.getSyncMetadata()
           // Reflect.get is calling the original method.
           // .bind is adding the parameters automatically
diff --git a/umap/static/umap/js/modules/sync/undo.js b/umap/static/umap/js/modules/sync/undo.js
index a9e7af2e..8ee57890 100644
--- a/umap/static/umap/js/modules/sync/undo.js
+++ b/umap/static/umap/js/modules/sync/undo.js
@@ -19,6 +19,9 @@ export class UndoManager {
     for (const button of document.querySelectorAll('.disabled-on-dirty')) {
       button.disabled = dirty
     }
+    for (const button of document.querySelectorAll('.enabled-on-dirty')) {
+      button.disabled = !dirty
+    }
   }
 
   isDirty() {
@@ -33,7 +36,7 @@ export class UndoManager {
 
   add(stage) {
     // FIXME make it more generic
-    if (stage.operation.key !== '_referenceVersion') {
+    if (!stage.operation || stage.operation.key !== '_referenceVersion') {
       stage.operation.dirty = true
       this._redoStack = []
       this._undoStack.push(stage)
@@ -54,8 +57,8 @@ export class UndoManager {
     if (!stage) return
     stage.operation.dirty = !stage.operation.dirty
     if (stage.operation.verb === 'batch') {
-      for (const op of stage.operations) {
-        this.applyOperation(this.copyOperation(op, redo))
+      for (const st of stage.stages) {
+        this.applyOperation(this.copyOperation(st, redo))
       }
     } else {
       this.applyOperation(this.copyOperation(stage, redo))
diff --git a/umap/static/umap/js/modules/sync/updaters.js b/umap/static/umap/js/modules/sync/updaters.js
index 082901bf..606ea63f 100644
--- a/umap/static/umap/js/modules/sync/updaters.js
+++ b/umap/static/umap/js/modules/sync/updaters.js
@@ -43,7 +43,6 @@ class BaseUpdater {
 
 export class MapUpdater extends BaseUpdater {
   update({ key, value }) {
-    console.log('updating', key, value)
     if (fieldInSchema(key)) {
       this.updateObjectValue(this._umap, key, value)
     }
@@ -61,16 +60,11 @@ export class DataLayerUpdater extends BaseUpdater {
   upsert({ value }) {
     // Upsert only happens when a new datalayer is created.
     try {
-      console.log(
-        'found datalayer with id',
-        value.id,
-        this.getDataLayerFromID(value.id)
-      )
+      this.getDataLayerFromID(value.id)
     } catch {
-      console.log('we are the fucking catch', value)
       const datalayer = this._umap.createDataLayer(value._umap_options || value, false)
       if (value.features) {
-        datalayer.addData(value)
+        datalayer.addData(value, true, false)
       }
     }
   }
@@ -149,3 +143,27 @@ export class FeatureUpdater extends BaseUpdater {
     return this.getDataLayerFromID(metadata.layerId)
   }
 }
+
+export class MapPermissionsUpdater extends BaseUpdater {
+  update({ key, value }) {
+    this.updateObjectValue(this._umap.permissions, key, value)
+    // if (fieldInSchema(key)) {
+    // }
+  }
+
+  getStoredObject(metadata) {
+    return this._umap.permissions
+  }
+}
+
+export class DataLayerPermissionsUpdater extends BaseUpdater {
+  update({ key, value, metadata }) {
+    this.updateObjectValue(this.getDataLayerFromID(metadata.id), key, value)
+    // if (fieldInSchema(key)) {
+    // }
+  }
+
+  getStoredObject(metadata) {
+    return this.getDataLayerFromID(metadata.id).permissions
+  }
+}
diff --git a/umap/static/umap/js/modules/ui/bar.js b/umap/static/umap/js/modules/ui/bar.js
index 28eb3677..d54ea866 100644
--- a/umap/static/umap/js/modules/ui/bar.js
+++ b/umap/static/umap/js/modules/ui/bar.js
@@ -34,7 +34,7 @@ const TOP_BAR_TEMPLATE = `
             <i class="icon icon-16 icon-eye"></i>
             <span class="">${translate('View')}</span>
         </button>
-        <button class="edit-save button round" type="button" data-ref="save">
+        <button class="edit-save button round enabled-on-dirty" type="button" data-ref="save">
             <i class="icon icon-16 icon-save"></i>
             <i class="icon icon-16 icon-save-disabled"></i>
             <span hidden data-ref="saveLabel">${translate('Save')}</span>
@@ -275,10 +275,7 @@ export class EditBar extends WithTemplate {
     DomEvent.disableClickPropagation(this.element)
     this._onClick('marker', () => this._leafletMap.editTools.startMarker())
     this._onClick('polyline', () => this._leafletMap.editTools.startPolyline())
-    this._onClick('multiline', () => {
-      console.log('click click')
-      this._umap.editedFeature.ui.editor.newShape()
-    })
+    this._onClick('multiline', () => this._umap.editedFeature.ui.editor.newShape())
     this._onClick('polygon', () => this._leafletMap.editTools.startPolygon())
     this._onClick('multipolygon', () => this._umap.editedFeature.ui.editor.newShape())
     this._onClick('caption', () => this._umap.editCaption())
diff --git a/umap/static/umap/js/modules/umap.js b/umap/static/umap/js/modules/umap.js
index 6e9ca6d8..20b5d9a4 100644
--- a/umap/static/umap/js/modules/umap.js
+++ b/umap/static/umap/js/modules/umap.js
@@ -96,6 +96,10 @@ export default class Umap extends ServerStored {
         this._leafletMap.latLng(center)
     }
 
+    // Needed for permissions
+    this.syncEngine = new SyncEngine(this)
+    this.sync = this.syncEngine.proxy(this)
+
     // Needed to render controls
     this.permissions = new MapPermissions(this)
     this.urls = new URLs(this.properties.urls)
@@ -130,9 +134,6 @@ export default class Umap extends ServerStored {
     this.share = new Share(this)
     this.rules = new Rules(this)
 
-    this.syncEngine = new SyncEngine(this)
-    this.sync = this.syncEngine.proxy(this)
-
     if (this.hasEditMode()) {
       this.editPanel = new EditPanel(this, this._leafletMap)
       this.fullPanel = new FullPanel(this, this._leafletMap)
@@ -606,7 +607,6 @@ export default class Umap extends ServerStored {
   }
 
   createDataLayer(options = {}, sync = true) {
-    console.log('createDatalayer', options)
     options.name =
       options.name || `${translate('Layer')} ${this.datalayersIndex.length + 1}`
     const datalayer = new DataLayer(this, this._leafletMap, options)
@@ -1269,7 +1269,7 @@ export default class Umap extends ServerStored {
   }
 
   disableEdit() {
-    if (this.isDirty) return
+    // if (this.isDirty) return
     this.drop.disable()
     document.body.classList.remove('umap-edit-enabled')
     this.editedFeature = null
@@ -1297,7 +1297,6 @@ export default class Umap extends ServerStored {
 
   getSyncMetadata() {
     return {
-      engine: this.sync,
       subject: 'map',
     }
   }
@@ -1495,7 +1494,7 @@ export default class Umap extends ServerStored {
       const form = builder.build()
       row.appendChild(form)
       row.classList.toggle('off', !datalayer.isVisible())
-      row.dataset.id = stamp(datalayer)
+      row.dataset.id = datalayer.id
     })
     const onReorder = (src, dst, initialIndex, finalIndex) => {
       const movedLayer = this.datalayers[src.dataset.id]
@@ -1526,7 +1525,7 @@ export default class Umap extends ServerStored {
   }
 
   getDataLayerByUmapId(id) {
-    const datalayer = this.findDataLayer((d) => d.id === id)
+    const datalayer = this.datalayers[id]
     if (!datalayer) throw new Error(`Can't find datalayer with id ${id}`)
     return datalayer
   }
diff --git a/umap/static/umap/js/umap.controls.js b/umap/static/umap/js/umap.controls.js
index 7ddecd1e..9dbddbac 100644
--- a/umap/static/umap/js/umap.controls.js
+++ b/umap/static/umap/js/umap.controls.js
@@ -740,7 +740,6 @@ U.Editable = L.Editable.extend({
       // (eg. line has only one drawn point)
       // So let's check if the layer has no more shape
       event.layer.feature.pullGeometry(false)
-      console.log('onEscape', event.layer.feature.hasGeom())
       if (!event.layer.feature.hasGeom()) {
         event.layer.feature.del()
       } else {
diff --git a/umap/sync/payloads.py b/umap/sync/payloads.py
index 2652578b..319eb2e2 100644
--- a/umap/sync/payloads.py
+++ b/umap/sync/payloads.py
@@ -15,7 +15,7 @@ class OperationMessage(BaseModel):
 
     kind: Literal["OperationMessage"] = "OperationMessage"
     verb: Literal["upsert", "update", "delete", "batch"]
-    subject: Literal["map", "datalayer", "feature", "batch"]
+    subject: Literal["map", "datalayer", "feature"]
     metadata: Optional[dict] = None
     key: Optional[str] = None
     operations: Optional[List] = None
diff --git a/umap/tests/integration/test_edit_datalayer.py b/umap/tests/integration/test_edit_datalayer.py
index ccd1fcec..bd06ca87 100644
--- a/umap/tests/integration/test_edit_datalayer.py
+++ b/umap/tests/integration/test_edit_datalayer.py
@@ -159,7 +159,6 @@ def test_can_create_new_datalayer(live_server, openmap, page, datalayer):
     page.locator('input[name="name"]').click()
     page.locator('input[name="name"]').fill("Layer A with a new name")
     expect(page.get_by_text("Layer A with a new name")).to_be_visible()
-    page.get_by_role("button", name="Save").click()
     with page.expect_response(re.compile(".*/datalayer/update/.*")):
         page.get_by_role("button", name="Save").click()
     assert DataLayer.objects.count() == 2
diff --git a/umap/tests/integration/test_optimistic_merge.py b/umap/tests/integration/test_optimistic_merge.py
index 16dad23f..fc7c0bd0 100644
--- a/umap/tests/integration/test_optimistic_merge.py
+++ b/umap/tests/integration/test_optimistic_merge.py
@@ -292,9 +292,10 @@ def test_should_display_alert_on_conflict(context, live_server, datalayer, openm
     # Change name on page two and save
     page_two.locator(".leaflet-marker-icon").click(modifiers=["Shift"])
     page_two.locator('input[name="name"]').fill("name from page two")
+    page_two.wait_for_timeout(300)  # Time for the input debounce.
 
     # Map should be in dirty status
-    expect(page_two.get_by_text("Undo")).to_be_visible()
+    expect(page_two.get_by_text("Save", exact=True)).to_be_enabled()
     with page_two.expect_response(re.compile(r".*/datalayer/update/.*")):
         page_two.get_by_role("button", name="Save").click()
 
@@ -306,7 +307,7 @@ def test_should_display_alert_on_conflict(context, live_server, datalayer, openm
     # We should have an alert with some actions
     expect(page_two.get_by_text("Whoops! Other contributor(s) changed")).to_be_visible()
     # Map should still be in dirty status
-    expect(page_two.get_by_text("Undo")).to_be_visible()
+    expect(page_two.get_by_text("Save", exact=True)).to_be_enabled()
 
     # Override data from page two
     with page_two.expect_response(re.compile(r".*/datalayer/update/.*")):
@@ -317,4 +318,4 @@ def test_should_display_alert_on_conflict(context, live_server, datalayer, openm
     data = json.loads(Path(saved.geojson.path).read_text())
     assert data["features"][0]["properties"]["name"] == "name from page two"
     # Map should not be in dirty status anymore
-    expect(page_two.get_by_text("Undo")).to_be_hidden()
+    expect(page_two.get_by_text("Save", exact=True)).to_be_disabled()
diff --git a/umap/tests/integration/test_save.py b/umap/tests/integration/test_save.py
index 866de812..c885d094 100644
--- a/umap/tests/integration/test_save.py
+++ b/umap/tests/integration/test_save.py
@@ -18,14 +18,13 @@ def test_reseting_map_would_remove_from_save_queue(
     page.locator('input[name="name"]').fill("new name")
     page.get_by_role("button", name="Undo").click()
     page.wait_for_timeout(500)
-    page.get_by_role("button", name="Edit").click()
     page.get_by_role("button", name="Manage layers").click()
     page.get_by_role("button", name="Edit", exact=True).click()
     page.locator('input[name="name"]').click()
     page.locator('input[name="name"]').fill("new datalayer name")
     page.wait_for_timeout(300)  # Time of the Input debounce
     with page.expect_response(re.compile(".*/datalayer/update/.*")):
-        page.get_by_role("button", name="Save").click()
+        page.get_by_role("button", name="Save", exact=True).click()
     assert len(requests) == 1
     assert requests == [
         (
diff --git a/umap/tests/integration/test_undo_redo.py b/umap/tests/integration/test_undo_redo.py
index ab3f48ba..bbe738eb 100644
--- a/umap/tests/integration/test_undo_redo.py
+++ b/umap/tests/integration/test_undo_redo.py
@@ -90,8 +90,8 @@ def test_can_undo_redo_layer_color_change(
 ):
     page.goto(f"{live_server.url}{map_with_polygon.get_absolute_url()}?edit")
 
-    expect(page.locator(".edit-undo")).to_be_hidden()
-    expect(page.locator(".edit-redo")).to_be_hidden()
+    expect(page.locator(".edit-undo")).to_be_disabled()
+    expect(page.locator(".edit-redo")).to_be_disabled()
     page.get_by_role("button", name="Manage layers").click()
     page.locator(".panel").get_by_title("Edit", exact=True).click()
     page.get_by_text("Shape properties").click()