From 89ab029cab4c7190d1a29e6b9519cf46d8d31165 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 7 Sep 2023 10:31:25 +0200 Subject: [PATCH 01/40] WIP: move edit_status from Map to DataLayer --- umap/forms.py | 70 ++++++++++++------- umap/migrations/0013_datalayer_edit_status.py | 22 ++++++ umap/models.py | 43 +++++++++--- .../umap/js/umap.datalayer.permissions.js | 70 +++++++++++++++++++ umap/static/umap/js/umap.features.js | 2 +- umap/static/umap/js/umap.layer.js | 22 +++--- umap/static/umap/js/umap.permissions.js | 4 ++ umap/templates/umap/js.html | 3 +- umap/templatetags/umap_tags.py | 2 +- umap/urls.py | 5 ++ umap/views.py | 36 ++++++++-- 11 files changed, 228 insertions(+), 51 deletions(-) create mode 100644 umap/migrations/0013_datalayer_edit_status.py create mode 100644 umap/static/umap/js/umap.datalayer.permissions.js diff --git a/umap/forms.py b/umap/forms.py index dc16f096..af937ed7 100644 --- a/umap/forms.py +++ b/umap/forms.py @@ -8,8 +8,12 @@ from django.forms.utils import ErrorList from .models import Map, DataLayer -DEFAULT_LATITUDE = settings.LEAFLET_LATITUDE if hasattr(settings, "LEAFLET_LATITUDE") else 51 -DEFAULT_LONGITUDE = settings.LEAFLET_LONGITUDE if hasattr(settings, "LEAFLET_LONGITUDE") else 2 +DEFAULT_LATITUDE = ( + settings.LEAFLET_LATITUDE if hasattr(settings, "LEAFLET_LATITUDE") else 51 +) +DEFAULT_LONGITUDE = ( + settings.LEAFLET_LONGITUDE if hasattr(settings, "LEAFLET_LONGITUDE") else 2 +) DEFAULT_CENTER = Point(DEFAULT_LONGITUDE, DEFAULT_LATITUDE) User = get_user_model() @@ -21,8 +25,8 @@ class FlatErrorList(ErrorList): def flat(self): if not self: - return u'' - return u' — '.join([e for e in self]) + return "" + return " — ".join([e for e in self]) class SendLinkForm(forms.Form): @@ -30,69 +34,83 @@ class SendLinkForm(forms.Form): class UpdateMapPermissionsForm(forms.ModelForm): - class Meta: model = Map - fields = ('edit_status', 'editors', 'share_status', 'owner') + fields = ("edit_status", "editors", "share_status", "owner") class AnonymousMapPermissionsForm(forms.ModelForm): - def __init__(self, *args, **kwargs): super(AnonymousMapPermissionsForm, self).__init__(*args, **kwargs) - help_text = _('Secret edit link is %s') % self.instance.get_anonymous_edit_url() - self.fields['edit_status'].help_text = _(help_text) + help_text = _("Secret edit link is %s") % self.instance.get_anonymous_edit_url() + self.fields["edit_status"].help_text = _(help_text) STATUS = ( - (Map.ANONYMOUS, _('Everyone can edit')), - (Map.OWNER, _('Only editable with secret edit link')) + (Map.ANONYMOUS, _("Everyone can edit")), + (Map.OWNER, _("Only editable with secret edit link")), ) edit_status = forms.ChoiceField(choices=STATUS) class Meta: model = Map - fields = ('edit_status', ) + fields = ("edit_status",) class DataLayerForm(forms.ModelForm): + class Meta: + model = DataLayer + fields = ("geojson", "name", "display_on_load", "rank", "settings") + + +class DataLayerPermissionsForm(forms.ModelForm): + class Meta: + model = DataLayer + fields = ("edit_status",) + + +class AnonymousDataLayerPermissionsForm(forms.ModelForm): + STATUS = ( + (Map.ANONYMOUS, _("Everyone can edit")), + (Map.OWNER, _("Only editable with secret edit link")), + ) + + edit_status = forms.ChoiceField(choices=STATUS) class Meta: model = DataLayer - fields = ('geojson', 'name', 'display_on_load', 'rank', 'settings') + fields = ("edit_status",) class MapSettingsForm(forms.ModelForm): - def __init__(self, *args, **kwargs): super(MapSettingsForm, self).__init__(*args, **kwargs) - self.fields['slug'].required = False - self.fields['center'].widget.map_srid = 4326 + self.fields["slug"].required = False + self.fields["center"].widget.map_srid = 4326 def clean_slug(self): - slug = self.cleaned_data.get('slug', None) - name = self.cleaned_data.get('name', None) + slug = self.cleaned_data.get("slug", None) + name = self.cleaned_data.get("name", None) if not slug and name: # If name is empty, don't do nothing, validation will raise # later on the process because name is required - self.cleaned_data['slug'] = slugify(name) or "map" - return self.cleaned_data['slug'][:50] + self.cleaned_data["slug"] = slugify(name) or "map" + return self.cleaned_data["slug"][:50] else: return "" def clean_center(self): - if not self.cleaned_data['center']: + if not self.cleaned_data["center"]: point = DEFAULT_CENTER - self.cleaned_data['center'] = point - return self.cleaned_data['center'] + self.cleaned_data["center"] = point + return self.cleaned_data["center"] class Meta: - fields = ('settings', 'name', 'center', 'slug') + fields = ("settings", "name", "center", "slug") model = Map class UserProfileForm(forms.ModelForm): - class Meta: model = User - fields = ('username', 'first_name', 'last_name') + fields = ("username", "first_name", "last_name") diff --git a/umap/migrations/0013_datalayer_edit_status.py b/umap/migrations/0013_datalayer_edit_status.py new file mode 100644 index 00000000..16f3691f --- /dev/null +++ b/umap/migrations/0013_datalayer_edit_status.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.2 on 2023-09-07 06:27 + +from django.db import migrations, models +import umap.models + + +class Migration(migrations.Migration): + dependencies = [ + ("umap", "0012_datalayer_settings"), + ] + + operations = [ + migrations.AddField( + model_name="datalayer", + name="edit_status", + field=models.SmallIntegerField( + choices=[(1, "Everyone"), (2, "Editors only"), (3, "Owner only")], + default=umap.models.get_default_edit_status, + verbose_name="edit status", + ), + ), + ] diff --git a/umap/models.py b/umap/models.py index 7b804642..fbeaa559 100644 --- a/umap/models.py +++ b/umap/models.py @@ -216,6 +216,9 @@ class Map(NamedModel): """ Define if a user can edit or not the instance, according to his account or the request. + + In ownership mode: only owner and editors + In anononymous mode: only "anonymous owners" (having edit cookie set) """ can = False if request and not self.owner: @@ -223,13 +226,9 @@ class Map(NamedModel): settings, "UMAP_ALLOW_ANONYMOUS", False ) and self.is_anonymous_owner(request): can = True - if self.edit_status == self.ANONYMOUS: + if user == self.owner: can = True - elif not user.is_authenticated: - pass - elif user == self.owner: - can = True - elif self.edit_status == self.EDITORS and user in self.editors.all(): + elif user in self.editors.all(): can = True return can @@ -303,6 +302,15 @@ class DataLayer(NamedModel): Layer to store Features in. """ + ANONYMOUS = 1 + EDITORS = 2 + OWNER = 3 + EDIT_STATUS = ( + (ANONYMOUS, _("Everyone")), + (EDITORS, _("Editors only")), + (OWNER, _("Owner only")), + ) + map = models.ForeignKey(Map, on_delete=models.CASCADE) description = models.TextField(blank=True, null=True, verbose_name=_("description")) geojson = models.FileField(upload_to=upload_to, blank=True, null=True) @@ -315,6 +323,11 @@ class DataLayer(NamedModel): settings = models.JSONField( blank=True, null=True, verbose_name=_("settings"), default=dict ) + edit_status = models.SmallIntegerField( + choices=EDIT_STATUS, + default=get_default_edit_status, + verbose_name=_("edit status"), + ) class Meta: ordering = ("rank",) @@ -346,8 +359,7 @@ class DataLayer(NamedModel): path.append(str(self.map.pk)) return os.path.join(*path) - @property - def metadata(self): + def metadata(self, user=None, request=None): # Retrocompat: minimal settings for maps not saved after settings property # has been introduced obj = self.settings or { @@ -355,6 +367,8 @@ class DataLayer(NamedModel): "displayOnLoad": self.display_on_load, } obj["id"] = self.pk + obj["permissions"] = {"edit_status": self.edit_status} + obj["allowEdit"] = self.can_edit(user, request) return obj def clone(self, map_inst=None): @@ -413,6 +427,19 @@ class DataLayer(NamedModel): if name.startswith(f'{self.pk}_') and name.endswith(".gz"): self.geojson.storage.delete(os.path.join(root, name)) + def can_edit(self, user=None, request=None): + """ + Define if a user can edit or not the instance, according to his account + or the request. + """ + can = self.map.can_edit(user, request) + if can: + # Owner or editor, no need for further checks. + return can + if self.edit_status == self.ANONYMOUS: + can = True + return can + class Star(models.Model): at = models.DateTimeField(auto_now=True) diff --git a/umap/static/umap/js/umap.datalayer.permissions.js b/umap/static/umap/js/umap.datalayer.permissions.js new file mode 100644 index 00000000..2ee16bab --- /dev/null +++ b/umap/static/umap/js/umap.datalayer.permissions.js @@ -0,0 +1,70 @@ +L.U.DataLayerPermissions = L.Class.extend({ + options: { + edit_status: null, + }, + + initialize: function (datalayer) { + this.options = L.Util.setOptions(this, datalayer.options.permissions) + this.datalayer = datalayer + let isDirty = false + const self = this + try { + Object.defineProperty(this, 'isDirty', { + get: function () { + return isDirty + }, + set: function (status) { + isDirty = status + if (status) self.datalayer.isDirty = status + }, + }) + } catch (e) { + // Certainly IE8, which has a limited version of defineProperty + } + }, + + getMap: function () { + return this.datalayer.map + }, + + edit: function (container) { + const fields = [ + [ + 'options.edit_status', + { + handler: 'IntSelect', + label: `${L._('Who can edit')} "${this.datalayer.getName()}"`, + selectOptions: this.datalayer.map.options.edit_statuses, + }, + ], + ], + builder = new L.U.FormBuilder(this, fields), + form = builder.build() + container.appendChild(form) + }, + + getUrl: function () { + return L.Util.template(this.datalayer.map.options.urls.datalayer_permissions, { + map_id: this.datalayer.map.options.umap_id, + pk: this.datalayer.umap_id, + }) + }, + save: function () { + if (!this.isDirty) return this.datalayer.map.continueSaving() + const formData = new FormData() + formData.append('edit_status', this.options.edit_status) + this.datalayer.map.post(this.getUrl(), { + data: formData, + context: this, + callback: function (data) { + this.commit() + this.isDirty = false + this.datalayer.map.continueSaving() + }, + }) + }, + + commit: function () { + L.Util.extend(this.datalayer.options.permissions, this.options) + }, +}) diff --git a/umap/static/umap/js/umap.features.js b/umap/static/umap/js/umap.features.js index 40ec7927..8063f325 100644 --- a/umap/static/umap/js/umap.features.js +++ b/umap/static/umap/js/umap.features.js @@ -40,7 +40,7 @@ L.U.FeatureMixin = { preInit: function () {}, isReadOnly: function () { - return this.datalayer && this.datalayer.isRemoteLayer() + return this.datalayer && (this.datalayer.isRemoteLayer() || this.datalayer.isReadOnly()) }, getSlug: function () { diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 0a83d688..6568ba23 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -261,6 +261,7 @@ L.U.DataLayer = L.Evented.extend({ } this.backupOptions() this.connectToMap() + this.permissions = new L.U.DataLayerPermissions(this) if (this.showAtLoad()) this.show() if (!this.umap_id) this.isDirty = true @@ -350,6 +351,13 @@ L.U.DataLayer = L.Evented.extend({ this.map.get(this._dataUrl(), { callback: function (geojson, response) { this._last_modified = response.getResponseHeader('Last-Modified') + console.log(this.getName(), this.options) + // FIXME: for now this 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 + // in DB, and we remove it from geojson flat files. + geojson['_umap_options']['allowEdit'] = this.options.allowEdit this.fromUmapGeoJSON(geojson) this.backupOptions() this.fire('loaded') @@ -1182,18 +1190,14 @@ L.U.DataLayer = L.Evented.extend({ } }, - metadata: function () { - return { - id: this.umap_id, - name: this.options.name, - displayOnLoad: this.options.displayOnLoad, - } - }, - getRank: function () { return this.map.datalayers_index.indexOf(this) }, + isReadOnly: function () { + return !this.options.allowEdit + }, + save: function () { if (this.isDeleted) return this.saveDelete() if (!this.isLoaded()) { @@ -1220,7 +1224,7 @@ L.U.DataLayer = L.Evented.extend({ this._loaded = true this.redraw() // Needed for reordering features this.isDirty = false - this.map.continueSaving() + this.permissions.save() }, context: this, headers: this._last_modified diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index 0454855d..13dcb750 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -122,6 +122,10 @@ L.U.MapPermissions = L.Class.extend({ this ) } + L.DomUtil.add('h3', '', container, L._('Datalayers')) + this.map.eachDataLayer((datalayer) => { + datalayer.permissions.edit(container) + }) this.map.ui.openPanel({ data: { html: container }, className: 'dark' }) }, diff --git a/umap/templates/umap/js.html b/umap/templates/umap/js.html index 566ec658..5b321fc4 100644 --- a/umap/templates/umap/js.html +++ b/umap/templates/umap/js.html @@ -34,11 +34,12 @@ + + - {% endcompress %} diff --git a/umap/templatetags/umap_tags.py b/umap/templatetags/umap_tags.py index c40691d1..3e7ae9c0 100644 --- a/umap/templatetags/umap_tags.py +++ b/umap/templatetags/umap_tags.py @@ -28,7 +28,7 @@ def umap_js(locale=None): @register.inclusion_tag('umap/map_fragment.html') def map_fragment(map_instance, **kwargs): layers = DataLayer.objects.filter(map=map_instance) - datalayer_data = [c.metadata for c in layers] + datalayer_data = [c.metadata() for c in layers] map_settings = map_instance.settings if "properties" not in map_settings: map_settings['properties'] = {} diff --git a/umap/urls.py b/umap/urls.py index a44aa11d..ee963ff7 100644 --- a/umap/urls.py +++ b/umap/urls.py @@ -154,6 +154,11 @@ map_urls = [ views.DataLayerDelete.as_view(), name="datalayer_delete", ), + re_path( + r"^map/(?P[\d]+)/datalayer/permissions/(?P\d+)/$", + views.UpdateDataLayerPermissions.as_view(), + name="datalayer_permissions", + ), ] if settings.FROM_EMAIL: map_urls.append( diff --git a/umap/views.py b/umap/views.py index 67d899b4..a07fbd56 100644 --- a/umap/views.py +++ b/umap/views.py @@ -47,6 +47,8 @@ from .forms import ( DEFAULT_CENTER, AnonymousMapPermissionsForm, DataLayerForm, + DataLayerPermissionsForm, + AnonymousDataLayerPermissionsForm, FlatErrorList, MapSettingsForm, SendLinkForm, @@ -551,11 +553,16 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView): return self.object.get_absolute_url() def get_datalayers(self): - datalayers = DataLayer.objects.filter(map=self.object) - return [l.metadata for l in datalayers] + return [ + l.metadata(self.request.user, self.request) + for l in self.object.datalayer_set.all() + ] def is_edit_allowed(self): - return self.object.can_edit(self.request.user, self.request) + return self.object.can_edit(self.request.user, self.request) or any( + d.can_edit(self.request.user, self.request) + for d in self.object.datalayer_set.all() + ) def get_umap_id(self): return self.object.pk @@ -883,7 +890,9 @@ class DataLayerCreate(FormLessEditMixin, GZipMixin, CreateView): form.instance.map = self.kwargs["map_inst"] self.object = form.save() # Simple response with only metadatas (including new id) - response = simple_json_response(**self.object.metadata) + response = simple_json_response( + **self.object.metadata(self.request.user, self.request) + ) response["Last-Modified"] = self.last_modified return response @@ -896,7 +905,9 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView): self.object = form.save() # Simple response with only metadatas (client should not reload all data # on save) - response = simple_json_response(**self.object.metadata) + response = simple_json_response( + **self.object.metadata(self.request.user, self.request) + ) response["Last-Modified"] = self.last_modified return response @@ -936,6 +947,21 @@ class DataLayerVersions(BaseDetailView): return simple_json_response(versions=self.object.versions) +class UpdateDataLayerPermissions(FormLessEditMixin, UpdateView): + model = DataLayer + pk_url_kwarg = "pk" + + def get_form_class(self): + if self.object.map.owner: + return DataLayerPermissionsForm + else: + return AnonymousDataLayerPermissionsForm + + def form_valid(self, form): + self.object = form.save() + return simple_json_response(info=_("Permissions updated with success!")) + + # ############## # # Picto # # ############## # From de907dcb500917b525c8d736b17c2fd672ae832e Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 07:26:12 +0200 Subject: [PATCH 02/40] Do not expose readonly datalayers for features --- umap/static/umap/js/umap.forms.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/static/umap/js/umap.forms.js b/umap/static/umap/js/umap.forms.js index 587549a5..5e5525bc 100644 --- a/umap/static/umap/js/umap.forms.js +++ b/umap/static/umap/js/umap.forms.js @@ -396,7 +396,7 @@ L.FormBuilder.DataLayerSwitcher = L.FormBuilder.Select.extend({ getOptions: function () { const options = [] this.builder.map.eachDataLayerReverse((datalayer) => { - if (datalayer.isLoaded() && !datalayer.isRemoteLayer() && datalayer.canBrowse()) { + if (datalayer.isLoaded() && !datalayer.isReadOnly() && datalayer.canBrowse()) { options.push([L.stamp(datalayer), datalayer.getName()]) } }) From 3d2e62c858ce7b9b19bc048d76947f4330ee63a5 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 09:39:28 +0200 Subject: [PATCH 03/40] Do not use Map.edit_status anymore But keep it for now, for data migration, and just in case --- umap/decorators.py | 15 +++++++-------- umap/forms.py | 20 +------------------- umap/static/umap/js/umap.permissions.js | 21 +-------------------- umap/templates/umap/map_table.html | 4 ++-- umap/urls.py | 17 ++++++++++------- umap/views.py | 14 ++++---------- 6 files changed, 25 insertions(+), 66 deletions(-) diff --git a/umap/decorators.py b/umap/decorators.py index c096c1f4..52faa127 100644 --- a/umap/decorators.py +++ b/umap/decorators.py @@ -26,9 +26,9 @@ def login_required_if_not_anonymous_allowed(view_func): return wrapper -def map_permissions_check(view_func): +def can_edit_map(view_func): """ - Used for URLs dealing with the map. + Used for URLs dealing with editing the map. """ @wraps(view_func) @@ -36,12 +36,11 @@ def map_permissions_check(view_func): map_inst = get_object_or_404(Map, pk=kwargs["map_id"]) user = request.user kwargs["map_inst"] = map_inst # Avoid rerequesting the map in the view - if map_inst.edit_status >= map_inst.EDITORS: - can_edit = map_inst.can_edit(user=user, request=request) - if not can_edit: - if map_inst.owner and not user.is_authenticated: - return simple_json_response(login_required=str(LOGIN_URL)) - return HttpResponseForbidden() + can_edit = map_inst.can_edit(user=user, request=request) + if not can_edit: + if map_inst.owner and not user.is_authenticated: + return simple_json_response(login_required=str(LOGIN_URL)) + return HttpResponseForbidden() return view_func(request, *args, **kwargs) return wrapper diff --git a/umap/forms.py b/umap/forms.py index af937ed7..636013eb 100644 --- a/umap/forms.py +++ b/umap/forms.py @@ -36,25 +36,7 @@ class SendLinkForm(forms.Form): class UpdateMapPermissionsForm(forms.ModelForm): class Meta: model = Map - fields = ("edit_status", "editors", "share_status", "owner") - - -class AnonymousMapPermissionsForm(forms.ModelForm): - def __init__(self, *args, **kwargs): - super(AnonymousMapPermissionsForm, self).__init__(*args, **kwargs) - help_text = _("Secret edit link is %s") % self.instance.get_anonymous_edit_url() - self.fields["edit_status"].help_text = _(help_text) - - STATUS = ( - (Map.ANONYMOUS, _("Everyone can edit")), - (Map.OWNER, _("Only editable with secret edit link")), - ) - - edit_status = forms.ChoiceField(choices=STATUS) - - class Meta: - model = Map - fields = ("edit_status",) + fields = ("editors", "share_status", "owner") class DataLayerForm(forms.ModelForm): diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index 13dcb750..1904d1da 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -5,7 +5,6 @@ L.U.MapPermissions = L.Class.extend({ owner: null, editors: [], share_status: null, - edit_status: null, }, initialize: function (map) { @@ -62,26 +61,10 @@ L.U.MapPermissions = L.Class.extend({ const helpText = L._('Secret edit link is:
{link}', { link: this.options.anonymous_edit_url, }) - fields.push([ - 'options.edit_status', - { - handler: 'IntSelect', - label: L._('Who can edit'), - selectOptions: this.map.options.anonymous_edit_statuses, - helpText: helpText, - }, - ]) + L.DomUtil.create('p', 'help-text', container, helpText) } } else { if (this.isOwner()) { - fields.push([ - 'options.edit_status', - { - handler: 'IntSelect', - label: L._('Who can edit'), - selectOptions: this.map.options.edit_statuses, - }, - ]) fields.push([ 'options.share_status', { @@ -151,8 +134,6 @@ L.U.MapPermissions = L.Class.extend({ for (let i = 0; i < this.options.editors.length; i++) formData.append('editors', this.options.editors[i].id) } - if (this.isOwner() || this.isAnonymousMap()) - formData.append('edit_status', this.options.edit_status) if (this.isOwner()) { formData.append('owner', this.options.owner && this.options.owner.id) formData.append('share_status', this.options.share_status) diff --git a/umap/templates/umap/map_table.html b/umap/templates/umap/map_table.html index eb8cef5a..2150838b 100644 --- a/umap/templates/umap/map_table.html +++ b/umap/templates/umap/map_table.html @@ -5,7 +5,7 @@ {% blocktrans %}Map{% endblocktrans %} {% blocktrans %}Name{% endblocktrans %} - {% blocktrans %}Who can see / edit{% endblocktrans %} + {% blocktrans %}Who can see{% endblocktrans %} {% blocktrans %}Last save{% endblocktrans %} {% blocktrans %}Owner{% endblocktrans %} {% blocktrans %}Actions{% endblocktrans %} @@ -19,7 +19,7 @@ {{ map_inst.name }} - {{ map_inst.get_share_status_display }} / {{ map_inst.get_edit_status_display }} + {{ map_inst.get_share_status_display }} {{ map_inst.modified_at }} {{ map_inst.owner }} diff --git a/umap/urls.py b/umap/urls.py index ee963ff7..630ea45a 100644 --- a/umap/urls.py +++ b/umap/urls.py @@ -13,7 +13,7 @@ from . import views from .decorators import ( jsonize_view, login_required_if_not_anonymous_allowed, - map_permissions_check, + can_edit_map, can_view_map, ) from .utils import decorated_patterns @@ -144,11 +144,6 @@ map_urls = [ views.DataLayerCreate.as_view(), name="datalayer_create", ), - re_path( - r"^map/(?P[\d]+)/datalayer/update/(?P\d+)/$", - views.DataLayerUpdate.as_view(), - name="datalayer_update", - ), re_path( r"^map/(?P[\d]+)/datalayer/delete/(?P\d+)/$", views.DataLayerDelete.as_view(), @@ -168,7 +163,15 @@ if settings.FROM_EMAIL: name="map_send_edit_link", ) ) -i18n_urls += decorated_patterns([map_permissions_check, never_cache], *map_urls) +datalayer_urls = [ + re_path( + r"^map/(?P[\d]+)/datalayer/update/(?P\d+)/$", + views.DataLayerUpdate.as_view(), + name="datalayer_update", + ), +] +i18n_urls += decorated_patterns([can_edit_map, never_cache], *map_urls) +i18n_urls += decorated_patterns([never_cache], *datalayer_urls) urlpatterns += i18n_patterns( re_path(r"^$", views.home, name="home"), re_path( diff --git a/umap/views.py b/umap/views.py index a07fbd56..a21f58e1 100644 --- a/umap/views.py +++ b/umap/views.py @@ -45,7 +45,6 @@ from .forms import ( DEFAULT_LATITUDE, DEFAULT_LONGITUDE, DEFAULT_CENTER, - AnonymousMapPermissionsForm, DataLayerForm, DataLayerPermissionsForm, AnonymousDataLayerPermissionsForm, @@ -460,7 +459,7 @@ class MapDetailMixin: (i, str(label)) for i, label in Map.SHARE_STATUS if i != Map.BLOCKED ], "anonymous_edit_statuses": [ - (i, str(label)) for i, label in AnonymousMapPermissionsForm.STATUS + (i, str(label)) for i, label in AnonymousDataLayerPermissionsForm.STATUS ], "umap_version": VERSION, } @@ -522,7 +521,6 @@ class MapDetailMixin: class PermissionsMixin: def get_permissions(self): permissions = {} - permissions["edit_status"] = self.object.edit_status permissions["share_status"] = self.object.share_status if self.object.owner: permissions["owner"] = { @@ -646,18 +644,12 @@ class MapUpdate(FormLessEditMixin, PermissionsMixin, UpdateView): class UpdateMapPermissions(FormLessEditMixin, UpdateView): model = Map pk_url_kwarg = "map_id" - - def get_form_class(self): - if self.object.owner: - return UpdateMapPermissionsForm - else: - return AnonymousMapPermissionsForm + form_class = UpdateMapPermissionsForm def get_form(self, form_class=None): form = super().get_form(form_class) user = self.request.user if self.object.owner and not user == self.object.owner: - del form.fields["edit_status"] del form.fields["share_status"] del form.fields["owner"] return form @@ -924,6 +916,8 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView): self.object = self.get_object() if self.object.map != self.kwargs["map_inst"]: return HttpResponseForbidden() + if not self.object.can_edit(user=self.request.user, request=self.request): + return HttpResponseForbidden() if not self.is_unmodified(): return HttpResponse(status=412) return super(DataLayerUpdate, self).post(request, *args, **kwargs) From 76239ef55c10009aa89fbabb804a1c72fb511892 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 09:41:17 +0200 Subject: [PATCH 04/40] Make DataLayer.isReadOnly explicit and true by default --- umap/static/umap/js/umap.layer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 6568ba23..02a8ab24 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -1195,7 +1195,7 @@ L.U.DataLayer = L.Evented.extend({ }, isReadOnly: function () { - return !this.options.allowEdit + return this.options.allowEdit === false }, save: function () { From 70a1a1d58416ba0d0e64cd65c5d263b79182115e Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 15:17:16 +0200 Subject: [PATCH 05/40] Move copyToClipboard to L.Util --- umap/static/umap/js/umap.core.js | 28 ++++++++++++++++++++++++++++ umap/static/umap/js/umap.js | 30 +----------------------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/umap/static/umap/js/umap.core.js b/umap/static/umap/js/umap.core.js index b03979f8..a9398945 100644 --- a/umap/static/umap/js/umap.core.js +++ b/umap/static/umap/js/umap.core.js @@ -257,6 +257,34 @@ L.Util.hasVar = (value) => { return typeof value === 'string' && value.indexOf('{') != -1 } +L.Util.copyToClipboard = function (textToCopy) { + // https://stackoverflow.com/a/65996386 + // Navigator clipboard api needs a secure context (https) + if (navigator.clipboard && window.isSecureContext) { + navigator.clipboard.writeText(textToCopy) + } else { + // Use the 'out of viewport hidden text area' trick + const textArea = document.createElement('textarea') + textArea.value = textToCopy + + // Move textarea out of the viewport so it's not visible + textArea.style.position = 'absolute' + textArea.style.left = '-999999px' + + document.body.prepend(textArea) + textArea.select() + + try { + document.execCommand('copy') + } catch (error) { + console.error(error) + } finally { + textArea.remove() + } + } + } + + L.DomUtil.add = (tagName, className, container, content) => { const el = L.DomUtil.create(tagName, className, container) if (content) { diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 27414e33..b1d8a42e 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1174,34 +1174,6 @@ L.U.Map.include({ formData.append('name', this.options.name) formData.append('center', JSON.stringify(this.geometry())) formData.append('settings', JSON.stringify(geojson)) - - function copyToClipboard(textToCopy) { - // https://stackoverflow.com/a/65996386 - // Navigator clipboard api needs a secure context (https) - if (navigator.clipboard && window.isSecureContext) { - navigator.clipboard.writeText(textToCopy) - } else { - // Use the 'out of viewport hidden text area' trick - const textArea = document.createElement('textarea') - textArea.value = textToCopy - - // Move textarea out of the viewport so it's not visible - textArea.style.position = 'absolute' - textArea.style.left = '-999999px' - - document.body.prepend(textArea) - textArea.select() - - try { - document.execCommand('copy') - } catch (error) { - console.error(error) - } finally { - textArea.remove() - } - } - } - this.post(this.getSaveUrl(), { data: formData, context: this, @@ -1233,7 +1205,7 @@ L.U.Map.include({ { label: L._('Copy link'), callback: () => { - copyToClipboard(data.permissions.anonymous_edit_url) + L.Util.copyToClipboard(data.permissions.anonymous_edit_url) this.ui.alert({ content: L._('Secret edit link copied to clipboard!'), level: 'info', From ee9acf34277d24a0bd740e2568198cfeb9e64ed2 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 15:22:08 +0200 Subject: [PATCH 06/40] Fix map check in DataLayerUpdate view --- umap/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/views.py b/umap/views.py index a21f58e1..05cf9d5f 100644 --- a/umap/views.py +++ b/umap/views.py @@ -914,7 +914,7 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView): def post(self, request, *args, **kwargs): self.object = self.get_object() - if self.object.map != self.kwargs["map_inst"]: + if self.object.map.pk != int(self.kwargs["map_id"]): return HttpResponseForbidden() if not self.object.can_edit(user=self.request.user, request=self.request): return HttpResponseForbidden() From 91892628552dd696afaea03a7466dd95d93f2754 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 15:32:37 +0200 Subject: [PATCH 07/40] Do not try to save map itself unless it has been modified --- umap/static/umap/js/umap.js | 64 +++++++++++++++++-------- umap/static/umap/js/umap.permissions.js | 2 +- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index b1d8a42e..405e3b62 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -192,16 +192,31 @@ L.U.Map.include({ this ) - let isDirty = false // global status + // FIXME naming + let hasDirty = false // global status + try { + Object.defineProperty(this, 'hasDirty', { + get: function () { + return hasDirty || this.dirty_datalayers.length + }, + set: function (status) { + if (!hasDirty && status) self.fire('hasdirty') + hasDirty = status + self.checkDirty() + }, + }) + } catch (e) { + // Certainly IE8, which has a limited version of defineProperty + } + let isDirty = false // self status try { Object.defineProperty(this, 'isDirty', { get: function () { - return isDirty || this.dirty_datalayers.length + return isDirty }, set: function (status) { - if (!isDirty && status) self.fire('isdirty') isDirty = status - self.checkDirty() + if (status) hasDirty = true }, }) } catch (e) { @@ -267,7 +282,7 @@ L.U.Map.include({ if (L.Util.queryString('download')) this.download() }) - window.onbeforeunload = () => this.isDirty || null + window.onbeforeunload = () => this.hasDirty || null this.backup() this.initContextMenu() this.on('click contextmenu.show', this.closeInplaceToolbar) @@ -506,7 +521,7 @@ L.U.Map.include({ key === L.U.Keys.E && modifierKey && this.editEnabled && - !this.isDirty + !this.hasDirty ) { L.DomEvent.stop(e) this.disableEdit() @@ -514,11 +529,11 @@ L.U.Map.include({ } if (key === L.U.Keys.S && modifierKey) { L.DomEvent.stop(e) - if (this.isDirty) { + if (this.hasDirty) { this.save() } } - if (key === L.U.Keys.Z && modifierKey && this.isDirty) { + if (key === L.U.Keys.Z && modifierKey && this.hasDirty) { L.DomEvent.stop(e) this.askForReset() } @@ -1047,17 +1062,18 @@ L.U.Map.include({ this.dirty_datalayers = [] this.updateDatalayersControl() this.initTileLayers() + this.hasDirty = false this.isDirty = false }, checkDirty: function () { - L.DomUtil.classIf(this._container, 'umap-is-dirty', this.isDirty) + L.DomUtil.classIf(this._container, 'umap-is-dirty', this.hasDirty) }, addDirtyDatalayer: function (datalayer) { if (this.dirty_datalayers.indexOf(datalayer) === -1) { this.dirty_datalayers.push(datalayer) - this.isDirty = true + this.hasDirty = true } }, @@ -1161,15 +1177,12 @@ L.U.Map.include({ return JSON.stringify(umapfile, null, 2) }, - save: function () { - if (!this.isDirty) return - if (this._default_extent) this.updateExtent() + saveSelf: function () { const geojson = { type: 'Feature', geometry: this.geometry(), properties: this.exportOptions(), } - this.backup() const formData = new FormData() formData.append('name', this.options.name) formData.append('center', JSON.stringify(this.geometry())) @@ -1215,7 +1228,7 @@ L.U.Map.include({ }, ] } - } else if (!this.permissions.isDirty) { + } else if (!this.permissions.hasDirty) { // Do not override local changes to permissions, // but update in case some other editors changed them in the meantime. this.permissions.setOptions(data.permissions) @@ -1225,16 +1238,25 @@ L.U.Map.include({ history.pushState({}, this.options.name, data.url) else window.location = data.url alert.content = data.info || alert.content - this.once('saved', function () { - this.isDirty = false - this.ui.alert(alert) - }) + this.once('saved', () => this.ui.alert(alert)) this.ui.closePanel() this.permissions.save() }, }) }, + save: function () { + if (!this.hasDirty) return + if (this._default_extent) this.updateExtent() + this.backup() + this.once('saved', () => { + this.hasDirty = false + this.isDirty = false + }) + if (this.isDirty) this.saveSelf() + else this.permissions.save() // Map itself has no change, check permissions and continue + }, + sendEditLink: function () { const url = L.Util.template(this.options.urls.map_send_edit_link, { map_id: this.options.umap_id, @@ -1761,7 +1783,7 @@ L.U.Map.include({ }, disableEdit: function () { - if (this.isDirty) return + if (this.hasDirty) return L.DomUtil.removeClass(document.body, 'umap-edit-enabled') this.editedFeature = null this.editEnabled = false @@ -1927,7 +1949,7 @@ L.U.Map.include({ if (this.options.allowEdit) { items.push('-') if (this.editEnabled) { - if (!this.isDirty) { + if (!this.hasDirty) { items.push({ text: `${L._('Stop editing')} (Ctrl+E)`, callback: this.disableEdit, diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index 1904d1da..7e7a608a 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -19,7 +19,7 @@ L.U.MapPermissions = L.Class.extend({ }, set: function (status) { isDirty = status - if (status) self.map.isDirty = status + if (status) self.map.hasDirty = status }, }) } catch (e) { From 35d7a5c550ac08e1d02d93eb5b2caeb17998baed Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 16:41:00 +0200 Subject: [PATCH 08/40] Remove unused icon in 16.svg --- umap/static/umap/img/16.svg | 1 - umap/static/umap/img/source/16.svg | 7 +++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/umap/static/umap/img/16.svg b/umap/static/umap/img/16.svg index 6fd25734..0c9fccc1 100644 --- a/umap/static/umap/img/16.svg +++ b/umap/static/umap/img/16.svg @@ -37,7 +37,6 @@ - diff --git a/umap/static/umap/img/source/16.svg b/umap/static/umap/img/source/16.svg index 56879165..5009bbcc 100644 --- a/umap/static/umap/img/source/16.svg +++ b/umap/static/umap/img/source/16.svg @@ -1,10 +1,10 @@ - + - - + + @@ -55,7 +55,6 @@ - From 1cefd4e851cdbc9ba4bfe1f91ec346ffa4643f56 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 16:44:45 +0200 Subject: [PATCH 09/40] Hide create/delete datalayers button + map settings to users without rights --- umap/static/umap/js/umap.controls.js | 38 ++++++++++++++++++---------- umap/static/umap/map.css | 3 +++ umap/views.py | 5 +++- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/umap/static/umap/js/umap.controls.js b/umap/static/umap/js/umap.controls.js index 696e7c3f..87baa753 100644 --- a/umap/static/umap/js/umap.controls.js +++ b/umap/static/umap/js/umap.controls.js @@ -273,7 +273,12 @@ L.U.ContinueLineAction = L.U.BaseVertexAction.extend({ }) // Leaflet.Toolbar doesn't allow twice same toolbar class… -L.U.SettingsToolbar = L.Toolbar.Control.extend({}) +L.U.SettingsToolbar = L.Toolbar.Control.extend({ + addTo: function (map) { + if (map.options.allowMapEdit === false) return + L.Toolbar.Control.prototype.addTo.call(this, map) + }, +}) L.U.DrawToolbar = L.Toolbar.Control.extend({ initialize: function (options) { L.Toolbar.Control.prototype.initialize.call(this, options) @@ -608,21 +613,26 @@ L.U.DataLayer.include({ edit.title = L._('Edit') table.title = L._('Edit properties in a table') remove.title = L._('Delete layer') + if (this.isReadOnly()) { + L.DomUtil.addClass(container, 'readonly') + } + else { + L.DomEvent.on(edit, 'click', this.edit, this) + L.DomEvent.on(table, 'click', this.tableEdit, this) + L.DomEvent.on( + remove, + 'click', + function () { + if (!this.isVisible()) return + if (!confirm(L._('Are you sure you want to delete this layer?'))) return + this._delete() + this.map.ui.closePanel() + }, + this + ) + } L.DomEvent.on(toggle, 'click', this.toggle, this) L.DomEvent.on(zoomTo, 'click', this.zoomTo, this) - L.DomEvent.on(edit, 'click', this.edit, this) - L.DomEvent.on(table, 'click', this.tableEdit, this) - L.DomEvent.on( - remove, - 'click', - function () { - if (!this.isVisible()) return - if (!confirm(L._('Are you sure you want to delete this layer?'))) return - this._delete() - this.map.ui.closePanel() - }, - this - ) L.DomUtil.addClass(container, this.getHidableClass()) L.DomUtil.classIf(container, 'off', !this.isVisible()) container.dataset.id = L.stamp(this) diff --git a/umap/static/umap/map.css b/umap/static/umap/map.css index d830fc0a..3ffe7c52 100644 --- a/umap/static/umap/map.css +++ b/umap/static/umap/map.css @@ -756,15 +756,18 @@ a.map-name:after { .umap-toggle-edit { background-position: -44px -48px; } +.readonly .layer-table-edit, .off .layer-table-edit { background-position: -74px -1px; } +.readonly .layer-edit, .off .layer-edit { background-position: -51px -72px; } .off .layer-zoom_to { background-position: -25px -54px; } +.readonly .layer-delete, .off .layer-delete { background-position: -122px -121px; } diff --git a/umap/views.py b/umap/views.py index 05cf9d5f..e0a15613 100644 --- a/umap/views.py +++ b/umap/views.py @@ -449,7 +449,10 @@ class MapDetailMixin: properties = { "urls": _urls_for_js(), "tilelayers": TileLayer.get_list(), - "allowEdit": self.is_edit_allowed(), + "allowEdit": self.is_edit_allowed(), # showEditMode + "allowMapEdit": self.object.can_edit(self.request.user, self.request) + if self.object + else True, # FIXME naming "default_iconUrl": "%sumap/img/marker.png" % settings.STATIC_URL, # noqa "umap_id": self.get_umap_id(), "starred": self.is_starred(), From e06da18f1ed79f3b464d022fa4be77f3201b24d7 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 17:14:10 +0200 Subject: [PATCH 10/40] Fix helptext not displayed in anonymous permissions panel --- umap/static/umap/js/umap.permissions.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index 7e7a608a..daa1bc6f 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -19,7 +19,9 @@ L.U.MapPermissions = L.Class.extend({ }, set: function (status) { isDirty = status - if (status) self.map.hasDirty = status + if (status) { + self.map.hasDirty = status + } }, }) } catch (e) { @@ -58,10 +60,8 @@ L.U.MapPermissions = L.Class.extend({ title = L.DomUtil.create('h4', '', container) if (this.isAnonymousMap()) { if (this.options.anonymous_edit_url) { - const helpText = L._('Secret edit link is:
{link}', { - link: this.options.anonymous_edit_url, - }) - L.DomUtil.create('p', 'help-text', container, helpText) + const helpText = `${L._('Secret edit link:')}
${this.options.anonymous_edit_url}` + L.DomUtil.add('p', 'help-text', container, helpText) } } else { if (this.isOwner()) { From e52b40807a85b56eb38b057a28ac34cf42337726 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Fri, 8 Sep 2023 17:14:37 +0200 Subject: [PATCH 11/40] Expose correct edit statuses in anonymous mode --- umap/views.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/umap/views.py b/umap/views.py index e0a15613..493cefad 100644 --- a/umap/views.py +++ b/umap/views.py @@ -457,15 +457,19 @@ class MapDetailMixin: "umap_id": self.get_umap_id(), "starred": self.is_starred(), "licences": dict((l.name, l.json) for l in Licence.objects.all()), - "edit_statuses": [(i, str(label)) for i, label in Map.EDIT_STATUS], "share_statuses": [ (i, str(label)) for i, label in Map.SHARE_STATUS if i != Map.BLOCKED ], - "anonymous_edit_statuses": [ - (i, str(label)) for i, label in AnonymousDataLayerPermissionsForm.STATUS - ], "umap_version": VERSION, } + if self.object.owner: + properties["edit_statuses"] = [ + (i, str(label)) for i, label in Map.EDIT_STATUS + ] + else: + properties["edit_statuses"] = [ + (i, str(label)) for i, label in AnonymousDataLayerPermissionsForm.STATUS + ] if self.get_short_url(): properties["shortUrl"] = self.get_short_url() From d6d55e619a9e88087ea515794ca5060394b0956d Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Sat, 9 Sep 2023 10:32:15 +0200 Subject: [PATCH 12/40] Fix existing permissions related tests --- umap/tests/test_map.py | 40 +++++++----------------------- umap/tests/test_map_views.py | 47 ++++++++---------------------------- umap/tests/test_views.py | 1 - 3 files changed, 19 insertions(+), 69 deletions(-) diff --git a/umap/tests/test_map.py b/umap/tests/test_map.py index ae193700..bc1283c3 100644 --- a/umap/tests/test_map.py +++ b/umap/tests/test_map.py @@ -9,55 +9,33 @@ from .base import MapFactory pytestmark = pytest.mark.django_db -def test_anonymous_can_edit_if_status_anonymous(map): +def test_anonymous_cannot_edit(map): anonymous = AnonymousUser() - map.edit_status = map.ANONYMOUS - map.save() - assert map.can_edit(anonymous) - - -def test_anonymous_cannot_edit_if_not_status_anonymous(map): - anonymous = AnonymousUser() - map.edit_status = map.OWNER - map.save() assert not map.can_edit(anonymous) -def test_non_editors_can_edit_if_status_anonymous(map, user): +def test_non_editors_cannot_edit(map, user): assert map.owner != user - map.edit_status = map.ANONYMOUS - map.save() - assert map.can_edit(user) - - -def test_non_editors_cannot_edit_if_not_status_anonymous(map, user): - map.edit_status = map.OWNER - map.save() assert not map.can_edit(user) -def test_editors_cannot_edit_if_status_owner(map, user): - map.edit_status = map.OWNER - map.editors.add(user) - map.save() - assert not map.can_edit(user) - - -def test_editors_can_edit_if_status_editors(map, user): - map.edit_status = map.EDITORS +def test_editors_can_edit(map, user): map.editors.add(user) map.save() assert map.can_edit(user) -def test_logged_in_user_should_be_allowed_for_anonymous_map_with_anonymous_edit_status(map, user, rf): # noqa +def test_owner_can_edit(map): + assert map.can_edit(map.owner) + + +def test_logged_in_user_should_not_be_allowed_for_anonymous_map(map, user, rf): map.owner = None - map.edit_status = map.ANONYMOUS map.save() url = reverse('map_update', kwargs={'map_id': map.pk}) request = rf.get(url) request.user = user - assert map.can_edit(user, request) + assert not map.can_edit(user, request) def test_clone_should_return_new_instance(map, user): diff --git a/umap/tests/test_map_views.py b/umap/tests/test_map_views.py index ab8519ab..25d14350 100644 --- a/umap/tests/test_map_views.py +++ b/umap/tests/test_map_views.py @@ -38,7 +38,6 @@ def test_create(client, user, post_data): assert created_map.center.x == 13.447265624999998 assert created_map.center.y == 48.94415123418794 assert j["permissions"] == { - "edit_status": 3, "share_status": 1, "owner": {"id": user.pk, "name": "Joe", "url": "/en/user/Joe/"}, "editors": [], @@ -167,24 +166,20 @@ def test_user_not_allowed_should_not_clone_map(client, map, user, settings): settings.UMAP_ALLOW_ANONYMOUS = False assert Map.objects.count() == 1 url = reverse("map_clone", kwargs={"map_id": map.pk}) - map.edit_status = map.OWNER - map.save() response = client.post(url) assert login_required(response) client.login(username=user.username, password="123123") response = client.post(url) assert response.status_code == 403 - map.edit_status = map.ANONYMOUS - map.save() client.logout() response = client.post(url) - assert response.status_code == 403 + assert response.status_code == 200 + assert "login_required" in json.loads(response.content) assert Map.objects.count() == 1 def test_clone_should_set_cloner_as_owner(client, map, user): url = reverse("map_clone", kwargs={"map_id": map.pk}) - map.edit_status = map.EDITORS map.editors.add(user) map.save() client.login(username=user.username, password="123123") @@ -302,19 +297,19 @@ def test_only_owner_can_delete(client, map, user): assert response.status_code == 403 -def test_map_editors_do_not_see_owner_change_input(client, map, user): +def test_map_editors_cannot_change_owner(client, map, user): + owner = map.owner map.editors.add(user) - map.edit_status = map.EDITORS map.save() url = reverse("map_update_permissions", kwargs={"map_id": map.pk}) client.login(username=user.username, password="123123") - response = client.get(url) - assert "id_owner" not in response + response = client.post(url, data={"owner": user.pk}) + assert response.status_code == 200 + assert map.owner == owner -def test_logged_in_user_can_edit_map_editable_by_anonymous(client, map, user): +def test_logged_in_user_cannot_edit_map(client, map, user): map.owner = None - map.edit_status = map.ANONYMOUS map.save() client.login(username=user.username, password="123123") url = reverse("map_update", kwargs={"map_id": map.pk}) @@ -324,8 +319,7 @@ def test_logged_in_user_can_edit_map_editable_by_anonymous(client, map, user): "name": new_name, } response = client.post(url, data) - assert response.status_code == 200 - assert Map.objects.get(pk=map.pk).name == new_name + assert response.status_code == 403 @pytest.mark.usefixtures("allow_anonymous") @@ -422,13 +416,9 @@ def test_bad_anonymous_edit_url_should_return_403(cookieclient, anonymap): @pytest.mark.usefixtures("allow_anonymous") -def test_clone_anonymous_map_should_not_be_possible_if_user_is_not_allowed( - client, anonymap, user -): # noqa +def test_clone_anonymous_map_should_not_be_possible(client, anonymap, user): # noqa assert Map.objects.count() == 1 url = reverse("map_clone", kwargs={"map_id": anonymap.pk}) - anonymap.edit_status = anonymap.OWNER - anonymap.save() response = client.post(url) assert response.status_code == 403 client.login(username=user.username, password="123123") @@ -437,23 +427,6 @@ def test_clone_anonymous_map_should_not_be_possible_if_user_is_not_allowed( assert Map.objects.count() == 1 -@pytest.mark.usefixtures("allow_anonymous") -def test_clone_map_should_be_possible_if_edit_status_is_anonymous( - client, anonymap -): # noqa - assert Map.objects.count() == 1 - url = reverse("map_clone", kwargs={"map_id": anonymap.pk}) - anonymap.edit_status = anonymap.ANONYMOUS - anonymap.save() - response = client.post(url) - assert response.status_code == 200 - assert Map.objects.count() == 2 - clone = Map.objects.latest("pk") - assert clone.pk != anonymap.pk - assert clone.name == "Clone of " + anonymap.name - assert clone.owner is None - - @pytest.mark.usefixtures("allow_anonymous") def test_anyone_can_access_anonymous_map(cookieclient, anonymap): url = reverse("map", args=(anonymap.slug, anonymap.pk)) diff --git a/umap/tests/test_views.py b/umap/tests/test_views.py index e53d2230..df96042a 100644 --- a/umap/tests/test_views.py +++ b/umap/tests/test_views.py @@ -282,7 +282,6 @@ def test_user_dashboard_display_user_maps(client, map): assert f"{map.get_absolute_url()}?share" in body assert f"{map.get_absolute_url()}?download" in body assert "Everyone (public)" in body - assert "Owner only" in body @pytest.mark.django_db From 42eb0e6dedb23ce82748e907670b0f3b49f77042 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Sat, 9 Sep 2023 11:44:35 +0200 Subject: [PATCH 13/40] Add more tests for datalayer permissions --- umap/models.py | 10 ++-- umap/tests/test_datalayer.py | 64 ++++++++++++++++++++- umap/tests/test_datalayer_views.py | 91 ++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 7 deletions(-) diff --git a/umap/models.py b/umap/models.py index fbeaa559..97df55f9 100644 --- a/umap/models.py +++ b/umap/models.py @@ -202,7 +202,7 @@ class Map(NamedModel): return settings.SITE_URL + path def is_anonymous_owner(self, request): - if self.owner: + if not request or self.owner: # edit cookies are only valid while map hasn't owner return False key, value = self.signed_cookie_elements @@ -221,12 +221,10 @@ class Map(NamedModel): In anononymous mode: only "anonymous owners" (having edit cookie set) """ can = False - if request and not self.owner: - if getattr( - settings, "UMAP_ALLOW_ANONYMOUS", False - ) and self.is_anonymous_owner(request): + if not self.owner: + if settings.UMAP_ALLOW_ANONYMOUS and self.is_anonymous_owner(request): can = True - if user == self.owner: + elif user == self.owner: can = True elif user in self.editors.all(): can = True diff --git a/umap/tests/test_datalayer.py b/umap/tests/test_datalayer.py index 5818a541..d2b70797 100644 --- a/umap/tests/test_datalayer.py +++ b/umap/tests/test_datalayer.py @@ -4,6 +4,7 @@ import pytest from django.core.files.base import ContentFile from .base import DataLayerFactory, MapFactory +from umap.models import DataLayer pytestmark = pytest.mark.django_db @@ -21,7 +22,7 @@ def test_datalayers_should_be_ordered_by_rank(map, datalayer): def test_upload_to(map, datalayer): map.pk = 302 datalayer.pk = 17 - assert datalayer.upload_to().startswith('datalayer/2/0/302/17_') + assert datalayer.upload_to().startswith("datalayer/2/0/302/17_") def test_save_should_use_pk_as_name(map, datalayer): @@ -81,3 +82,64 @@ def test_should_remove_old_versions_on_save(datalayer, map, settings): assert os.path.basename(other) in files assert os.path.basename(other + ".gz") in files assert os.path.basename(older) not in files + assert os.path.basename(older + ".gz") not in files + + +def test_anonymous_cannot_edit_in_editors_mode(datalayer): + datalayer.edit_status = DataLayer.EDITORS + datalayer.save() + assert not datalayer.can_edit() + + +def test_owner_can_edit_in_editors_mode(datalayer, user): + datalayer.edit_status = DataLayer.EDITORS + datalayer.save() + assert datalayer.can_edit(datalayer.map.owner) + + +def test_editor_can_edit_in_editors_mode(datalayer, user): + map = datalayer.map + map.editors.add(user) + map.save() + datalayer.edit_status = DataLayer.EDITORS + datalayer.save() + assert datalayer.can_edit(user) + + +def test_anonymous_can_edit_in_public_mode(datalayer): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + assert datalayer.can_edit() + + +def test_owner_can_edit_in_public_mode(datalayer, user): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + assert datalayer.can_edit(datalayer.map.owner) + + +def test_editor_can_edit_in_public_mode(datalayer, user): + map = datalayer.map + map.editors.add(user) + map.save() + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + assert datalayer.can_edit(user) + + +def test_anonymous_cannot_edit_in_anonymous_owner_mode(datalayer): + datalayer.edit_status = DataLayer.OWNER + datalayer.save() + map = datalayer.map + map.owner = None + map.save() + assert not datalayer.can_edit() + + +def test_anonymous_can_edit_in_anonymous_owner_but_public_mode(datalayer): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + map = datalayer.map + map.owner = None + map.save() + assert datalayer.can_edit() diff --git a/umap/tests/test_datalayer_views.py b/umap/tests/test_datalayer_views.py index 43a6d49d..01c65db7 100644 --- a/umap/tests/test_datalayer_views.py +++ b/umap/tests/test_datalayer_views.py @@ -245,3 +245,94 @@ def test_update_readonly(client, datalayer, map, post_data, settings): client.login(username=map.owner.username, password="123123") response = client.post(url, post_data, follow=True) assert response.status_code == 403 + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_owner_can_edit_in_anonymous_owner_mode( + datalayer, cookieclient, anonymap, post_data +): + datalayer.edit_status = DataLayer.OWNER + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = cookieclient.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_can_edit_in_anonymous_owner_but_public_mode( + datalayer, client, anonymap, post_data +): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_cannot_edit_in_anonymous_owner_mode( + datalayer, client, anonymap, post_data +): + datalayer.edit_status = DataLayer.OWNER + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 403 + + +def test_anonymous_cannot_edit_in_owner_mode(datalayer, client, map, post_data): + datalayer.edit_status = DataLayer.OWNER + datalayer.save() + url = reverse("datalayer_update", args=(map.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 403 + + +def test_anonymous_can_edit_in_owner_but_public_mode(datalayer, client, map, post_data): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + url = reverse("datalayer_update", args=(map.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name + + +def test_owner_can_edit_in_owner_mode(datalayer, client, map, post_data): + client.login(username=map.owner.username, password="123123") + datalayer.edit_status = DataLayer.OWNER + datalayer.save() + url = reverse("datalayer_update", args=(map.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name + + +def test_editor_can_edit_in_editors_mode(datalayer, client, map, post_data): + client.login(username=map.owner.username, password="123123") + datalayer.edit_status = DataLayer.EDITORS + datalayer.save() + url = reverse("datalayer_update", args=(map.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name From 168cc01c2e2bf39872d05682d9a9c02c94f1ba1d Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Sun, 10 Sep 2023 09:34:13 +0200 Subject: [PATCH 14/40] Do not try to access self.object in MapCreate --- umap/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/umap/views.py b/umap/views.py index 493cefad..1bb36493 100644 --- a/umap/views.py +++ b/umap/views.py @@ -451,7 +451,7 @@ class MapDetailMixin: "tilelayers": TileLayer.get_list(), "allowEdit": self.is_edit_allowed(), # showEditMode "allowMapEdit": self.object.can_edit(self.request.user, self.request) - if self.object + if getattr(self, "object", None) else True, # FIXME naming "default_iconUrl": "%sumap/img/marker.png" % settings.STATIC_URL, # noqa "umap_id": self.get_umap_id(), @@ -462,7 +462,7 @@ class MapDetailMixin: ], "umap_version": VERSION, } - if self.object.owner: + if getattr(self, "object", None) and self.object.owner: properties["edit_statuses"] = [ (i, str(label)) for i, label in Map.EDIT_STATUS ] From d902546da4d082691e1886518d87b2bd5c277061 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 11 Sep 2023 12:19:42 +0200 Subject: [PATCH 15/40] Add map update playwright tests --- .gitignore | 3 +- pytest.ini | 2 +- umap/tests/integration/test_map_update.py | 64 +++++++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 umap/tests/integration/test_map_update.py diff --git a/.gitignore b/.gitignore index d236ae74..5b5d15b3 100644 --- a/.gitignore +++ b/.gitignore @@ -17,4 +17,5 @@ __pycache__/ build/ dist/ *.egg-info/ - +playwright/.auth/ +test-results/ diff --git a/pytest.ini b/pytest.ini index d9fcbd49..610ba029 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,3 +1,3 @@ [pytest] DJANGO_SETTINGS_MODULE=umap.tests.settings -addopts = "--pdbcls=IPython.terminal.debugger:Pdb --no-migrations" +addopts = --pdbcls=IPython.terminal.debugger:Pdb --no-migrations diff --git a/umap/tests/integration/test_map_update.py b/umap/tests/integration/test_map_update.py new file mode 100644 index 00000000..42eacfb4 --- /dev/null +++ b/umap/tests/integration/test_map_update.py @@ -0,0 +1,64 @@ +from time import sleep + +import pytest +from playwright.sync_api import expect + +from umap.models import Map + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def loggedin_page(context, user, settings, live_server): + settings.ENABLE_ACCOUNT_LOGIN = True + page = context.new_page() + page.goto(f"{live_server.url}/en/") + page.locator(".login").click() + page.get_by_placeholder("Username").fill("Gabriel") + page.get_by_placeholder("Password").fill("123123") + page.locator('#login_form input[type="submit"]').click() + sleep(1) # Time for ajax login POST to proceed + return page + + +def test_map_update_with_owner(map, live_server, loggedin_page): + loggedin_page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = loggedin_page.locator("#map") + expect(map_el).to_be_visible() + enable = loggedin_page.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + disable = loggedin_page.get_by_role("link", name="Disable editing") + expect(disable).to_be_visible() + save = loggedin_page.get_by_title("Save current edits (Ctrl+S)") + expect(save).to_be_visible() + add_marker = loggedin_page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = loggedin_page.get_by_title("Edit map settings") + expect(edit_settings).to_be_visible() + edit_permissions = loggedin_page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + + +def test_map_update_with_anonymous(map, live_server, page): + page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_hidden() + + +def test_owner_permissions_form(map, datalayer, live_server, loggedin_page): + loggedin_page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + edit_permissions = loggedin_page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + edit_permissions.click() + select = loggedin_page.locator(".umap-field-share_status select") + expect(select).to_be_visible() + # expect(select).to_have_value(Map.PUBLIC) # Does not work + owner_field = loggedin_page.locator(".umap-field-owner") + expect(owner_field).to_be_visible() + editors_field = loggedin_page.locator(".umap-field-editors input") + expect(editors_field).to_be_visible() + datalayer_label = loggedin_page.get_by_text('Who can edit "Donau"') + expect(datalayer_label).to_be_visible() From a15aa3566a00f891ec6a664a99a20349af61dc54 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 12 Sep 2023 11:07:10 +0200 Subject: [PATCH 16/40] Add playwright tests for anonymous map --- .../integration/test_anonymous_owned_map.py | 81 +++++++++++++ umap/tests/integration/test_map_update.py | 64 ---------- umap/tests/integration/test_owned_map.py | 112 ++++++++++++++++++ 3 files changed, 193 insertions(+), 64 deletions(-) create mode 100644 umap/tests/integration/test_anonymous_owned_map.py delete mode 100644 umap/tests/integration/test_map_update.py create mode 100644 umap/tests/integration/test_owned_map.py diff --git a/umap/tests/integration/test_anonymous_owned_map.py b/umap/tests/integration/test_anonymous_owned_map.py new file mode 100644 index 00000000..8cf377e3 --- /dev/null +++ b/umap/tests/integration/test_anonymous_owned_map.py @@ -0,0 +1,81 @@ +import pytest +from django.core.signing import get_cookie_signer +from playwright.sync_api import expect + +from umap.models import DataLayer + +pytestmark = [pytest.mark.django_db, pytest.mark.usefixtures("allow_anonymous")] + + +@pytest.fixture +def owner_session(anonymap, context, live_server): + key, value = anonymap.signed_cookie_elements + signed = get_cookie_signer(salt=key).sign(value) + context.add_cookies([{"name": key, "value": signed, "url": live_server.url}]) + return context.new_page() + + +def test_map_load_with_owner(anonymap, live_server, owner_session): + owner_session.goto(f"{live_server.url}{anonymap.get_absolute_url()}") + map_el = owner_session.locator("#map") + expect(map_el).to_be_visible() + enable = owner_session.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + disable = owner_session.get_by_role("link", name="Disable editing") + expect(disable).to_be_visible() + save = owner_session.get_by_title("Save current edits (Ctrl+S)") + expect(save).to_be_visible() + add_marker = owner_session.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = owner_session.get_by_title("Edit map settings") + expect(edit_settings).to_be_visible() + edit_permissions = owner_session.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + + +def test_map_load_with_anonymous(anonymap, live_server, page): + page.goto(f"{live_server.url}{anonymap.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_hidden() + + +def test_map_load_with_anonymous_but_editable_layer( + anonymap, live_server, page, datalayer +): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + page.goto(f"{live_server.url}{anonymap.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + disable = page.get_by_role("link", name="Disable editing") + expect(disable).to_be_visible() + save = page.get_by_title("Save current edits (Ctrl+S)") + expect(save).to_be_visible() + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = page.get_by_title("Edit map settings") + expect(edit_settings).to_be_hidden() + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_hidden() + + +def test_owner_permissions_form(map, datalayer, live_server, owner_session): + owner_session.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + edit_permissions = owner_session.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + 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") + expect(editors_field).to_be_hidden() + datalayer_label = owner_session.get_by_text('Who can edit "Donau"') + expect(datalayer_label).to_be_visible() diff --git a/umap/tests/integration/test_map_update.py b/umap/tests/integration/test_map_update.py deleted file mode 100644 index 42eacfb4..00000000 --- a/umap/tests/integration/test_map_update.py +++ /dev/null @@ -1,64 +0,0 @@ -from time import sleep - -import pytest -from playwright.sync_api import expect - -from umap.models import Map - -pytestmark = pytest.mark.django_db - - -@pytest.fixture -def loggedin_page(context, user, settings, live_server): - settings.ENABLE_ACCOUNT_LOGIN = True - page = context.new_page() - page.goto(f"{live_server.url}/en/") - page.locator(".login").click() - page.get_by_placeholder("Username").fill("Gabriel") - page.get_by_placeholder("Password").fill("123123") - page.locator('#login_form input[type="submit"]').click() - sleep(1) # Time for ajax login POST to proceed - return page - - -def test_map_update_with_owner(map, live_server, loggedin_page): - loggedin_page.goto(f"{live_server.url}{map.get_absolute_url()}") - map_el = loggedin_page.locator("#map") - expect(map_el).to_be_visible() - enable = loggedin_page.get_by_role("link", name="Edit") - expect(enable).to_be_visible() - enable.click() - disable = loggedin_page.get_by_role("link", name="Disable editing") - expect(disable).to_be_visible() - save = loggedin_page.get_by_title("Save current edits (Ctrl+S)") - expect(save).to_be_visible() - add_marker = loggedin_page.get_by_title("Draw a marker") - expect(add_marker).to_be_visible() - edit_settings = loggedin_page.get_by_title("Edit map settings") - expect(edit_settings).to_be_visible() - edit_permissions = loggedin_page.get_by_title("Update permissions and editors") - expect(edit_permissions).to_be_visible() - - -def test_map_update_with_anonymous(map, live_server, page): - page.goto(f"{live_server.url}{map.get_absolute_url()}") - map_el = page.locator("#map") - expect(map_el).to_be_visible() - enable = page.get_by_role("link", name="Edit") - expect(enable).to_be_hidden() - - -def test_owner_permissions_form(map, datalayer, live_server, loggedin_page): - loggedin_page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") - edit_permissions = loggedin_page.get_by_title("Update permissions and editors") - expect(edit_permissions).to_be_visible() - edit_permissions.click() - select = loggedin_page.locator(".umap-field-share_status select") - expect(select).to_be_visible() - # expect(select).to_have_value(Map.PUBLIC) # Does not work - owner_field = loggedin_page.locator(".umap-field-owner") - expect(owner_field).to_be_visible() - editors_field = loggedin_page.locator(".umap-field-editors input") - expect(editors_field).to_be_visible() - datalayer_label = loggedin_page.get_by_text('Who can edit "Donau"') - expect(datalayer_label).to_be_visible() diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py new file mode 100644 index 00000000..4f6bd8ba --- /dev/null +++ b/umap/tests/integration/test_owned_map.py @@ -0,0 +1,112 @@ +from time import sleep + +import pytest +from playwright.sync_api import expect + +from umap.models import DataLayer + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def login(context, user, settings, live_server): + def do_login(username): + # TODO use storage state to do login only once per session + # https://playwright.dev/python/docs/auth + settings.ENABLE_ACCOUNT_LOGIN = True + page = context.new_page() + page.goto(f"{live_server.url}/en/") + page.locator(".login").click() + page.get_by_placeholder("Username").fill(username) + page.get_by_placeholder("Password").fill("123123") + page.locator('#login_form input[type="submit"]').click() + sleep(1) # Time for ajax login POST to proceed + return page + + return do_login + + +def test_map_update_with_owner(map, live_server, login): + page = login(map.owner.username) + page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + disable = page.get_by_role("link", name="Disable editing") + expect(disable).to_be_visible() + save = page.get_by_title("Save current edits (Ctrl+S)") + expect(save).to_be_visible() + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = page.get_by_title("Edit map settings") + expect(edit_settings).to_be_visible() + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() + + +def test_map_update_with_anonymous(map, live_server, page): + page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_hidden() + + +def test_map_update_with_anonymous_but_editable_datalayer( + map, datalayer, live_server, page +): + datalayer.edit_status = DataLayer.ANONYMOUS + datalayer.save() + page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = page.get_by_title("Edit map settings") + expect(edit_settings).to_be_hidden() + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_hidden() + + +def test_owner_permissions_form(map, datalayer, live_server, login): + page = login(map.owner.username) + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + 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() + # expect(select).to_have_value(Map.PUBLIC) # Does not work + 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 "Donau"') + expect(datalayer_label).to_be_visible() + + +def test_map_update_with_editor(map, live_server, login, user): + map.editors.add(user) + map.save() + page = login(user.username) + page.goto(f"{live_server.url}{map.get_absolute_url()}") + map_el = page.locator("#map") + expect(map_el).to_be_visible() + enable = page.get_by_role("link", name="Edit") + expect(enable).to_be_visible() + enable.click() + disable = page.get_by_role("link", name="Disable editing") + expect(disable).to_be_visible() + save = page.get_by_title("Save current edits (Ctrl+S)") + expect(save).to_be_visible() + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + edit_settings = page.get_by_title("Edit map settings") + expect(edit_settings).to_be_visible() + edit_permissions = page.get_by_title("Update permissions and editors") + expect(edit_permissions).to_be_visible() From af44b3a0a62660b61228d215e1251c2d6fcfc63f Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 12 Sep 2023 11:19:03 +0200 Subject: [PATCH 17/40] Add playwright test for editor's permissions form --- umap/tests/integration/test_owned_map.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 4f6bd8ba..3795d21a 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -110,3 +110,22 @@ def test_map_update_with_editor(map, live_server, login, user): expect(edit_settings).to_be_visible() edit_permissions = page.get_by_title("Update permissions and editors") expect(edit_permissions).to_be_visible() + + +def test_permissions_form_with_editor(map, datalayer, live_server, login, user): + map.editors.add(user) + map.save() + page = login(user.username) + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + 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() + # expect(select).to_have_value(Map.PUBLIC) # Does not work + 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_visible() + datalayer_label = page.get_by_text('Who can edit "Donau"') + expect(datalayer_label).to_be_visible() From 2d1d9281d9149b89b49f72a5aade8403a636db39 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 12 Sep 2023 11:36:04 +0200 Subject: [PATCH 18/40] Add SQL migration to populate DataLayer.edit_status from Map.edit_status --- umap/migrations/0014_migrate_edit_status.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 umap/migrations/0014_migrate_edit_status.py diff --git a/umap/migrations/0014_migrate_edit_status.py b/umap/migrations/0014_migrate_edit_status.py new file mode 100644 index 00000000..bf1b1e1d --- /dev/null +++ b/umap/migrations/0014_migrate_edit_status.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.2 on 2023-09-12 09:24 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("umap", "0013_datalayer_edit_status"), + ] + + operations = [ + migrations.RunSQL( + "UPDATE umap_datalayer SET edit_status=(" + "SELECT edit_status FROM umap_map WHERE umap_map.id=umap_datalayer.map_id);" + ) + ] From de7c693c7b7919d611cbb5a41a274cde43dfbb33 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 12 Sep 2023 16:15:30 +0200 Subject: [PATCH 19/40] Remove console.log --- umap/static/umap/js/umap.layer.js | 1 - 1 file changed, 1 deletion(-) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 02a8ab24..1140a47a 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -351,7 +351,6 @@ L.U.DataLayer = L.Evented.extend({ this.map.get(this._dataUrl(), { callback: function (geojson, response) { this._last_modified = response.getResponseHeader('Last-Modified') - console.log(this.getName(), this.options) // FIXME: for now this 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 From 24e4aed8d39b8c21a02545ea8436b3a8c14092f1 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 12 Sep 2023 17:02:18 +0200 Subject: [PATCH 20/40] Rename allowEdit in editMode --- umap/models.py | 2 +- umap/static/umap/js/umap.controls.js | 4 ++-- umap/static/umap/js/umap.js | 21 +++++++++++++-------- umap/static/umap/js/umap.layer.js | 7 ++++--- umap/static/umap/test/_pre.js | 2 +- umap/templatetags/umap_tags.py | 2 +- umap/tests/test_map_views.py | 6 +++--- umap/views.py | 22 +++++++++++++--------- 8 files changed, 38 insertions(+), 28 deletions(-) diff --git a/umap/models.py b/umap/models.py index 97df55f9..8de2105d 100644 --- a/umap/models.py +++ b/umap/models.py @@ -366,7 +366,7 @@ class DataLayer(NamedModel): } obj["id"] = self.pk obj["permissions"] = {"edit_status": self.edit_status} - obj["allowEdit"] = self.can_edit(user, request) + obj["editMode"] = "advanced" if self.can_edit(user, request) else 'disabled' return obj def clone(self, map_inst=None): diff --git a/umap/static/umap/js/umap.controls.js b/umap/static/umap/js/umap.controls.js index 87baa753..97a6665b 100644 --- a/umap/static/umap/js/umap.controls.js +++ b/umap/static/umap/js/umap.controls.js @@ -275,7 +275,7 @@ L.U.ContinueLineAction = L.U.BaseVertexAction.extend({ // Leaflet.Toolbar doesn't allow twice same toolbar class… L.U.SettingsToolbar = L.Toolbar.Control.extend({ addTo: function (map) { - if (map.options.allowMapEdit === false) return + if (map.options.editMode !== 'advanced') return L.Toolbar.Control.prototype.addTo.call(this, map) }, }) @@ -1467,7 +1467,7 @@ L.U.IframeExporter = L.Evented.extend({ miniMap: false, scrollWheelZoom: false, zoomControl: true, - allowEdit: false, + editMode: 'disabled', moreControl: true, searchControl: null, tilelayersControl: null, diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 405e3b62..592a2a77 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -15,7 +15,7 @@ L.Map.mergeOptions({ default_interactive: true, default_labelDirection: 'auto', attributionControl: false, - allowEdit: true, + editMode: 'advanced', embedControl: true, zoomControl: true, datalayersControl: true, @@ -103,7 +103,7 @@ L.U.Map.include({ L.Util.setBooleanFromQueryString(this.options, 'moreControl') L.Util.setBooleanFromQueryString(this.options, 'scaleControl') L.Util.setBooleanFromQueryString(this.options, 'miniMap') - L.Util.setBooleanFromQueryString(this.options, 'allowEdit') + L.Util.setBooleanFromQueryString(this.options, 'editMode') L.Util.setBooleanFromQueryString(this.options, 'displayDataBrowserOnLoad') L.Util.setBooleanFromQueryString(this.options, 'displayCaptionOnLoad') L.Util.setBooleanFromQueryString(this.options, 'captionBar') @@ -122,7 +122,7 @@ L.U.Map.include({ if (this.datalayersOnLoad) this.datalayersOnLoad = this.datalayersOnLoad.toString().split(',') - if (L.Browser.ielt9) this.options.allowEdit = false // TODO include ie9 + if (L.Browser.ielt9) this.options.editMode = 'disabled' // TODO include ie9 let editedFeature = null const self = this @@ -235,7 +235,7 @@ L.U.Map.include({ this.isDirty = true this._default_extent = true this.options.name = L._('Untitled map') - this.options.allowEdit = true + this.options.editMode = 'advanced' const datalayer = this.createDataLayer() datalayer.connectToMap() this.enableEdit() @@ -253,7 +253,7 @@ L.U.Map.include({ this.slideshow = new L.U.Slideshow(this, this.options.slideshow) this.permissions = new L.U.MapPermissions(this) this.initCaptionBar() - if (this.options.allowEdit) { + if (this.hasEditMode()) { this.editTools = new L.U.Editable(this) this.ui.on( 'panel:closed panel:open', @@ -292,7 +292,7 @@ L.U.Map.include({ this.helpMenuActions = {} this._controls = {} - if (this.options.allowEdit && !this.options.noControl) { + if (this.hasEditMode() && !this.options.noControl) { new L.U.EditControl(this).addTo(this) new L.U.DrawToolbar({ map: this }).addTo(this) @@ -511,7 +511,7 @@ L.U.Map.include({ else this.ui.closePanel() } - if (!this.options.allowEdit) return + if (!this.hasEditMode()) return /* Edit mode only shortcuts */ if (key === L.U.Keys.E && modifierKey && !this.editEnabled) { @@ -1790,6 +1790,11 @@ L.U.Map.include({ this.fire('edit:disabled') }, + hasEditMode: function () { + return this.options.editMode === 'simple' || this.options.editMode === 'advanced' + }, + + getDisplayName: function () { return this.options.name || L._('Untitled map') }, @@ -1946,7 +1951,7 @@ L.U.Map.include({ items = items.concat(e.relatedTarget.getContextMenuItems(e)) } } - if (this.options.allowEdit) { + if (this.hasEditMode()) { items.push('-') if (this.editEnabled) { if (!this.hasDirty) { diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 1140a47a..ff159888 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -356,7 +356,7 @@ L.U.DataLayer = L.Evented.extend({ // So do not let all options to be reset // Fix is a proper migration so all datalayers settings are // in DB, and we remove it from geojson flat files. - geojson['_umap_options']['allowEdit'] = this.options.allowEdit + geojson['_umap_options']['editMode'] = this.options.editMode this.fromUmapGeoJSON(geojson) this.backupOptions() this.fire('loaded') @@ -496,7 +496,7 @@ L.U.DataLayer = L.Evented.extend({ }) // No browser cache for owners/editors. - if (this.map.options.allowEdit) url = `${url}?${Date.now()}` + if (this.map.hasEditMode()) url = `${url}?${Date.now()}` return url }, @@ -1194,7 +1194,8 @@ L.U.DataLayer = L.Evented.extend({ }, isReadOnly: function () { - return this.options.allowEdit === false + // isReadOnly must return true if unset + return this.options.editMode === 'disabled' }, save: function () { diff --git a/umap/static/umap/test/_pre.js b/umap/static/umap/test/_pre.js index df561c5d..25323c2c 100644 --- a/umap/static/umap/test/_pre.js +++ b/umap/static/umap/test/_pre.js @@ -190,7 +190,7 @@ function initMap(options) { name: 'name of the map', description: 'The description of the map', locale: 'en', - allowEdit: true, + editMode: 'advanced', moreControl: true, scaleControl: true, miniMap: false, diff --git a/umap/templatetags/umap_tags.py b/umap/templatetags/umap_tags.py index 3e7ae9c0..70bc244e 100644 --- a/umap/templatetags/umap_tags.py +++ b/umap/templatetags/umap_tags.py @@ -37,7 +37,7 @@ def map_fragment(map_instance, **kwargs): 'datalayers': datalayer_data, 'urls': _urls_for_js(), 'STATIC_URL': settings.STATIC_URL, - "allowEdit": False, + "editMode": 'disabled', 'hash': False, 'attributionControl': False, 'scrollWheelZoom': False, diff --git a/umap/tests/test_map_views.py b/umap/tests/test_map_views.py index 25d14350..a0f4bdcf 100644 --- a/umap/tests/test_map_views.py +++ b/umap/tests/test_map_views.py @@ -127,9 +127,9 @@ def test_wrong_slug_should_redirect_to_canonical(client, map): def test_wrong_slug_should_redirect_with_query_string(client, map): url = reverse("map", kwargs={"map_id": map.pk, "slug": "wrong-slug"}) - url = "{}?allowEdit=0".format(url) + url = "{}?editMode=simple".format(url) canonical = reverse("map", kwargs={"map_id": map.pk, "slug": map.slug}) - canonical = "{}?allowEdit=0".format(canonical) + canonical = "{}?editMode=simple".format(canonical) response = client.get(url) assert response.status_code == 301 assert response["Location"] == canonical @@ -137,7 +137,7 @@ def test_wrong_slug_should_redirect_with_query_string(client, map): def test_should_not_consider_the_query_string_for_canonical_check(client, map): url = reverse("map", kwargs={"map_id": map.pk, "slug": map.slug}) - url = "{}?allowEdit=0".format(url) + url = "{}?editMode=simple".format(url) response = client.get(url) assert response.status_code == 200 diff --git a/umap/views.py b/umap/views.py index 1bb36493..e58d75ee 100644 --- a/umap/views.py +++ b/umap/views.py @@ -449,10 +449,7 @@ class MapDetailMixin: properties = { "urls": _urls_for_js(), "tilelayers": TileLayer.get_list(), - "allowEdit": self.is_edit_allowed(), # showEditMode - "allowMapEdit": self.object.can_edit(self.request.user, self.request) - if getattr(self, "object", None) - else True, # FIXME naming + "editMode": self.edit_mode, "default_iconUrl": "%sumap/img/marker.png" % settings.STATIC_URL, # noqa "umap_id": self.get_umap_id(), "starred": self.is_starred(), @@ -500,8 +497,9 @@ class MapDetailMixin: def get_datalayers(self): return [] - def is_edit_allowed(self): - return True + @property + def edit_mode(self): + return "advanced" def get_umap_id(self): return None @@ -563,11 +561,17 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView): for l in self.object.datalayer_set.all() ] - def is_edit_allowed(self): - return self.object.can_edit(self.request.user, self.request) or any( + @property + def edit_mode(self): + edit_mode = 'disabled' + if self.object.can_edit(self.request.user, self.request): + edit_mode = "advanced" + elif any( d.can_edit(self.request.user, self.request) for d in self.object.datalayer_set.all() - ) + ): + edit_mode = "simple" + return edit_mode def get_umap_id(self): return self.object.pk From ddada8fb2b9831bccd6fde21e15bdb2c84e23d1e Mon Sep 17 00:00:00 2001 From: David Larlet Date: Tue, 12 Sep 2023 11:31:54 -0400 Subject: [PATCH 21/40] Remove the hasDirty concept And only save the map in case of an `advanced` `editMode`. --- umap/static/umap/js/umap.js | 49 +++++++++---------------- umap/static/umap/js/umap.permissions.js | 12 ++++-- 2 files changed, 25 insertions(+), 36 deletions(-) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 592a2a77..5ae8dcba 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -192,22 +192,6 @@ L.U.Map.include({ this ) - // FIXME naming - let hasDirty = false // global status - try { - Object.defineProperty(this, 'hasDirty', { - get: function () { - return hasDirty || this.dirty_datalayers.length - }, - set: function (status) { - if (!hasDirty && status) self.fire('hasdirty') - hasDirty = status - self.checkDirty() - }, - }) - } catch (e) { - // Certainly IE8, which has a limited version of defineProperty - } let isDirty = false // self status try { Object.defineProperty(this, 'isDirty', { @@ -216,7 +200,7 @@ L.U.Map.include({ }, set: function (status) { isDirty = status - if (status) hasDirty = true + this.checkDirty() }, }) } catch (e) { @@ -282,7 +266,7 @@ L.U.Map.include({ if (L.Util.queryString('download')) this.download() }) - window.onbeforeunload = () => this.hasDirty || null + window.onbeforeunload = () => this.isDirty || null this.backup() this.initContextMenu() this.on('click contextmenu.show', this.closeInplaceToolbar) @@ -521,7 +505,7 @@ L.U.Map.include({ key === L.U.Keys.E && modifierKey && this.editEnabled && - !this.hasDirty + !this.isDirty ) { L.DomEvent.stop(e) this.disableEdit() @@ -529,11 +513,11 @@ L.U.Map.include({ } if (key === L.U.Keys.S && modifierKey) { L.DomEvent.stop(e) - if (this.hasDirty) { + if (this.isDirty) { this.save() } } - if (key === L.U.Keys.Z && modifierKey && this.hasDirty) { + if (key === L.U.Keys.Z && modifierKey && this.isDirty) { L.DomEvent.stop(e) this.askForReset() } @@ -1062,18 +1046,17 @@ L.U.Map.include({ this.dirty_datalayers = [] this.updateDatalayersControl() this.initTileLayers() - this.hasDirty = false this.isDirty = false }, checkDirty: function () { - L.DomUtil.classIf(this._container, 'umap-is-dirty', this.hasDirty) + L.DomUtil.classIf(this._container, 'umap-is-dirty', this.isDirty) }, addDirtyDatalayer: function (datalayer) { if (this.dirty_datalayers.indexOf(datalayer) === -1) { this.dirty_datalayers.push(datalayer) - this.hasDirty = true + this.isDirty = true } }, @@ -1228,7 +1211,7 @@ L.U.Map.include({ }, ] } - } else if (!this.permissions.hasDirty) { + } else if (!this.permissions.isDirty) { // Do not override local changes to permissions, // but update in case some other editors changed them in the meantime. this.permissions.setOptions(data.permissions) @@ -1246,15 +1229,18 @@ L.U.Map.include({ }, save: function () { - if (!this.hasDirty) return + if (!this.isDirty) return if (this._default_extent) this.updateExtent() this.backup() this.once('saved', () => { - this.hasDirty = false this.isDirty = false }) - if (this.isDirty) this.saveSelf() - else this.permissions.save() // Map itself has no change, check permissions and continue + if (this.options.editMode === 'advanced') { + // Only save the map if the user has the rights to do so. + this.saveSelf() + } else { + this.permissions.save() + } }, sendEditLink: function () { @@ -1783,7 +1769,7 @@ L.U.Map.include({ }, disableEdit: function () { - if (this.hasDirty) return + if (this.isDirty) return L.DomUtil.removeClass(document.body, 'umap-edit-enabled') this.editedFeature = null this.editEnabled = false @@ -1794,7 +1780,6 @@ L.U.Map.include({ return this.options.editMode === 'simple' || this.options.editMode === 'advanced' }, - getDisplayName: function () { return this.options.name || L._('Untitled map') }, @@ -1954,7 +1939,7 @@ L.U.Map.include({ if (this.hasEditMode()) { items.push('-') if (this.editEnabled) { - if (!this.hasDirty) { + if (!this.isDirty) { items.push({ text: `${L._('Stop editing')} (Ctrl+E)`, callback: this.disableEdit, diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index daa1bc6f..a34b8167 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -20,7 +20,7 @@ L.U.MapPermissions = L.Class.extend({ set: function (status) { isDirty = status if (status) { - self.map.hasDirty = status + self.map.isDirty = status } }, }) @@ -60,7 +60,9 @@ L.U.MapPermissions = L.Class.extend({ title = L.DomUtil.create('h4', '', container) if (this.isAnonymousMap()) { if (this.options.anonymous_edit_url) { - const helpText = `${L._('Secret edit link:')}
${this.options.anonymous_edit_url}` + const helpText = `${L._('Secret edit link:')}
${ + this.options.anonymous_edit_url + }` L.DomUtil.add('p', 'help-text', container, helpText) } } else { @@ -182,6 +184,8 @@ L.U.MapPermissions = L.Class.extend({ }, getShareStatusDisplay: function () { - return Object.fromEntries(this.map.options.share_statuses)[this.options.share_status] - } + return Object.fromEntries(this.map.options.share_statuses)[ + this.options.share_status + ] + }, }) From 6b269125d42c040669d5f2797f4faed886309791 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 13 Sep 2023 12:00:20 +0200 Subject: [PATCH 22/40] Make sure only owner see the delete map button --- umap/static/umap/js/umap.js | 26 ++++++++++----- umap/tests/integration/test_owned_map.py | 42 ++++++++++++++++++++---- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 5ae8dcba..59be04ae 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1713,20 +1713,28 @@ L.U.Map.include({ _advancedActions: function (container) { const advancedActions = L.DomUtil.createFieldset(container, L._('Advanced actions')) const advancedButtons = L.DomUtil.create('div', 'button-bar half', advancedActions) - const del = L.DomUtil.create('a', 'button umap-delete', advancedButtons) - del.href = '#' - del.textContent = L._('Delete') - L.DomEvent.on(del, 'click', L.DomEvent.stop).on(del, 'click', this.del, this) + if (this.permissions.isOwner()) { + const del = L.DomUtil.create('a', 'button umap-delete', advancedButtons) + del.href = '#' + del.title = L._('Delete map') + del.textContent = L._('Delete') + L.DomEvent.on(del, 'click', L.DomEvent.stop).on(del, 'click', this.del, this) + const empty = L.DomUtil.create('a', 'button umap-empty', advancedButtons) + empty.href = '#' + empty.textContent = L._('Empty') + empty.title = L._('Delete all layers') + L.DomEvent.on(empty, 'click', L.DomEvent.stop).on( + empty, + 'click', + this.empty, + this + ) + } const clone = L.DomUtil.create('a', 'button umap-clone', advancedButtons) clone.href = '#' clone.textContent = L._('Clone') clone.title = L._('Clone this map') L.DomEvent.on(clone, 'click', L.DomEvent.stop).on(clone, 'click', this.clone, this) - const empty = L.DomUtil.create('a', 'button umap-empty', advancedButtons) - empty.href = '#' - empty.textContent = L._('Empty') - empty.title = L._('Delete all layers') - L.DomEvent.on(empty, 'click', L.DomEvent.stop).on(empty, 'click', this.empty, this) const download = L.DomUtil.create('a', 'button umap-download', advancedButtons) download.href = '#' download.textContent = L._('Download') diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 3795d21a..5938c6eb 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -9,15 +9,15 @@ pytestmark = pytest.mark.django_db @pytest.fixture -def login(context, user, settings, live_server): - def do_login(username): +def login(context, settings, live_server): + def do_login(user): # TODO use storage state to do login only once per session # https://playwright.dev/python/docs/auth settings.ENABLE_ACCOUNT_LOGIN = True page = context.new_page() page.goto(f"{live_server.url}/en/") page.locator(".login").click() - page.get_by_placeholder("Username").fill(username) + page.get_by_placeholder("Username").fill(user.username) page.get_by_placeholder("Password").fill("123123") page.locator('#login_form input[type="submit"]').click() sleep(1) # Time for ajax login POST to proceed @@ -27,7 +27,7 @@ def login(context, user, settings, live_server): def test_map_update_with_owner(map, live_server, login): - page = login(map.owner.username) + page = login(map.owner) page.goto(f"{live_server.url}{map.get_absolute_url()}") map_el = page.locator("#map") expect(map_el).to_be_visible() @@ -74,7 +74,7 @@ def test_map_update_with_anonymous_but_editable_datalayer( def test_owner_permissions_form(map, datalayer, live_server, login): - page = login(map.owner.username) + 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") expect(edit_permissions).to_be_visible() @@ -93,7 +93,7 @@ def test_owner_permissions_form(map, datalayer, live_server, login): def test_map_update_with_editor(map, live_server, login, user): map.editors.add(user) map.save() - page = login(user.username) + page = login(user) page.goto(f"{live_server.url}{map.get_absolute_url()}") map_el = page.locator("#map") expect(map_el).to_be_visible() @@ -115,7 +115,7 @@ def test_map_update_with_editor(map, live_server, login, user): def test_permissions_form_with_editor(map, datalayer, live_server, login, user): map.editors.add(user) map.save() - page = login(user.username) + page = login(user) page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") edit_permissions = page.get_by_title("Update permissions and editors") expect(edit_permissions).to_be_visible() @@ -129,3 +129,31 @@ def test_permissions_form_with_editor(map, datalayer, live_server, login, user): expect(editors_field).to_be_visible() datalayer_label = page.get_by_text('Who can edit "Donau"') expect(datalayer_label).to_be_visible() + + +def test_owner_has_delete_map_button(map, live_server, login): + page = login(map.owner) + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + settings = page.get_by_title("Edit map settings") + expect(settings).to_be_visible() + settings.click() + advanced = page.get_by_text("Advanced actions") + expect(advanced).to_be_visible() + advanced.click() + delete = page.get_by_role("link", name="Delete") + expect(delete).to_be_visible() + + +def test_editor_do_not_have_delete_map_button(map, live_server, login, user): + map.editors.add(user) + map.save() + page = login(user) + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + settings = page.get_by_title("Edit map settings") + expect(settings).to_be_visible() + settings.click() + advanced = page.get_by_text("Advanced actions") + expect(advanced).to_be_visible() + advanced.click() + delete = page.get_by_role("link", name="Delete") + expect(delete).to_be_hidden() From 84e3aa7121ddffbc284d6485bf69f2d89c80f2a7 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 13 Sep 2023 16:53:19 +0200 Subject: [PATCH 23/40] Add default value for DataLayer.options.editMode --- umap/static/umap/js/umap.layer.js | 5 +++-- umap/static/umap/test/Map.Export.js | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index ff159888..417cb01f 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -193,6 +193,7 @@ L.U.DataLayer = L.Evented.extend({ options: { displayOnLoad: true, browsable: true, + editMode: 'advanced', }, initialize: function (map, data) { @@ -201,8 +202,8 @@ L.U.DataLayer = L.Evented.extend({ this._layers = {} this._geojson = null this._propertiesIndex = [] - this._loaded = false // Are layer metadata loaded - this._dataloaded = false // Are layer data loaded + this._loaded = false // Are layer metadata loaded + this._dataloaded = false // Are layer data loaded this.parentPane = this.map.getPane('overlayPane') this.pane = this.map.createPane(`datalayer${L.stamp(this)}`, this.parentPane) diff --git a/umap/static/umap/test/Map.Export.js b/umap/static/umap/test/Map.Export.js index 1c17a803..09ce3588 100644 --- a/umap/static/umap/test/Map.Export.js +++ b/umap/static/umap/test/Map.Export.js @@ -207,6 +207,7 @@ describe('L.U.Map.Export', function () { _umap_options: { displayOnLoad: true, browsable: true, + editMode: 'advanced', iconClass: 'Default', name: 'Elephants', id: 62, From 36befefbc6ced0093bb9c261008aa773668fa9ee Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 13 Sep 2023 16:53:48 +0200 Subject: [PATCH 24/40] Fix permissions related frontend tests --- umap/static/umap/test/Map.js | 4 ++-- umap/static/umap/test/Permissions.js | 9 ++++----- umap/static/umap/test/_pre.js | 20 +++++++++++++++++++- umap/static/umap/test/index.html | 1 + 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/umap/static/umap/test/Map.js b/umap/static/umap/test/Map.js index c6a9eb15..9bcc7cfb 100644 --- a/umap/static/umap/test/Map.js +++ b/umap/static/umap/test/Map.js @@ -105,7 +105,7 @@ describe('L.U.Map', function () { window.confirm = oldConfirm }) - it('should ask for confirmation on delete link click', function (done) { + it('should ask for confirmation on delete link click', function () { var button = qs('a.update-map-settings') assert.ok(button, 'update map info button exists') happen.click(button) @@ -117,7 +117,7 @@ describe('L.U.Map', function () { this.server.respond() assert(window.confirm.calledOnce) window.confirm.restore() - done() + }) }) diff --git a/umap/static/umap/test/Permissions.js b/umap/static/umap/test/Permissions.js index 5f4364ac..4fb2ae70 100644 --- a/umap/static/umap/test/Permissions.js +++ b/umap/static/umap/test/Permissions.js @@ -34,11 +34,11 @@ describe('L.Permissions', function () { describe('#anonymous with cookie', function () { var button - it('should only allow edit_status', function () { + it('should not allow share_status nor owner', function () { this.map.permissions.options.anonymous_edit_url = 'http://anonymous.url' + delete this.map.permissions.options.owner button = qs('a.update-map-permissions') happen.click(button) - expect(qs('select[name="edit_status"]')).to.be.ok expect(qs('select[name="share_status"]')).not.to.be.ok expect(qs('input.edit-owner')).not.to.be.ok }) @@ -49,9 +49,10 @@ describe('L.Permissions', function () { it('should only allow editors', function () { this.map.permissions.options.owner = { id: 1, url: '/url', name: 'jojo' } + delete this.map.permissions.options.anonymous_edit_url + delete this.map.options.user button = qs('a.update-map-permissions') happen.click(button) - expect(qs('select[name="edit_status"]')).not.to.be.ok expect(qs('select[name="share_status"]')).not.to.be.ok expect(qs('input.edit-owner')).not.to.be.ok expect(qs('input.edit-editors')).to.be.ok @@ -66,8 +67,6 @@ describe('L.Permissions', function () { this.map.options.user = { id: 1, url: '/url', name: 'jojo' } button = qs('a.update-map-permissions') happen.click(button) - expect(qs('select[name="edit_status"]')).to.be.ok - expect(qs('select[name="share_status"]')).to.be.ok expect(qs('input.edit-owner')).to.be.ok expect(qs('input.edit-editors')).to.be.ok }) diff --git a/umap/static/umap/test/_pre.js b/umap/static/umap/test/_pre.js index 25323c2c..dda0c79e 100644 --- a/umap/static/umap/test/_pre.js +++ b/umap/static/umap/test/_pre.js @@ -198,6 +198,20 @@ function initMap(options) { displayCaptionOnLoad: false, displayPopupFooter: false, displayDataBrowserOnLoad: false, + permissions: { + share_status: 1, + owner: { + id: 1, + name: 'ybon', + url: '/en/user/ybon/', + }, + editors: [], + }, + user: { + id: 1, + name: 'foofoo', + url: '/en/me', + }, }, } default_options.properties.datalayers.push(defaultDatalayerData()) @@ -319,7 +333,11 @@ var RESPONSES = { datalayer64_GET: { crs: null, type: 'FeatureCollection', - _umap_options: defaultDatalayerData({name: 'hidden', id: 64, displayOnLoad: false }), + _umap_options: defaultDatalayerData({ + name: 'hidden', + id: 64, + displayOnLoad: false, + }), features: [ { geometry: { diff --git a/umap/static/umap/test/index.html b/umap/static/umap/test/index.html index cf4928f2..67a8aa59 100644 --- a/umap/static/umap/test/index.html +++ b/umap/static/umap/test/index.html @@ -37,6 +37,7 @@ + From 0afb0bff824aa32ad3b09ba76ff6e2c0f7981c31 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 13 Sep 2023 17:16:28 +0200 Subject: [PATCH 25/40] Include layer name in translation --- umap/static/umap/js/umap.datalayer.permissions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/static/umap/js/umap.datalayer.permissions.js b/umap/static/umap/js/umap.datalayer.permissions.js index 2ee16bab..c9d3f42d 100644 --- a/umap/static/umap/js/umap.datalayer.permissions.js +++ b/umap/static/umap/js/umap.datalayer.permissions.js @@ -33,7 +33,7 @@ L.U.DataLayerPermissions = L.Class.extend({ 'options.edit_status', { handler: 'IntSelect', - label: `${L._('Who can edit')} "${this.datalayer.getName()}"`, + label: L._('Who can edit "{layer}"', { layer: this.datalayer.getName() }), selectOptions: this.datalayer.map.options.edit_statuses, }, ], From 5f5196a52d4f1d7324872fc51c740ba643f46a63 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 18 Sep 2023 17:42:22 +0200 Subject: [PATCH 26/40] Make sure to select only editable layers for attachin to features --- umap/static/umap/js/umap.js | 4 +-- umap/tests/base.py | 34 +++++++++++++++--- umap/tests/conftest.py | 2 +- .../integration/test_anonymous_owned_map.py | 35 ++++++++++++++++++- umap/tests/integration/test_owned_map.py | 4 +-- 5 files changed, 69 insertions(+), 10 deletions(-) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 59be04ae..765f8d65 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1310,14 +1310,14 @@ L.U.Map.include({ datalayer = this.lastUsedDataLayer if ( datalayer && - !datalayer.isRemoteLayer() && + !datalayer.isReadOnly() && datalayer.canBrowse() && datalayer.isVisible() ) { return datalayer } datalayer = this.findDataLayer((datalayer) => { - if (!datalayer.isRemoteLayer() && datalayer.canBrowse()) { + if (!datalayer.isReadOnly() && datalayer.canBrowse()) { fallback = datalayer if (datalayer.isVisible()) return true } diff --git a/umap/tests/base.py b/umap/tests/base.py index 0beab93a..d3f160da 100644 --- a/umap/tests/base.py +++ b/umap/tests/base.py @@ -2,6 +2,7 @@ import json import factory from django.contrib.auth import get_user_model +from django.core.files.base import ContentFile from django.urls import reverse from umap.forms import DEFAULT_CENTER @@ -9,6 +10,25 @@ from umap.models import DataLayer, Licence, Map, TileLayer User = get_user_model() +DATALAYER_DATA = { + "type": "FeatureCollection", + "features": [ + { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [13.68896484375, 48.55297816440071], + }, + "properties": { + "_umap_options": {"color": "DarkCyan", "iconClass": "Ball"}, + "name": "Here", + "description": "Da place anonymous again 755", + }, + } + ], + "_umap_options": {"displayOnLoad": True, "name": "Donau", "id": 926}, +} + class LicenceFactory(factory.django.DjangoModelFactory): name = "WTFPL" @@ -82,10 +102,16 @@ class DataLayerFactory(factory.django.DjangoModelFactory): name = "test datalayer" description = "test description" display_on_load = True - settings = {"displayOnLoad": True, "browsable": True, name: "test datalayer"} - geojson = factory.django.FileField( - data="""{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[13.68896484375,48.55297816440071]},"properties":{"_umap_options":{"color":"DarkCyan","iconClass":"Ball"},"name":"Here","description":"Da place anonymous again 755"}}],"_umap_options":{"displayOnLoad":true,"name":"Donau","id":926}}""" - ) # noqa + settings = {"displayOnLoad": True, "browsable": True, "name": "test datalayer"} + geojson = factory.django.FileField() + + @factory.post_generation + def geojson_data(obj, create, extracted, **kwargs): + data = DATALAYER_DATA.copy() + obj.settings["name"] = obj.name + data["_umap_options"] = obj.settings + with open(obj.geojson.path, mode="w") as truc: + truc.write(json.dumps(data)) class Meta: model = DataLayer diff --git a/umap/tests/conftest.py b/umap/tests/conftest.py index 6d168835..3465994b 100644 --- a/umap/tests/conftest.py +++ b/umap/tests/conftest.py @@ -74,7 +74,7 @@ def allow_anonymous(settings): @pytest.fixture def datalayer(map): - return DataLayerFactory(map=map, name="Default Datalayer") + return DataLayerFactory(map=map) @pytest.fixture diff --git a/umap/tests/integration/test_anonymous_owned_map.py b/umap/tests/integration/test_anonymous_owned_map.py index 8cf377e3..54e87fd9 100644 --- a/umap/tests/integration/test_anonymous_owned_map.py +++ b/umap/tests/integration/test_anonymous_owned_map.py @@ -1,9 +1,13 @@ +import re + import pytest from django.core.signing import get_cookie_signer from playwright.sync_api import expect from umap.models import DataLayer +from ..base import DataLayerFactory + pytestmark = [pytest.mark.django_db, pytest.mark.usefixtures("allow_anonymous")] @@ -77,5 +81,34 @@ def test_owner_permissions_form(map, datalayer, live_server, owner_session): expect(owner_field).to_be_hidden() editors_field = owner_session.locator(".umap-field-editors input") expect(editors_field).to_be_hidden() - datalayer_label = owner_session.get_by_text('Who can edit "Donau"') + datalayer_label = owner_session.get_by_text('Who can edit "test datalayer"') expect(datalayer_label).to_be_visible() + + +def test_anonymous_can_add_marker_on_editable_layer( + anonymap, datalayer, live_server, page +): + datalayer.edit_status = DataLayer.OWNER + datalayer.name = "Should not be in the select" + datalayer.save() # Non editable by anonymous users + assert datalayer.map == anonymap + other = DataLayerFactory(map=anonymap, edit_status=DataLayer.ANONYMOUS, name="Editable") + assert other.map == anonymap + page.goto(f"{live_server.url}{anonymap.get_absolute_url()}?edit") + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + marker = page.locator(".leaflet-marker-icon") + map_el = page.locator("#map") + expect(marker).to_have_count(2) + expect(map_el).not_to_have_class(re.compile("umap-ui")) + add_marker.click() + map_el.click(position={"x": 100, "y": 100}) + expect(marker).to_have_count(3) + # Edit panel is open + expect(map_el).to_have_class(re.compile("umap-ui")) + datalayer_select = page.locator("select[name='datalayer']") + expect(datalayer_select).to_be_visible() + options = page.locator("select[name='datalayer'] option") + 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) diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 5938c6eb..061676f8 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -86,7 +86,7 @@ def test_owner_permissions_form(map, datalayer, live_server, login): 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 "Donau"') + datalayer_label = page.get_by_text('Who can edit "test datalayer"') expect(datalayer_label).to_be_visible() @@ -127,7 +127,7 @@ def test_permissions_form_with_editor(map, datalayer, live_server, login, user): expect(owner_field).to_be_hidden() editors_field = page.locator(".umap-field-editors input") expect(editors_field).to_be_visible() - datalayer_label = page.get_by_text('Who can edit "Donau"') + datalayer_label = page.get_by_text('Who can edit "test datalayer"') expect(datalayer_label).to_be_visible() From 360b6415cb05b880b3e0f420006bf9014b705411 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 18 Sep 2023 17:42:52 +0200 Subject: [PATCH 27/40] Make OWNER mode the default edit_status for anonymous owned maps --- umap/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/forms.py b/umap/forms.py index 636013eb..e6893690 100644 --- a/umap/forms.py +++ b/umap/forms.py @@ -53,8 +53,8 @@ class DataLayerPermissionsForm(forms.ModelForm): class AnonymousDataLayerPermissionsForm(forms.ModelForm): STATUS = ( - (Map.ANONYMOUS, _("Everyone can edit")), (Map.OWNER, _("Only editable with secret edit link")), + (Map.ANONYMOUS, _("Everyone can edit")), ) edit_status = forms.ChoiceField(choices=STATUS) From 3dc4efe7b1764eb6ce758bb0c0de847b86855d5e Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 18 Sep 2023 17:50:35 +0200 Subject: [PATCH 28/40] Remove data migration for edit_status --- umap/migrations/0014_migrate_edit_status.py | 16 ---------------- 1 file changed, 16 deletions(-) delete mode 100644 umap/migrations/0014_migrate_edit_status.py diff --git a/umap/migrations/0014_migrate_edit_status.py b/umap/migrations/0014_migrate_edit_status.py deleted file mode 100644 index bf1b1e1d..00000000 --- a/umap/migrations/0014_migrate_edit_status.py +++ /dev/null @@ -1,16 +0,0 @@ -# Generated by Django 4.2.2 on 2023-09-12 09:24 - -from django.db import migrations - - -class Migration(migrations.Migration): - dependencies = [ - ("umap", "0013_datalayer_edit_status"), - ] - - operations = [ - migrations.RunSQL( - "UPDATE umap_datalayer SET edit_status=(" - "SELECT edit_status FROM umap_map WHERE umap_map.id=umap_datalayer.map_id);" - ) - ] From 3cbd6cca40fadfbcea82f4e728d9ef8391109471 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 18 Sep 2023 20:05:17 +0200 Subject: [PATCH 29/40] Add back Map.edit_status Revert "Fix existing permissions related tests" This reverts commit 36d7d87301c54a1a40bc6bbc164120768b258fae. WIP --- umap/decorators.py | 11 +++--- umap/forms.py | 15 +++++++- umap/models.py | 20 +++++++--- umap/static/umap/js/umap.permissions.js | 20 ++++++++++ umap/templates/umap/map_table.html | 4 +- umap/tests/integration/test_owned_map.py | 5 ++- umap/tests/test_map.py | 47 +++++++++++++++++++----- umap/tests/test_map_views.py | 47 +++++++++++++++++++----- umap/tests/test_views.py | 1 + umap/views.py | 12 +++++- 10 files changed, 146 insertions(+), 36 deletions(-) diff --git a/umap/decorators.py b/umap/decorators.py index 52faa127..b9b5232a 100644 --- a/umap/decorators.py +++ b/umap/decorators.py @@ -36,11 +36,12 @@ def can_edit_map(view_func): map_inst = get_object_or_404(Map, pk=kwargs["map_id"]) user = request.user kwargs["map_inst"] = map_inst # Avoid rerequesting the map in the view - can_edit = map_inst.can_edit(user=user, request=request) - if not can_edit: - if map_inst.owner and not user.is_authenticated: - return simple_json_response(login_required=str(LOGIN_URL)) - return HttpResponseForbidden() + if map_inst.edit_status >= map_inst.EDITORS: + can_edit = map_inst.can_edit(user=user, request=request) + if not can_edit: + if map_inst.owner and not user.is_authenticated: + return simple_json_response(login_required=str(LOGIN_URL)) + return HttpResponseForbidden() return view_func(request, *args, **kwargs) return wrapper diff --git a/umap/forms.py b/umap/forms.py index e6893690..1b024d18 100644 --- a/umap/forms.py +++ b/umap/forms.py @@ -36,7 +36,20 @@ class SendLinkForm(forms.Form): class UpdateMapPermissionsForm(forms.ModelForm): class Meta: model = Map - fields = ("editors", "share_status", "owner") + fields = ("edit_status", "editors", "share_status", "owner") + + +class AnonymousMapPermissionsForm(forms.ModelForm): + STATUS = ( + (Map.OWNER, _("Only editable with secret edit link")), + (Map.ANONYMOUS, _("Everyone can edit")), + ) + + edit_status = forms.ChoiceField(choices=STATUS) + + class Meta: + model = Map + fields = ("edit_status",) class DataLayerForm(forms.ModelForm): diff --git a/umap/models.py b/umap/models.py index 8de2105d..3f8848fa 100644 --- a/umap/models.py +++ b/umap/models.py @@ -221,12 +221,16 @@ class Map(NamedModel): In anononymous mode: only "anonymous owners" (having edit cookie set) """ can = False - if not self.owner: + if request and not self.owner: if settings.UMAP_ALLOW_ANONYMOUS and self.is_anonymous_owner(request): can = True + if self.edit_status == self.ANONYMOUS: + can = True + elif user is None: + can = False elif user == self.owner: can = True - elif user in self.editors.all(): + elif self.edit_status == self.EDITORS and user in self.editors.all(): can = True return can @@ -430,12 +434,16 @@ class DataLayer(NamedModel): Define if a user can edit or not the instance, according to his account or the request. """ - can = self.map.can_edit(user, request) - if can: - # Owner or editor, no need for further checks. - return can + can = False + if not self.map.owner: + if settings.UMAP_ALLOW_ANONYMOUS and self.map.is_anonymous_owner(request): + can = True if self.edit_status == self.ANONYMOUS: can = True + elif user is not None and user == self.map.owner: + can = True + elif self.edit_status == self.EDITORS and user in self.map.editors.all(): + can = True return can diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index a34b8167..45321db4 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -5,6 +5,7 @@ L.U.MapPermissions = L.Class.extend({ owner: null, editors: [], share_status: null, + edit_status: null, }, initialize: function (map) { @@ -64,9 +65,26 @@ L.U.MapPermissions = L.Class.extend({ this.options.anonymous_edit_url }` L.DomUtil.add('p', 'help-text', container, helpText) + fields.push([ + 'options.edit_status', + { + handler: 'IntSelect', + label: L._('Who can edit'), + selectOptions: this.map.options.edit_statuses, + helpText: helpText, + }, + ]) } } else { if (this.isOwner()) { + fields.push([ + 'options.edit_status', + { + handler: 'IntSelect', + label: L._('Who can edit'), + selectOptions: this.map.options.edit_statuses, + }, + ]) fields.push([ 'options.share_status', { @@ -136,6 +154,8 @@ L.U.MapPermissions = L.Class.extend({ for (let i = 0; i < this.options.editors.length; i++) formData.append('editors', this.options.editors[i].id) } + if (this.isOwner() || this.isAnonymousMap()) + formData.append('edit_status', this.options.edit_status) if (this.isOwner()) { formData.append('owner', this.options.owner && this.options.owner.id) formData.append('share_status', this.options.share_status) diff --git a/umap/templates/umap/map_table.html b/umap/templates/umap/map_table.html index 2150838b..eb8cef5a 100644 --- a/umap/templates/umap/map_table.html +++ b/umap/templates/umap/map_table.html @@ -5,7 +5,7 @@ {% blocktrans %}Map{% endblocktrans %} {% blocktrans %}Name{% endblocktrans %} - {% blocktrans %}Who can see{% endblocktrans %} + {% blocktrans %}Who can see / edit{% endblocktrans %} {% blocktrans %}Last save{% endblocktrans %} {% blocktrans %}Owner{% endblocktrans %} {% blocktrans %}Actions{% endblocktrans %} @@ -19,7 +19,7 @@ {{ map_inst.name }} - {{ map_inst.get_share_status_display }} + {{ map_inst.get_share_status_display }} / {{ map_inst.get_edit_status_display }} {{ map_inst.modified_at }} {{ map_inst.owner }} diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 061676f8..171b8ca8 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -3,7 +3,7 @@ from time import sleep import pytest from playwright.sync_api import expect -from umap.models import DataLayer +from umap.models import DataLayer, Map pytestmark = pytest.mark.django_db @@ -91,6 +91,7 @@ def test_owner_permissions_form(map, datalayer, live_server, login): def test_map_update_with_editor(map, live_server, login, user): + map.edit_status = Map.EDITORS map.editors.add(user) map.save() page = login(user) @@ -113,6 +114,7 @@ def test_map_update_with_editor(map, live_server, login, user): def test_permissions_form_with_editor(map, datalayer, live_server, login, user): + map.edit_status = Map.EDITORS map.editors.add(user) map.save() page = login(user) @@ -145,6 +147,7 @@ def test_owner_has_delete_map_button(map, live_server, login): def test_editor_do_not_have_delete_map_button(map, live_server, login, user): + map.edit_status = Map.EDITORS map.editors.add(user) map.save() page = login(user) diff --git a/umap/tests/test_map.py b/umap/tests/test_map.py index bc1283c3..d0bb52e7 100644 --- a/umap/tests/test_map.py +++ b/umap/tests/test_map.py @@ -9,33 +9,62 @@ from .base import MapFactory pytestmark = pytest.mark.django_db -def test_anonymous_cannot_edit(map): +def test_anonymous_can_edit_if_status_anonymous(map): anonymous = AnonymousUser() + map.edit_status = map.ANONYMOUS + map.save() + assert map.can_edit(anonymous) + + +def test_anonymous_cannot_edit_if_not_status_anonymous(map): + anonymous = AnonymousUser() + map.edit_status = map.OWNER + map.save() assert not map.can_edit(anonymous) -def test_non_editors_cannot_edit(map, user): +def test_non_editors_can_edit_if_status_anonymous(map, user): assert map.owner != user + map.edit_status = map.ANONYMOUS + map.save() + assert map.can_edit(user) + + +def test_non_editors_cannot_edit_if_not_status_anonymous(map, user): + map.edit_status = map.OWNER + map.save() assert not map.can_edit(user) -def test_editors_can_edit(map, user): +def test_editors_cannot_edit_if_status_owner(map, user): + map.edit_status = map.OWNER + map.editors.add(user) + map.save() + assert not map.can_edit(user) + + +def test_editors_can_edit_if_status_editors(map, user): + map.edit_status = map.EDITORS map.editors.add(user) map.save() assert map.can_edit(user) -def test_owner_can_edit(map): - assert map.can_edit(map.owner) - - -def test_logged_in_user_should_not_be_allowed_for_anonymous_map(map, user, rf): +def test_logged_in_user_should_be_allowed_for_anonymous_map_with_anonymous_edit_status(map, user, rf): # noqa map.owner = None + map.edit_status = map.ANONYMOUS map.save() url = reverse('map_update', kwargs={'map_id': map.pk}) request = rf.get(url) request.user = user - assert not map.can_edit(user, request) + assert map.can_edit(user, request) + + +def test_anonymous_user_should_not_be_allowed_for_anonymous_map(map, user, rf): # noqa + map.owner = None + map.edit_status = map.OWNER + map.save() + assert not map.can_edit() def test_clone_should_return_new_instance(map, user): diff --git a/umap/tests/test_map_views.py b/umap/tests/test_map_views.py index a0f4bdcf..01e78c59 100644 --- a/umap/tests/test_map_views.py +++ b/umap/tests/test_map_views.py @@ -38,6 +38,7 @@ def test_create(client, user, post_data): assert created_map.center.x == 13.447265624999998 assert created_map.center.y == 48.94415123418794 assert j["permissions"] == { + "edit_status": 3, "share_status": 1, "owner": {"id": user.pk, "name": "Joe", "url": "/en/user/Joe/"}, "editors": [], @@ -166,20 +167,24 @@ def test_user_not_allowed_should_not_clone_map(client, map, user, settings): settings.UMAP_ALLOW_ANONYMOUS = False assert Map.objects.count() == 1 url = reverse("map_clone", kwargs={"map_id": map.pk}) + map.edit_status = map.OWNER + map.save() response = client.post(url) assert login_required(response) client.login(username=user.username, password="123123") response = client.post(url) assert response.status_code == 403 + map.edit_status = map.ANONYMOUS + map.save() client.logout() response = client.post(url) - assert response.status_code == 200 - assert "login_required" in json.loads(response.content) + assert response.status_code == 403 assert Map.objects.count() == 1 def test_clone_should_set_cloner_as_owner(client, map, user): url = reverse("map_clone", kwargs={"map_id": map.pk}) + map.edit_status = map.EDITORS map.editors.add(user) map.save() client.login(username=user.username, password="123123") @@ -297,19 +302,19 @@ def test_only_owner_can_delete(client, map, user): assert response.status_code == 403 -def test_map_editors_cannot_change_owner(client, map, user): - owner = map.owner +def test_map_editors_do_not_see_owner_change_input(client, map, user): map.editors.add(user) + map.edit_status = map.EDITORS map.save() url = reverse("map_update_permissions", kwargs={"map_id": map.pk}) client.login(username=user.username, password="123123") - response = client.post(url, data={"owner": user.pk}) - assert response.status_code == 200 - assert map.owner == owner + response = client.get(url) + assert "id_owner" not in response -def test_logged_in_user_cannot_edit_map(client, map, user): +def test_logged_in_user_can_edit_map_editable_by_anonymous(client, map, user): map.owner = None + map.edit_status = map.ANONYMOUS map.save() client.login(username=user.username, password="123123") url = reverse("map_update", kwargs={"map_id": map.pk}) @@ -319,7 +324,8 @@ def test_logged_in_user_cannot_edit_map(client, map, user): "name": new_name, } response = client.post(url, data) - assert response.status_code == 403 + assert response.status_code == 200 + assert Map.objects.get(pk=map.pk).name == new_name @pytest.mark.usefixtures("allow_anonymous") @@ -416,9 +422,13 @@ def test_bad_anonymous_edit_url_should_return_403(cookieclient, anonymap): @pytest.mark.usefixtures("allow_anonymous") -def test_clone_anonymous_map_should_not_be_possible(client, anonymap, user): # noqa +def test_clone_anonymous_map_should_not_be_possible_if_user_is_not_allowed( + client, anonymap, user +): # noqa assert Map.objects.count() == 1 url = reverse("map_clone", kwargs={"map_id": anonymap.pk}) + anonymap.edit_status = anonymap.OWNER + anonymap.save() response = client.post(url) assert response.status_code == 403 client.login(username=user.username, password="123123") @@ -427,6 +437,23 @@ def test_clone_anonymous_map_should_not_be_possible(client, anonymap, user): # assert Map.objects.count() == 1 +@pytest.mark.usefixtures("allow_anonymous") +def test_clone_map_should_be_possible_if_edit_status_is_anonymous( + client, anonymap +): # noqa + assert Map.objects.count() == 1 + url = reverse("map_clone", kwargs={"map_id": anonymap.pk}) + anonymap.edit_status = anonymap.ANONYMOUS + anonymap.save() + response = client.post(url) + assert response.status_code == 200 + assert Map.objects.count() == 2 + clone = Map.objects.latest("pk") + assert clone.pk != anonymap.pk + assert clone.name == "Clone of " + anonymap.name + assert clone.owner is None + + @pytest.mark.usefixtures("allow_anonymous") def test_anyone_can_access_anonymous_map(cookieclient, anonymap): url = reverse("map", args=(anonymap.slug, anonymap.pk)) diff --git a/umap/tests/test_views.py b/umap/tests/test_views.py index df96042a..e53d2230 100644 --- a/umap/tests/test_views.py +++ b/umap/tests/test_views.py @@ -282,6 +282,7 @@ def test_user_dashboard_display_user_maps(client, map): assert f"{map.get_absolute_url()}?share" in body assert f"{map.get_absolute_url()}?download" in body assert "Everyone (public)" in body + assert "Owner only" in body @pytest.mark.django_db diff --git a/umap/views.py b/umap/views.py index e58d75ee..f7fe3327 100644 --- a/umap/views.py +++ b/umap/views.py @@ -48,6 +48,7 @@ from .forms import ( DataLayerForm, DataLayerPermissionsForm, AnonymousDataLayerPermissionsForm, + AnonymousMapPermissionsForm, FlatErrorList, MapSettingsForm, SendLinkForm, @@ -526,6 +527,7 @@ class MapDetailMixin: class PermissionsMixin: def get_permissions(self): permissions = {} + permissions["edit_status"] = self.object.edit_status permissions["share_status"] = self.object.share_status if self.object.owner: permissions["owner"] = { @@ -563,7 +565,7 @@ class MapView(MapDetailMixin, PermissionsMixin, DetailView): @property def edit_mode(self): - edit_mode = 'disabled' + edit_mode = "disabled" if self.object.can_edit(self.request.user, self.request): edit_mode = "advanced" elif any( @@ -655,12 +657,18 @@ class MapUpdate(FormLessEditMixin, PermissionsMixin, UpdateView): class UpdateMapPermissions(FormLessEditMixin, UpdateView): model = Map pk_url_kwarg = "map_id" - form_class = UpdateMapPermissionsForm + + def get_form_class(self): + if self.object.owner: + return UpdateMapPermissionsForm + else: + return AnonymousMapPermissionsForm def get_form(self, form_class=None): form = super().get_form(form_class) user = self.request.user if self.object.owner and not user == self.object.owner: + del form.fields["edit_status"] del form.fields["share_status"] del form.fields["owner"] return form From 4e54a93ee0c7086dcdf1a3714f630acd4d9a18a7 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 19 Sep 2023 08:19:20 +0200 Subject: [PATCH 30/40] Add DataLayer.INHERIT edit status option --- umap/forms.py | 5 ++- umap/models.py | 6 ++- umap/tests/test_datalayer.py | 64 ++++++++++++++++++++++++++++-- umap/tests/test_datalayer_views.py | 49 +++++++++++++++++++++++ 4 files changed, 117 insertions(+), 7 deletions(-) diff --git a/umap/forms.py b/umap/forms.py index 1b024d18..ec94e7a8 100644 --- a/umap/forms.py +++ b/umap/forms.py @@ -66,8 +66,9 @@ class DataLayerPermissionsForm(forms.ModelForm): class AnonymousDataLayerPermissionsForm(forms.ModelForm): STATUS = ( - (Map.OWNER, _("Only editable with secret edit link")), - (Map.ANONYMOUS, _("Everyone can edit")), + (DataLayer.INHERIT, _("Inherit")), + (DataLayer.OWNER, _("Only editable with secret edit link")), + (DataLayer.ANONYMOUS, _("Everyone can edit")), ) edit_status = forms.ChoiceField(choices=STATUS) diff --git a/umap/models.py b/umap/models.py index 3f8848fa..6c9c9fef 100644 --- a/umap/models.py +++ b/umap/models.py @@ -304,10 +304,12 @@ class DataLayer(NamedModel): Layer to store Features in. """ + INHERIT = 0 ANONYMOUS = 1 EDITORS = 2 OWNER = 3 EDIT_STATUS = ( + (INHERIT, _("Inherit")), (ANONYMOUS, _("Everyone")), (EDITORS, _("Editors only")), (OWNER, _("Owner only")), @@ -327,7 +329,7 @@ class DataLayer(NamedModel): ) edit_status = models.SmallIntegerField( choices=EDIT_STATUS, - default=get_default_edit_status, + default=INHERIT, verbose_name=_("edit status"), ) @@ -434,6 +436,8 @@ class DataLayer(NamedModel): Define if a user can edit or not the instance, according to his account or the request. """ + if self.edit_status == self.INHERIT: + return self.map.can_edit(user, request) can = False if not self.map.owner: if settings.UMAP_ALLOW_ANONYMOUS and self.map.is_anonymous_owner(request): diff --git a/umap/tests/test_datalayer.py b/umap/tests/test_datalayer.py index d2b70797..3464d430 100644 --- a/umap/tests/test_datalayer.py +++ b/umap/tests/test_datalayer.py @@ -4,7 +4,7 @@ import pytest from django.core.files.base import ContentFile from .base import DataLayerFactory, MapFactory -from umap.models import DataLayer +from umap.models import DataLayer, Map pytestmark = pytest.mark.django_db @@ -136,10 +136,66 @@ def test_anonymous_cannot_edit_in_anonymous_owner_mode(datalayer): assert not datalayer.can_edit() -def test_anonymous_can_edit_in_anonymous_owner_but_public_mode(datalayer): - datalayer.edit_status = DataLayer.ANONYMOUS +def test_owner_can_edit_in_inherit_mode_and_map_in_owner_mode(datalayer): + datalayer.edit_status = DataLayer.INHERIT datalayer.save() map = datalayer.map - map.owner = None + map.edit_status = Map.OWNER + map.save() + assert datalayer.can_edit(map.owner) + + +def test_editors_cannot_edit_in_inherit_mode_and_map_in_owner_mode(datalayer, user): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.editors.add(user) + map.edit_status = Map.OWNER + map.save() + assert not datalayer.can_edit(user) + + +def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_owner_mode(datalayer): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.edit_status = Map.OWNER + map.save() + assert not datalayer.can_edit() + + +def test_owner_can_edit_in_inherit_mode_and_map_in_editors_mode(datalayer): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.edit_status = Map.EDITORS + map.save() + assert datalayer.can_edit(map.owner) + + +def test_editors_can_edit_in_inherit_mode_and_map_in_editors_mode(datalayer, user): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.editors.add(user) + map.edit_status = Map.EDITORS + map.save() + assert datalayer.can_edit(user) + + +def test_anonymous_cannot_edit_in_inherit_mode_and_map_in_editors_mode(datalayer): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.edit_status = Map.EDITORS + map.save() + assert not datalayer.can_edit() + + +def test_anonymous_can_edit_in_inherit_mode_and_map_in_public_mode(datalayer): + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + map = datalayer.map + map.edit_status = Map.ANONYMOUS map.save() assert datalayer.can_edit() diff --git a/umap/tests/test_datalayer_views.py b/umap/tests/test_datalayer_views.py index 01c65db7..46b94f07 100644 --- a/umap/tests/test_datalayer_views.py +++ b/umap/tests/test_datalayer_views.py @@ -336,3 +336,52 @@ def test_editor_can_edit_in_editors_mode(datalayer, client, map, post_data): assert response.status_code == 200 modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) assert modified_datalayer.name == name + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_owner_can_edit_if_inherit_and_map_in_owner_mode( + datalayer, cookieclient, anonymap, post_data +): + anonymap.edit_status = Map.OWNER + anonymap.save() + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = cookieclient.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_user_cannot_edit_if_inherit_and_map_in_owner_mode( + datalayer, client, anonymap, post_data +): + anonymap.edit_status = Map.OWNER + anonymap.save() + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 403 + + +@pytest.mark.usefixtures("allow_anonymous") +def test_anonymous_user_can_edit_if_inherit_and_map_in_public_mode( + datalayer, client, anonymap, post_data +): + anonymap.edit_status = Map.ANONYMOUS + anonymap.save() + datalayer.edit_status = DataLayer.INHERIT + datalayer.save() + url = reverse("datalayer_update", args=(anonymap.pk, datalayer.pk)) + name = "new name" + post_data["name"] = name + response = client.post(url, post_data, follow=True) + assert response.status_code == 200 + modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) + assert modified_datalayer.name == name From 6ba8166f866654669242f16ce3c241c9ac78f598 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 19 Sep 2023 08:22:11 +0200 Subject: [PATCH 31/40] Reset DataLayer.edit_status migration --- umap/migrations/0013_datalayer_edit_status.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/umap/migrations/0013_datalayer_edit_status.py b/umap/migrations/0013_datalayer_edit_status.py index 16f3691f..c62523df 100644 --- a/umap/migrations/0013_datalayer_edit_status.py +++ b/umap/migrations/0013_datalayer_edit_status.py @@ -1,7 +1,6 @@ -# Generated by Django 4.2.2 on 2023-09-07 06:27 +# Generated by Django 4.2.2 on 2023-09-19 06:20 from django.db import migrations, models -import umap.models class Migration(migrations.Migration): @@ -14,8 +13,13 @@ class Migration(migrations.Migration): model_name="datalayer", name="edit_status", field=models.SmallIntegerField( - choices=[(1, "Everyone"), (2, "Editors only"), (3, "Owner only")], - default=umap.models.get_default_edit_status, + choices=[ + (0, "Inherit"), + (1, "Everyone"), + (2, "Editors only"), + (3, "Owner only"), + ], + default=0, verbose_name="edit status", ), ), From 5ffd1a1b10326e0e4f011b941a185607564b29a4 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 19 Sep 2023 08:37:06 +0200 Subject: [PATCH 32/40] Expose DataLayer.edit_status separately to the front They now differ from the Map.edit_status ones --- umap/static/umap/js/umap.datalayer.permissions.js | 4 ++-- umap/tests/integration/test_anonymous_owned_map.py | 12 +++++++++++- umap/tests/integration/test_owned_map.py | 8 ++++++++ umap/views.py | 6 ++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/umap/static/umap/js/umap.datalayer.permissions.js b/umap/static/umap/js/umap.datalayer.permissions.js index c9d3f42d..9e3e6211 100644 --- a/umap/static/umap/js/umap.datalayer.permissions.js +++ b/umap/static/umap/js/umap.datalayer.permissions.js @@ -34,11 +34,11 @@ L.U.DataLayerPermissions = L.Class.extend({ { handler: 'IntSelect', label: L._('Who can edit "{layer}"', { layer: this.datalayer.getName() }), - selectOptions: this.datalayer.map.options.edit_statuses, + selectOptions: this.datalayer.map.options.datalayer_edit_statuses, }, ], ], - builder = new L.U.FormBuilder(this, fields), + builder = new L.U.FormBuilder(this, fields, {className: 'umap-form datalayer-permissions'}), form = builder.build() container.appendChild(form) }, diff --git a/umap/tests/integration/test_anonymous_owned_map.py b/umap/tests/integration/test_anonymous_owned_map.py index 54e87fd9..20bc201e 100644 --- a/umap/tests/integration/test_anonymous_owned_map.py +++ b/umap/tests/integration/test_anonymous_owned_map.py @@ -83,6 +83,14 @@ def test_owner_permissions_form(map, datalayer, live_server, owner_session): expect(editors_field).to_be_hidden() datalayer_label = owner_session.get_by_text('Who can edit "test datalayer"') expect(datalayer_label).to_be_visible() + options = owner_session.locator( + ".datalayer-permissions select[name='edit_status'] option" + ) + expect(options).to_have_count(3) + option = owner_session.locator( + ".datalayer-permissions select[name='edit_status'] option:checked" + ) + expect(option).to_have_text("Inherit") def test_anonymous_can_add_marker_on_editable_layer( @@ -92,7 +100,9 @@ def test_anonymous_can_add_marker_on_editable_layer( datalayer.name = "Should not be in the select" datalayer.save() # Non editable by anonymous users assert datalayer.map == anonymap - other = DataLayerFactory(map=anonymap, edit_status=DataLayer.ANONYMOUS, name="Editable") + other = DataLayerFactory( + map=anonymap, edit_status=DataLayer.ANONYMOUS, name="Editable" + ) assert other.map == anonymap page.goto(f"{live_server.url}{anonymap.get_absolute_url()}?edit") add_marker = page.get_by_title("Draw a marker") diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 171b8ca8..3bb9477e 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -88,6 +88,14 @@ 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" + ) + 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_map_update_with_editor(map, live_server, login, user): diff --git a/umap/views.py b/umap/views.py index f7fe3327..a0aa014e 100644 --- a/umap/views.py +++ b/umap/views.py @@ -464,8 +464,14 @@ class MapDetailMixin: 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 + ] 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 ] if self.get_short_url(): From e13f3ac235bd01110702d39bc84df60c58133b35 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 09:01:58 +0200 Subject: [PATCH 33/40] Add DataLayer.isDataReadonly In some places we need to know if a given datalayer can accept new features, or not, whether because being readonly or being remote --- umap/static/umap/js/umap.forms.js | 2 +- umap/static/umap/js/umap.js | 4 ++-- umap/static/umap/js/umap.layer.js | 5 ++++ umap/tests/integration/test_map.py | 37 ++++++++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 umap/tests/integration/test_map.py diff --git a/umap/static/umap/js/umap.forms.js b/umap/static/umap/js/umap.forms.js index 5e5525bc..37c7c85f 100644 --- a/umap/static/umap/js/umap.forms.js +++ b/umap/static/umap/js/umap.forms.js @@ -396,7 +396,7 @@ L.FormBuilder.DataLayerSwitcher = L.FormBuilder.Select.extend({ getOptions: function () { const options = [] this.builder.map.eachDataLayerReverse((datalayer) => { - if (datalayer.isLoaded() && !datalayer.isReadOnly() && datalayer.canBrowse()) { + if (datalayer.isLoaded() && !datalayer.isDataReadOnly() && datalayer.canBrowse()) { options.push([L.stamp(datalayer), datalayer.getName()]) } }) diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index 765f8d65..df1b0c16 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1310,14 +1310,14 @@ L.U.Map.include({ datalayer = this.lastUsedDataLayer if ( datalayer && - !datalayer.isReadOnly() && + !datalayer.isDataReadOnly() && datalayer.canBrowse() && datalayer.isVisible() ) { return datalayer } datalayer = this.findDataLayer((datalayer) => { - if (!datalayer.isReadOnly() && datalayer.canBrowse()) { + if (!datalayer.isDataReadOnly() && datalayer.canBrowse()) { fallback = datalayer if (datalayer.isVisible()) return true } diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 417cb01f..bc4b5684 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -1199,6 +1199,11 @@ L.U.DataLayer = L.Evented.extend({ return this.options.editMode === 'disabled' }, + isDataReadOnly: function () { + // This layer cannot accept features + return this.isReadOnly() || this.isRemoteLayer() + }, + save: function () { if (this.isDeleted) return this.saveDelete() if (!this.isLoaded()) { diff --git a/umap/tests/integration/test_map.py b/umap/tests/integration/test_map.py new file mode 100644 index 00000000..a5dd0dbc --- /dev/null +++ b/umap/tests/integration/test_map.py @@ -0,0 +1,37 @@ +import pytest +from playwright.sync_api import expect + +from umap.models import Map + +pytestmark = pytest.mark.django_db + + +def test_remote_layer_should_not_be_used_as_datalayer_for_created_features( + map, live_server, datalayer, page +): + # Faster than doing a login + map.edit_status = Map.ANONYMOUS + map.save() + datalayer.settings["remoteData"] = { + "url": "https://overpass-api.de/api/interpreter?data=[out:xml];node[harbour=yes]({south},{west},{north},{east});out body;", + "format": "osm", + "from": "10", + } + datalayer.save() + page.goto(f"{live_server.url}{map.get_absolute_url()}?edit") + toggle = page.get_by_title("See data layers") + expect(toggle).to_be_visible() + toggle.click() + layers = page.locator(".umap-browse-datalayers li") + expect(layers).to_have_count(1) + map_el = page.locator("#map") + add_marker = page.get_by_title("Draw a marker") + expect(add_marker).to_be_visible() + marker = page.locator(".leaflet-marker-icon") + expect(marker).to_have_count(0) + add_marker.click() + map_el.click(position={"x": 100, "y": 100}) + expect(marker).to_have_count(1) + # A new datalayer has been created to host this created feature + # given the remote one cannot accept new features + expect(layers).to_have_count(2) From dfd04c33b0b1ddd3b941d9a6c02e14e076d86b1a Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 09:46:05 +0200 Subject: [PATCH 34/40] Fix typo in tests --- umap/tests/base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/umap/tests/base.py b/umap/tests/base.py index d3f160da..bc75c19e 100644 --- a/umap/tests/base.py +++ b/umap/tests/base.py @@ -107,11 +107,13 @@ class DataLayerFactory(factory.django.DjangoModelFactory): @factory.post_generation def geojson_data(obj, create, extracted, **kwargs): + # Make sure DB settings and file settings are aligned. + # At some point, file settings should be removed, but we are not there yet. data = DATALAYER_DATA.copy() obj.settings["name"] = obj.name data["_umap_options"] = obj.settings - with open(obj.geojson.path, mode="w") as truc: - truc.write(json.dumps(data)) + with open(obj.geojson.path, mode="w") as f: + f.write(json.dumps(data)) class Meta: model = DataLayer From a9b9a7e95551e62498c2c35eeec15fa41fe76e58 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 09:50:13 +0200 Subject: [PATCH 35/40] Tests: reuse name var --- umap/tests/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/tests/base.py b/umap/tests/base.py index bc75c19e..2b7703e4 100644 --- a/umap/tests/base.py +++ b/umap/tests/base.py @@ -102,7 +102,7 @@ class DataLayerFactory(factory.django.DjangoModelFactory): name = "test datalayer" description = "test description" display_on_load = True - settings = {"displayOnLoad": True, "browsable": True, "name": "test datalayer"} + settings = {"displayOnLoad": True, "browsable": True, "name": name} geojson = factory.django.FileField() @factory.post_generation From f05bdb2ac34b6b4339183fc46213ca1f78025d5d Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 09:54:06 +0200 Subject: [PATCH 36/40] Use datalayer.isDataReadOnly in feature.isReadOnly --- umap/static/umap/js/umap.features.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/umap/static/umap/js/umap.features.js b/umap/static/umap/js/umap.features.js index 8063f325..c3f2e75c 100644 --- a/umap/static/umap/js/umap.features.js +++ b/umap/static/umap/js/umap.features.js @@ -40,7 +40,7 @@ L.U.FeatureMixin = { preInit: function () {}, isReadOnly: function () { - return this.datalayer && (this.datalayer.isRemoteLayer() || this.datalayer.isReadOnly()) + return this.datalayer && this.datalayer.isDataReadOnly() }, getSlug: function () { From 73f9b883e18b7081f0704d8db2b5adb0bd0dcdb9 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 18:31:48 +0200 Subject: [PATCH 37/40] Bump playwright --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 8a99f166..a0869ca9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,7 +53,7 @@ dev = [ ] test = [ "factory-boy==3.2.1", - "playwright==1.37.0", + "playwright==1.38.0", "pytest==6.2.5", "pytest-django==4.5.2", "pytest-playwright==0.4.2", From a04acb828d38384db18d1005c72ef8bf62433b86 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 21 Sep 2023 10:25:59 +0200 Subject: [PATCH 38/40] Update Map.can_edit docstring --- umap/models.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/umap/models.py b/umap/models.py index 6c9c9fef..86c69518 100644 --- a/umap/models.py +++ b/umap/models.py @@ -217,8 +217,13 @@ class Map(NamedModel): Define if a user can edit or not the instance, according to his account or the request. - In ownership mode: only owner and editors - In anononymous mode: only "anonymous owners" (having edit cookie set) + In owner mode: + - only owner by default (OWNER) + - any editor if mode is EDITORS + - anyone otherwise (ANONYMOUS) + In anonymous owner mode: + - only owner (has ownership cookie) by default (OWNER) + - anyone otherwise (ANONYMOUS) """ can = False if request and not self.owner: From 157146dc04337fb8b53391ad2db13b9f77264fc1 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Thu, 21 Sep 2023 10:42:09 +0200 Subject: [PATCH 39/40] Do not make map name and share status label clickable unless user can edit --- umap/static/umap/js/umap.controls.js | 10 ++++++---- umap/static/umap/js/umap.js | 1 + umap/static/umap/js/umap.permissions.js | 1 + 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/umap/static/umap/js/umap.controls.js b/umap/static/umap/js/umap.controls.js index 97a6665b..2c67911c 100644 --- a/umap/static/umap/js/umap.controls.js +++ b/umap/static/umap/js/umap.controls.js @@ -1003,11 +1003,13 @@ L.U.Map.include({ } update() this.once('saved', L.bind(update, this)) - name.href = '#' - share_status.href = '#' logo.href = '/' - L.DomEvent.on(name, 'click', this.edit, this) - L.DomEvent.on(share_status, 'click', this.permissions.edit, this.permissions) + if (this.options.editMode === 'advanced') { + name.href = '#' + share_status.href = '#' + L.DomEvent.on(name, 'click', this.edit, this) + L.DomEvent.on(share_status, 'click', this.permissions.edit, this.permissions) + } this.on('postsync', L.bind(update, this)) const save = L.DomUtil.create('a', 'leaflet-control-edit-save button', container) save.href = '#' diff --git a/umap/static/umap/js/umap.js b/umap/static/umap/js/umap.js index df1b0c16..6883c025 100644 --- a/umap/static/umap/js/umap.js +++ b/umap/static/umap/js/umap.js @@ -1749,6 +1749,7 @@ L.U.Map.include({ edit: function () { if (!this.editEnabled) return + if (this.options.editMode !== 'advanced') return const container = L.DomUtil.create('div', 'umap-edit-container'), metadataFields = ['options.name', 'options.description'], title = L.DomUtil.create('h3', '', container) diff --git a/umap/static/umap/js/umap.permissions.js b/umap/static/umap/js/umap.permissions.js index 45321db4..f3cfbfa5 100644 --- a/umap/static/umap/js/umap.permissions.js +++ b/umap/static/umap/js/umap.permissions.js @@ -51,6 +51,7 @@ L.U.MapPermissions = L.Class.extend({ }, edit: function () { + if (this.map.options.editMode !== 'advanced') return if (!this.map.options.umap_id) return this.map.ui.alert({ content: L._('Please save the map first'), From c36ea1e4b803b3589e7434f3b3063bbbb4719b58 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Wed, 20 Sep 2023 18:26:04 +0200 Subject: [PATCH 40/40] 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,