From c36ea1e4b803b3589e7434f3b3063bbbb4719b58 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 18:26:04 +0200 Subject: [PATCH] Fix error when trying to change map owner This bug has been introduced with this change: https://github.com/umap-project/umap/commit/8b4842ff212541278957d74f86e886fb5c4d082f That was not the correct fix, and this one should be the proper one. We don't want to edit the permissions reference until we do save, otherwise user cannot save as it is already no more the owner. So: - change permissions.options - save - commit those changes to map.options.permissions - use only those values to check for isOwner and isAnonymousMap --- umap/static/umap/js/umap.js | 2 + umap/static/umap/js/umap.permissions.js | 6 +-- .../integration/test_anonymous_owned_map.py | 27 +++++++++- umap/tests/integration/test_owned_map.py | 50 +++++++++++++++++-- umap/views.py | 27 +++++----- 5 files changed, 91 insertions(+), 21 deletions(-) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 6883c025..784b0f3c 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1180,6 +1180,7 @@ L.U.Map.include({ alert.content = L._('Congratulations, your map has been created!') this.options.umap_id = data.id this.permissions.setOptions(data.permissions) + this.permissions.commit() if ( data.permissions && data.permissions.anonymous_edit_url && @@ -1215,6 +1216,7 @@ L.U.Map.include({ // Do not override local changes to permissions, // but update in case some other editors changed them in the meantime. this.permissions.setOptions(data.permissions) + this.permissions.commit() } // Update URL in case the name has changed. if (history && history.pushState) diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index f3cfbfa5..7e277503 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -37,13 +37,13 @@ L.U.MapPermissions = L.Class.extend({ isOwner: function () { return ( this.map.options.user && - this.map.permissions.options.owner && - this.map.options.user.id == this.map.permissions.options.owner.id + this.map.options.permissions.owner && + this.map.options.user.id == this.map.options.permissions.owner.id ) }, isAnonymousMap: function () { - return !this.map.permissions.options.owner + return !this.map.options.permissions.owner }, getMap: function () { diff --git a/umap/tests/integration/test_anonymous_owned_map.py b/umap/tests/integration/test_anonymous_owned_map.py index 20bc201e..daf999a0 100644 --- a/umap/tests/integration/test_anonymous_owned_map.py +++ b/umap/tests/integration/test_anonymous_owned_map.py @@ -1,4 +1,5 @@ import re +from time import sleep import pytest from django.core.signing import get_cookie_signer @@ -76,7 +77,6 @@ def test_owner_permissions_form(map, datalayer, live_server, owner_session): edit_permissions.click() select = owner_session.locator(".umap-field-share_status select") expect(select).to_be_hidden() - # expect(select).to_have_value(Map.PUBLIC) # Does not work owner_field = owner_session.locator(".umap-field-owner") expect(owner_field).to_be_hidden() editors_field = owner_session.locator(".umap-field-editors input") @@ -122,3 +122,28 @@ def test_anonymous_can_add_marker_on_editable_layer( expect(options).to_have_count(1) # Only Editable layer should be listed option = page.locator("select[name='datalayer'] option:checked") expect(option).to_have_text(other.name) + + +def test_can_change_perms_after_create(tilelayer, live_server, page): + page.goto(f"{live_server.url}/en/map/new") + save = page.get_by_title("Save current edits") + expect(save).to_be_visible() + save.click() + sleep(1) # Let save ajax go back + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + edit_permissions.click() + select = page.locator(".umap-field-share_status select") + expect(select).to_be_hidden() + owner_field = page.locator(".umap-field-owner") + expect(owner_field).to_be_hidden() + editors_field = page.locator(".umap-field-editors input") + expect(editors_field).to_be_hidden() + datalayer_label = page.get_by_text('Who can edit "Layer 1"') + expect(datalayer_label).to_be_visible() + options = page.locator(".datalayer-permissions select[name='edit_status'] option") + expect(options).to_have_count(3) + option = page.locator( + ".datalayer-permissions select[name='edit_status'] option:checked" + ) + expect(option).to_have_text("Inherit") diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 3bb9477e..75becdcc 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -88,9 +88,7 @@ def test_owner_permissions_form(map, datalayer, live_server, login): expect(editors_field).to_be_visible() datalayer_label = page.get_by_text('Who can edit "test datalayer"') expect(datalayer_label).to_be_visible() - options = page.locator( - ".datalayer-permissions select[name='edit_status'] option" - ) + options = page.locator(".datalayer-permissions select[name='edit_status'] option") expect(options).to_have_count(4) option = page.locator( ".datalayer-permissions select[name='edit_status'] option:checked" @@ -168,3 +166,49 @@ def test_editor_do_not_have_delete_map_button(map, live_server, login, user): advanced.click() delete = page.get_by_role("link", name="Delete") expect(delete).to_be_hidden() + + +def test_can_change_perms_after_create(tilelayer, live_server, login, user): + page = login(user) + page.goto(f"{live_server.url}/en/map/new") + save = page.get_by_title("Save current edits") + expect(save).to_be_visible() + save.click() + sleep(1) # Let save ajax go back + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + edit_permissions.click() + select = page.locator(".umap-field-share_status select") + expect(select).to_be_visible() + option = page.locator("select[name='share_status'] option:checked") + expect(option).to_have_text("Everyone (public)") + owner_field = page.locator(".umap-field-owner") + expect(owner_field).to_be_visible() + editors_field = page.locator(".umap-field-editors input") + expect(editors_field).to_be_visible() + datalayer_label = page.get_by_text('Who can edit "Layer 1"') + expect(datalayer_label).to_be_visible() + options = page.locator(".datalayer-permissions select[name='edit_status'] option") + expect(options).to_have_count(4) + option = page.locator( + ".datalayer-permissions select[name='edit_status'] option:checked" + ) + expect(option).to_have_text("Inherit") + + +def test_can_change_owner(map, live_server, login, user): + page = login(map.owner) + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + edit_permissions = page.get_by_title("Update permissions and editors") + edit_permissions.click() + close = page.locator(".umap-field-owner .close") + close.click() + input = page.locator("input.edit-owner") + input.type(user.username) + input.press("Tab") + save = page.get_by_title("Save current edits") + expect(save).to_be_visible() + save.click() + sleep(1) # Let save ajax go + modified = Map.objects.get(pk=map.pk) + assert modified.owner == user diff --git a/umap/views.py b/umap/views.py index a0aa014e..da460047 100644 --- a/umap/views.py +++ b/umap/views.py @@ -447,6 +447,7 @@ class MapDetailMixin: def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) + user = self.request.user properties = { "urls": _urls_for_js(), "tilelayers": TileLayer.get_list(), @@ -460,20 +461,19 @@ class MapDetailMixin: ], "umap_version": VERSION, } - if getattr(self, "object", None) and self.object.owner: - properties["edit_statuses"] = [ - (i, str(label)) for i, label in Map.EDIT_STATUS - ] - properties["datalayer_edit_statuses"] = [ - (i, str(label)) for i, label in DataLayer.EDIT_STATUS - ] + created = bool(getattr(self, "object", None)) + if (created and self.object.owner) or (not created and not user.is_anonymous): + map_statuses = Map.EDIT_STATUS + datalayer_statuses = DataLayer.EDIT_STATUS else: - properties["edit_statuses"] = [ - (i, str(label)) for i, label in AnonymousMapPermissionsForm.STATUS - ] - properties["datalayer_edit_statuses"] = [ - (i, str(label)) for i, label in AnonymousDataLayerPermissionsForm.STATUS - ] + map_statuses = AnonymousMapPermissionsForm.STATUS + datalayer_statuses = AnonymousDataLayerPermissionsForm.STATUS + properties["edit_statuses"] = [ + (i, str(label)) for i, label in map_statuses + ] + properties["datalayer_edit_statuses"] = [ + (i, str(label)) for i, label in datalayer_statuses + ] if self.get_short_url(): properties["shortUrl"] = self.get_short_url() @@ -486,7 +486,6 @@ class MapDetailMixin: locale = to_locale(lang) properties["locale"] = locale context["locale"] = locale - user = self.request.user if not user.is_anonymous: properties["user"] = { "id": user.pk,