From 5279b7109fd2a0324d0a778c67424c8b2594e220 Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Thu, 4 Nov 2021 16:32:47 +0100 Subject: [PATCH 1/2] maps: move properties from layer to cell (#57760) --- combo/apps/maps/forms.py | 4 +-- .../migrations/0017_auto_20211104_1559.py | 23 ++++++++++++++++ .../migrations/0018_auto_20211104_1559.py | 25 ++++++++++++++++++ .../migrations/0019_auto_20211104_1603.py | 17 ++++++++++++ combo/apps/maps/models.py | 26 +++++++++++-------- .../maps/templates/maps/map_cell_form.html | 2 -- combo/apps/maps/views.py | 3 ++- tests/test_maps_cells.py | 11 +++++--- tests/test_maps_manager.py | 18 ++++++++++--- 9 files changed, 106 insertions(+), 23 deletions(-) create mode 100644 combo/apps/maps/migrations/0017_auto_20211104_1559.py create mode 100644 combo/apps/maps/migrations/0018_auto_20211104_1559.py create mode 100644 combo/apps/maps/migrations/0019_auto_20211104_1603.py diff --git a/combo/apps/maps/forms.py b/combo/apps/maps/forms.py index 45d8c877..64431947 100644 --- a/combo/apps/maps/forms.py +++ b/combo/apps/maps/forms.py @@ -66,7 +66,6 @@ class MapLayerForm(forms.ModelForm): 'icon_colour', 'cache_duration', 'include_user_identifier', - 'properties', 'geojson_query_parameter', 'geojson_accepts_circle_param', ] @@ -87,7 +86,7 @@ class MapLayerForm(forms.ModelForm): class MapLayerOptionsForm(forms.ModelForm): class Meta: model = MapLayerOptions - fields = ['map_layer', 'opacity'] + fields = ['map_layer', 'opacity', 'properties'] widgets = {'opacity': forms.NumberInput(attrs={'step': 0.1, 'min': 0, 'max': 1})} def __init__(self, *args, **kwargs): @@ -107,3 +106,4 @@ class MapLayerOptionsForm(forms.ModelForm): else: self.fields['opacity'].required = True self.fields['opacity'].initial = 1 + del self.fields['properties'] diff --git a/combo/apps/maps/migrations/0017_auto_20211104_1559.py b/combo/apps/maps/migrations/0017_auto_20211104_1559.py new file mode 100644 index 00000000..c5448b09 --- /dev/null +++ b/combo/apps/maps/migrations/0017_auto_20211104_1559.py @@ -0,0 +1,23 @@ +# Generated by Django 2.2.19 on 2021-11-04 14:59 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('maps', '0016_auto_20210927_1945'), + ] + + operations = [ + migrations.AddField( + model_name='maplayeroptions', + name='properties', + field=models.CharField( + blank=True, + help_text='List of properties to include, separated by commas', + max_length=500, + verbose_name='Properties', + ), + ), + ] diff --git a/combo/apps/maps/migrations/0018_auto_20211104_1559.py b/combo/apps/maps/migrations/0018_auto_20211104_1559.py new file mode 100644 index 00000000..ec9eed30 --- /dev/null +++ b/combo/apps/maps/migrations/0018_auto_20211104_1559.py @@ -0,0 +1,25 @@ +# Generated by Django 2.2.19 on 2021-11-04 14:59 + +from django.db import migrations + + +def populate_cell_properties(apps, schema_editor): + Map = apps.get_model('maps', 'Map') + MapLayerOptions = apps.get_model('maps', 'MapLayerOptions') + + for cell in Map.objects.all(): + for layer in cell.layers.all(): + MapLayerOptions.objects.update_or_create( + map_cell=cell, map_layer=layer, defaults={'properties': layer.properties} + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('maps', '0017_auto_20211104_1559'), + ] + + operations = [ + migrations.RunPython(populate_cell_properties, migrations.RunPython.noop), + ] diff --git a/combo/apps/maps/migrations/0019_auto_20211104_1603.py b/combo/apps/maps/migrations/0019_auto_20211104_1603.py new file mode 100644 index 00000000..6cd5693f --- /dev/null +++ b/combo/apps/maps/migrations/0019_auto_20211104_1603.py @@ -0,0 +1,17 @@ +# Generated by Django 2.2.19 on 2021-11-04 15:03 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('maps', '0018_auto_20211104_1559'), + ] + + operations = [ + migrations.RemoveField( + model_name='maplayer', + name='properties', + ), + ] diff --git a/combo/apps/maps/models.py b/combo/apps/maps/models.py index dde62eca..af58c9c0 100644 --- a/combo/apps/maps/models.py +++ b/combo/apps/maps/models.py @@ -21,6 +21,7 @@ from django import forms from django.conf import settings from django.core import serializers, validators from django.db import models +from django.db.models import OuterRef, Subquery from django.urls import reverse from django.utils.encoding import python_2_unicode_compatible from django.utils.html import escape @@ -129,12 +130,6 @@ class MapLayer(models.Model): icon_colour = models.CharField(_('Icon colour'), max_length=7, default='#000000') cache_duration = models.PositiveIntegerField(_('Cache duration'), default=60, help_text=_('In seconds.')) include_user_identifier = models.BooleanField(_('Include user identifier in request'), default=True) - properties = models.CharField( - _('Properties'), - max_length=500, - blank=True, - help_text=_('List of properties to include, separated by commas'), - ) geojson_query_parameter = models.CharField( _("Query parameter for fulltext requests"), max_length=100, @@ -195,7 +190,7 @@ class MapLayer(models.Model): layer = next(serializers.deserialize('json', json.dumps([json_layer]), ignorenonexistent=True)) layer.save() - def get_geojson(self, request): + def get_geojson(self, request, properties=''): geojson_url = get_templated_url(self.geojson_url) query_parameter = self.geojson_query_parameter @@ -252,9 +247,8 @@ class MapLayer(models.Model): else: features = data - properties = [] - if self.properties: - properties = [x.strip() for x in self.properties.split(',')] + properties = [x.strip() for x in properties.split(',') if x.strip()] + if properties: for feature in features: if 'display_fields' in feature['properties']: # w.c.s. content, filter fields on varnames @@ -457,6 +451,10 @@ class Map(CellBase): def get_geojson_layers(self): if not self.pk: return [] + options = MapLayerOptions.objects.filter(map_cell=self, map_layer=OuterRef('pk')) + layers = self.layers.filter(kind='geojson').annotate( + properties=Subquery(options.values('properties')) + ) return [ { 'url': reverse('mapcell-geojson', kwargs={'cell_id': self.pk, 'layer_slug': l.slug}), @@ -466,7 +464,7 @@ class Map(CellBase): 'marker_colour': l.marker_colour, 'properties': [x.strip() for x in l.properties.split(',')], } - for l in self.layers.filter(kind='geojson') + for l in layers ] def get_cell_extra_context(self, context): @@ -537,6 +535,12 @@ class MapLayerOptions(models.Model): null=True, help_text=_('Float value between 0 (transparent) and 1 (opaque)'), ) + properties = models.CharField( + _('Properties'), + max_length=500, + blank=True, + help_text=_('List of properties to include, separated by commas'), + ) class Meta: db_table = 'maps_map_layers' diff --git a/combo/apps/maps/templates/maps/map_cell_form.html b/combo/apps/maps/templates/maps/map_cell_form.html index ab5e2a15..0e0c4d08 100644 --- a/combo/apps/maps/templates/maps/map_cell_form.html +++ b/combo/apps/maps/templates/maps/map_cell_form.html @@ -11,9 +11,7 @@ {% for option in options %}
  • {{ option.map_layer.label }} {% if option.map_layer.kind == 'tiles' %}({{ option.map_layer.get_kind_display }}){% endif %} - {% if option.map_layer.kind == 'tiles' %} {% trans "Edit" %} - {% endif %} {% trans "Delete" %}
    • {% endfor %} diff --git a/combo/apps/maps/views.py b/combo/apps/maps/views.py index 865a3ad5..84069796 100644 --- a/combo/apps/maps/views.py +++ b/combo/apps/maps/views.py @@ -29,6 +29,7 @@ class GeojsonView(View): layer = get_object_or_404(cell.layers.all(), kind='geojson', slug=kwargs['layer_slug']) if not cell.page.is_visible(request.user) or not cell.is_visible(user=request.user): return HttpResponseForbidden() - geojson = layer.get_geojson(request) + options = cell.maplayeroptions_set.get(map_layer=layer) + geojson = layer.get_geojson(request, options.properties) content_type = 'application/json' return HttpResponse(json.dumps(geojson), content_type=content_type) diff --git a/tests/test_maps_cells.py b/tests/test_maps_cells.py index fce9c674..9de8dff6 100644 --- a/tests/test_maps_cells.py +++ b/tests/test_maps_cells.py @@ -581,7 +581,7 @@ def test_get_geojson_properties(app, layer, user): cell.title = 'Map' cell.save() layer.save() - MapLayerOptions.objects.create(map_cell=cell, map_layer=layer) + options = MapLayerOptions.objects.create(map_cell=cell, map_layer=layer) with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as requests_get: layer.geojson_url = 'http://example.org/geojson?t1' @@ -596,8 +596,9 @@ def test_get_geojson_properties(app, layer, user): with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as requests_get: layer.geojson_url = 'http://example.org/geojson?t2' - layer.properties = 'name, hop' layer.save() + options.properties = 'name, hop' + options.save() requests_get.return_value = mock.Mock( content=SAMPLE_GEOJSON_CONTENT, json=lambda: json.loads(SAMPLE_GEOJSON_CONTENT), status_code=200 ) @@ -608,8 +609,9 @@ def test_get_geojson_properties(app, layer, user): with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as requests_get: layer.geojson_url = 'http://example.org/geojson?t3' - layer.properties = '' layer.save() + options.properties = '' + options.save() requests_get.return_value = mock.Mock( content=SAMPLE_WCS_GEOJSON_CONTENT, json=lambda: json.loads(SAMPLE_WCS_GEOJSON_CONTENT), @@ -621,8 +623,9 @@ def test_get_geojson_properties(app, layer, user): with mock.patch('combo.utils.requests_wrapper.RequestsSession.request') as requests_get: layer.geojson_url = 'http://example.org/geojson?t4' - layer.properties = 'id' layer.save() + options.properties = 'id' + options.save() requests_get.return_value = mock.Mock( content=SAMPLE_WCS_GEOJSON_CONTENT, json=lambda: json.loads(SAMPLE_WCS_GEOJSON_CONTENT), diff --git a/tests/test_maps_manager.py b/tests/test_maps_manager.py index a8046f0d..c901e8c8 100644 --- a/tests/test_maps_manager.py +++ b/tests/test_maps_manager.py @@ -88,7 +88,6 @@ def test_add_tiles_layer(app, admin_user): assert 'marker_colour' not in resp.context['form'].fields assert 'icon' not in resp.context['form'].fields assert 'icon_colour' not in resp.context['form'].fields - assert 'properties' not in resp.context['form'].fields assert 'geojson_query_parameter' not in resp.context['form'].fields assert 'geojson_accepts_circle_param' not in resp.context['form'].fields resp.forms[0]['label'] = 'Test' @@ -150,7 +149,6 @@ def test_edit_tiles_layer(app, admin_user, tiles_layer): assert 'marker_colour' not in resp.context['form'] assert 'icon' not in resp.context['form'] assert 'icon_colour' not in resp.context['form'] - assert 'properties' not in resp.context['form'] resp.forms[0]['tiles_default'] = False resp = resp.forms[0].submit() assert resp.location.endswith('/manage/maps/') @@ -238,10 +236,22 @@ def test_add_delete_layer(app, admin_user, layer, tiles_layer): assert options.map_layer == layer resp = resp.follow() - assert '/layer/%s/edit/' % options.pk not in resp.text assert list(cell.get_free_geojson_layers()) == [] assert list(cell.get_free_tiles_layers()) == [tiles_layer] assert '/add-layer/geojson/' not in resp.text + + resp = resp.click(href='.*/layer/%s/edit/$' % options.pk) + assert 'map_layer' not in resp.context['form'].fields + assert 'opacity' not in resp.context['form'].fields + resp.form['properties'] = 'a, b' + resp = resp.form.submit() + assert resp.status_int == 302 + assert resp.location.endswith('/manage/pages/%s/#cell-%s' % (page.pk, cell.get_reference())) + options = MapLayerOptions.objects.get() + options.refresh_from_db() + assert options.properties == 'a, b' + + resp = resp.follow() resp = resp.click(href='.*/layer/%s/delete/$' % options.pk) resp = resp.forms[0].submit() assert resp.status_int == 302 @@ -253,6 +263,7 @@ def test_add_delete_layer(app, admin_user, layer, tiles_layer): assert list(cell.get_free_tiles_layers()) == [tiles_layer] resp = resp.click(href='.*/add-layer/tiles/$') assert list(resp.context['form'].fields['map_layer'].queryset) == [tiles_layer] + assert 'properties' not in resp.context['form'].fields resp.forms[0]['map_layer'] = tiles_layer.pk resp.forms[0]['opacity'] = 1 resp = resp.forms[0].submit() @@ -267,6 +278,7 @@ def test_add_delete_layer(app, admin_user, layer, tiles_layer): resp = resp.follow() resp = resp.click(href='.*/layer/%s/edit/$' % options.pk) assert 'map_layer' not in resp.context['form'].fields + assert 'properties' not in resp.context['form'].fields resp.forms[0]['opacity'] = 0.5 resp = resp.forms[0].submit() assert resp.status_int == 302 -- 2.30.2