wip: allow to undo/sync rules

When editing Rule(s), we are not editing the map data itself, but a
sort of proxy objects. This was done mainly because map.properties.rules
is an array of object, and at this time Leaflet.FormBuilder did not know
how to edit an array (something like properties.rules.0.condition).
Now that we integrated FormBuilder, it still does not know how to do this
but we could teach it, or find another way (real Proxy or use reference
to the original object in the Rule).

umap/static/umap/js/modules/form/builder.js

@@ -70,21 +70,7 @@ export class Form extends Utils.WithEvents {
   }
 
   setter(field, value) {
-    const path = field.split('.')
-    let obj = this.obj
-    let what
-    for (let i = 0, l = path.length; i < l; i++) {
-      what = path[i]
-      if (what === path[l - 1]) {
-        if (typeof value === 'undefined') {
-          delete obj[what]
-        } else {
-          obj[what] = value
-        }
-      } else {
-        obj = obj[what]
-      }
-    }
+    Utils.setObjectValue(this.obj, field, value)
   }
 
   restoreField(field) {
@@ -191,7 +177,11 @@ export class MutatingForm extends Form {
 
   setter(field, value) {
     const oldValue = this.getter(field)
+    if ('setter' in this.obj) {
+      this.obj.setter(field, value)
+    } else {
       super.setter(field, value)
+    }
     if ('render' in this.obj) {
       this.obj.render([field], this)
     }

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

@@ -17,20 +17,10 @@ class Rule {
     this.parse()
   }
 
-  get isDirty() {
-    return this._isDirty
-  }
-
-  set isDirty(status) {
-    this._isDirty = status
-    if (status) this._umap.isDirty = status
-  }
-
   constructor(umap, condition = '', options = {}) {
     // TODO make this public properties when browser coverage is ok
     // cf https://caniuse.com/?search=public%20class%20field
     this._condition = null
-    this._isDirty = false
     this.OPERATORS = [
       ['>', this.gt],
       ['<', this.lt],
@@ -190,17 +180,25 @@ class Rule {
 
   _delete() {
     this._umap.rules.rules = this._umap.rules.rules.filter((rule) => rule !== this)
+    this._umap.rules.commit()
+  }
+
+  setter(key, value) {
+    const oldRules = Utils.CopyJSON(this._umap.properties.rules || {})
+    Utils.setObjectValue(this, key, value)
+    this._umap.rules.commit()
+    this._umap.sync.update('properties.rules', this._umap.properties.rules, oldRules)
   }
 }
 
 export default class Rules {
   constructor(umap) {
     this._umap = umap
-    this.rules = []
     this.load()
   }
 
   load() {
+    this.rules = []
     if (!this._umap.properties.rules?.length) return
     for (const { condition, options } of this._umap.properties.rules) {
       if (!condition) continue
@@ -222,8 +220,8 @@ export default class Rules {
     else if (finalIndex > initialIndex) newIdx = referenceIdx
     else newIdx = referenceIdx + 1
     this.rules.splice(newIdx, 0, moved)
-    moved.isDirty = true
     this._umap.render(['rules'])
+    this.commit()
   }
 
   edit(container) {
@@ -242,7 +240,6 @@ export default class Rules {
 
   addRule() {
     const rule = new Rule(this._umap)
-    rule.isDirty = true
     this.rules.push(rule)
     rule.edit(map)
   }

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

@@ -1,4 +1,4 @@
-import { fieldInSchema } from '../utils.js'
+import * as Utils from '../utils.js'
 
 /**
  * Updaters are classes able to convert messages
@@ -10,27 +10,6 @@ class BaseUpdater {
     this._umap = umap
   }
 
-  updateObjectValue(obj, key, value) {
-    const parts = key.split('.')
-    const lastKey = parts.pop()
-
-    // Reduce the current list of attributes,
-    // to find the object to set the property onto
-    const objectToSet = parts.reduce((currentObj, part) => {
-      if (currentObj !== undefined && part in currentObj) return currentObj[part]
-    }, obj)
-
-    // In case the given path doesn't exist, stop here
-    if (objectToSet === undefined) return
-
-    // Set the value (or delete it)
-    if (typeof value === 'undefined') {
-      delete objectToSet[lastKey]
-    } else {
-      objectToSet[lastKey] = value
-    }
-  }
-
   getDataLayerFromID(layerId) {
     return this._umap.getDataLayerByUmapId(layerId)
   }
@@ -43,8 +22,8 @@ class BaseUpdater {
 
 export class MapUpdater extends BaseUpdater {
   update({ key, value }) {
-    if (fieldInSchema(key)) {
-      this.updateObjectValue(this._umap, key, value)
+    if (Utils.fieldInSchema(key)) {
+      Utils.setObjectValue(this._umap, key, value)
     }
 
     this._umap.onPropertiesUpdated([key])
@@ -73,8 +52,8 @@ export class DataLayerUpdater extends BaseUpdater {
 
   update({ key, metadata, value }) {
     const datalayer = this.getDataLayerFromID(metadata.id)
-    if (fieldInSchema(key)) {
-      this.updateObjectValue(datalayer, key, value)
+    if (Utils.fieldInSchema(key)) {
+      Utils.setObjectValue(datalayer, key, value)
     } else {
       console.debug(
         'Not applying update for datalayer because key is not in the schema',
@@ -127,7 +106,7 @@ export class FeatureUpdater extends BaseUpdater {
       const feature = this.getFeatureFromMetadata(metadata)
       feature.geometry = value
     } else {
-      this.updateObjectValue(feature, key, value)
+      Utils.setObjectValue(feature, key, value)
       feature.datalayer.indexProperties(feature)
     }
 
@@ -148,8 +127,8 @@ export class FeatureUpdater extends BaseUpdater {
 
 export class MapPermissionsUpdater extends BaseUpdater {
   update({ key, value }) {
-    if (fieldInSchema(key)) {
-      this.updateObjectValue(this._umap.permissions, key, value)
+    if (Utils.fieldInSchema(key)) {
+      Utils.setObjectValue(this._umap.permissions, key, value)
     }
   }
 
@@ -160,8 +139,8 @@ export class MapPermissionsUpdater extends BaseUpdater {
 
 export class DataLayerPermissionsUpdater extends BaseUpdater {
   update({ key, value, metadata }) {
-    if (fieldInSchema(key)) {
-      this.updateObjectValue(this.getDataLayerFromID(metadata.id), key, value)
+    if (Utils.fieldInSchema(key)) {
+      Utils.setObjectValue(this.getDataLayerFromID(metadata.id), key, value)
     }
   }
 

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

@@ -1162,7 +1162,6 @@ export default class Umap {
   }
 
   async save() {
-    this.rules.commit()
     const geojson = {
       type: 'Feature',
       geometry: this.geometry(),
@@ -1323,6 +1322,9 @@ export default class Umap {
           this.bottomBar.redraw()
           break
         case 'data':
+          if (fields.includes('properties.rules')) {
+            this.rules.load()
+          }
           this.eachVisibleDataLayer((datalayer) => {
             datalayer.redraw()
           })

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

@@ -484,6 +484,27 @@ export const debounce = (callback, wait) => {
   }
 }
 
+export function setObjectValue(obj, key, value) {
+  const parts = key.split('.')
+  const lastKey = parts.pop()
+
+  // Reduce the current list of attributes,
+  // to find the object to set the property onto
+  const objectToSet = parts.reduce((currentObj, part) => {
+    if (currentObj !== undefined && part in currentObj) return currentObj[part]
+  }, obj)
+
+  // In case the given path doesn't exist, stop here
+  if (objectToSet === undefined) return
+
+  // Set the value (or delete it)
+  if (typeof value === 'undefined') {
+    delete objectToSet[lastKey]
+  } else {
+    objectToSet[lastKey] = value
+  }
+}
+
 export const COLORS = [
   'Black',
   'Navy',

umap/static/umap/unittests/sync.js

@@ -49,63 +49,6 @@ describe('#dispatch', () => {
   })
 })
 
-describe('Updaters', () => {
-  describe('BaseUpdater', () => {
-    let updater
-    let map
-    let obj
-
-    beforeEach(() => {
-      map = {}
-      updater = new MapUpdater(map)
-      obj = {}
-    })
-    it('should be able to set object properties', () => {
-      let obj = {}
-      updater.updateObjectValue(obj, 'foo', 'foo')
-      expect(obj).deep.equal({ foo: 'foo' })
-    })
-
-    it('should be able to set object properties recursively on existing objects', () => {
-      let obj = { foo: {} }
-      updater.updateObjectValue(obj, 'foo.bar', 'foo')
-      expect(obj).deep.equal({ foo: { bar: 'foo' } })
-    })
-
-    it('should be able to set object properties recursively on deep objects', () => {
-      let obj = { foo: { bar: { baz: {} } } }
-      updater.updateObjectValue(obj, 'foo.bar.baz.test', 'value')
-      expect(obj).deep.equal({ foo: { bar: { baz: { test: 'value' } } } })
-    })
-
-    it('should be able to replace object properties recursively on deep objects', () => {
-      let obj = { foo: { bar: { baz: { test: 'test' } } } }
-      updater.updateObjectValue(obj, 'foo.bar.baz.test', 'value')
-      expect(obj).deep.equal({ foo: { bar: { baz: { test: 'value' } } } })
-    })
-
-    it('should not set object properties recursively on non-existing objects', () => {
-      let obj = { foo: {} }
-      updater.updateObjectValue(obj, 'bar.bar', 'value')
-
-      expect(obj).deep.equal({ foo: {} })
-    })
-
-    it('should delete keys for undefined values', () => {
-      let obj = { foo: 'foo' }
-      updater.updateObjectValue(obj, 'foo', undefined)
-
-      expect(obj).deep.equal({})
-    })
-
-    it('should delete keys for undefined values, recursively', () => {
-      let obj = { foo: { bar: 'bar' } }
-      updater.updateObjectValue(obj, 'foo.bar', undefined)
-
-      expect(obj).deep.equal({ foo: {} })
-    })
-  })
-})
 
 describe('Operations', () => {
   describe('haveSameContext', () => {

umap/static/umap/unittests/utils.js

@@ -862,4 +862,51 @@ describe('Utils', () => {
       assert.equal(Utils.isObject(''), false)
     })
   })
+
+  describe('setObjectValue', () => {
+    it('should be able to set object properties', () => {
+      let obj = {}
+      Utils.setObjectValue(obj, 'foo', 'foo')
+      expect(obj).deep.equal({ foo: 'foo' })
+    })
+
+    it('should be able to set object properties recursively on existing objects', () => {
+      let obj = { foo: {} }
+      Utils.setObjectValue(obj, 'foo.bar', 'foo')
+      expect(obj).deep.equal({ foo: { bar: 'foo' } })
+    })
+
+    it('should be able to set object properties recursively on deep objects', () => {
+      let obj = { foo: { bar: { baz: {} } } }
+      Utils.setObjectValue(obj, 'foo.bar.baz.test', 'value')
+      expect(obj).deep.equal({ foo: { bar: { baz: { test: 'value' } } } })
+    })
+
+    it('should be able to replace object properties recursively on deep objects', () => {
+      let obj = { foo: { bar: { baz: { test: 'test' } } } }
+      Utils.setObjectValue(obj, 'foo.bar.baz.test', 'value')
+      expect(obj).deep.equal({ foo: { bar: { baz: { test: 'value' } } } })
+    })
+
+    it('should not set object properties recursively on non-existing objects', () => {
+      let obj = { foo: {} }
+      Utils.setObjectValue(obj, 'bar.bar', 'value')
+
+      expect(obj).deep.equal({ foo: {} })
+    })
+
+    it('should delete keys for undefined values', () => {
+      let obj = { foo: 'foo' }
+      Utils.setObjectValue(obj, 'foo', undefined)
+
+      expect(obj).deep.equal({})
+    })
+
+    it('should delete keys for undefined values, recursively', () => {
+      let obj = { foo: { bar: 'bar' } }
+      Utils.setObjectValue(obj, 'foo.bar', undefined)
+
+      expect(obj).deep.equal({ foo: {} })
+    })
+  })
 })

umap/tests/integration/test_conditional_rules.py

@@ -261,6 +261,9 @@ def test_can_create_new_rule(live_server, page, openmap):
     page.get_by_title("AliceBlue").first.click()
     colors = getColors(markers)
     assert colors.count("rgb(240, 248, 255)") == 3
+    page.get_by_role("button", name="Undo").click()
+    colors = getColors(markers)
+    assert colors.count("rgb(240, 248, 255)") == 0
 
 
 def test_can_deactive_rule_from_list(live_server, page, openmap):