diff --git a/README.md b/README.md index 4faf2e3d..fdec3d60 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,6 @@ Afterwards run it with: | Option | Description | |--------------------------|------------------------------------------------------------------------------------------------------------------------------------| -| USE_LAZY_LOADING | Loads point data only after the user clicks a point. If set to false, point data is loaded together with the initial map. | | FAKE_LOGIN | If set to true, allows access to the admin panel by simply selecting the role instead of logging in. **DO NOT USE IN PRODUCTION!** | | SHOW_ACCESSIBILITY_TABLE | If set as true it shows special view to help with accessing application. | diff --git a/docs/configuration.rst b/docs/configuration.rst index 4562b0d2..5bc840a5 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -60,7 +60,6 @@ Everything below in one file — copy it and delete what you do not need: max_size: 5242880 # 5 MiB FEATURE_FLAGS: - USE_LAZY_LOADING: true CATEGORIES_HELP: true SHOW_SEARCH_BAR: true SHOW_SUGGEST_NEW_POINT_BUTTON: true @@ -165,8 +164,7 @@ Basic keys Feature flags ------------- -``FEATURE_FLAGS`` is a flat mapping of flag name to boolean. Unset flags are off, with one -exception: ``USE_LAZY_LOADING`` defaults to on. +``FEATURE_FLAGS`` is a flat mapping of flag name to boolean. Unset flags are off. Flags fall into two groups: some change what the backend does, others are handed to the frontend to decide what to render. Both are set the same way. @@ -178,13 +176,6 @@ frontend to decide what to render. Both are set the same way. * - Flag - Acts on - Effect - * - ``USE_LAZY_LOADING`` - - backend - - **On by default.** Builds the location model from ``location_obligatory_fields`` - and ``categories`` in your data source, so submitted points are validated against - them, and the "suggest a new point" form is generated from them. Set it to - ``false`` and only ``uuid``, ``position`` and ``remark`` are validated, and the - suggest form has no fields — see the note below. * - ``CATEGORIES_HELP`` - both - Enables the help-tooltip data in ``/api/categories-full``, and makes the frontend @@ -219,13 +210,6 @@ frontend to decide what to render. Both are set the same way. Never enable ``FAKE_LOGIN`` in production. It hands a logged-in session to anyone who asks for one. -.. note:: - - ``USE_LAZY_LOADING`` is named for behaviour that is now unconditional: point details - have their own endpoint (``/api/location/``) whether the flag is set or not. - What the flag still controls is schema validation, as described above. Leave it on - unless you have a reason not to. - The frontend receives the whole ``FEATURE_FLAGS`` mapping, so a plugin or a custom build can read flags Goodmap itself does not know about. diff --git a/docs/data-source.rst b/docs/data-source.rst index 1c567376..53c7acc6 100644 --- a/docs/data-source.rst +++ b/docs/data-source.rst @@ -27,8 +27,9 @@ and their schema, alongside platzky's ``site_content`` section: Note that ``plugins`` is a **sibling** of ``map``, not a key inside it. -Only ``data`` and ``categories`` are structurally required; ``suggestions`` and -``reports`` are created by the app as users submit things. +Only ``data`` is structurally required. ``categories`` defaults to no categories +if omitted (a map with only plain, unfiltered points is a valid setup); ``suggestions`` +and ``reports`` are created by the app as users submit things. Points ------ @@ -63,9 +64,9 @@ ordinary field of your own: Used as the marker popup's **subtitle**. ``remark`` (optional) - Free text. Its presence — not its content — is exposed by ``/api/locations`` as a - boolean, so the frontend can flag points that have something noteworthy without - fetching them all. + Free text. Its presence — not its content — is exposed by ``/api/locations`` as + ``marker.badge: true`` (see :ref:`data-source-marker-styles`), so the frontend can + flag points that have something noteworthy without fetching them all. Everything else is yours. Custom fields are only *shown* if you list them in ``visible_data``, and only *filterable* if you list them in ``categories``. @@ -79,6 +80,8 @@ Everything else is yours. Custom fields are only *shown* if you list them in Field schema ------------ +.. _data-model-location_obligatory_fields: + ``location_obligatory_fields`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -108,12 +111,6 @@ This drives three things at once: - **Length limits.** String fields are capped at 200 characters, lists at 20 items of at most 100 characters each. -.. important:: - - This key is only read when the ``USE_LAZY_LOADING`` feature flag is on. With it off, - nothing beyond ``uuid``/``position``/``remark`` is validated and the suggest form comes - up empty. See :ref:`config-feature-flags`. - .. _data-model-visible_data: ``visible_data`` and ``meta_data`` @@ -260,6 +257,61 @@ Each category's active mode is exposed as ``filter_mode`` in the ``/api/categories-full`` response, so a custom frontend can render the right control — checkbox or radio — without hardcoding category names. +.. _data-source-marker-styles: + +Marker styles +------------- + +``marker_styles`` picks which of your fields drive each point's pin icon and color, and +supplies the lookup tables those values are resolved through. It is entirely optional — +a map with no ``marker_styles`` still renders, just with plain pins. + +.. code-block:: json + + { + "marker_styles": { + "icon_field": "type_of_place", + "color_field": "transparency", + "icons": { + "big bridge": "https://cdn.example.com/bridge.svg", + "container": {"provider": "phosphor", "value": "shipping-container"} + }, + "colors": { + "lacking": "#c62828", + "full": "#2e7d32" + } + } + } + +``icon_field``, ``color_field`` + Names of fields on your points whose *value* selects the icon/color for that point. + Either or both may be omitted. + + Each must be declared in :ref:`data-model-location_obligatory_fields` — a field + every point is guaranteed to have, so that styling is never driven by something + only some of your points carry. A name that isn't declared there is ignored, and + pins get no icon/color from it. + +``icons`` + Maps a value of ``icon_field`` to either a plain URL string, or + ``{"provider": "phosphor", "value": ""}`` to use a `Phosphor + `_ icon by name instead of hosting your own SVG. + ``{"provider": "url", "value": "..."}`` is the plain string spelled out explicitly. + + ``phosphor`` and ``url`` are the providers GoodMap knows; each one's URL is built + server-side, so the browser only ever receives finished URLs and a new provider needs + no frontend release. An entry GoodMap cannot make sense of — an unknown ``provider``, + a missing ``value`` — is logged as a warning at startup and left out, costing that one + pin its icon rather than breaking the map. + +``colors`` + Maps a value of ``color_field`` to a CSS color. + +A point whose ``icon_field``/``color_field`` value has no entry in ``icons``/``colors`` +simply renders without that part of the styling — this is not an error. A point with a +``remark`` (see above) always gets the asterisk badge regardless of whether its icon/color +matched anything. + User submissions ---------------- diff --git a/docs/http-api.rst b/docs/http-api.rst index 49023dd6..a362a08a 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -126,8 +126,11 @@ Query parameters: curl 'http://localhost:5000/api/locations?accessible_by=bikes&lat=51.10&lon=17.05&limit=5' -Each point comes back as ``uuid``, ``position`` and ``has_remark`` — a **boolean**, whether -the point has a remark, not its text. +Each point comes back as ``uuid`` and ``position``, plus a ``marker`` object with the pin +styling: ``icon``/``color`` (the raw values of whichever fields this deployment's +``marker_styles`` config names, see :ref:`data-source-marker-styles`) and ``badge: true`` +when the point has a remark. ``marker`` is left out entirely when none of that applies to +a point, and inside it each key is left out rather than sent as ``null``/``false``. A ``lat``, ``lon`` or ``limit`` that cannot mean anything — not a number, or outside the range above — is a ``400 {"message": "Invalid request data"}`` rather than a silently @@ -147,10 +150,11 @@ Takes every parameter of :ref:`api-locations`, plus ``zoom`` (integer, **0–16* bad ``lat``, is a ``400``. Points and clusters come back in one list, told apart by ``type``. A ``"point"`` carries -a real ``uuid`` you can pass to :ref:`api-location-detail`; a ``"cluster"`` carries a -freshly-generated ``cluster_uuid`` (not stable across requests — it is a render key, not -an identifier) and the number of points it stands for. ``position`` is -``[latitude, longitude]``, as everywhere else. +a real ``uuid`` you can pass to :ref:`api-location-detail`, plus the same ``marker`` object +as ``/api/locations``; a ``"cluster"`` carries a freshly-generated ``cluster_uuid`` (not +stable across requests — it is a render key, not an identifier) and the number of points +it stands for, but no ``marker``. ``position`` is ``[latitude, longitude]``, as everywhere +else. .. _api-location-detail: diff --git a/docs/quickstart.rst b/docs/quickstart.rst index e6c2fe08..7da7b986 100644 --- a/docs/quickstart.rst +++ b/docs/quickstart.rst @@ -131,7 +131,6 @@ Create ``config.yml`` next to it: PATH: data.json FEATURE_FLAGS: - USE_LAZY_LOADING: true SHOW_SEARCH_BAR: true SHOW_SUGGEST_NEW_POINT_BUTTON: true diff --git a/e2e-tests/e2e_stress_test_config.yml b/e2e-tests/e2e_stress_test_config.yml index 8cf34d8c..50aba4fe 100644 --- a/e2e-tests/e2e_stress_test_config.yml +++ b/e2e-tests/e2e_stress_test_config.yml @@ -19,7 +19,6 @@ LANGUAGES: country: PL FEATURE_FLAGS: - USE_LAZY_LOADING: True SHOW_ACCESSIBILITY_TABLE: True USE_SERVER_SIDE_CLUSTERING: False CATEGORIES_HELP: True diff --git a/e2e-tests/e2e_test_config.template.yml b/e2e-tests/e2e_test_config.template.yml index e73ba88d..f1571613 100644 --- a/e2e-tests/e2e_test_config.template.yml +++ b/e2e-tests/e2e_test_config.template.yml @@ -21,7 +21,6 @@ LANGUAGES: country: PL FEATURE_FLAGS: - USE_LAZY_LOADING: True SHOW_ACCESSIBILITY_TABLE: True USE_SERVER_SIDE_CLUSTERING: False CATEGORIES_HELP: True diff --git a/e2e-tests/e2e_test_data_initial.json b/e2e-tests/e2e_test_data_initial.json index 17e8b58d..0f4e7474 100644 --- a/e2e-tests/e2e_test_data_initial.json +++ b/e2e-tests/e2e_test_data_initial.json @@ -112,7 +112,8 @@ "is_free": "true", "speed_limit": "10", "amenities": [ - "benches" + "benches", + "toilets" ], "uuid": "5986e755-1eaa-4121-a01c-4fef1d5d1da1" }, @@ -272,6 +273,25 @@ "cars" ] }, + "marker_styles": { + "icon_field": "type_of_place", + "color_field": "speed_limit", + "icons": { + "big bridge": { + "provider": "phosphor", + "value": "bridge" + }, + "small bridge": { + "provider": "phosphor", + "value": "footprints" + } + }, + "colors": { + "10": "#2e7d32", + "30": "#ef6c00", + "50": "#c62828" + } + }, "visible_data": [ "remark", "accessible_by", diff --git a/e2e-tests/tests/basic/test_marker_styles.py b/e2e-tests/tests/basic/test_marker_styles.py new file mode 100644 index 00000000..3d98d5f1 --- /dev/null +++ b/e2e-tests/tests/basic/test_marker_styles.py @@ -0,0 +1,105 @@ +""" +Marker Styles Tests + +Tests that the map picks pin icon/color per marker_styles (icon_field: +type_of_place, color_field: speed_limit - see e2e_test_data_initial.json), and +that a location with both a remark and a marker_styles match keeps its +type/color styling with an asterisk badge overlay, rather than losing it to a +plain, unstyled asterisk badge (see getTypedMarkerIcon.jsx/MarkerPopup.jsx). +""" + +from playwright.sync_api import Page, expect + +from tests.conftest import BASE_URL, MARKER_LOAD_TIMEOUT, open_test_popup + +# "big bridge" and "small bridge" each get their own Phosphor Icons (MIT) type +# icon - see e2e_test_data_initial.json's marker_styles.icons ({provider: +# "phosphor", value: "..."}, resolved to a jsdelivr CDN URL - see +# resolvePhosphorIconUrl.js) and getTypedMarkerIcon.jsx (icon URLs are CSS +# mask-image'd onto the pin, tinted by the matched color, rather than embedded +# as inline SVG path data). +PHOSPHOR_ICONS_CDN_BASE = "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill" +BIG_BRIDGE_TYPE_ICON_URL = f"{PHOSPHOR_ICONS_CDN_BASE}/bridge-fill.svg" +SMALL_BRIDGE_TYPE_ICON_URL = f"{PHOSPHOR_ICONS_CDN_BASE}/footprints-fill.svg" + + +class TestMarkerStyles: + """Test suite for marker_styles-driven pin icons/colors""" + + def test_fast_bridge_marker_uses_type_icon_and_red_speed_color(self, page: Page): + """Pokoju (big bridge, speed_limit=50, no remark) is the only seeded bridge + with all three of lighting+benches+toilets (amenities is an "and" category - + see test_and_filter_within_category_narrows_results in test_map.py), so + checking all three isolates its marker without relying on clustering + distance/zoom assumptions.""" + page.goto(BASE_URL, wait_until="domcontentloaded") + + # "cars" is checked by default (Pokoju is cars-accessible); narrow further. + for amenity in ("lighting", "benches", "toilets"): + page.get_by_role("checkbox", name=amenity, exact=False).click() + + marker = page.locator(".custom-typed-marker-icon") + expect(marker).to_have_count(1, timeout=MARKER_LOAD_TIMEOUT) + + # The pin shape itself (a masked div, not an inline ), filled with + # speed_limit=50's color. + pin = marker.locator(".custom-typed-marker-pin") + expect(pin).to_have_css("background-color", "rgb(198, 40, 40)") # #c62828 + # The type_of_place icon, configured for "big bridge" - masked onto a div + # via CSS rather than embedded as an inline . + type_icon = marker.locator(".custom-typed-marker-type-icon") + expect(type_icon).to_have_count(1) + expect(type_icon).to_have_css("mask-image", f'url("{BIG_BRIDGE_TYPE_ICON_URL}")') + # No remark on Pokoju, so no asterisk badge. + expect(marker.locator("span")).to_have_count(0) + + def test_slow_bridge_marker_uses_type_icon_and_green_speed_color(self, page: Page): + """Piaskowy (small bridge, speed_limit=10, no remark, toilets) is the + only seeded speed<=10 bridge with toilets - the other two speed=10 + bridges (Zwierzyniecka, Tumski) have lighting/benches but neither has + toilets, so combining the speed_limit=10 radio with the toilets + checkbox isolates it without relying on clustering distance/zoom + assumptions. "cars" is unchecked first since Piaskowy is + pedestrians-only.""" + page.goto(BASE_URL, wait_until="domcontentloaded") + + page.get_by_role("checkbox", name="cars", exact=False).click() + page.get_by_role("radio", name="10 km/h", exact=False).click() + page.get_by_role("checkbox", name="toilets", exact=False).click() + + marker = page.locator(".custom-typed-marker-icon") + expect(marker).to_have_count(1, timeout=MARKER_LOAD_TIMEOUT) + + pin = marker.locator(".custom-typed-marker-pin") + expect(pin).to_have_css("background-color", "rgb(46, 125, 50)") # #2e7d32 (speed_limit=10) + type_icon = marker.locator(".custom-typed-marker-type-icon") + expect(type_icon).to_have_count(1) + expect(type_icon).to_have_css("mask-image", f'url("{SMALL_BRIDGE_TYPE_ICON_URL}")') + # No remark on Piaskowy, so no asterisk badge. + expect(marker.locator("span")).to_have_count(0) + + def test_remarked_bridge_keeps_type_and_color_styling_with_asterisk_badge(self, page: Page): + """Zwierzyniecka has both a remark and marker_styles-matching fields + (small bridge, speed_limit=10) - it should render its normal typed/colored + pin plus an asterisk badge, not fall back to our own pin in the plain + fallback color with no type icon (every type_of_place/speed_limit value + happens to be covered by marker_styles in this seeded dataset, so that + fallback-color path isn't exercised here - it's covered at the unit + level instead, see getTypedMarkerIcon.test.jsx's "returns our own pin in + the fallback color with just the badge" case). Also guards against ever + reintroducing the old PNG-based asterisk icon this replaced. + """ + page.goto(BASE_URL, wait_until="domcontentloaded") + open_test_popup(page) + + expect(page.locator('img[alt="Marker-Asterisk"]')).to_have_count(0) + + marker = page.locator(".custom-typed-marker-icon") + expect(marker).to_have_count(1, timeout=MARKER_LOAD_TIMEOUT) + + pin = marker.locator(".custom-typed-marker-pin") + expect(pin).to_have_css("background-color", "rgb(46, 125, 50)") # #2e7d32 (speed_limit=10) + type_icon = marker.locator(".custom-typed-marker-type-icon") + expect(type_icon).to_have_count(1) + expect(type_icon).to_have_css("mask-image", f'url("{SMALL_BRIDGE_TYPE_ICON_URL}")') + expect(marker.locator("span")).to_have_text("*") diff --git a/examples/e2e_test_config.yml b/examples/e2e_test_config.yml index f80e4970..4276a68c 100644 --- a/examples/e2e_test_config.yml +++ b/examples/e2e_test_config.yml @@ -16,7 +16,6 @@ LANGUAGES: country: PL FEATURE_FLAGS: - USE_LAZY_LOADING: True USE_SERVER_SIDE_CLUSTERING: False SHOW_ACCESSIBILITY_TABLE: True FAKE_LOGIN: False diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 5e22d659..3f4eec7f 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -7938,6 +7938,50 @@ "node": "^10.12.0 || >=12.0.0" } }, + "node_modules/file-loader": { + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/file-loader/-/file-loader-6.2.0.tgz", + "integrity": "sha512-qo3glqyTa61Ytg4u73GultjHGjdRyig3tG6lPtyX/jOEJvHif9uB0/OCI2Kif6ctF3caQTW2G5gym21oAsI4pw==", + "dev": true, + "license": "MIT", + "optional": true, + "peer": true, + "dependencies": { + "loader-utils": "^2.0.0", + "schema-utils": "^3.0.0" + }, + "engines": { + "node": ">= 10.13.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/webpack" + }, + "peerDependencies": { + "webpack": "^4.0.0 || ^5.0.0" + } + }, + "node_modules/file-loader/node_modules/schema-utils": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-3.3.0.tgz", + "integrity": "sha512-pN/yOAvcC+5rQ5nERGuwrjLlYvLTbCibnZ1I7B1LaiAz9BRBlE9GMgE/eqV30P7aJQUf7Ddimy/RsbYO/GrVGg==", + "dev": true, + "license": "MIT", + "optional": true, + "peer": true, + "dependencies": { + "@types/json-schema": "^7.0.8", + "ajv": "^6.12.5", + "ajv-keywords": "^3.5.2" + }, + "engines": { + "node": ">= 10.13.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/webpack" + } + }, "node_modules/fill-range": { "version": "7.1.1", "resolved": "https://registry.npmjs.org/fill-range/-/fill-range-7.1.1.tgz", diff --git a/frontend/src/components/MarkerPopup/MarkerPopup.jsx b/frontend/src/components/MarkerPopup/MarkerPopup.jsx index 6bec59fb..2b117279 100644 --- a/frontend/src/components/MarkerPopup/MarkerPopup.jsx +++ b/frontend/src/components/MarkerPopup/MarkerPopup.jsx @@ -1,8 +1,7 @@ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useMemo } from 'react'; import PropTypes from 'prop-types'; import { Marker } from 'react-leaflet'; import { isMobile } from 'react-device-detect'; -import { Icon } from 'leaflet'; import { useTranslation } from 'react-i18next'; import httpService from '../../services/http/httpService'; import useMapStore from '../Map/store/map.store'; @@ -10,7 +9,7 @@ import useMapStore from '../Map/store/map.store'; import LocationDetailsBox from './LocationDetails'; import MobilePopup from './MobilePopup'; import DesktopPopup from './DesktopPopup'; -import iconAsterisk from '../../res/img/marker-icon-asterisk.png'; +import getTypedMarkerIcon from './getTypedMarkerIcon'; /** * Wrapper component that fetches full location details and renders them in a popup. @@ -69,17 +68,6 @@ LocationDetailsBoxWrapper.propTypes = { }).isRequired, }; -/** - * Custom Leaflet icon for markers with remarks/special annotations. - * Displays an asterisk icon to visually distinguish remarked locations from standard markers. - */ -const asteriskIcon = new Icon({ - iconUrl: iconAsterisk, - iconSize: [40, 48], // size of the icon - iconAnchor: [19, 46], // point of the icon which will correspond to marker's location - popupAnchor: [0, -40], // point from which the popup should open relative to the iconAnchor -}); - /** * Interactive map marker component that displays location details in a popup when clicked. * Supports special visual indication for locations with remarks using an asterisk icon. @@ -87,7 +75,7 @@ const asteriskIcon = new Icon({ * @param {Object} props - Component props * @param {Object} props.place - Location data object * @param {number[]} props.place.position - Coordinates [latitude, longitude] - * @param {boolean} [props.place.has_remark] - Whether this location has a remark (uses asterisk icon if true) + * @param {Object} [props.place.marker] - Pin styling; marker.badge true adds an asterisk badge * @returns {React.ReactElement} Leaflet Marker component with click-to-show-details functionality */ const MarkerPopup = ({ place }) => { @@ -114,18 +102,21 @@ const MarkerPopup = ({ place }) => { setIsClicked(true); }; + // react-leaflet compares `icon` by identity, so a fresh DivIcon on every render + // would tear down and rebuild the marker's DOM - and every MarkerPopup re-renders + // whenever anything writes selectedLocationId. `place` is one entry of the fetched + // location list (see Markers.jsx), so its identity only changes on a refetch. + const typedIcon = useMemo(() => getTypedMarkerIcon(place), [place]); + const markerProps = { position: place.position, eventHandlers: { click: handleMarkerClick, }, - alt: place.has_remark ? 'Marker-Asterisk' : 'Marker', }; - // Only add icon prop if we have a custom icon (for remarks) - // This prevents passing undefined which can cause issues with MarkerClusterGroup - if (place.has_remark) { - markerProps.icon = asteriskIcon; + if (typedIcon) { + markerProps.icon = typedIcon; } return ( @@ -139,7 +130,11 @@ const MarkerPopup = ({ place }) => { MarkerPopup.propTypes = { place: PropTypes.shape({ position: PropTypes.arrayOf(PropTypes.number).isRequired, - has_remark: PropTypes.bool, // eslint-disable-line camelcase -- matches backend API schema property name + marker: PropTypes.shape({ + icon: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool]), + color: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool]), + badge: PropTypes.bool, + }), uuid: PropTypes.string.isRequired, }).isRequired, }; diff --git a/frontend/src/components/MarkerPopup/ReportProblemForm.jsx b/frontend/src/components/MarkerPopup/ReportProblemForm.jsx index 6c46004a..ec66f7b6 100644 --- a/frontend/src/components/MarkerPopup/ReportProblemForm.jsx +++ b/frontend/src/components/MarkerPopup/ReportProblemForm.jsx @@ -212,7 +212,7 @@ const ReportProblemForm = ({ placeId }) => { if (schemaError) { return ( - {t('loadReportFormError')} + {t('loadReportFormError')}
{t('retry')} diff --git a/frontend/src/components/MarkerPopup/getTypedMarkerIcon.jsx b/frontend/src/components/MarkerPopup/getTypedMarkerIcon.jsx new file mode 100644 index 00000000..88187789 --- /dev/null +++ b/frontend/src/components/MarkerPopup/getTypedMarkerIcon.jsx @@ -0,0 +1,151 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { DivIcon } from 'leaflet'; +import ReactDOMServer from 'react-dom/server'; +import PIN_SHAPE_URL from '../../res/svg/marker-pin.svg'; + +const PIN_WIDTH = 45; +const PIN_HEIGHT = 50; +// The marker's default color (used whenever marker.color doesn't match) is +// always the page's own secondary color, not a separately configurable value. +const FALLBACK_COLOR = globalThis.SECONDARY_COLOR || 'black'; + +const TYPE_ICON_SIZE = 20; +const TYPE_ICON_OFFSET_TOP = 8; +const TYPE_ICON_OFFSET_LEFT = 12; + +/** + * A configured marker_styles.icons entry, as a usable URL. + * + * Entries arrive already resolved: the backend turns whichever icon provider the + * deployment configured into a finished URL at startup (see goodmap's + * marker_styles.py), so a provider can be added there without this bundle changing. + * The type guard is just belt-and-braces against a non-string reaching the CSS. + * + * @param {string|undefined} icon + * @returns {string} The URL, or '' if unset or not a string + */ +const resolveIconUrl = icon => (typeof icon === 'string' ? icon : ''); + +/** + * Own-property lookup in a config table. A point's field value is arbitrary data, + * so a plain `table[key]` would resolve "toString"/"constructor" to an inherited + * function - truthy, and interpolated straight into the pin's CSS. + * + * @param {Object|undefined} table - icons/colors from MARKER_STYLES + * @param {*} key - this point's raw icon_field/color_field value + * @returns {*} The matching entry, or undefined + */ +const lookup = (table, key) => + table != null && Object.hasOwn(table, key) ? table[key] : undefined; + +const maskStyle = (url, color) => ({ + backgroundColor: color, + WebkitMaskImage: `url(${url})`, + maskImage: `url(${url})`, + WebkitMaskSize: '100% 100%', + maskSize: '100% 100%', + WebkitMaskRepeat: 'no-repeat', + maskRepeat: 'no-repeat', +}); + +/** + * Pin shape masked to `color`, optionally holding a type icon (`typeIconUrl`) + * inside its head, and an asterisk badge when `hasRemark` is set - so a + * remarked location keeps its type/color styling (or just its fallback color, + * if nothing else matched) instead of losing it to an unrelated asterisk icon. + */ +const PinIcon = ({ color, typeIconUrl, hasRemark }) => ( +
+
+ {typeIconUrl !== '' && ( +
+ )} + {hasRemark && ( + [-1, 1].map(y => `${x}px ${y}px 0 ${color}`)) + .join(', '), + }} + > + * + + )} +
+); + +PinIcon.propTypes = { + color: PropTypes.string.isRequired, + typeIconUrl: PropTypes.string.isRequired, + hasRemark: PropTypes.bool.isRequired, +}; + +/** + * Builds a Leaflet icon for `place`: colored/typed from the deployment's + * marker styling lookup table (window.MARKER_STYLES, see goodmap's + * marker_styles.resolve_marker_styles) when `place.marker`'s icon/color match an + * entry, our own pin in the fallback color with just the asterisk badge when + * `place.marker.badge` is set but nothing matched, or `null` (falls back to + * Leaflet's default marker) when there's neither a match nor a badge to show. + * + * MARKER_STYLES.icons maps a value to a plain URL string; whichever icon provider + * the deployment configured was already resolved away server-side. + * + * @param {Object} place - Location data, as returned by GET /api/locations + * @param {Object} [place.marker] - Pin styling: {icon, color, badge} + * @returns {import('leaflet').DivIcon|null} + */ +const getTypedMarkerIcon = place => { + const { icons, colors } = globalThis.MARKER_STYLES || {}; + const marker = place.marker || {}; + + const typeIconUrl = resolveIconUrl(lookup(icons, marker.icon)); + const matchedColor = lookup(colors, marker.color) || ''; + const hasRemark = Boolean(marker.badge); + + if (!typeIconUrl && !matchedColor && !hasRemark) { + return null; + } + + return new DivIcon({ + html: ReactDOMServer.renderToString( + , + ), + className: 'custom-typed-marker-icon', + iconSize: [PIN_WIDTH, PIN_HEIGHT], + iconAnchor: [PIN_WIDTH / 2, PIN_HEIGHT], + popupAnchor: [0, -PIN_HEIGHT], + }); +}; + +export default getTypedMarkerIcon; diff --git a/frontend/src/res/img/marker-icon-asterisk.png b/frontend/src/res/img/marker-icon-asterisk.png deleted file mode 100644 index 9e5cf850..00000000 Binary files a/frontend/src/res/img/marker-icon-asterisk.png and /dev/null differ diff --git a/frontend/src/res/svg/marker-pin.svg b/frontend/src/res/svg/marker-pin.svg new file mode 100644 index 00000000..395dd963 --- /dev/null +++ b/frontend/src/res/svg/marker-pin.svg @@ -0,0 +1 @@ + diff --git a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx index 26b446fd..086448ae 100644 --- a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx @@ -24,17 +24,15 @@ describe('MarkerPopup integration with MarkerClusterGroup', () => { { position: [51.1095, 17.0525], uuid: 'location-1', - has_remark: false, // eslint-disable-line camelcase }, { position: [51.10655, 17.0555], uuid: 'location-2', - has_remark: true, // eslint-disable-line camelcase + marker: { badge: true }, }, { position: [51.1085, 17.0535], uuid: 'location-3', - has_remark: false, // eslint-disable-line camelcase }, ]; diff --git a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx index 9689d6ca..0cc8c1e5 100644 --- a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx @@ -2,6 +2,7 @@ import React from 'react'; import '@testing-library/jest-dom'; import { render, screen, fireEvent, act, waitFor } from '@testing-library/react'; import { MapContainer } from 'react-leaflet'; +import { Marker as LeafletMarker } from 'leaflet'; import MarkerPopup from '../../src/components/MarkerPopup/MarkerPopup'; import httpService from '../../src/services/http/httpService'; @@ -10,7 +11,6 @@ jest.mock('../../src/services/http/httpService'); const location = { position: [51.1095, 17.0525], uuid: '21231', - has_remark: false, // eslint-disable-line camelcase -- matches backend API schema property name }; const locationData = { @@ -105,9 +105,8 @@ describe('MarkerPopup with remark', () => { globalThis.fetch.mockRestore(); }); - it('should render marker popup with asterisks when remark is true', () => { - // eslint-disable-next-line camelcase -- matches backend API schema property name - const locationWhenRemarkIsTrue = { ...location, has_remark: true }; + it('should render our own pin with an asterisk badge when remark is true', () => { + const locationWhenRemarkIsTrue = { ...location, marker: { badge: true } }; act(() => { render( { , ); }); - expect(screen.getByAltText(/Marker-Asterisk/i)).toBeInTheDocument(); + const marker = document.querySelector('.custom-typed-marker-icon'); + expect(marker).toBeInTheDocument(); + expect(marker.querySelector('span')).toHaveTextContent('*'); }); it('should pass custom icon prop when remark is true', () => { - // eslint-disable-next-line camelcase -- matches backend API schema property name - const locationWithRemark = { ...location, has_remark: true }; + const locationWithRemark = { ...location, marker: { badge: true } }; act(() => { render( { ); }); - const marker = screen.getByAltText(/Marker-Asterisk/i); - const leafletMarker = marker.closest('.leaflet-marker-icon'); + const marker = document.querySelector('.custom-typed-marker-icon'); - // When remark is true, marker should have custom asterisk icon - expect(leafletMarker).toBeInTheDocument(); + // When remark is true, marker should have our own pin, not Leaflet's default icon + expect(marker).toBeInTheDocument(); - // Verify custom asterisk icon dimensions (40x48) are applied - const style = window.getComputedStyle(leafletMarker); - expect(style.width).toBe('40px'); // asteriskIcon width - expect(style.height).toBe('48px'); // asteriskIcon height + // Verify our pin's dimensions (45x50) are applied, not Leaflet's default (25x41) + const style = window.getComputedStyle(marker); + expect(style.width).toBe('45px'); + expect(style.height).toBe('50px'); + }); + + it('does not rebuild the icon on a re-render that leaves place.marker alone', () => { + // react-leaflet compares icon by identity (updateMarker in react-leaflet/lib/Marker.js), + // so a fresh DivIcon per render re-runs setIcon - and with it renderToString and a + // full rewrite of the pin - on every store write, for every marker on the map. + const setIcon = jest.spyOn(LeafletMarker.prototype, 'setIcon'); + const locationWithRemark = { ...location, marker: { badge: true } }; + const tree = zoom => ( + + + + ); + + // render/rerender already flush their own updates, so no act() wrapper here. + const { rerender } = render(tree(10)); + setIcon.mockClear(); + + rerender(tree(11)); + + try { + expect(setIcon).not.toHaveBeenCalled(); + } finally { + setIcon.mockRestore(); + } }); }); diff --git a/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx b/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx new file mode 100644 index 00000000..ca4c928c --- /dev/null +++ b/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx @@ -0,0 +1,227 @@ +import getTypedMarkerIcon from '../../src/components/MarkerPopup/getTypedMarkerIcon'; + +// window.MARKER_STYLES is server-rendered JSON (see goodmap's map.html/db.get_marker_styles), +// so fixtures are parsed from JSON strings here too - keeps the snake_case backend field +// names (default_color) faithful to what actually arrives. +const setMarkerStyles = json => { + globalThis.MARKER_STYLES = JSON.parse(json); +}; + +describe('getTypedMarkerIcon', () => { + afterEach(() => { + delete globalThis.MARKER_STYLES; + }); + + it('returns null when window.MARKER_STYLES is not set (legacy/unconfigured backend)', () => { + expect(getTypedMarkerIcon({ uuid: '1', position: [50, 50] })).toBeNull(); + }); + + it('returns null when window.MARKER_STYLES is set but empty (default db config)', () => { + setMarkerStyles('{}'); + expect(getTypedMarkerIcon({ uuid: '1', position: [50, 50] })).toBeNull(); + }); + + it('returns null when place.marker has no matching icon or color entry', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "colors": { "open": "#2e7d32" } + }`); + + expect( + getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'unknownType' }, + }), + ).toBeNull(); + }); + + it.each(['toString', 'constructor', 'valueOf', 'hasOwnProperty'])( + 'returns null for the inherited Object.prototype member %s', + member => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "colors": { "open": "#2e7d32" } + }`); + + expect( + getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: member, color: member }, + }), + ).toBeNull(); + }, + ); + + it('builds a DivIcon when marker.icon matches a configured type icon', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'parcelLocker' }, + }); + + expect(icon).not.toBeNull(); + expect(icon.options.html).toContain('https://cdn.example.com/parcel-locker.svg'); + expect(icon.options.html).toContain('background-color:black'); // fallback color, no marker.color set + expect(icon.options.iconSize).toEqual([45, 50]); + }); + + it('masks the icon URL through CSS so it picks up the matched color, instead of embedding SVG path data', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "colors": { "open": "#2e7d32" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'parcelLocker', color: 'open' }, + }); + + // both the pin body (our own marker-pin.svg) and the type icon are CSS-masked + //
s tinted via background-color, not inline SVG , so + // any icon set (not just single-path ones) works for either. + expect(icon.options.html).toContain( + 'mask-image:url(https://cdn.example.com/parcel-locker.svg)', + ); + expect(icon.options.html).not.toContain(' { + setMarkerStyles(`{ + "colors": { "open": "#2e7d32" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { color: 'open' }, + }); + + expect(icon).not.toBeNull(); + expect(icon.options.html).toContain('#2e7d32'); + }); + + it('picks the color matching each value on a multi-tier color (e.g. speed-based coloring)', () => { + setMarkerStyles(`{ + "colors": { "10": "#2e7d32", "30": "#ef6c00", "50": "#c62828" } + }`); + + const iconFor = color => + getTypedMarkerIcon({ uuid: '1', position: [50, 50], marker: { color } }); + + expect(iconFor('10').options.html).toContain('#2e7d32'); + expect(iconFor('30').options.html).toContain('#ef6c00'); + expect(iconFor('50').options.html).toContain('#c62828'); + }); + + it('adds an asterisk badge when marker.badge is set and a match was found', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'parcelLocker', badge: true }, + }); + + expect(icon).not.toBeNull(); + expect(icon.options.html).toContain('https://cdn.example.com/parcel-locker.svg'); // keeps the type icon + expect(icon.options.html).toContain('>*'); // asterisk badge overlay + }); + + it('omits the asterisk badge when marker.badge is not set', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'parcelLocker' }, + }); + + expect(icon.options.html).not.toContain('>*'); + }); + + it('returns our own pin in the fallback color with just the badge when marker.badge is set but nothing matches', () => { + setMarkerStyles('{}'); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { badge: true }, + }); + + expect(icon).not.toBeNull(); + expect(icon.options.html).toContain('background-color:black'); // fallback color + expect(icon.options.html).toContain('>*'); + expect(icon.options.html).not.toContain('custom-typed-marker-type-icon'); + }); + + it('still returns null when there is neither a match nor a badge to show', () => { + setMarkerStyles('{}'); + + expect(getTypedMarkerIcon({ uuid: '1', position: [50, 50] })).toBeNull(); + }); + + it('ignores a configured default_color and uses the page fallback color instead', () => { + setMarkerStyles(`{ + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "default_color": "#123456" + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'parcelLocker' }, + }); + + expect(icon.options.html).not.toContain('#123456'); + expect(icon.options.html).toContain('background-color:black'); + }); +}); + +describe('getTypedMarkerIcon icon value shapes', () => { + afterEach(() => { + delete globalThis.MARKER_STYLES; + }); + + // The backend resolves whichever icon provider the deployment configured into a + // finished URL before it ever reaches window.MARKER_STYLES (see goodmap's + // marker_styles.py), so a plain URL string is the only shape this sees. + it('uses an icon entry as a direct URL', () => { + setMarkerStyles(`{ + "icons": { "container": "https://cdn.example.com/c.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'container' }, + }); + + expect(icon.options.html).toContain('mask-image:url(https://cdn.example.com/c.svg)'); + }); + + it('ignores a non-string icon entry rather than putting it in the CSS', () => { + setMarkerStyles(`{ + "icons": { "container": { "provider": "phosphor", "value": "shipping-container" } } + }`); + + expect( + getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + marker: { icon: 'container' }, + }), + ).toBeNull(); + }); +}); diff --git a/frontend/webpack.config.js b/frontend/webpack.config.js index aed77ad2..423790dc 100644 --- a/frontend/webpack.config.js +++ b/frontend/webpack.config.js @@ -6,7 +6,7 @@ const deps = require('./package.json').dependencies; module.exports = (env, argv) => { const IS_PROD = argv.mode === 'production'; - const runOnAllInterfaces = env && env.serve === 'network'; + const runOnAllInterfaces = env?.serve === 'network'; return { plugins: [ @@ -21,6 +21,7 @@ module.exports = (env, argv) => { ], cache: { type: 'filesystem', + name: env?.serve ? 'dev-server' : 'build', cacheDirectory: path.resolve(__dirname, '.webpack-cache'), buildDependencies: { config: [__filename], diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index e59b7915..fa111072 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -5,7 +5,7 @@ and request/response validation. """ -from typing import Any, Literal +from typing import Any, Literal, NamedTuple from pydantic import BaseModel, Field, RootModel @@ -68,18 +68,67 @@ class SuccessResponse(BaseModel): message: str = Field(..., description="Success message") +class PinMarkerFields(NamedTuple): + """Which of this deployment's fields drive pin icon/color, threaded through to + marker_style_values() by name, keeping the two roles distinct (a frozenset of + both would lose which is which).""" + + icon_field: str | None = None + color_field: str | None = None + + +class MarkerInfo(BaseModel): + """Pin styling for one point: keys into the deployment's MARKER_STYLES lookup + tables, not resolved server-side to a URL/hex value.""" + + icon: str | int | float | bool | None = Field( + None, description="Raw icon_field value; key into MARKER_STYLES.icons" + ) + color: str | int | float | bool | None = Field( + None, description="Raw color_field value; key into MARKER_STYLES.colors" + ) + badge: bool | None = Field( + None, description="Present and true only when the point has a remark" + ) + + class LocationBasicInfo(BaseModel): - """One point as returned by the list endpoint: identity and position only.""" + """One point as returned by the list endpoint: identity, position, and pin styling.""" uuid: str = Field(..., description="Location UUID") position: tuple[Latitude, Longitude] = Field(..., description=_POSITION_DESCRIPTION) - has_remark: bool = Field( - ..., description="Whether the point has a remark, not the remark itself" + marker: MarkerInfo | None = Field( + None, + description="Pin styling for this point; absent when nothing applies " + "(no icon/color match and no remark)", ) class LocationList(RootModel[list[LocationBasicInfo]]): - """List of points, each with identity and position only.""" + """List of points, each with identity, position, and pin styling.""" + + +def marker_style_values(location: BaseModel, pin_marker_fields: PinMarkerFields) -> dict[str, Any]: + """Pin styling data for `location`, as /api/locations includes it for every point. + + Returns {"marker": {...}} with icon/color (this location's value of the field + pin_marker_fields names, if it has one) and badge (present and true only when the + point has a remark), or {} when none of those apply. This is API response shaping, + not something the location domain model needs to know how to do itself - it + belongs alongside the models it fills, not on LocationBase. + """ + marker: dict[str, Any] = {} + if pin_marker_fields.icon_field is not None: + value = getattr(location, pin_marker_fields.icon_field, None) + if value is not None: + marker["icon"] = value + if pin_marker_fields.color_field is not None: + value = getattr(location, pin_marker_fields.color_field, None) + if value is not None: + marker["color"] = value + if getattr(location, "remark", None): + marker["badge"] = True + return {"marker": marker} if marker else {} class ClusterInfo(BaseModel): @@ -95,6 +144,9 @@ class ClusterInfo(BaseModel): cluster_count: int | None = Field( None, description="Number of points the cluster stands for; null for a point" ) + marker: MarkerInfo | None = Field( + None, description="Pin styling for a point; null for a cluster or unstyled point" + ) class ClusterList(RootModel[list[ClusterInfo]]): diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 09be7455..3bf188e0 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -29,8 +29,10 @@ LocationReportRequest, LocationReportResponse, LocationSchemaResponse, + PinMarkerFields, SuccessResponse, VersionResponse, + marker_style_values, ) from goodmap.clustering import ( MAX_ZOOM, @@ -118,20 +120,27 @@ def make_tuple_translation(keys_to_translate): return [(x, gettext(x)) for x in keys_to_translate] -def get_locations_from_request(database, request_args): +def get_locations_from_request(database, request_args, pin_marker_fields): """ Shared helper to fetch locations from database based on request arguments. Args: database: Database instance request_args: Request arguments (flask.request.args) + pin_marker_fields: This deployment's marker_styles icon_field/color_field + names - merged into each location's basic_info as a nested `marker` + object so the frontend can style pins without a further per-location + request. Returns: - List of locations as basic_info dicts + List of locations as basic_info dicts, each merged with marker_style_values. """ query_params = request_args.to_dict(flat=False) all_locations = database.get_locations(query_params) - return [x.basic_info() for x in all_locations] + return [ + {**location.basic_info(), **marker_style_values(location, pin_marker_fields)} + for location in all_locations + ] def photo_attachment_from_request(photo_attachment_config: AttachmentConfig): @@ -195,6 +204,7 @@ def core_pages( photo_attachment_config: AttachmentConfig, feature_flags: FeatureFlagSet, shortcodes: dict[str, Shortcode], + pin_marker_fields: PinMarkerFields, ) -> Blueprint: core_api_blueprint = Blueprint("api", __name__, url_prefix="/api") @@ -341,10 +351,11 @@ def report_location(): def get_locations(): """Get list of locations with basic info. - Returns locations filtered by query parameters, - showing only uuid, position, and whether each has a remark. + Returns locations filtered by query parameters: uuid, position, and a + `marker` object (icon/color/badge) with everything needed to render a + styled pin. """ - locations = get_locations_from_request(database, request.args) + locations = get_locations_from_request(database, request.args, pin_marker_fields) return jsonify(locations) @core_api_blueprint.route("/locations-clustered", methods=["GET"]) @@ -363,7 +374,7 @@ def get_locations_clustered(): query_params = request.args.to_dict(flat=False) zoom = int(query_params.get("zoom", [7])[0]) - points = get_locations_from_request(database, request.args) + points = get_locations_from_request(database, request.args, pin_marker_fields) if not points: return jsonify([]) diff --git a/goodmap/clustering.py b/goodmap/clustering.py index aa048236..1528893f 100644 --- a/goodmap/clustering.py +++ b/goodmap/clustering.py @@ -25,11 +25,12 @@ def map_clustering_data_to_proper_lazy_loading_object(input_array): Args: input_array: List of cluster dicts with 'count', 'longitude', 'latitude', - and 'uuid' keys. + 'uuid' and 'marker' keys. Returns: List of response dicts with 'position', 'uuid', 'cluster_uuid', - 'cluster_count', and 'type' keys. + 'cluster_count' and 'type' keys, plus 'marker' for points that have + any pin styling. """ response_array = [] for item in input_array: @@ -41,6 +42,10 @@ def map_clustering_data_to_proper_lazy_loading_object(input_array): "cluster_count": None, "type": "point", } + # Left out entirely rather than sent as null for an unstyled point, so a + # point here looks exactly like the same point from /api/locations. + if item.get("marker") is not None: + response_object["marker"] = item["marker"] response_array.append(response_object) continue response_object = { @@ -61,17 +66,19 @@ def match_clusters_uuids(points, clusters): Match single-point clusters to their original point UUIDs. For clusters containing exactly one point, this function attempts to match the cluster - coordinates back to the original point to retrieve its UUID. The 'uuid' key is optional - and will only be present in single-point clusters where a matching point is found. + coordinates back to the original point to retrieve its UUID and marker styling. The + 'uuid'/'marker' keys are optional and will only be present in single-point clusters + where a matching point is found. Args: - points: List of point dicts with 'position' and 'uuid' keys + points: List of point dicts with 'position', 'uuid' and 'marker' keys clusters: List of cluster dicts with 'longitude', 'latitude', and 'count' keys. - For single-point clusters (count=1), a 'uuid' key will be added if a - matching point is found (modified in place) + For single-point clusters (count=1), 'uuid' and 'marker' keys will be + added, from the matching point if found or None otherwise (modified + in place) Returns: - The modified clusters list with 'uuid' keys added to matched single-point clusters + The modified clusters list with 'uuid'/'marker' keys added to single-point clusters """ points_coords = [(point["position"][0], point["position"][1]) for point in points] tree = KDTree(points_coords) @@ -82,6 +89,7 @@ def match_clusters_uuids(points, clusters): if dist < DISTANCE_THRESHOLD: closest_point = points[idx] cluster["uuid"] = closest_point["uuid"] + cluster["marker"] = closest_point.get("marker") else: # Log warning when no match is found - indicates data inconsistency logger.warning( @@ -93,4 +101,5 @@ def match_clusters_uuids(points, clusters): DISTANCE_THRESHOLD, ) cluster["uuid"] = None + cluster["marker"] = None return clusters diff --git a/goodmap/data_models/location.py b/goodmap/data_models/location.py index 183ef652..8b363cdd 100644 --- a/goodmap/data_models/location.py +++ b/goodmap/data_models/location.py @@ -85,10 +85,8 @@ def model_dump(self, **kwargs) -> dict[str, Any]: return super().model_dump(**kwargs) def basic_info(self) -> dict[str, Any]: - """Get basic location information summary.""" - data = self.model_dump(include={"uuid", "position"}) - data["has_remark"] = bool(self.remark) - return data + """Get basic location information summary: identity and position only.""" + return self.model_dump(include={"uuid", "position"}) _TYPE_MAPPING: dict[str, type] = { diff --git a/goodmap/db.py b/goodmap/db.py index 7683861d..3b92263a 100644 --- a/goodmap/db.py +++ b/goodmap/db.py @@ -562,19 +562,83 @@ def get_meta_data(db): return globals()[f"{db.module_name}_get_meta_data"] +# ------------------------------------------------ +# get_marker_styles + + +def google_json_db_get_marker_styles(self) -> dict[str, Any]: + """ + Retrieve marker icon/color styling configuration from Google Cloud Storage JSON blob. + + Returns: + dict: Pin styling lookup table (icon_field, color_field, icons, colors). + Returns empty dict if not found. + """ + return self.data.get("map", {}).get("marker_styles", {}) + + +def json_file_db_get_marker_styles(self) -> dict[str, Any]: + """ + Retrieve marker icon/color styling configuration from JSON file database. + + Returns: + dict: Pin styling lookup table (icon_field, color_field, icons, colors). + Returns empty dict if not found. + """ + return self.data.get("map", {}).get("marker_styles", {}) + + +def json_db_get_marker_styles(self) -> dict[str, Any]: + """ + Retrieve marker icon/color styling configuration from in-memory JSON database. + + Returns: + dict: Pin styling lookup table (icon_field, color_field, icons, colors). + Returns empty dict if not found. + """ + return self.data.get("marker_styles", {}) + + +def mongodb_db_get_marker_styles(self) -> dict[str, Any]: + """ + Retrieve marker icon/color styling configuration from MongoDB. + + Returns: + dict: Pin styling lookup table (icon_field, color_field, icons, colors). + Returns empty dict if config document not found or field missing. + """ + config_doc = self.db.config.find_one({"_id": "map_config"}) + if config_doc: + return config_doc.get("marker_styles", {}) + return {} + + +def get_marker_styles(db): + """ + Get the appropriate get_marker_styles function for the given database backend. + + Args: + db: Database instance (must have module_name attribute). + + Returns: + callable: Backend-specific get_marker_styles function. + """ + return globals()[f"{db.module_name}_get_marker_styles"] + + # ------------------------------------------------ # get_categories def json_db_get_categories(self): """Return category keys from in-memory JSON database.""" - return self.data["categories"].keys() + return self.data.get("categories", {}).keys() def json_file_db_get_categories(self): """Return category keys from JSON file database.""" with open(self.data_file_path, "r") as file: - return json.load(file)["map"]["categories"].keys() + return json.load(file)["map"].get("categories", {}).keys() def google_json_db_get_categories(self): @@ -603,7 +667,7 @@ def json_db_get_category_data(self, category_type=None): """Return category data from in-memory JSON database, optionally filtered by type.""" if category_type: return { - "categories": {category_type: self.data["categories"].get(category_type, [])}, + "categories": {category_type: self.data.get("categories", {}).get(category_type, [])}, "categories_help": self.data.get("categories_help", []), "categories_options_help": { category_type: self.data.get("categories_options_help", {}).get(category_type, []) @@ -618,7 +682,7 @@ def json_db_get_category_data(self, category_type=None): }, } return { - "categories": self.data["categories"], + "categories": self.data.get("categories", {}), "categories_help": self.data.get("categories_help", []), "categories_options_help": self.data.get("categories_options_help", {}), "categories_default_checked": self.data.get("categories_default_checked", {}), @@ -632,7 +696,7 @@ def json_file_db_get_category_data(self, category_type=None): data = json.load(file)["map"] if category_type: return { - "categories": {category_type: data["categories"].get(category_type, [])}, + "categories": {category_type: data.get("categories", {}).get(category_type, [])}, "categories_help": data.get("categories_help", []), "categories_options_help": { category_type: data.get("categories_options_help", {}).get(category_type, []) @@ -645,7 +709,7 @@ def json_file_db_get_category_data(self, category_type=None): }, } return { - "categories": data["categories"], + "categories": data.get("categories", {}), "categories_help": data.get("categories_help", []), "categories_options_help": data.get("categories_options_help", {}), "categories_default_checked": data.get("categories_default_checked", {}), @@ -781,7 +845,7 @@ def get_locations_list_from_raw_data(map_data, query, location_model): """Filter and validate locations from raw map data based on query parameters. Args: - map_data: Dict containing 'data' and 'categories' keys. + map_data: Dict containing a 'data' key, and optionally 'categories'. query: Dict of query parameters for filtering. location_model: Pydantic model class to validate each location. @@ -790,7 +854,7 @@ def get_locations_list_from_raw_data(map_data, query, location_model): """ filtered_locations = get_queried_data( map_data["data"], - map_data["categories"], + map_data.get("categories", {}), query, map_data.get("categories_filter_mode", {}), ) @@ -1777,6 +1841,7 @@ def extend_db_with_goodmap_queries(db, location_model): db.extend("get_data", get_data(db)) db.extend("get_visible_data", get_visible_data(db)) db.extend("get_meta_data", get_meta_data(db)) + db.extend("get_marker_styles", get_marker_styles(db)) db.extend("get_locations", get_locations(db, location_model)) db.extend("get_locations_paginated", get_locations_paginated(db, location_model)) db.extend("get_location", get_location(db, location_model)) diff --git a/goodmap/feature_flags.py b/goodmap/feature_flags.py index 61ffe81c..2d6abb0f 100644 --- a/goodmap/feature_flags.py +++ b/goodmap/feature_flags.py @@ -5,15 +5,10 @@ Flags: CategoriesHelp: Display help text alongside map categories to guide users. - UseLazyLoading: Defer loading of location fields until they are needed, - improving initial page load performance. EnableAdminPanel: Expose the admin panel for managing map data. """ from platzky import FeatureFlag CategoriesHelp = FeatureFlag(alias="CATEGORIES_HELP", description="Show category help text") -UseLazyLoading = FeatureFlag( - alias="USE_LAZY_LOADING", default=True, description="Enable lazy loading of location fields" -) EnableAdminPanel = FeatureFlag(alias="ENABLE_ADMIN_PANEL", description="Enable admin panel") diff --git a/goodmap/goodmap.py b/goodmap/goodmap.py index 64139404..d4acbb0b 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -13,17 +13,20 @@ from platzky.models import CmsModule from platzky.plugin.content_transformer import ContentTransformerPluginBase from platzky.shortcodes import Shortcode -from pydantic import BaseModel from goodmap.api.admin_api import admin_pages +from goodmap.api.api_models import PinMarkerFields from goodmap.api.core_api import core_pages from goodmap.config import GoodmapConfig from goodmap.data_models.location import create_location_model from goodmap.db import ( extend_db_with_goodmap_queries, + get_category_data, get_location_obligatory_fields, + get_marker_styles, ) -from goodmap.feature_flags import EnableAdminPanel, UseLazyLoading +from goodmap.feature_flags import EnableAdminPanel +from goodmap.marker_styles import resolve_marker_styles from goodmap.plugin import CAPABILITY_BASES, GoodmapPluginBase logger = logging.getLogger(__name__) @@ -110,34 +113,6 @@ def _add_cors(response): return None, [] -def _setup_location_model( - db: Any, -) -> tuple[list[Any], dict[str, Any], type[BaseModel], Any]: - """Configure location model and db with lazy-loading and categories support. - - Args: - db: The database instance to extend with location queries. - - Returns: - Tuple of (obligatory_fields, categories, location_model, db). - """ - obligatory_fields = get_location_obligatory_fields(db) - location_model = create_location_model(obligatory_fields, {}) - extended_db = extend_db_with_goodmap_queries(db, location_model) - - try: - category_data = extended_db.get_category_data() - categories = category_data.get("categories", {}) - except (KeyError, AttributeError): - categories = {} - - if categories: - location_model = create_location_model(obligatory_fields, categories) - extended_db = extend_db_with_goodmap_queries(extended_db, location_model) - - return obligatory_fields, categories, location_model, extended_db - - def create_app(config_path: str) -> platzky.Engine: """Create Goodmap application from YAML configuration file. @@ -195,12 +170,39 @@ def create_app_from_config(config: GoodmapConfig) -> platzky.Engine: if app.config.get("MAX_CONTENT_LENGTH") is None: app.config["MAX_CONTENT_LENGTH"] = config.attachment.max_size + MULTIPART_OVERHEAD_ALLOWANCE - if app.is_enabled(UseLazyLoading): - location_obligatory_fields, _, location_model, app.db = _setup_location_model(app.db) - else: - location_obligatory_fields = [] - location_model = create_location_model([], {}) - app.db = extend_db_with_goodmap_queries(app.db, location_model) + # Build this deployment's location model from its data source and extend app.db + # with the query functions it needs. categories/marker_styles are both optional + # (see docs/data-source.rst) - every backend's get_category_data()/ + # get_marker_styles() already defaults them to {} internally. + # + # marker_styles is resolved once, here, so window.MARKER_STYLES.icons is a flat + # {value: url} table and the frontend never has to know about icon providers. + location_obligatory_fields = get_location_obligatory_fields(app.db) + categories = get_category_data(app.db)(app.db)["categories"] + marker_styles = resolve_marker_styles(get_marker_styles(app.db)(app.db)) + + location_model = create_location_model(location_obligatory_fields, categories) + app.db = extend_db_with_goodmap_queries(app.db, location_model) + + obligatory_field_names = {name for name, _ in location_obligatory_fields} + # pin_marker_fields is app-wiring knowledge - which of this deployment's fields + # marker_styles.icon_field/color_field actually point at - not something the + # location model itself needs to know; threaded to core_pages() for + # goodmap.api.api_models.marker_style_values() to use. A configured field that + # isn't actually an obligatory field of this deployment's locations is dropped + # rather than trusted blindly. + pin_marker_fields = PinMarkerFields( + icon_field=( + marker_styles.get("icon_field") + if marker_styles.get("icon_field") in obligatory_field_names + else None + ), + color_field=( + marker_styles.get("color_field") + if marker_styles.get("color_field") in obligatory_field_names + else None + ), + ) app.extensions["goodmap"] = {"location_obligatory_fields": location_obligatory_fields} @@ -268,6 +270,7 @@ def handle_csrf_error(error): photo_attachment_config=photo_attachment_config, feature_flags=config.feature_flags, shortcodes=shortcodes, + pin_marker_fields=pin_marker_fields, ) app.register_blueprint(cp) @@ -285,11 +288,16 @@ def index(): Returns: Rendered map.html template with feature flags and the plugin manifest """ + # The startup-time marker_styles, not a fresh read: pin_marker_fields (which + # decides what /api/locations puts in marker.icon/color) is frozen at startup + # too, so re-reading here would hand the frontend lookup tables keyed on a + # field the API is no longer sending values from. return render_template( "map.html", feature_flags=config.feature_flags, goodmap_frontend_lib_url=config.goodmap_frontend_lib_url, plugin_manifest=plugin_manifest, + marker_styles=marker_styles, ) @goodmap.route("/goodmap-admin") diff --git a/goodmap/templates/goodmap-admin.html b/goodmap/templates/goodmap-admin.html index efd6bc85..3d4bcaf4 100644 --- a/goodmap/templates/goodmap-admin.html +++ b/goodmap/templates/goodmap-admin.html @@ -741,7 +741,6 @@

{{ gettext("Reports") }}

window.SHOW_SUGGEST_NEW_POINT_BUTTON = {{ feature_flags.SHOW_SUGGEST_NEW_POINT_BUTTON | default(false) | tojson }}; window.SHOW_SEARCH_BAR = {{ feature_flags.SHOW_SEARCH_BAR | default(false) | tojson }}; - window.USE_LAZY_LOADING = {{ feature_flags.USE_LAZY_LOADING | default(false) | tojson }}; window.SHOW_ACCESSIBILITY_TABLE = {{ feature_flags.SHOW_ACCESSIBILITY_TABLE | default(false) | tojson }}; diff --git a/goodmap/templates/map.html b/goodmap/templates/map.html index d1b1eacb..fb18e967 100644 --- a/goodmap/templates/map.html +++ b/goodmap/templates/map.html @@ -116,11 +116,14 @@ window.SHOW_SUGGEST_NEW_POINT_BUTTON = {{ feature_flags.SHOW_SUGGEST_NEW_POINT_BUTTON | default(false) | tojson }}; window.SHOW_SEARCH_BAR = {{ feature_flags.SHOW_SEARCH_BAR | default(false) | tojson }}; -window.USE_LAZY_LOADING = {{ feature_flags.USE_LAZY_LOADING | default(false) | tojson }}; window.USE_SERVER_SIDE_CLUSTERING = {{ feature_flags.USE_SERVER_SIDE_CLUSTERING | default(false) | tojson }}; window.SHOW_ACCESSIBILITY_TABLE = {{ feature_flags.SHOW_ACCESSIBILITY_TABLE | default(false) | tojson }}; window.FEATURE_FLAGS = {{ feature_flags | tojson }}; window.PLUGIN_MANIFEST = {{ plugin_manifest | tojson }}; +// Deployment-specific pin icon/color lookup table. icons are already resolved to plain +// URLs here - see goodmap/marker_styles.py's resolve_marker_styles; the raw stored +// config (which may use {provider, value}) comes from goodmap/db.py's get_marker_styles. +window.MARKER_STYLES = {{ marker_styles | tojson }}; {% endblock %} diff --git a/tests/unit_tests/conftest.py b/tests/unit_tests/conftest.py index 55155568..2f97c2d7 100644 --- a/tests/unit_tests/conftest.py +++ b/tests/unit_tests/conftest.py @@ -5,7 +5,7 @@ from platzky import FeatureFlag, FeatureFlagSet from goodmap.config import GoodmapConfig -from goodmap.feature_flags import CategoriesHelp, EnableAdminPanel, UseLazyLoading +from goodmap.feature_flags import CategoriesHelp, EnableAdminPanel from goodmap.goodmap import create_app_from_config @@ -101,7 +101,7 @@ def multipart_suggest_post(client, location, photo=None): def create_test_app( - feature_flags=make_flag_set(CategoriesHelp, UseLazyLoading, EnableAdminPanel), + feature_flags=make_flag_set(CategoriesHelp, EnableAdminPanel), db_overrides=None, ): """Create a test app with optional feature flags and db overrides.""" diff --git a/tests/unit_tests/data_models/test_location.py b/tests/unit_tests/data_models/test_location.py index 3be93ab8..4ae1c104 100644 --- a/tests/unit_tests/data_models/test_location.py +++ b/tests/unit_tests/data_models/test_location.py @@ -1,8 +1,9 @@ import warnings +from typing import cast import pytest -from goodmap.data_models.location import create_location_model +from goodmap.data_models.location import LocationBase, create_location_model from goodmap.exceptions import LocationValidationError @@ -129,6 +130,27 @@ def test_category_validation_rejects_invalid_list_item(): location_model(uuid="2", tags=["red", "yellow"], position=(50, 50)) +def test_basic_info_is_identity_and_position_only(): + """basic_info() carries uuid/position only, even for a category field a + deployment's marker_styles config might reference and even when the location + has a remark - the marker object (icon/color/badge) is shaped separately + (see goodmap.api.api_models.marker_style_values), merged in alongside + basic_info() by the API layer rather than known to the domain model itself.""" + location_model = create_location_model( + obligatory_fields=[("type_of_place", "str"), ("name", "str")], + categories={"type_of_place": ["parcel_locker", "container"]}, + ) + location = location_model( + uuid="1", + name="test", + type_of_place="parcel_locker", + position=(50, 50), + remark="a remark", + ) + location = cast(LocationBase, location) + assert location.basic_info() == {"uuid": "1", "position": (50, 50)} + + def test_create_location_model_with_int_field(): """Test that non-str simple fields (like int) are created without max_length.""" location_model = create_location_model(obligatory_fields=[("capacity", "int")], categories={}) diff --git a/tests/unit_tests/test_api_models.py b/tests/unit_tests/test_api_models.py new file mode 100644 index 00000000..f5f62c7c --- /dev/null +++ b/tests/unit_tests/test_api_models.py @@ -0,0 +1,60 @@ +from typing import cast + +from goodmap.api.api_models import PinMarkerFields, marker_style_values +from goodmap.data_models.location import LocationBase, create_location_model + + +def test_marker_style_values_includes_badge_and_configured_field_values(): + """marker_style_values() includes badge when true (drives the asterisk + badge), plus the icon/color field values off the given location.""" + location_model = create_location_model( + obligatory_fields=[("type_of_place", "str"), ("name", "str")], + categories={"type_of_place": ["parcel_locker", "container"]}, + ) + location = location_model( + uuid="1", + name="test", + type_of_place="parcel_locker", + position=(50, 50), + remark="a remark", + ) + location = cast(LocationBase, location) + assert marker_style_values(location, PinMarkerFields(icon_field="type_of_place")) == { + "marker": {"icon": "parcel_locker", "badge": True}, + } + + +def test_marker_style_values_sets_icon_and_color_independently(): + location_model = create_location_model( + obligatory_fields=[("type_of_place", "str"), ("transparency", "str"), ("name", "str")], + categories={"type_of_place": ["parcel_locker"], "transparency": ["lacking"]}, + ) + location = location_model( + uuid="1", + name="test", + type_of_place="parcel_locker", + transparency="lacking", + position=(50, 50), + ) + location = cast(LocationBase, location) + fields = PinMarkerFields(icon_field="type_of_place", color_field="transparency") + assert marker_style_values(location, fields) == { + "marker": {"icon": "parcel_locker", "color": "lacking"}, + } + + +def test_marker_style_values_omits_marker_when_no_remark_and_no_style_fields(): + location_model = create_location_model(obligatory_fields=[("name", "str")], categories={}) + location = location_model(uuid="1", name="test", position=(50, 50)) + location = cast(LocationBase, location) + assert marker_style_values(location, PinMarkerFields()) == {} + + +def test_marker_style_values_ignores_style_field_the_location_does_not_have(): + """A style field that isn't actually one of this location's attributes (e.g. + misconfigured marker_styles, or narrowed away upstream) is simply skipped, + not an error.""" + location_model = create_location_model(obligatory_fields=[("name", "str")], categories={}) + location = location_model(uuid="1", name="test", position=(50, 50)) + location = cast(LocationBase, location) + assert marker_style_values(location, PinMarkerFields(icon_field="nonexistent_field")) == {} diff --git a/tests/unit_tests/test_clustering.py b/tests/unit_tests/test_clustering.py index 6b32c127..cd1fedb8 100644 --- a/tests/unit_tests/test_clustering.py +++ b/tests/unit_tests/test_clustering.py @@ -10,7 +10,15 @@ def test_map_clustering_data_single_point(): """Test mapping clustering data for a single point""" - input_data = [{"longitude": 50.0, "latitude": 60.0, "count": 1, "uuid": "test-uuid"}] + input_data = [ + { + "longitude": 50.0, + "latitude": 60.0, + "count": 1, + "uuid": "test-uuid", + "marker": {"icon": "container"}, + } + ] result = map_clustering_data_to_proper_lazy_loading_object(input_data) @@ -20,6 +28,17 @@ def test_map_clustering_data_single_point(): assert result[0]["cluster_uuid"] is None assert result[0]["cluster_count"] is None assert result[0]["position"] == [50.0, 60.0] + assert result[0]["marker"] == {"icon": "container"} + + +def test_map_clustering_data_single_point_without_marker(): + """A point with no marker styling has no `marker` key at all, exactly as the + same point comes back from /api/locations.""" + input_data = [{"longitude": 50.0, "latitude": 60.0, "count": 1, "uuid": "test-uuid"}] + + result = map_clustering_data_to_proper_lazy_loading_object(input_data) + + assert "marker" not in result[0] def test_map_clustering_data_cluster(): @@ -39,7 +58,7 @@ def test_map_clustering_data_cluster(): def test_match_clusters_uuids_exact_match(): """Test matching cluster UUIDs with exact coordinate match""" points = [ - {"position": [50.0, 60.0], "uuid": "uuid-1"}, + {"position": [50.0, 60.0], "uuid": "uuid-1", "marker": {"icon": "container"}}, {"position": [51.0, 61.0], "uuid": "uuid-2"}, ] @@ -51,7 +70,9 @@ def test_match_clusters_uuids_exact_match(): result = match_clusters_uuids(points, clusters) assert result[0]["uuid"] == "uuid-1" + assert result[0]["marker"] == {"icon": "container"} assert result[1]["uuid"] == "uuid-2" + assert result[1]["marker"] is None def test_match_clusters_uuids_multi_point_cluster(): @@ -67,8 +88,9 @@ def test_match_clusters_uuids_multi_point_cluster(): result = match_clusters_uuids(points, clusters) - # Multi-point cluster should not get a uuid assigned + # Multi-point cluster should not get a uuid/marker assigned assert "uuid" not in result[0] + assert "marker" not in result[0] def test_match_clusters_uuids_no_match_warning(): @@ -90,6 +112,7 @@ def test_match_clusters_uuids_no_match_warning(): warning_call = mock_logger.warning.call_args[0][0] assert "No matching UUID found" in warning_call assert result[0]["uuid"] is None + assert result[0]["marker"] is None def test_match_clusters_uuids_floating_point_precision(): diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index 6dbb03e5..cc94b045 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -272,12 +272,11 @@ def test_get_locations(test_app): { "uuid": "11111111-1111-1111-1111-111111111111", "position": [50, 50], - "has_remark": True, + "marker": {"badge": True}, }, { "uuid": "22222222-2222-2222-2222-222222222222", "position": [60, 60], - "has_remark": False, }, ] @@ -315,6 +314,39 @@ def test_get_locations_accepts_valid_and_undeclared_parameters(test_app, query): assert response.status_code == 200 +def test_get_locations_includes_category_field_for_pin_styling(): + """/api/locations should surface the field marker_styles.icon_field points at + (e.g. a point-type category), so the frontend can pick a marker icon/color + without a full per-location detail fetch.""" + client = create_test_app( + db_overrides={ + "categories": {"point_type": ["parcel_locker", "container"]}, + "location_obligatory_fields": [("point_type", "str"), ("name", "str")], + "marker_styles": {"icon_field": "point_type", "icons": {}, "colors": {}}, + "data": [ + { + "name": "locker-1", + "position": [50, 50], + "point_type": "parcel_locker", + "uuid": "11111111-1111-1111-1111-111111111111", + }, + ], + "visible_data": ["name", "point_type"], + } + ) + + response = client.get("/api/locations") + + assert response.status_code == 200 + assert response.json == [ + { + "uuid": "11111111-1111-1111-1111-111111111111", + "position": [50, 50], + "marker": {"icon": "parcel_locker"}, + }, + ] + + def test_get_locations_multi_value_same_category_uses_or_semantics(): """Selecting several checkboxes within one category should return the union of matches, not only entries that have every selected value.""" @@ -938,12 +970,24 @@ def test_location_clustering_high_zoom_no_clusters(test_app): assert data[1]["type"] == "point" +def test_location_clustering_point_entries_carry_marker(test_app): + """The clustered endpoint's point entries carry the same `marker` object as + /api/locations - the clustering pass must not drop it (goodmap/clustering.py).""" + response = test_app.get("/api/locations-clustered?zoom=16") + assert response.status_code == 200 + data = response.json + by_uuid = {entry["uuid"]: entry for entry in data} + assert by_uuid["11111111-1111-1111-1111-111111111111"]["marker"] == {"badge": True} + assert "marker" not in by_uuid["22222222-2222-2222-2222-222222222222"] + + def test_location_clustering_low_zoom_creates_clusters(test_app): response = test_app.get("/api/locations-clustered?zoom=1") assert response.status_code == 200 data = response.json assert len(data) == 1 assert data[0]["type"] == "cluster" + assert "marker" not in data[0] @pytest.mark.parametrize( @@ -1029,7 +1073,6 @@ def test_issue_options_defaults_to_empty_when_missing(): config_data = get_test_config_data() config_data["FEATURE_FLAGS"] = { "CATEGORIES_HELP": True, - "USE_LAZY_LOADING": True, "ENABLE_ADMIN_PANEL": True, } config_data["DB"]["DATA"].pop("reported_issue_types", None) @@ -1040,6 +1083,7 @@ def test_issue_options_defaults_to_empty_when_missing(): def test_get_locations_from_request_helper(test_app): + from goodmap.api.api_models import PinMarkerFields from goodmap.api.core_api import get_locations_from_request class MockArgs: @@ -1049,7 +1093,9 @@ def to_dict(self, flat=False): mock_request_args = MockArgs() with test_app.application.app_context(): - locations = get_locations_from_request(test_app.application.db, mock_request_args) + locations = get_locations_from_request( + test_app.application.db, mock_request_args, PinMarkerFields() + ) assert isinstance(locations, list) if locations: assert isinstance(locations[0], dict) diff --git a/tests/unit_tests/test_db.py b/tests/unit_tests/test_db.py index d915ce2a..84270b19 100644 --- a/tests/unit_tests/test_db.py +++ b/tests/unit_tests/test_db.py @@ -26,6 +26,7 @@ google_json_db_get_data, google_json_db_get_location_obligatory_fields, google_json_db_get_locations_paginated, + google_json_db_get_marker_styles, google_json_db_get_meta_data, google_json_db_get_visible_data, json_db_add_location, @@ -38,6 +39,7 @@ json_db_get_category_data, json_db_get_data, json_db_get_location_obligatory_fields, + json_db_get_locations, json_db_get_report, json_db_get_reports, json_db_get_suggestion, @@ -56,7 +58,9 @@ json_file_db_get_category_data, json_file_db_get_data, json_file_db_get_location_obligatory_fields, + json_file_db_get_locations, json_file_db_get_locations_paginated, + json_file_db_get_marker_styles, json_file_db_get_meta_data, json_file_db_get_report, json_file_db_get_reports, @@ -81,6 +85,7 @@ mongodb_db_get_location_obligatory_fields, mongodb_db_get_locations, mongodb_db_get_locations_paginated, + mongodb_db_get_marker_styles, mongodb_db_get_meta_data, mongodb_db_get_report, mongodb_db_get_reports, @@ -296,6 +301,45 @@ def test_json_file_db_get_meta_data_empty(): assert result == {} +@mock.patch( + "builtins.open", + mock.mock_open( + read_data=json.dumps( + { + "map": { + "marker_styles": { + "icon_field": "type_of_place", + "color_field": "status", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + "colors": {"open": "#2e7d32"}, + } + } + } + ) + ), +) +def test_json_file_db_get_marker_styles(): + db = JsonFile("/fake/path/data.json") + result = json_file_db_get_marker_styles(db) + assert result == { + "icon_field": "type_of_place", + "color_field": "status", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + "colors": {"open": "#2e7d32"}, + } + + +@mock.patch("builtins.open", mock.mock_open(read_data=json.dumps({"map": {}}))) +def test_json_file_db_get_marker_styles_empty(): + db = JsonFile("/fake/path/data.json") + result = json_file_db_get_marker_styles(db) + assert result == {} + + # Test get_visible_data and get_meta_data for google_json_db @mock.patch("platzky.db.google_json_db.Client") def test_google_json_db_get_visible_data(mock_cli): @@ -337,6 +381,41 @@ def test_google_json_db_get_meta_data_empty(mock_cli): assert result == {} +@mock.patch("platzky.db.google_json_db.Client") +def test_google_json_db_get_marker_styles(mock_cli): + blob = mock_cli.return_value.bucket.return_value.blob.return_value + blob.download_as_text.return_value = json.dumps( + { + "map": { + "marker_styles": { + "icon_field": "type_of_place", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + } + } + } + ) + db = GoogleJsonDb("bucket", "blob") + result = google_json_db_get_marker_styles(db) + assert result == { + "icon_field": "type_of_place", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + } + + +@mock.patch("platzky.db.google_json_db.Client") +def test_google_json_db_get_marker_styles_empty(mock_cli): + mock_cli.return_value.bucket.return_value.blob.return_value.download_as_text.return_value = ( + json.dumps({"map": {}}) + ) + db = GoogleJsonDb("bucket", "blob") + result = google_json_db_get_marker_styles(db) + assert result == {} + + def test_get_location_from_raw_data_found(): raw = {"data": [{"uuid": "X", "position": [0, 0]}]} Location = create_location_model([], {}) @@ -1092,6 +1171,54 @@ def test_mongodb_db_get_meta_data_no_config(mock_client): assert result == {} +@mock.patch("platzky.db.mongodb_db.MongoClient") +def test_mongodb_db_get_marker_styles(mock_client): + mock_db = mock.Mock() + mock_client.return_value.__getitem__.return_value = mock_db + mock_db.config.find_one.return_value = { + "_id": "map_config", + "marker_styles": { + "icon_field": "type_of_place", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + }, + } + + db = MongoDB("mongodb://localhost:27017", "test_db") + result = mongodb_db_get_marker_styles(db) + assert result == { + "icon_field": "type_of_place", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + } + + +@mock.patch("platzky.db.mongodb_db.MongoClient") +def test_mongodb_db_get_marker_styles_empty(mock_client): + mock_db = mock.Mock() + mock_client.return_value.__getitem__.return_value = mock_db + mock_db.config.find_one.return_value = { + "_id": "map_config", + } + + db = MongoDB("mongodb://localhost:27017", "test_db") + result = mongodb_db_get_marker_styles(db) + assert result == {} + + +@mock.patch("platzky.db.mongodb_db.MongoClient") +def test_mongodb_db_get_marker_styles_no_config(mock_client): + mock_db = mock.Mock() + mock_client.return_value.__getitem__.return_value = mock_db + mock_db.config.find_one.return_value = None + + db = MongoDB("mongodb://localhost:27017", "test_db") + result = mongodb_db_get_marker_styles(db) + assert result == {} + + @mock.patch("platzky.db.mongodb_db.MongoClient") def test_mongodb_db_get_location(mock_client): mock_db = mock.Mock() @@ -1371,6 +1498,28 @@ def test_json_file_db_get_categories(tmp_path): assert list(categories) == ["test-category"] +def test_json_db_without_categories_serves_locations(): + """A data source with no `categories` at all is a valid setup - a map of plain, + unfilterable points - so neither listing categories nor querying locations may + depend on the key being there.""" + uncategorized = {key: value for key, value in data.items() if key != "categories"} + db = in_memory_json_db(uncategorized) + + assert list(json_db_get_categories(db)) == [] + assert len(json_db_get_locations(db, {}, LocationBase)) == 2 + + +def test_json_file_db_without_categories_serves_locations(tmp_path): + uncategorized = {key: value for key, value in data.items() if key != "categories"} + test_file = tmp_path / "test.json" + test_file.write_text(json.dumps({"map": uncategorized})) + + db = JsonFile(str(test_file)) + + assert list(json_file_db_get_categories(db)) == [] + assert len(json_file_db_get_locations(db, {}, LocationBase)) == 2 + + @mock.patch("platzky.db.google_json_db.Client") def test_google_json_db_get_categories(mock_cli): mock_cli.return_value.bucket.return_value.blob.return_value.download_as_text.return_value = ( diff --git a/tests/unit_tests/test_goodmap.py b/tests/unit_tests/test_goodmap.py index 5fab3be2..ce5e37ac 100644 --- a/tests/unit_tests/test_goodmap.py +++ b/tests/unit_tests/test_goodmap.py @@ -14,7 +14,7 @@ from goodmap import goodmap from goodmap.config import GoodmapConfig -from goodmap.feature_flags import EnableAdminPanel, UseLazyLoading +from goodmap.feature_flags import EnableAdminPanel from goodmap.plugin import ( CAPABILITY_BASES, MapOverlayPluginBase, @@ -37,7 +37,14 @@ def test_create_app(): def test_create_app_from_config(): with patch("platzky.platzky.create_app_from_config", MagicMock()) as mock_platzky_app_creation: mock_platzky_app_creation.return_value.is_enabled.return_value = False - with patch("goodmap.goodmap.extend_db_with_goodmap_queries", MagicMock()) as mock_extend_db: + with ( + patch("goodmap.goodmap.extend_db_with_goodmap_queries", MagicMock()) as mock_extend_db, + patch("goodmap.goodmap.get_location_obligatory_fields", return_value=[]), + patch("goodmap.goodmap.get_category_data") as mock_get_category_data, + patch("goodmap.goodmap.get_marker_styles") as mock_get_marker_styles, + ): + mock_get_category_data.return_value.return_value = {"categories": {}} + mock_get_marker_styles.return_value.return_value = {} goodmap.create_app_from_config(config) mock_platzky_app_creation.assert_called_once_with( config, @@ -56,12 +63,13 @@ def test_create_app_delegation(mock_parse_yaml, mock_create_app_from_config): @mock.patch("goodmap.goodmap.get_location_obligatory_fields") -def test_use_lazy_loading_branch(mock_get_location_obligatory_fields): +def test_location_model_is_always_built_from_the_data_source(mock_get_location_obligatory_fields): + """Building the location model from location_obligatory_fields/categories is + unconditional - there's no flag that skips it (see feature_flags.py).""" config = GoodmapConfig( APP_NAME="test_lazy", SECRET_KEY="secret", DB=JsonDbConfig(DATA={"site_content": {}, "location_obligatory_fields": []}, TYPE="json"), - FEATURE_FLAGS=make_flag_set(UseLazyLoading), ) app = goodmap.create_app_from_config(config) @@ -107,6 +115,116 @@ def test_frontend_lib_url_uses_bundled_static_when_present(): assert 'src="/static/frontend/index.min.js"' in response.data.decode("utf-8") +def test_map_route_marker_styles(): + """The frontend picks pin icon/color per marker_styles.config's iconField/colorField + at runtime from window.MARKER_STYLES - a deployment-specific lookup table that lives + in the database (like categories/visible_data), not hardcoded in the frontend build. + Deployments that don't configure it get an empty object instead, so the frontend + falls back to Leaflet's default marker - no behavior change.""" + configured_config = GoodmapConfig( + APP_NAME="test_app", + SECRET_KEY="test_secret", + USE_WWW=False, + BLOG_PREFIX="/blog", + DB=JsonDbConfig( + DATA={ + "site_content": {"pages": []}, + "categories": {"type_of_place": ["parcel_locker", "container"]}, + "marker_styles": { + "icon_field": "type_of_place", + "icons": { + "parcel_locker": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/package-fill.svg" + }, + "colors": {}, + }, + }, + TYPE="json", + ), + ) + configured_app = goodmap.create_app_from_config(configured_config) + configured_app.config["WTF_CSRF_ENABLED"] = False # NOSONAR + + response = configured_app.test_client().get("/map") + assert response.status_code == 200 + + response_text = response.data.decode("utf-8") + assert "MARKER_STYLES" in response_text + assert "icon_field" in response_text + assert "parcel_locker" in response_text + + unconfigured_app = goodmap.create_app_from_config(_minimal_config()) + unconfigured_app.config["WTF_CSRF_ENABLED"] = False # NOSONAR + + response = unconfigured_app.test_client().get("/map") + assert response.status_code == 200 + assert "window.MARKER_STYLES={};" in response.data.decode("utf-8") + + +def test_map_route_marker_styles_stay_in_step_with_the_api(): + """window.MARKER_STYLES comes from the startup-time config, not a fresh read per + request. The field /api/locations reads marker.icon from is fixed at startup, so a + /map that served newer lookup tables would key them on values the API isn't + sending.""" + data = { + "site_content": {"pages": []}, + "location_obligatory_fields": [["type_of_place", "str"]], + "marker_styles": { + "icon_field": "type_of_place", + "icons": {"parcel_locker": "https://cdn.example.com/package.svg"}, + "colors": {}, + }, + } + app = goodmap.create_app_from_config( + GoodmapConfig( + APP_NAME="test_app", + SECRET_KEY="test_secret", + USE_WWW=False, + BLOG_PREFIX="/blog", + DB=JsonDbConfig(DATA=data, TYPE="json"), + ) + ) + app.config["WTF_CSRF_ENABLED"] = False # NOSONAR + + with mock.patch.object(app.db, "get_marker_styles") as fresh_read: + response_text = app.test_client().get("/map").data.decode("utf-8") + + fresh_read.assert_not_called() + assert "parcel_locker" in response_text + + +def test_map_route_serves_icons_already_resolved_to_urls(): + """window.MARKER_STYLES.icons is a flat {value: url} table: the tagged + {provider, value} form a data source may use is resolved at startup, so supporting a + new provider never needs a frontend release.""" + data = { + "site_content": {"pages": []}, + "location_obligatory_fields": [["type_of_place", "str"]], + "marker_styles": { + "icon_field": "type_of_place", + "icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}}, + "colors": {}, + }, + } + app = goodmap.create_app_from_config( + GoodmapConfig( + APP_NAME="test_app", + SECRET_KEY="test_secret", + USE_WWW=False, + BLOG_PREFIX="/blog", + DB=JsonDbConfig(DATA=data, TYPE="json"), + ) + ) + app.config["WTF_CSRF_ENABLED"] = False # NOSONAR + + response_text = app.test_client().get("/map").data.decode("utf-8") + + assert ( + "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg" + in response_text + ) + assert "provider" not in response_text + + def test_map_route_includes_photo_constraints(): """The frontend sources photo upload limits (max size, allowed types) live from the backend's AttachmentConfig rather than hardcoding its own copy - this test @@ -239,8 +357,9 @@ def test_map_route_overrides_photo_constraints(): assert photo["allowed_extensions"] == ["jpeg", "jpg", "png"] -def test_location_schema_endpoint_with_lazy_loading(): - """The schema includes obligatory_fields when USE_LAZY_LOADING is enabled.""" +def test_location_schema_endpoint_includes_obligatory_fields(): + """The schema includes this deployment's obligatory_fields - unconditional, + there's no flag that skips building the location model from them.""" config = GoodmapConfig( APP_NAME="test_app", SECRET_KEY="test_secret", @@ -258,7 +377,6 @@ def test_location_schema_endpoint_with_lazy_loading(): }, TYPE="json", ), - FEATURE_FLAGS=make_flag_set(UseLazyLoading), ) app = goodmap.create_app_from_config(config) # CSRF protection must be disabled in test environment to allow API testing diff --git a/tests/unit_tests/test_marker_styles.py b/tests/unit_tests/test_marker_styles.py new file mode 100644 index 00000000..10ea10b1 --- /dev/null +++ b/tests/unit_tests/test_marker_styles.py @@ -0,0 +1,157 @@ +import copy +from unittest import mock + +import pytest + +from goodmap.marker_styles import ICON_PROVIDERS, PhosphorIconProvider, resolve_marker_styles + +# The literal URL the frontend's resolvePhosphorIconUrl.js builds for the same icon name +# (see frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx). Spelled out rather than +# imported from the module under test, so the two implementations drifting apart while +# the frontend shim is still in place shows up here. +PHOSPHOR_BRIDGE_URL = ( + "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg" +) + + +def test_phosphor_provider_builds_the_whole_cdn_url_from_an_icon_name(): + """Pins the provider itself, independently of the resolution plumbing around it - + this is the URL the frontend used to build for itself.""" + assert PhosphorIconProvider().resolve("bridge") == PHOSPHOR_BRIDGE_URL + + +def test_a_provider_added_to_the_registry_is_picked_up(): + """The registry's whole point: a new provider is a class plus a dict entry, with no + edit to the resolution path.""" + + class SpriteProvider: + def resolve(self, value): + return f"https://sprites.example/{value}.svg" + + styles = {"icons": {"big bridge": {"provider": "sprite", "value": "bridge"}}} + + with mock.patch.dict(ICON_PROVIDERS, {"sprite": SpriteProvider()}): + resolved = resolve_marker_styles(styles) + + assert resolved["icons"] == {"big bridge": "https://sprites.example/bridge.svg"} + # ...and it is gone again once unregistered, so the patch really was what mattered. + assert resolve_marker_styles(styles)["icons"] == {} + + +def test_resolves_phosphor_entry_to_cdn_url(): + styles = {"icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}}} + + assert resolve_marker_styles(styles)["icons"] == {"big bridge": PHOSPHOR_BRIDGE_URL} + + +def test_resolves_url_provider_entry_to_its_value(): + styles = {"icons": {"container": {"provider": "url", "value": "https://e.example/c.svg"}}} + + assert resolve_marker_styles(styles)["icons"] == {"container": "https://e.example/c.svg"} + + +def test_passes_plain_string_entry_through_unchanged(): + styles = {"icons": {"container": "https://e.example/c.svg"}} + + assert resolve_marker_styles(styles)["icons"] == {"container": "https://e.example/c.svg"} + + +@pytest.mark.parametrize( + "entry", + [ + {"provider": "phosphorr", "value": "bridge"}, + {"provider": None, "value": "bridge"}, + {"value": "bridge"}, + {"provider": "phosphor"}, + {"provider": "phosphor", "value": ""}, + {"provider": "phosphor", "value": 7}, + "", + 7, + None, + ["https://e.example/c.svg"], + ], + ids=[ + "unknown-provider", + "null-provider", + "no-provider", + "no-value", + "empty-value", + "non-string-value", + "empty-string", + "number", + "null", + "list", + ], +) +def test_unresolvable_entry_is_dropped_with_a_warning_naming_it(entry): + styles = {"icons": {"big bridge": entry}} + + with mock.patch("goodmap.marker_styles.logger") as mock_logger: + assert resolve_marker_styles(styles)["icons"] == {} + + mock_logger.warning.assert_called_once() + assert "big bridge" in mock_logger.warning.call_args[0][1:] + + +def test_one_bad_entry_does_not_drop_its_good_siblings(): + """A single typo costs that pin its icon, not every other pin's.""" + styles = { + "icons": { + "big bridge": {"provider": "phosphor", "value": "bridge"}, + "broken": {"provider": "nope", "value": "x"}, + "plain": "https://e.example/c.svg", + } + } + + assert resolve_marker_styles(styles)["icons"] == { + "big bridge": PHOSPHOR_BRIDGE_URL, + "plain": "https://e.example/c.svg", + } + + +def test_non_object_icons_resolves_to_nothing_rather_than_reaching_the_frontend(): + with mock.patch("goodmap.marker_styles.logger") as mock_logger: + assert resolve_marker_styles({"icons": "oops"})["icons"] == {} + + mock_logger.warning.assert_called_once() + + +def test_empty_marker_styles_stays_empty(): + assert resolve_marker_styles({}) == {} + + +def test_missing_icons_key_is_not_invented(): + assert resolve_marker_styles({"icon_field": "type_of_place"}) == {"icon_field": "type_of_place"} + + +def test_every_other_key_is_carried_through_untouched(): + """colors maps straight to CSS colors and never had a tagged form, so it - like the + two field names - must survive resolution unchanged.""" + styles = { + "icon_field": "type_of_place", + "color_field": "speed_limit", + "colors": {"10": "#2e7d32", "50": "#c62828"}, + "icons": {"plain": "https://e.example/c.svg"}, + } + + resolved = resolve_marker_styles(styles) + + assert resolved["icon_field"] == "type_of_place" + assert resolved["color_field"] == "speed_limit" + assert resolved["colors"] == {"10": "#2e7d32", "50": "#c62828"} + + +def test_does_not_mutate_the_config_it_was_given(): + """For the json backend this dict is the db's live in-memory config, so resolving in + place would rewrite what the deployment has stored.""" + styles = { + "icon_field": "type_of_place", + "icons": {"big bridge": {"provider": "phosphor", "value": "bridge"}}, + } + before = copy.deepcopy(styles) + icons_before = styles["icons"] + + resolve_marker_styles(styles) + + assert styles == before + assert styles["icons"] is icons_before