chore: introduce SaveManager class (#2240)

The last refactor of the save process
(0fdb70ce66) has introduced a bug: the
save flow was not more stopped when a layer fail to save. This ends with
a broken UI, as the map is not dirty anymore, so the save button is not
active anymore, while at least one layer still needs to be saved.

Save can fail in two scenarios:
  - there is a conflict (status 412)
  - the server is down or as an issue (eg. disk is full)

I tried a more modest scenario (listening for status in map.save and
recallign the map save after a conflict), but this ended with calling
map.save uselessly even more.

So I decided to try this refactor, which also totally remove the useless
map.save we were sending at each save until now.

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

@@ -20,6 +20,7 @@ import { translate } from '../i18n.js'
 import { DataLayerPermissions } from '../permissions.js'
 import { Point, LineString, Polygon } from './features.js'
 import TableEditor from '../tableeditor.js'
+import { ServerStored } from '../saving.js'
 
 export const LAYER_TYPES = [
   DefaultLayer,
@@ -35,8 +36,9 @@ const LAYER_MAP = LAYER_TYPES.reduce((acc, klass) => {
   return acc
 }, {})
 
-export class DataLayer {
+export class DataLayer extends ServerStored {
   constructor(map, data) {
+    super()
     this.map = map
     this.sync = map.sync_engine.proxy(this)
     this._index = Array()
@@ -58,7 +60,6 @@ export class DataLayer {
       editMode: 'advanced',
     }
 
-    this._isDirty = false
     this._isDeleted = false
     this.setUmapId(data.id)
     this.setOptions(data)
@@ -89,23 +90,16 @@ export class DataLayer {
     if (this.isVisible()) this.propagateShow()
   }
 
-  set isDirty(status) {
+  onDirty(status) {
-    this._isDirty = status
     if (status) {
-      this.map.isDirty = true
       // A layer can be made dirty by indirect action (like dragging layers)
       // we need to have it loaded before saving it.
       if (!this.isLoaded()) this.fetchData()
     } else {
-      this.map.checkDirty()
       this.isDeleted = false
     }
   }
 
-  get isDirty() {
-    return this._isDirty
-  }
-
   set isDeleted(status) {
     this._isDeleted = status
     if (status) this.isDirty = status
@@ -582,7 +576,6 @@ export class DataLayer {
 
   reset() {
     if (!this.umap_id) this.erase()
-
     this.resetOptions()
     this.parentPane.appendChild(this.pane)
     if (this._leaflet_events_bk && !this._leaflet_events) {
@@ -1057,8 +1050,9 @@ export class DataLayer {
     const headers = this._reference_version
       ? { 'X-Datalayer-Reference': this._reference_version }
       : {}
-    await this._trySave(saveUrl, headers, formData)
+    const status = await this._trySave(saveUrl, headers, formData)
     this._geojson = geojson
+    return status
   }
 
   async _trySave(url, headers, formData) {
@@ -1071,7 +1065,15 @@ export class DataLayer {
               'This situation is tricky, you have to choose carefully which version is pertinent.'
           ),
           async () => {
-            await this._trySave(url, {}, formData)
+            // Save again this layer
+            const status = await this._trySave(url, {}, formData)
+            if (status) {
+              this.isDirty = false
+
+              // Call the main save, in case something else needs to be saved
+              // as the conflict stopped the saving flow
+              await this.map.saveAll()
+            }
           }
         )
       }
@@ -1089,11 +1091,11 @@ export class DataLayer {
       this.setUmapId(data.id)
       this.updateOptions(data)
       this.backupOptions()
+      this.backupData()
       this.connectToMap()
       this._loaded = true
       this.redraw() // Needed for reordering features
-      this.isDirty = false
+      return true
-      this.permissions.save()
     }
   }
 
@@ -1102,7 +1104,7 @@ export class DataLayer {
       await this.map.server.post(this.getDeleteUrl())
     }
     delete this.map.datalayers[stamp(this)]
-    this.isDirty = false
+    return true
   }
 
   getMap() {
@@ -1130,7 +1132,9 @@ export class DataLayer {
   }
 
   renderLegend() {
-    for (const container of document.querySelectorAll(`.${this.cssId} .datalayer-legend`)) {
+    for (const container of document.querySelectorAll(
+      `.${this.cssId} .datalayer-legend`
+    )) {
       container.innerHTML = ''
       if (this.layer.renderLegend) return this.layer.renderLegend(container)
       const color = DomUtil.create('span', 'datalayer-color', container)

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

@@ -31,6 +31,7 @@ import { DataLayer, LAYER_TYPES } from './data/layer.js'
 import { DataLayerPermissions, MapPermissions } from './permissions.js'
 import { Point, LineString, Polygon } from './data/features.js'
 import { LeafletMarker, LeafletPolyline, LeafletPolygon } from './rendering/ui.js'
+import * as SAVEMANAGER from './saving.js'
 
 // Import modules and export them to the global scope.
 // For the not yet module-compatible JS out there.
@@ -70,6 +71,7 @@ window.U = {
   Request,
   RequestError,
   Rules,
+  SAVEMANAGER,
   SCHEMA,
   ServerRequest,
   Share,

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

@@ -1,26 +1,19 @@
 import { DomUtil } from '../../vendors/leaflet/leaflet-src.esm.js'
 import { translate } from './i18n.js'
 import { uMapAlert as Alert } from '../components/alerts/alert.js'
+import { ServerStored } from './saving.js'
 import * as Utils from './utils.js'
 
 // Dedicated object so we can deal with a separate dirty status, and thus
 // call the endpoint only when needed, saving one call at each save.
-export class MapPermissions {
+export class MapPermissions extends ServerStored {
   constructor(map) {
+    super()
     this.setOptions(map.options.permissions)
     this.map = map
     this._isDirty = false
   }
 
-  set isDirty(status) {
-    this._isDirty = status
-    if (status) this.map.isDirty = status
-  }
-
-  get isDirty() {
-    return this._isDirty
-  }
-
   setOptions(options) {
     this.options = Object.assign(
       {
@@ -200,8 +193,8 @@ export class MapPermissions {
     )
     if (!error) {
       this.commit()
-      this.isDirty = false
       this.map.fire('postsync')
+      return true
     }
   }
 
@@ -233,8 +226,9 @@ export class MapPermissions {
   }
 }
 
-export class DataLayerPermissions {
+export class DataLayerPermissions extends ServerStored {
   constructor(datalayer) {
+    super()
     this.options = Object.assign(
       {
         edit_status: null,
@@ -243,16 +237,6 @@ export class DataLayerPermissions {
     )
 
     this.datalayer = datalayer
-    this._isDirty = false
-  }
-
-  set isDirty(status) {
-    this._isDirty = status
-    if (status) this.datalayer.isDirty = status
-  }
-
-  get isDirty() {
-    return this._isDirty
   }
 
   get map() {
@@ -297,12 +281,13 @@ export class DataLayerPermissions {
     )
     if (!error) {
       this.commit()
-      this.isDirty = false
+      return true
     }
   }
 
   commit() {
     this.datalayer.options.permissions = Object.assign(
+      {},
       this.datalayer.options.permissions,
       this.options
     )

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

@@ -0,0 +1,48 @@
+const _queue = new Set()
+
+export let isDirty = false
+
+export async function save() {
+  for (const obj of _queue) {
+    const ok = await obj.save()
+    if (!ok) break
+    remove(obj)
+  }
+}
+
+export function add(obj) {
+  _queue.add(obj)
+  _onUpdate()
+}
+
+export function remove(obj) {
+  _queue.delete(obj)
+  _onUpdate()
+}
+
+export function has(obj) {
+  return _queue.has(obj)
+}
+
+function _onUpdate() {
+  console.log(_queue)
+  isDirty = Boolean(_queue.size)
+  document.body.classList.toggle('umap-is-dirty', isDirty)
+}
+
+export class ServerStored {
+  set isDirty(status) {
+    if (status) {
+      add(this)
+    } else {
+      remove(this)
+    }
+    this.onDirty(status)
+  }
+
+  get isDirty() {
+    return has(this)
+  }
+
+  onDirty(status) {}
+}

umap/static/umap/js/umap.controls.js

@@ -734,7 +734,7 @@ const ControlsMixin = {
       'leaflet-control-edit-save button',
       rightContainer,
       L.DomUtil.add('span', '', null, L._('Save')),
-      this.save,
+      this.saveAll,
       this
     )
     L.DomEvent.on(

umap/static/umap/js/umap.js

@@ -153,13 +153,17 @@ U.Map = L.Map.extend({
       this.options.onLoadPanel = 'datafilters'
     }
 
-    let isDirty = false // self status
+    // TODO: remove me when moved to modules
+    // and inheriting from ServerStored
     try {
       Object.defineProperty(this, 'isDirty', {
-        get: () => isDirty,
+        get: () => U.SAVEMANAGER.has(this),
-        set: function (status) {
+        set: (status) => {
-          isDirty = status
+          if (status) {
-          this.checkDirty()
+            U.SAVEMANAGER.add(this)
+          } else {
+            U.SAVEMANAGER.remove(this)
+          }
         },
       })
     } catch (e) {
@@ -200,7 +204,7 @@ U.Map = L.Map.extend({
       this.propagate()
     }
 
-    window.onbeforeunload = () => (this.editEnabled && this.isDirty) || null
+    window.onbeforeunload = () => (this.editEnabled && U.SAVEMANAGER.isDirty) || null
     this.backup()
   },
 
@@ -601,13 +605,13 @@ U.Map = L.Map.extend({
       let used = true
       switch (e.key) {
         case 'e':
-          if (!this.isDirty) this.disableEdit()
+          if (!U.SAVEMANAGER.isDirty) this.disableEdit()
           break
         case 's':
-          if (this.isDirty) this.save()
+          if (U.SAVEMANAGER.isDirty) this.saveAll()
           break
         case 'z':
-          if (this.isDirty) this.askForReset()
+          if (U.SAVEMANAGER.isDirty) this.askForReset()
           break
         case 'm':
           this.editTools.startMarker()
@@ -1015,10 +1019,6 @@ U.Map = L.Map.extend({
     this.onDataLayersChanged()
   },
 
-  checkDirty: function () {
-    this._container.classList.toggle('umap-is-dirty', this.isDirty)
-  },
-
   exportOptions: function () {
     const properties = {}
     for (const option of Object.keys(U.SCHEMA)) {
@@ -1029,7 +1029,7 @@ U.Map = L.Map.extend({
     return properties
   },
 
-  saveSelf: async function () {
+  save: async function () {
     this.rules.commit()
     const geojson = {
       type: 'Feature',
@@ -1048,7 +1048,7 @@ U.Map = L.Map.extend({
       return
     }
     if (data.login_required) {
-      window.onLogin = () => this.save()
+      window.onLogin = () => this.saveAll()
       window.open(data.login_required)
       return
     }
@@ -1093,21 +1093,11 @@ U.Map = L.Map.extend({
     return true
   },
 
-  save: async function () {
+  saveAll: async function () {
-    if (!this.isDirty) return
+    if (!U.SAVEMANAGER.isDirty) return
     if (this._default_extent) this._setCenterAndZoom()
     this.backup()
-    if (this.options.editMode === 'advanced') {
+    await U.SAVEMANAGER.save()
-      // Only save the map if the user has the rights to do so.
-      const ok = await this.saveSelf()
-      if (!ok) return
-    }
-    await this.permissions.save()
-    // Iter over all datalayers, including deleted.
-    for (const datalayer of Object.values(this.datalayers)) {
-      if (datalayer.isDirty) await datalayer.save()
-    }
-    this.isDirty = false
     // Do a blind render for now, as we are not sure what could
     // have changed, we'll be more subtil when we'll remove the
     // save action

umap/static/umap/map.css

@@ -596,7 +596,7 @@ ul.photon-autocomplete {
 }
 .umap-edit-enabled .leaflet-control-edit-save,
 .umap-edit-enabled .leaflet-control-edit-disable,
-.umap-edit-enabled .umap-is-dirty .leaflet-control-edit-cancel {
+.umap-edit-enabled.umap-is-dirty .leaflet-control-edit-cancel {
     display: inline-block;
 }
 .umap-is-dirty .leaflet-control-edit-disable {

umap/tests/integration/test_edit_datalayer.py

@@ -120,7 +120,7 @@ def test_can_change_name(live_server, openmap, page, datalayer):
     page.locator('input[name="name"]').press("Control+a")
     page.locator('input[name="name"]').fill("new name")
     expect(page.locator(".umap-browser .datalayer")).to_contain_text("new name")
-    expect(page.locator(".umap-is-dirty")).to_be_visible()
+    expect(page.locator("body")).to_have_class(re.compile(".*umap-is-dirty.*"))
     with page.expect_response(re.compile(".*/datalayer/update/.*")):
         page.get_by_role("button", name="Save").click()
     saved = DataLayer.objects.last()

umap/tests/integration/test_optimistic_merge.py

@@ -276,21 +276,38 @@ def test_should_display_alert_on_conflict(context, live_server, datalayer, openm
     page_two = context.new_page()
     page_two.goto(f"{live_server.url}{openmap.get_absolute_url()}?edit")
 
+    # Change name on page one and save
     page_one.locator(".leaflet-marker-icon").click(modifiers=["Shift"])
-    page_one.locator('input[name="name"]').fill("new name")
+    page_one.locator('input[name="name"]').fill("name from page one")
     with page_one.expect_response(re.compile(r".*/datalayer/update/.*")):
         page_one.get_by_role("button", name="Save").click()
 
+    # Change name on page two and save
     page_two.locator(".leaflet-marker-icon").click(modifiers=["Shift"])
-    page_two.locator('input[name="name"]').fill("custom name")
+    page_two.locator('input[name="name"]').fill("name from page two")
+
+    # Map should be in dirty status
+    expect(page_two.get_by_text("Cancel edits")).to_be_visible()
     with page_two.expect_response(re.compile(r".*/datalayer/update/.*")):
         page_two.get_by_role("button", name="Save").click()
+
+    # Make sure data is unchanged on the server
     saved = DataLayer.objects.last()
     data = json.loads(Path(saved.geojson.path).read_text())
-    assert data["features"][0]["properties"]["name"] == "new name"
+    assert data["features"][0]["properties"]["name"] == "name from page one"
+
+    # 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("Cancel edits")).to_be_visible()
+
+    # Override data from page two
     with page_two.expect_response(re.compile(r".*/datalayer/update/.*")):
         page_two.get_by_text("Keep your changes and loose theirs").click()
+
+    # Make sure server has page two data
     saved = DataLayer.objects.last()
     data = json.loads(Path(saved.geojson.path).read_text())
-    assert data["features"][0]["properties"]["name"] == "custom name"
+    assert data["features"][0]["properties"]["name"] == "name from page two"
+    # Map should not be in dirty status anymore
+    expect(page_two.get_by_text("Cancel edits")).to_be_hidden()

umap/tests/integration/test_save.py

@@ -0,0 +1,35 @@
+import re
+
+
+def test_reseting_map_would_remove_from_save_queue(
+    live_server, openmap, page, datalayer
+):
+    page.goto(f"{live_server.url}{openmap.get_absolute_url()}?edit")
+    page.get_by_role("link", name="Edit map name and caption").click()
+    requests = []
+
+    def register_request(request):
+        if request.url.endswith(".png"):
+            return
+        requests.append((request.method, request.url))
+
+    page.on("request", register_request)
+    page.locator('input[name="name"]').click()
+    page.locator('input[name="name"]').fill("new name")
+    page.get_by_role("button", name="Cancel edits").click()
+    page.get_by_role("button", name="OK").click()
+    page.wait_for_timeout(500)
+    page.get_by_role("button", name="Edit").click()
+    page.get_by_role("link", 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")
+    with page.expect_response(re.compile(".*/datalayer/update/.*")):
+        page.get_by_role("button", name="Save").click()
+    assert len(requests) == 1
+    assert requests == [
+        (
+            "POST",
+            f"{live_server.url}/en/map/{openmap.pk}/datalayer/update/{datalayer.pk}/",
+        ),
+    ]