Add new leaflet map background configuration #4881

robinmolen · 2024-12-03T11:19:26Z

Part of #2173

Changes

Adds a new admin config page/model for Leaflet map backgrounds. The map components can use these new background definitions to display custom backgrounds on the tile layer (for the formio-builder and sdk).

When a map component is fetched (by the formio-builder and/or sdk), the url for the background is fetched and passed along. This allows admins to update the Leaflet map background urls and makes sure that all map components use the new configuration.

This PR also adds some tests for the existing rewrite_for_request map component logic (updating the component, if global config should be used).

Checklist

Check off the items that are completed or not relevant.

Impact on features
- Checked copying a form
- Checked import/export of a form
- Config checks in the configuration overview admin page
- Problem detection in the admin email digest is handled
Release management
- I have labelled the PR as "needs-backport" accordingly
I have updated the translations assets (you do NOT need to provide translations)
- Ran ./bin/makemessages_js.sh
- Ran ./bin/compilemessages_js.sh
Commit hygiene
- Commit messages refer to the relevant Github issue
- Commit messages explain the "why" of change, not the how

codecov · 2024-12-03T11:56:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.62%. Comparing base (ac7be79) to head (b58cddf).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4881      +/-   ##
==========================================
+ Coverage   96.59%   96.62%   +0.02%     
==========================================
  Files         758      760       +2     
  Lines       25787    25822      +35     
  Branches     3379     3382       +3     
==========================================
+ Hits        24910    24951      +41     
+ Misses        612      606       -6     
  Partials      265      265

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vaszig · 2024-12-05T07:44:30Z

src/openforms/config/models/map.py

+class LeafletMapBackground(models.Model):
+    identifier = models.SlugField(
+        _("identifier"),
+        help_text=_("A unique identifier for the Leaflet map background"),


I think we should add a max length here, something like max_length=50

vaszig · 2024-12-05T07:44:56Z

src/openforms/config/models/map.py

+    )
+    url = models.URLField(
+        _("background url"),
+        help_text=_(


The same here (max length).

vaszig · 2024-12-05T07:50:51Z

src/openforms/config/tests/test_admin.py

+
+
+@disable_admin_mfa()
+class LeafletMapBackgroundTests(WebTest):


I guess these were added for test coverage?Otherwise they do not make sense (testing the already tested django code).

Check, yeah this just tests the creation and pages for the new map object.

The tests aren't particular exciting, but maybe nice as "sanity check" tests :)

vaszig · 2024-12-05T08:07:33Z

src/openforms/formio/components/custom.py

+                    identifier=component["backgroundIdentifier"]
+                )
+                # Write the background url information
+                component["url"] = leaflet_map_background.url


Let's move this out from the try block and emphasize only on the line which will cause the exception.

Yeah sounds good. Will do 👍

robinmolen · 2024-12-09T13:50:04Z

This PR is depended on open-formulieren/formio-builder#197. When the formio-builder PR is merged, the @open-formulieren/formio-builder version in the PR should be updated.

Once that has been done, this PR can be merged

robinmolen · 2024-12-16T08:32:00Z

The formio-builder has been updated. This PR can now be fully reviewed

The map background PR (#4881) will implement this properly. For the time being, this will fix the issue

sergei-maertens · 2024-12-16T15:17:20Z

src/openforms/js/components/form/map.js

+    const MAP_TILE_LAYERS = jsonScriptToVar('config-MAP_TILE_LAYERS', {
+      default: [],
+    });


I'd move this outside of the method onto the module level - since the data is supposed to already be in the DOM before the JS starts to run, we can evaluate this at load-time rather than doing the same evaluation every time a map gets rendered.

sergei-maertens · 2024-12-16T15:20:13Z

src/openforms/formio/tests/test_dynamic_config.py

+                        "lat": "43.23",
+                        "lng": "41.23",


are these stored as string or floats with the actual formio-builder? The types say number: https://github.com/open-formulieren/types/blob/5e2e2a219b4280c162c6eeffa6a51e333706bf49/src/formio/components/map.ts#L40

Ah check, yeah these should be floats. Changing it 👍

sergei-maertens · 2024-12-16T15:23:57Z

src/openforms/formio/components/custom.py

+        if component.get("tileLayerIdentifier", False):
+            try:
+                tile_layer = MapTileLayer.objects.get(
+                    identifier=component["tileLayerIdentifier"]
+                )
+            except MapTileLayer.DoesNotExist:
+                return None
+
+            # Add the tile layer url information
+            component["tileLayerUrl"] = tile_layer.url


With some shortcuts and our beloved :walrus: operator

Suggested change

if component.get("tileLayerIdentifier", False):

try:

tile_layer = MapTileLayer.objects.get(

identifier=component["tileLayerIdentifier"]

)

except MapTileLayer.DoesNotExist:

return None

# Add the tile layer url information

component["tileLayerUrl"] = tile_layer.url

if (identifier := component.get("tileLayerIdentifier")) is not None:

tile_layer = MapTileLayer.objects.filter(identifier=identifier).first()

if tile_layer is not None:

# Add the tile layer url information

component["tileLayerUrl"] = tile_layer.url

the fallback to False introduces yet another type to handle (now we have str | None rather than str | None | bool)

Ah okay cool. Yeah that objects.filter is also pretty nice

sergei-maertens · 2024-12-16T15:24:43Z

src/openforms/formio/components/custom.py

@@ -189,6 +189,20 @@ def build_serializer_field(
 class Map(BasePlugin[Component]):


it may be a good idea to define a MapComponent type that's more specific and includes the extra properties that are component specific.

yeah definitely, seeing as this component will get a bit bigger with the other changes..
I've added a MapComponent type and replaced some instances of Component with MapComponent

sergei-maertens · 2024-12-16T15:26:35Z

src/openforms/fixtures/admin_index_unlisted.json

@@ -1,5 +1,6 @@
 [
    "config.RichTextColor",
+    "config.MapTileLayer",


These should go in the configuration group, since the unlisted stuff doesn't get displayed to non-superusers, I think

(and that's probably a problem for the rich text colors)

Ahhh okay, yeah that would be a big problem..

For now i've left RichTextColors in the unlisted file

sergei-maertens · 2024-12-16T15:27:25Z

src/openforms/config/models/map.py

+        _("identifier"),
+        unique=True,
+        max_length=50,
+        help_text=_("A unique identifier for the tile layer"),


Suggested change

help_text=_("A unique identifier for the tile layer"),

help_text=_("A unique identifier for the tile layer."),

sergei-maertens · 2024-12-16T15:28:13Z

src/openforms/config/models/map.py

+    url = models.URLField(
+        _("tile layer url"),
+        max_length=255,
+        help_text=_(


I think you need a disclaimer that the tiler layers must use EPSG:28992 (Rijksdriehoek coordinate system), using other systems will probably lead to weird results.

sergei-maertens · 2024-12-16T15:30:05Z

src/openforms/config/admin.py

+    fields = [
+        "label",
+        "identifier",
+        "url",
+    ]
+    list_display = [
+        "label",
+        "identifier",
+        "url",
+    ]


I tend to favour tuples rather than lists for these properties, because:

tuples are immutable

lists defined at the module level are mutable and people tend to try nasty things where they dynamically patch the admin class and add stuff (in tests) and then they get surprises 3 months later

In general I avoid using mutable datastructures at the module-level

sergei-maertens · 2024-12-16T15:30:22Z

src/openforms/conf/locale/nl/LC_MESSAGES/django.po

Can you revert this? We'll handle translations close to release.

Yes, will do 👍

The map configuration can be edited at any moment. To make sure that we always return the correct tile layer url, fetch the url and add it to the component object dynamically. Using the mutate_config_dynamically we can now use the tile_layer_url for form steps and the form summary.

robinmolen linked an issue Dec 3, 2024 that may be closed by this pull request

Kaartmateriaal - Selecteeren eigen achtergrondkaart (PDOK/WMS/WMTS) #2173

Closed

robinmolen changed the title ~~Feature/2173 leaflet map background~~ Add new leaflet map background configuration Dec 3, 2024

robinmolen force-pushed the feature/2173-leaflet-map-background branch from 3f7f899 to ad008b4 Compare December 3, 2024 11:48

robinmolen requested review from sergei-maertens and vaszig and removed request for sergei-maertens December 3, 2024 13:16

vaszig requested changes Dec 5, 2024

View reviewed changes

robinmolen force-pushed the feature/2173-leaflet-map-background branch 2 times, most recently from fa2ddc5 to 5b507f9 Compare December 9, 2024 13:43

robinmolen force-pushed the feature/2173-leaflet-map-background branch 4 times, most recently from 9636d7d to c5444bc Compare December 16, 2024 08:29

robinmolen added a commit that referenced this pull request Dec 16, 2024

💩 Temporary fix for formio-builder

25761b7

The map background PR (#4881) will implement this properly. For the time being, this will fix the issue

robinmolen mentioned this pull request Dec 16, 2024

Temporary fix for formio-builder #4928

Closed

10 tasks

sergei-maertens reviewed Dec 16, 2024

View reviewed changes

vaszig mentioned this pull request Dec 16, 2024

[#4319] Make title of document configurable in StUF-ZDS #4906

Merged

11 tasks

robinmolen added 8 commits December 17, 2024 14:15

✨ [#2173] Add map tile layer admin configuration

6891fa1

✨ [#2173] Populate WebformBuilder getMapTileLayers

cfe9972

✨ [#2173] Show map background in form design view

ecaa45f

✨ [#2173] Add default map tile layer fixtures

d7eb7bd

✅ [#2173] Added tests for map component

7815e2f

⬆️ [#2173] Upgraded formio-builder dependency

9f90a8f

🏷️ [#2173] Add MapComponent type definition

b58cddf

robinmolen force-pushed the feature/2173-leaflet-map-background branch from c5444bc to b58cddf Compare December 17, 2024 13:17

sergei-maertens approved these changes Dec 17, 2024

View reviewed changes

sergei-maertens merged commit 77b340a into master Dec 17, 2024
28 of 32 checks passed

sergei-maertens deleted the feature/2173-leaflet-map-background branch December 17, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new leaflet map background configuration #4881

Add new leaflet map background configuration #4881

robinmolen commented Dec 3, 2024

codecov bot commented Dec 3, 2024 •

edited

Loading

vaszig Dec 5, 2024

vaszig Dec 5, 2024

vaszig Dec 5, 2024

robinmolen Dec 9, 2024

vaszig Dec 5, 2024

robinmolen Dec 9, 2024

robinmolen commented Dec 9, 2024

robinmolen commented Dec 16, 2024

sergei-maertens Dec 16, 2024

sergei-maertens Dec 16, 2024

robinmolen Dec 17, 2024

sergei-maertens Dec 16, 2024

robinmolen Dec 17, 2024

sergei-maertens Dec 16, 2024

robinmolen Dec 17, 2024

sergei-maertens Dec 16, 2024

robinmolen Dec 17, 2024

robinmolen Dec 17, 2024 •

edited

Loading

sergei-maertens Dec 16, 2024

sergei-maertens Dec 16, 2024

sergei-maertens Dec 16, 2024

sergei-maertens Dec 16, 2024

robinmolen Dec 17, 2024



		@disable_admin_mfa()
		class LeafletMapBackgroundTests(WebTest):

		@@ -189,6 +189,20 @@ def build_serializer_field(
		class Map(BasePlugin[Component]):

	help_text=_("A unique identifier for the tile layer"),
	help_text=_("A unique identifier for the tile layer."),

Add new leaflet map background configuration #4881

Add new leaflet map background configuration #4881

Conversation

robinmolen commented Dec 3, 2024

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinmolen commented Dec 9, 2024

robinmolen commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinmolen Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2024 •

edited

Loading

robinmolen Dec 17, 2024 •

edited

Loading