From 7dadb83ff5c970c776a8565a1e4af6204ff258c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20M=C3=A9taireau?= Date: Tue, 22 Oct 2024 16:17:34 +0200 Subject: [PATCH] feat(sync): Let the clients set layers UUID This make it possible to synchronize datalayers before their creation on the server, allowing at the same time to solve issues related to them not being saved (e.g. duplication of geometries) We use the DataLayer._referenceVersion to track if a DataLayer has been saved on the server. After a save, the _referenceVersion is synched with other peers. To pass the reference version from the server to the frontend, we have two options: - use a header - populate the `options.version` field In the case of a GET on a Datalayer, we could not use the `options` scenario because: - the value in the file is not up to date (it was the value the client has before the save) - the python cannot change it on the fly, as the file is served by nginx So we decided to keep using a header. But on the map view, we load all datalayers metadatas in the map options, so here we cannot use the header scenario, so in this specific case we had to populate `options._referenceVersion`. At the same time, we also changed: - Umap.options.umap_id => Umap.id - DataLayer.umap_id => Datalayer.id - fixed the version number returned by DataLayer.version_metadata --- umap/migrations/0023_alter_datalayer_uuid.py | 19 ++++ umap/models.py | 22 +++-- umap/static/umap/js/modules/data/features.js | 2 +- umap/static/umap/js/modules/data/layer.js | 96 +++++++++++-------- umap/static/umap/js/modules/permissions.js | 10 +- umap/static/umap/js/modules/schema.js | 5 + umap/static/umap/js/modules/share.js | 6 +- umap/static/umap/js/modules/sync/updaters.js | 5 +- umap/static/umap/js/modules/umap.js | 34 ++++--- umap/static/umap/js/modules/urls.js | 4 +- umap/static/umap/unittests/URLs.js | 30 +++--- umap/tests/base.py | 2 + umap/tests/integration/test_edit_datalayer.py | 23 +++-- umap/tests/integration/test_import.py | 6 +- .../integration/test_optimistic_merge.py | 14 ++- umap/tests/integration/test_websocket_sync.py | 76 ++++++++++++++- umap/tests/test_datalayer_views.py | 16 ++++ umap/urls.py | 4 +- umap/views.py | 22 ++++- 19 files changed, 282 insertions(+), 114 deletions(-) create mode 100644 umap/migrations/0023_alter_datalayer_uuid.py diff --git a/umap/migrations/0023_alter_datalayer_uuid.py b/umap/migrations/0023_alter_datalayer_uuid.py new file mode 100644 index 00000000..df537554 --- /dev/null +++ b/umap/migrations/0023_alter_datalayer_uuid.py @@ -0,0 +1,19 @@ +# Generated by Django 5.1.2 on 2024-11-15 11:03 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("umap", "0022_add_team"), + ] + + operations = [ + migrations.AlterField( + model_name="datalayer", + name="uuid", + field=models.UUIDField( + editable=False, primary_key=True, serialize=False, unique=True + ), + ), + ] diff --git a/umap/models.py b/umap/models.py index 27bc9aae..c7e1d388 100644 --- a/umap/models.py +++ b/umap/models.py @@ -250,7 +250,7 @@ class Map(NamedModel): "hash": False, "scrollWheelZoom": False, "noControl": True, - "umap_id": self.pk, + "id": self.pk, "schema": self.extra_schema, "slideshow": {}, } @@ -444,9 +444,7 @@ class DataLayer(NamedModel): (COLLABORATORS, _("Editors and team only")), (OWNER, _("Owner only")), ) - uuid = models.UUIDField( - unique=True, primary_key=True, default=uuid.uuid4, editable=False - ) + uuid = models.UUIDField(unique=True, primary_key=True, editable=False) old_id = models.IntegerField(null=True, blank=True) map = models.ForeignKey(Map, on_delete=models.CASCADE) description = models.TextField(blank=True, null=True, verbose_name=_("description")) @@ -525,12 +523,13 @@ class DataLayer(NamedModel): obj["id"] = self.pk obj["permissions"] = {"edit_status": self.edit_status} obj["editMode"] = "advanced" if self.can_edit(request) else "disabled" + obj["_referenceVersion"] = self.reference_version return obj def clone(self, map_inst=None): new = self.__class__.objects.get(pk=self.pk) new._state.adding = True - new.pk = None + new.pk = uuid.uuid4() if map_inst: new.map = map_inst new.geojson = File(new.geojson.file.file) @@ -543,11 +542,20 @@ class DataLayer(NamedModel): valid_prefixes.append(name.startswith("%s_" % self.old_id)) return any(valid_prefixes) and name.endswith(".geojson") + def extract_version_number(self, path): + version = path.split(".")[0] + if "_" in version: + return version.split("_")[-1] + return version + + @property + def reference_version(self): + return self.extract_version_number(self.geojson.path) + def version_metadata(self, name): - els = name.split(".")[0].split("_") return { "name": name, - "at": els[1], + "at": self.extract_version_number(name), "size": self.geojson.storage.size(self.get_version_path(name)), } diff --git a/umap/static/umap/js/modules/data/features.js b/umap/static/umap/js/modules/data/features.js index 894b3a3a..76702d62 100644 --- a/umap/static/umap/js/modules/data/features.js +++ b/umap/static/umap/js/modules/data/features.js @@ -136,7 +136,7 @@ class Feature { subject: 'feature', metadata: { id: this.id, - layerId: this.datalayer?.umap_id || null, + layerId: this.datalayer.id, featureType: this.getClassName(), }, } diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js index 6ac45029..cdb2c9ee 100644 --- a/umap/static/umap/js/modules/data/layer.js +++ b/umap/static/umap/js/modules/data/layer.js @@ -38,7 +38,7 @@ const LAYER_MAP = LAYER_TYPES.reduce((acc, klass) => { }, {}) export class DataLayer extends ServerStored { - constructor(umap, leafletMap, data) { + constructor(umap, leafletMap, data = {}) { super() this._umap = umap this.sync = umap.sync_engine.proxy(this) @@ -51,10 +51,7 @@ 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 = 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 }) @@ -66,7 +63,11 @@ export class DataLayer extends ServerStored { } this._isDeleted = false - this.setUmapId(data.id) + this._referenceVersion = data._referenceVersion + // Do not save it later. + delete data._referenceVersion + data.id = data.id || crypto.randomUUID() + this.setOptions(data) if (!Utils.isObject(this.options.remoteData)) { @@ -84,7 +85,8 @@ export class DataLayer extends ServerStored { this.backupOptions() this.connectToMap() this.permissions = new DataLayerPermissions(this._umap, this) - if (!this.umap_id) { + + if (!this.createdOnServer) { if (this.showAtLoad()) this.show() this.isDirty = true } @@ -95,6 +97,14 @@ export class DataLayer extends ServerStored { if (this.isVisible()) this.propagateShow() } + get id() { + return this.options.id + } + + get createdOnServer() { + return Boolean(this._referenceVersion) + } + onDirty(status) { if (status) { // A layer can be made dirty by indirect action (like dragging layers) @@ -121,9 +131,7 @@ export class DataLayer extends ServerStored { getSyncMetadata() { return { subject: 'datalayer', - metadata: { - id: this.umap_id || null, - }, + metadata: { id: this.id }, } } @@ -160,7 +168,7 @@ export class DataLayer extends ServerStored { autoLoaded() { if (!this._umap.datalayersFromQueryString) return this.options.displayOnLoad const datalayerIds = this._umap.datalayersFromQueryString - let loadMe = datalayerIds.includes(this.umap_id.toString()) + let loadMe = datalayerIds.includes(this.id.toString()) if (this.options.old_id) { loadMe = loadMe || datalayerIds.includes(this.options.old_id.toString()) } @@ -215,13 +223,13 @@ export class DataLayer extends ServerStored { } async fetchData() { - if (!this.umap_id) return + if (!this.createdOnServer) return if (this._loading) return this._loading = true const [geojson, response, error] = await this._umap.server.get(this._dataUrl()) if (!error) { - this._reference_version = response.headers.get('X-Datalayer-Version') - // FIXME: for now this property is set dynamically from backend + this.setReferenceVersion({ response, sync: false }) + // FIXME: for now the _umap_options property is set dynamically from backend // And thus it's not in the geojson file in the server // So do not let all options to be reset // Fix is a proper migration so all datalayers settings are @@ -257,6 +265,7 @@ export class DataLayer extends ServerStored { async fromUmapGeoJSON(geojson) { if (geojson._storage) geojson._umap_options = geojson._storage // Retrocompat + geojson._umap_options.id = this.id if (geojson._umap_options) this.setOptions(geojson._umap_options) if (this.isRemoteLayer()) await this.fetchRemoteData() else this.fromGeoJSON(geojson, false) @@ -313,18 +322,13 @@ export class DataLayer extends ServerStored { } isLoaded() { - return !this.umap_id || this._loaded + return !this.createdOnServer || this._loaded } hasDataLoaded() { return this._dataloaded } - setUmapId(id) { - // Datalayer is null when listening creation form - if (!this.umap_id && id) this.umap_id = id - } - backupOptions() { this._backupOptions = Utils.CopyJSON(this.options) } @@ -357,8 +361,8 @@ export class DataLayer extends ServerStored { _dataUrl() { let url = this._umap.urls.get('datalayer_view', { - pk: this.umap_id, - map_id: this._umap.properties.umap_id, + pk: this.id, + map_id: this._umap.id, }) // No browser cache for owners/editors. @@ -437,7 +441,7 @@ export class DataLayer extends ServerStored { // otherwise the layer becomes uneditable. this.makeFeatures(geojson, sync) } catch (err) { - console.log('Error with DataLayer', this.umap_id) + console.log('Error with DataLayer', this.id) console.error(err) } } @@ -524,22 +528,22 @@ export class DataLayer extends ServerStored { getDeleteUrl() { return this._umap.urls.get('datalayer_delete', { - pk: this.umap_id, - map_id: this._umap.properties.umap_id, + pk: this.id, + map_id: this._umap.id, }) } getVersionsUrl() { return this._umap.urls.get('datalayer_versions', { - pk: this.umap_id, - map_id: this._umap.properties.umap_id, + pk: this.id, + map_id: this._umap.id, }) } getVersionUrl(name) { return this._umap.urls.get('datalayer_version', { - pk: this.umap_id, - map_id: this._umap.properties.umap_id, + pk: this.id, + map_id: this._umap.id, name: name, }) } @@ -579,7 +583,8 @@ export class DataLayer extends ServerStored { } reset() { - if (!this.umap_id) this.erase() + if (!this.createdOnServer) this.erase() + this.resetOptions() this.parentPane.appendChild(this.pane) if (this._leaflet_events_bk && !this._leaflet_events) { @@ -809,7 +814,7 @@ export class DataLayer extends ServerStored { }, this ) - if (this.umap_id) { + if (this.createdOnServer) { const filename = `${Utils.slugify(this.options.name)}.geojson` const download = Utils.loadTemplate(` @@ -1034,6 +1039,11 @@ export class DataLayer extends ServerStored { return this.isReadOnly() || this.isRemoteLayer() } + setReferenceVersion({ response, sync }) { + this._referenceVersion = response.headers.get('X-Datalayer-Version') + this.sync.update('_referenceVersion', this._referenceVersion) + } + async save() { if (this.isDeleted) return await this.saveDelete() if (!this.isLoaded()) { @@ -1048,14 +1058,15 @@ export class DataLayer extends ServerStored { // Filename support is shaky, don't do it for now. const blob = new Blob([JSON.stringify(geojson)], { type: 'application/json' }) formData.append('geojson', blob) - const saveUrl = this._umap.urls.get('datalayer_save', { - map_id: this._umap.properties.umap_id, - pk: this.umap_id, + const saveURL = this._umap.urls.get('datalayer_save', { + map_id: this._umap.id, + pk: this.id, + created: this.createdOnServer, }) - const headers = this._reference_version - ? { 'X-Datalayer-Reference': this._reference_version } + const headers = this._referenceVersion + ? { 'X-Datalayer-Reference': this._referenceVersion } : {} - const status = await this._trySave(saveUrl, headers, formData) + const status = await this._trySave(saveURL, headers, formData) this._geojson = geojson return status } @@ -1090,11 +1101,12 @@ export class DataLayer extends ServerStored { this.fromGeoJSON(data.geojson) delete data.geojson } - this._reference_version = response.headers.get('X-Datalayer-Version') - this.sync.update('_reference_version', this._reference_version) - - this.setUmapId(data.id) + delete data.id + delete data._referenceVersion this.updateOptions(data) + + this.setReferenceVersion({ response, sync: true }) + this.backupOptions() this.backupData() this.connectToMap() @@ -1105,7 +1117,7 @@ export class DataLayer extends ServerStored { } async saveDelete() { - if (this.umap_id) { + if (this.createdOnServer) { await this._umap.server.post(this.getDeleteUrl()) } delete this._umap.datalayers[stamp(this)] diff --git a/umap/static/umap/js/modules/permissions.js b/umap/static/umap/js/modules/permissions.js index 020ba0f7..5d2ece72 100644 --- a/umap/static/umap/js/modules/permissions.js +++ b/umap/static/umap/js/modules/permissions.js @@ -149,7 +149,7 @@ export class MapPermissions extends ServerStored { edit() { if (this._umap.properties.editMode !== 'advanced') return - if (!this._umap.properties.umap_id) { + if (!this._umap.id) { Alert.info(translate('Please save the map first')) return } @@ -199,13 +199,13 @@ export class MapPermissions extends ServerStored { getUrl() { return this._umap.urls.get('map_update_permissions', { - map_id: this._umap.properties.umap_id, + map_id: this._umap.id, }) } getAttachUrl() { return this._umap.urls.get('map_attach_owner', { - map_id: this._umap.properties.umap_id, + map_id: this._umap.id, }) } @@ -262,8 +262,8 @@ export class DataLayerPermissions extends ServerStored { getUrl() { return this._umap.urls.get('datalayer_permissions', { - map_id: this._umap.properties.umap_id, - pk: this.datalayer.umap_id, + map_id: this._umap.id, + pk: this.datalayer.id, }) } diff --git a/umap/static/umap/js/modules/schema.js b/umap/static/umap/js/modules/schema.js index f2dab903..6ac145e4 100644 --- a/umap/static/umap/js/modules/schema.js +++ b/umap/static/umap/js/modules/schema.js @@ -563,4 +563,9 @@ export const SCHEMA = { type: Object, impacts: ['data'], }, + + _referenceVersion: { + type: Number, + impacts: ['data'], + }, } diff --git a/umap/static/umap/js/modules/share.js b/umap/static/umap/js/modules/share.js index 130ec460..91965679 100644 --- a/umap/static/umap/js/modules/share.js +++ b/umap/static/umap/js/modules/share.js @@ -61,7 +61,7 @@ export default class Share { translate('All data and settings of the map') ) const downloadUrl = this._umap.urls.get('map_download', { - map_id: this._umap.properties.umap_id, + map_id: this._umap.id, }) const link = Utils.loadTemplate(`
@@ -205,8 +205,8 @@ class IframeExporter { } if (this.options.keepCurrentDatalayers) { this._umap.eachDataLayer((datalayer) => { - if (datalayer.isVisible() && datalayer.umap_id) { - datalayers.push(datalayer.umap_id) + if (datalayer.isVisible() && datalayer.createdOnServer) { + datalayers.push(datalayer.id) } }) this.queryString.datalayers = datalayers.join(',') diff --git a/umap/static/umap/js/modules/sync/updaters.js b/umap/static/umap/js/modules/sync/updaters.js index a38975e9..bcc311d1 100644 --- a/umap/static/umap/js/modules/sync/updaters.js +++ b/umap/static/umap/js/modules/sync/updaters.js @@ -32,8 +32,7 @@ class BaseUpdater { } getDataLayerFromID(layerId) { - if (layerId) return this._umap.getDataLayerByUmapId(layerId) - return this._umap.defaultEditDataLayer() + return this._umap.getDataLayerByUmapId(layerId) } applyMessage(payload) { @@ -54,7 +53,7 @@ export class MapUpdater extends BaseUpdater { export class DataLayerUpdater extends BaseUpdater { upsert({ value }) { - // Inserts does not happen (we use multiple updates instead). + // Upsert only happens when a new datalayer is created. this._umap.createDataLayer(value, false) this._umap.render([]) } diff --git a/umap/static/umap/js/modules/umap.js b/umap/static/umap/js/modules/umap.js index 688bbe73..b8a03f2e 100644 --- a/umap/static/umap/js/modules/umap.js +++ b/umap/static/umap/js/modules/umap.js @@ -43,6 +43,10 @@ export default class Umap extends ServerStored { this.init(element, geojson) } + get id() { + return this.properties.id + } + async init(element, geojson) { this.properties = Object.assign( { @@ -166,7 +170,7 @@ export default class Umap extends ServerStored { } // Creation mode - if (!this.properties.umap_id) { + if (!this.id) { if (!this.properties.preview) { this.isDirty = true this.enableEdit() @@ -285,7 +289,7 @@ export default class Umap extends ServerStored { } if (this.searchParams.has('download')) { const download_url = this.urls.get('map_download', { - map_id: this.properties.umap_id, + map_id: this.id, }) window.location = download_url } @@ -563,7 +567,7 @@ export default class Umap extends ServerStored { createDataLayer(options = {}, sync = true) { options.name = options.name || `${translate('Layer')} ${this.datalayersIndex.length + 1}` - const datalayer = new DataLayer(this, this._leafletMap, options, sync) + const datalayer = new DataLayer(this, this._leafletMap, options) if (sync !== false) { datalayer.sync.upsert(datalayer.options) @@ -1098,7 +1102,7 @@ export default class Umap extends ServerStored { formData.append('name', this.properties.name) formData.append('center', JSON.stringify(this.geometry())) formData.append('settings', JSON.stringify(geojson)) - const uri = this.urls.get('map_save', { map_id: this.properties.umap_id }) + const uri = this.urls.get('map_save', { map_id: this.id }) const [data, _, error] = await this.server.post(uri, {}, formData) // FIXME: login_required response will not be an error, so it will not // stop code while it should @@ -1113,8 +1117,8 @@ export default class Umap extends ServerStored { return } this.properties.user = data.user - if (!this.properties.umap_id) { - this.properties.umap_id = data.id + if (!this.id) { + this.properties.id = data.id this.permissions.setProperties(data.permissions) this.permissions.commit() if (data.permissions?.anonymous_edit_url) { @@ -1228,7 +1232,7 @@ export default class Umap extends ServerStored { this.sync.stop() } else { const ws_token_uri = this.urls.get('map_websocket_auth_token', { - map_id: this.properties.umap_id, + map_id: this.id, }) await this.sync.authenticate( ws_token_uri, @@ -1398,8 +1402,10 @@ export default class Umap extends ServerStored { this.editPanel.open({ content: container }) } - getDataLayerByUmapId(umap_id) { - return this.findDataLayer((d) => d.umap_id === umap_id) + getDataLayerByUmapId(id) { + const datalayer = this.findDataLayer((d) => d.id === id) + if (!datalayer) throw new Error(`Can't find datalayer with id ${id}`) + return datalayer } firstVisibleDatalayer() { @@ -1433,10 +1439,10 @@ export default class Umap extends ServerStored { } async star() { - if (!this.properties.umap_id) { + if (!this.id) { return Alert.error(translate('Please save the map first')) } - const url = this.urls.get('map_star', { map_id: this.properties.umap_id }) + const url = this.urls.get('map_star', { map_id: this.id }) const [data, response, error] = await this.server.post(url) if (error) { return @@ -1530,7 +1536,7 @@ export default class Umap extends ServerStored { this.dialog .confirm(translate('Are you sure you want to delete this map?')) .then(async () => { - const url = this.urls.get('map_delete', { map_id: this.properties.umap_id }) + const url = this.urls.get('map_delete', { map_id: this.id }) const [data, response, error] = await this.server.post(url) if (data.redirect) window.location = data.redirect }) @@ -1542,7 +1548,7 @@ export default class Umap extends ServerStored { translate('Are you sure you want to clone this map and all its datalayers?') ) .then(async () => { - const url = this.urls.get('map_clone', { map_id: this.properties.umap_id }) + const url = this.urls.get('map_clone', { map_id: this.id }) const [data, response, error] = await this.server.post(url) if (data.redirect) window.location = data.redirect }) @@ -1552,7 +1558,7 @@ export default class Umap extends ServerStored { const sendLink = this.properties.urls.map_send_edit_link && this.urls.get('map_send_edit_link', { - map_id: this.properties.umap_id, + map_id: this.id, }) await this.server.post(sendLink, {}, formData) } diff --git a/umap/static/umap/js/modules/urls.js b/umap/static/umap/js/modules/urls.js index c3cb8a2a..0c44c516 100644 --- a/umap/static/umap/js/modules/urls.js +++ b/umap/static/umap/js/modules/urls.js @@ -25,8 +25,8 @@ export default class URLs { } // Update the layer if pk is passed, create otherwise. - datalayer_save({ map_id, pk }, ...options) { - if (pk) return this.get('datalayer_update', { map_id, pk }, ...options) + datalayer_save({ map_id, pk, created }, ...options) { + if (created) return this.get('datalayer_update', { map_id, pk }, ...options) return this.get('datalayer_create', { map_id, pk }, ...options) } } diff --git a/umap/static/umap/unittests/URLs.js b/umap/static/umap/unittests/URLs.js index 53c7448f..3edd3560 100644 --- a/umap/static/umap/unittests/URLs.js +++ b/umap/static/umap/unittests/URLs.js @@ -8,10 +8,10 @@ import URLs from '../js/modules/urls.js' describe('URLs', () => { // Mock server URLs that will be used for testing const mockServerUrls = { - map_create: '/maps/create', - map_update: '/maps/{map_id}/update', - datalayer_create: '/maps/{map_id}/datalayers/create', - datalayer_update: '/maps/{map_id}/datalayers/{pk}/update', + map_create: '/maps/create/', + map_update: '/maps/{map_id}/update/', + datalayer_create: '/maps/{map_id}/datalayers/{pk}/create/', + datalayer_update: '/maps/{map_id}/datalayers/{pk}/update/', } let urls = new URLs(mockServerUrls) @@ -22,37 +22,37 @@ describe('URLs', () => { }) it('should return the correct templated URL for known urlNames', () => { - expect(urls.get('map_create')).to.be.equal('/maps/create') - expect(urls.get('map_update', { map_id: '123' })).to.be.equal('/maps/123/update') + expect(urls.get('map_create')).to.be.equal('/maps/create/') + expect(urls.get('map_update', { map_id: '123' })).to.be.equal('/maps/123/update/') }) it('should return the correct templated URL when provided with parameters', () => { expect(urls.get('datalayer_update', { map_id: '123', pk: '456' })).to.be.equal( - '/maps/123/datalayers/456/update' + '/maps/123/datalayers/456/update/' ) }) }) describe('map_save()', () => { it('should return the create URL if no map_id is provided', () => { - expect(urls.map_save({})).to.be.equal('/maps/create') + expect(urls.map_save({})).to.be.equal('/maps/create/') }) it('should return the update URL if a map_id is provided', () => { - expect(urls.map_save({ map_id: '123' })).to.be.equal('/maps/123/update') + expect(urls.map_save({ map_id: '123' })).to.be.equal('/maps/123/update/') }) }) describe('datalayer_save()', () => { - it('should return the create URL if no pk is provided', () => { - expect(urls.datalayer_save({ map_id: '123' })).to.be.equal( - '/maps/123/datalayers/create' + it('should return the create URL if created is false', () => { + expect(urls.datalayer_save({ map_id: '123', pk: '00000000-0000-0000-0000-000000000000', created: false })).to.be.equal( + '/maps/123/datalayers/00000000-0000-0000-0000-000000000000/create/' ) }) - it('should return the update URL if a pk is provided', () => { - expect(urls.datalayer_save({ map_id: '123', pk: '456' })).to.be.equal( - '/maps/123/datalayers/456/update' + it('should return the update URL if created is true', () => { + expect(urls.datalayer_save({ map_id: '123', pk: '00000000-0000-0000-0000-000000000000', created: true })).to.be.equal( + '/maps/123/datalayers/00000000-0000-0000-0000-000000000000/update/' ) }) }) diff --git a/umap/tests/base.py b/umap/tests/base.py index 12b672fc..ebd2af49 100644 --- a/umap/tests/base.py +++ b/umap/tests/base.py @@ -1,5 +1,6 @@ import copy import json +import uuid import factory from django.contrib.auth import get_user_model @@ -113,6 +114,7 @@ class MapFactory(factory.django.DjangoModelFactory): class DataLayerFactory(factory.django.DjangoModelFactory): + uuid = factory.LazyFunction(lambda: uuid.uuid4()) map = factory.SubFactory(MapFactory) name = "test datalayer" description = "test description" diff --git a/umap/tests/integration/test_edit_datalayer.py b/umap/tests/integration/test_edit_datalayer.py index ce433051..bc22c223 100644 --- a/umap/tests/integration/test_edit_datalayer.py +++ b/umap/tests/integration/test_edit_datalayer.py @@ -129,32 +129,43 @@ def test_can_change_name(live_server, openmap, page, datalayer): def test_can_create_new_datalayer(live_server, openmap, page, datalayer): + """ + Test the creation and editing of a new datalayer. + + This test verifies that: + 1. A new datalayer can be created and saved. + 2. The newly created datalayer appears in the UI and is saved in the database. + 3. Editing the same datalayer updates it instead of creating a new one. + 4. The UI reflects the changes and the database is updated correctly. + 5. The 'dirty' state of the map is managed correctly during these operations. + """ + page.goto( f"{live_server.url}{openmap.get_absolute_url()}?edit&onLoadPanel=databrowser" ) page.get_by_role("link", name="Manage layers").click() page.get_by_role("button", name="Add a layer").click() page.locator('input[name="name"]').click() - page.locator('input[name="name"]').fill("my new layer") - expect(page.get_by_text("my new layer")).to_be_visible() + page.locator('input[name="name"]').fill("Layer A") + expect(page.get_by_text("Layer A")).to_be_visible() with page.expect_response(re.compile(".*/datalayer/create/.*")): page.get_by_role("button", name="Save").click() assert DataLayer.objects.count() == 2 saved = DataLayer.objects.last() - assert saved.name == "my new layer" + assert saved.name == "Layer A" expect(page.locator(".umap-is-dirty")).to_be_hidden() # Edit again, it should not create a new datalayer page.get_by_role("link", name="Manage layers").click() page.locator(".panel.right").get_by_title("Edit", exact=True).first.click() page.locator('input[name="name"]').click() - page.locator('input[name="name"]').fill("my new layer with a new name") - expect(page.get_by_text("my new layer with a new name")).to_be_visible() + 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 saved = DataLayer.objects.last() - assert saved.name == "my new layer with a new name" + assert saved.name == "Layer A with a new name" expect(page.locator(".umap-is-dirty")).to_be_hidden() diff --git a/umap/tests/integration/test_import.py b/umap/tests/integration/test_import.py index ad0a42eb..4752eee7 100644 --- a/umap/tests/integration/test_import.py +++ b/umap/tests/integration/test_import.py @@ -92,11 +92,11 @@ def test_umap_import_from_textarea(live_server, tilelayer, page, settings): expect( page.locator('img[src="https://tile.openstreetmap.fr/hot/6/32/21.png"]') ).to_be_visible() - # Should not have imported umap_id, while in the file options - assert not page.evaluate("U.MAP.properties.umap_id") + # Should not have imported id, while in the file options + assert not page.evaluate("U.MAP.properties.id") with page.expect_response(re.compile(r".*/datalayer/create/.*")): page.get_by_role("button", name="Save").click() - assert page.evaluate("U.MAP.properties.umap_id") + assert page.evaluate("U.MAP.properties.id") def test_import_geojson_from_textarea(tilelayer, live_server, page): diff --git a/umap/tests/integration/test_optimistic_merge.py b/umap/tests/integration/test_optimistic_merge.py index a4af4b7e..3c718ec1 100644 --- a/umap/tests/integration/test_optimistic_merge.py +++ b/umap/tests/integration/test_optimistic_merge.py @@ -49,6 +49,7 @@ def test_created_markers_are_merged(context, live_server, tilelayer): "name": "test datalayer", "editMode": "advanced", "inCaption": True, + "id": str(datalayer.pk), } # Now navigate to this map from another tab @@ -78,12 +79,14 @@ def test_created_markers_are_merged(context, live_server, tilelayer): sleep(1) # No change after the save expect(marker_pane_p2).to_have_count(2) - assert DataLayer.objects.get(pk=datalayer.pk).settings == { + datalayer_v2 = DataLayer.objects.get(pk=datalayer.pk) + assert datalayer_v2.settings == { "browsable": True, "displayOnLoad": True, "name": "test datalayer", "inCaption": True, "editMode": "advanced", + "id": str(datalayer.pk), } # Now create another marker in the first tab @@ -94,7 +97,8 @@ def test_created_markers_are_merged(context, live_server, tilelayer): save_p1.click() # Should now get the other marker too expect(marker_pane_p1).to_have_count(3) - assert DataLayer.objects.get(pk=datalayer.pk).settings == { + datalayer_v3 = DataLayer.objects.get(pk=datalayer.pk) + assert datalayer_v3.settings == { "browsable": True, "displayOnLoad": True, "name": "test datalayer", @@ -112,7 +116,8 @@ def test_created_markers_are_merged(context, live_server, tilelayer): save_p1.click() sleep(1) # Should now get the other marker too - assert DataLayer.objects.get(pk=datalayer.pk).settings == { + datalayer_v4 = DataLayer.objects.get(pk=datalayer.pk) + assert datalayer_v4.settings == { "browsable": True, "displayOnLoad": True, "name": "test datalayer", @@ -132,7 +137,8 @@ def test_created_markers_are_merged(context, live_server, tilelayer): save_p2.click() sleep(1) # Should now get the other markers too - assert DataLayer.objects.get(pk=datalayer.pk).settings == { + datalayer_v5 = DataLayer.objects.get(pk=datalayer.pk) + assert datalayer_v5.settings == { "browsable": True, "displayOnLoad": True, "name": "test datalayer", diff --git a/umap/tests/integration/test_websocket_sync.py b/umap/tests/integration/test_websocket_sync.py index 8c99c366..2807bee1 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -3,7 +3,7 @@ import re import pytest from playwright.sync_api import expect -from umap.models import Map +from umap.models import DataLayer, Map from ..base import DataLayerFactory, MapFactory @@ -267,7 +267,7 @@ def test_websocket_connection_can_sync_cloned_polygons( b_polygon = peerB.locator("path") # Clone on peer B and save - b_polygon.click(button="right") + b_polygon.click(button="right", delay=200) peerB.get_by_role("button", name="Clone this feature").click() expect(peerB.locator("path")).to_have_count(2) @@ -343,3 +343,75 @@ def test_websocket_connection_can_sync_late_joining_peer( # Clean up: close edit mode peerB.locator("body").press("Escape") + + +@pytest.mark.xdist_group(name="websockets") +def test_should_sync_datalayers(new_page, live_server, websocket_server, tilelayer): + map = MapFactory(name="sync", edit_status=Map.ANONYMOUS) + map.settings["properties"]["syncEnabled"] = True + map.save() + + assert not DataLayer.objects.count() + + # 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") + + # Create a new layer from peerA + peerA.get_by_role("link", name="Manage layers").click() + peerA.get_by_role("button", name="Add a layer").click() + + # Check layer has been sync to peerB + peerB.get_by_role("button", name="Open browser").click() + expect(peerB.get_by_text("Layer 1")).to_be_visible() + + # Draw a marker in layer 1 from peerA + peerA.get_by_role("link", name="Draw a marker (Ctrl+M)").click() + peerA.locator("#map").click() + + # Check marker is visible from peerB + expect(peerB.locator(".leaflet-marker-icon")).to_be_visible() + + # Save layer to the server + with peerA.expect_response(re.compile(".*/datalayer/create/.*")): + peerA.get_by_role("button", name="Save").click() + + assert DataLayer.objects.count() == 1 + + # Create another layer from peerA and draw a marker on it (without saving to server) + peerA.get_by_role("link", name="Manage layers").click() + peerA.get_by_role("button", name="Add a layer").click() + peerA.get_by_role("link", name="Draw a marker (Ctrl+M)").click() + peerA.locator("#map").click() + + # Make sure this new marker is in Layer 2 for peerB + expect(peerB.get_by_text("Layer 2")).to_be_visible() + peerB.locator(".panel.left").get_by_role("button", name="Show/hide layer").nth( + 1 + ).click() + expect(peerB.locator(".leaflet-marker-icon")).to_be_visible() + + # Now draw a marker from peerB + peerB.get_by_role("link", name="Draw a marker (Ctrl+M)").click() + peerB.locator("#map").click() + peerB.locator('input[name="name"]').fill("marker from peerB") + + # Save from peer B + with peerB.expect_response(re.compile(".*/datalayer/create/.*")): + peerB.get_by_role("button", name="Save").click() + + assert DataLayer.objects.count() == 2 + + # Check this new marker is visible from peerA + peerA.get_by_role("button", name="Open browser").click() + peerA.locator(".panel.left").get_by_role("button", name="Show/hide layer").nth( + 1 + ).click() + + # Now peerA saves the layer 2 to the server + with peerA.expect_response(re.compile(".*/datalayer/update/.*")): + peerA.get_by_role("button", name="Save").click() + + assert DataLayer.objects.count() == 2 diff --git a/umap/tests/test_datalayer_views.py b/umap/tests/test_datalayer_views.py index f50510d1..13000d88 100644 --- a/umap/tests/test_datalayer_views.py +++ b/umap/tests/test_datalayer_views.py @@ -1,6 +1,7 @@ import json from copy import deepcopy from pathlib import Path +from uuid import uuid4 import pytest from django.core.files.base import ContentFile @@ -105,6 +106,21 @@ def test_gzip_should_be_created_if_accepted(client, datalayer, map, post_data): assert Path(flat).stat().st_mtime_ns == Path(gzipped).stat().st_mtime_ns +def test_create_datalayer(client, map, post_data): + uuid = str(uuid4()) + url = reverse("datalayer_create", args=(map.pk, uuid)) + client.login(username=map.owner.username, password="123123") + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + new_datalayer = DataLayer.objects.get(pk=uuid) + assert new_datalayer.name == "name" + assert new_datalayer.rank == 0 + # Test response is a json + data = json.loads(response.content.decode()) + assert "id" in data + assert data["id"] == uuid + + def test_update(client, datalayer, map, post_data): url = reverse("datalayer_update", args=(map.pk, datalayer.pk)) client.login(username=map.owner.username, password="123123") diff --git a/umap/urls.py b/umap/urls.py index aaab2beb..b3afb557 100644 --- a/umap/urls.py +++ b/umap/urls.py @@ -155,8 +155,8 @@ map_urls = [ views.MapClone.as_view(), name="map_clone", ), - re_path( - r"^map/(?P[\d]+)/datalayer/create/$", + path( + "map//datalayer/create//", views.DataLayerCreate.as_view(), name="datalayer_create", ), diff --git a/umap/views.py b/umap/views.py index 5726a838..864cc57b 100644 --- a/umap/views.py +++ b/umap/views.py @@ -598,7 +598,7 @@ class MapDetailMixin(SessionMixin): "tilelayers": TileLayer.get_list(), "editMode": self.edit_mode, "schema": Map.extra_schema, - "umap_id": self.get_umap_id(), + "id": self.get_id(), "starred": self.is_starred(), "licences": dict((l.name, l.json) for l in Licence.objects.all()), "share_statuses": [ @@ -655,7 +655,7 @@ class MapDetailMixin(SessionMixin): def edit_mode(self): return "advanced" - def get_umap_id(self): + def get_id(self): return None def is_starred(self): @@ -725,6 +725,8 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView): return self.object.get_absolute_url() def get_datalayers(self): + # When initializing datalayers from map, we cannot get the reference version + # the normal way, which is from the header X-Reference-Version return [dl.metadata(self.request) for dl in self.object.datalayer_set.all()] @property @@ -736,7 +738,7 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView): edit_mode = "simple" return edit_mode - def get_umap_id(self): + def get_id(self): return self.object.pk def get_short_url(self): @@ -1181,9 +1183,19 @@ class DataLayerCreate(FormLessEditMixin, GZipMixin, CreateView): def form_valid(self, form): form.instance.map = self.kwargs["map_inst"] + + uuid = self.kwargs["pk"] + # Check if UUID already exists + if DataLayer.objects.filter(uuid=uuid).exists(): + return HttpResponseBadRequest("UUID already exists") + + form.instance.uuid = uuid self.object = form.save() - # Simple response with only metadata (including new id) - response = simple_json_response(**self.object.metadata(self.request)) + assert uuid == self.object.uuid + + # Simple response with only metadata + data = self.object.metadata(self.request) + response = simple_json_response(**data) response["X-Datalayer-Version"] = self.version return response