diff --git a/umap/migrations/0023_alter_datalayer_uuid.py b/umap/migrations/0023_alter_datalayer_uuid.py new file mode 100644 index 000000000..1180c4f41 --- /dev/null +++ b/umap/migrations/0023_alter_datalayer_uuid.py @@ -0,0 +1,18 @@ +# 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 27bc9aae3..51d0e068a 100644 --- a/umap/models.py +++ b/umap/models.py @@ -250,7 +250,7 @@ def preview_settings(self): "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")) @@ -530,7 +528,7 @@ def metadata(self, request=None): 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 +541,20 @@ def is_valid_version(self, name): 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 894b3a3a4..76702d620 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 6ac450298..b22ac5baf 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,13 @@ export class DataLayer extends ServerStored { } this._isDeleted = false - this.setUmapId(data.id) + this._referenceVersion = data._referenceVersion + // Do not save + delete data._referenceVersion + console.log(data) + data.id ??= crypto.randomUUID() + console.log(data) + this.setOptions(data) if (!Utils.isObject(this.options.remoteData)) { @@ -84,7 +87,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 +99,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 +133,7 @@ export class DataLayer extends ServerStored { getSyncMetadata() { return { subject: 'datalayer', - metadata: { - id: this.umap_id || null, - }, + metadata: { id: this.id }, } } @@ -160,7 +170,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 +225,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 +267,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 +324,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 +363,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 +443,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 +530,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 +585,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 +816,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 +1041,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 +1060,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 +1103,11 @@ 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 this.updateOptions(data) + + this.setReferenceVersion({ response, sync: true }) + this.backupOptions() this.backupData() this.connectToMap() @@ -1105,7 +1118,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 020ba0f7e..5d2ece729 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 f2dab9039..6ac145e49 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 130ec4607..919656795 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 a38975e97..bcc311d11 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 688bbe73f..b8a03f2e0 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 c3cb8a2a6..0c44c516d 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 53c7448f0..3edd35603 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 12b672fc7..ebd2af490 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 Meta: 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 ce4330515..bc22c2234 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 ad0a42eb6..4752eee72 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 a4af4b7e2..3c718ec1b 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 8c99c3664..2807bee1e 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -3,7 +3,7 @@ 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 f50510d1d..13000d882 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 aaab2beb8..b3afb5571 100644 --- a/umap/urls.py +++ b/umap/urls.py @@ -155,8 +155,8 @@ 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 5726a838a..29318fa32 100644 --- a/umap/views.py +++ b/umap/views.py @@ -598,7 +598,7 @@ def get_map_properties(self): "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 @@ def get_datalayers(self): def edit_mode(self): return "advanced" - def get_umap_id(self): + def get_id(self): return None def is_starred(self): @@ -725,7 +725,12 @@ def get_canonical_url(self): return self.object.get_absolute_url() def get_datalayers(self): - return [dl.metadata(self.request) for dl in self.object.datalayer_set.all()] + # When initializing datalayers from map, we cannot get the reference version + # the normal way, which is from the header X-Reference-Version + return [ + {"_referenceVersion": dl.reference_version, **dl.metadata(self.request)} + for dl in self.object.datalayer_set.all() + ] @property def edit_mode(self): @@ -736,7 +741,7 @@ def edit_mode(self): 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 +1186,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