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..bdf0e37d 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 ------ @@ -108,12 +109,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`` 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..a0c95225 100644 --- a/e2e-tests/e2e_test_data_initial.json +++ b/e2e-tests/e2e_test_data_initial.json @@ -112,7 +112,7 @@ "is_free": "true", "speed_limit": "10", "amenities": [ - "benches" + "toilets" ], "uuid": "5986e755-1eaa-4121-a01c-4fef1d5d1da1" }, @@ -272,6 +272,19 @@ "cars" ] }, + "marker_styles": { + "icon_field": "type_of_place", + "color_field": "speed_limit", + "icons": { + "big bridge": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg", + "small bridge": "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/footprints-fill.svg" + }, + "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..62e3f200 --- /dev/null +++ b/e2e-tests/tests/basic/test_marker_styles.py @@ -0,0 +1,106 @@ +""" +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 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). +BIG_BRIDGE_TYPE_ICON_URL = ( + "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/bridge-fill.svg" +) +SMALL_BRIDGE_TYPE_ICON_URL = ( + "https://cdn.jsdelivr.net/npm/@phosphor-icons/core@2/assets/fill/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/src/components/Map/store/markerStyles.store.js b/frontend/src/components/Map/store/markerStyles.store.js new file mode 100644 index 00000000..8608056a --- /dev/null +++ b/frontend/src/components/Map/store/markerStyles.store.js @@ -0,0 +1,14 @@ +import { create } from 'zustand'; + +/** + * uuid -> resolved marker-styling field values (whatever marker_styles.icon_field/ + * color_field point at), lazily fetched once a client-side-clustered marker becomes + * individually visible - see lazy-load-marker-styling-plan.md. A uuid with no + * matching styling is still recorded, as {}, so it isn't re-requested forever. + */ +const useMarkerStylesStore = create(set => ({ + stylesByUuid: {}, + mergeStyles: styles => set(state => ({ stylesByUuid: { ...state.stylesByUuid, ...styles } })), +})); + +export default useMarkerStylesStore; diff --git a/frontend/src/components/MarkerPopup/MarkerPopup.jsx b/frontend/src/components/MarkerPopup/MarkerPopup.jsx index 6bec59fb..60ae7752 100644 --- a/frontend/src/components/MarkerPopup/MarkerPopup.jsx +++ b/frontend/src/components/MarkerPopup/MarkerPopup.jsx @@ -2,15 +2,16 @@ import React, { useState, useEffect } 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'; +import useMarkerStylesStore from '../Map/store/markerStyles.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'; +import requestMarkerStyle from './requestMarkerStyle'; /** * Wrapper component that fetches full location details and renders them in a popup. @@ -69,17 +70,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,12 +77,13 @@ 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 {boolean} [props.place.has_remark] - Whether this location has a remark (adds an asterisk badge if true) * @returns {React.ReactElement} Leaflet Marker component with click-to-show-details functionality */ const MarkerPopup = ({ place }) => { const selectedLocationId = useMapStore(state => state.selectedLocationId); const setSelectedLocationId = useMapStore(state => state.setSelectedLocationId); + const lazyMarkerStyle = useMarkerStylesStore(state => state.stylesByUuid[place.uuid]); const [isClicked, setIsClicked] = useState(false); // TODO: this only opens the popup if `place`'s Marker is actually attached to @@ -114,18 +105,22 @@ const MarkerPopup = ({ place }) => { setIsClicked(true); }; + const handleMarkerVisible = () => { + requestMarkerStyle(place.uuid); + }; + const markerProps = { position: place.position, eventHandlers: { click: handleMarkerClick, + add: handleMarkerVisible, }, - 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; + const styledPlace = lazyMarkerStyle ? { ...place, ...lazyMarkerStyle } : place; + const typedIcon = getTypedMarkerIcon(styledPlace); + if (typedIcon) { + markerProps.icon = typedIcon; } return ( 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..cdbc1dc1 --- /dev/null +++ b/frontend/src/components/MarkerPopup/getTypedMarkerIcon.jsx @@ -0,0 +1,125 @@ +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 color_field 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; + +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 + * db.get_marker_styles) when it matches, our own pin in the fallback color + * with just the asterisk badge when `place.has_remark` is set but nothing + * matched, or `null` (falls back to Leaflet's default marker) when there's + * neither a match nor a remark to show. + * + * @param {Object} place - Location data from GET /api/locations, merged with any + * styling lazily fetched for it from GET /api/locations/marker-styles (has_remark + * and marker_styles field values aren't in the initial /api/locations response - + * see lazy-load-marker-styling-plan.md) + * @returns {import('leaflet').DivIcon|null} + */ +const getTypedMarkerIcon = place => { + const markerStyles = globalThis.MARKER_STYLES || {}; + const { icon_field: iconField, color_field: colorField, icons, colors } = markerStyles; + + const typeIconUrl = icons?.[place[iconField]] || ''; + const matchedColor = colors?.[place[colorField]] || ''; + const hasRemark = Boolean(place.has_remark); + + 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/components/MarkerPopup/requestMarkerStyle.js b/frontend/src/components/MarkerPopup/requestMarkerStyle.js new file mode 100644 index 00000000..59d6029d --- /dev/null +++ b/frontend/src/components/MarkerPopup/requestMarkerStyle.js @@ -0,0 +1,54 @@ +import httpService from '../../services/http/httpService'; +import useMarkerStylesStore from '../Map/store/markerStyles.store'; + +const BATCH_DEBOUNCE_MS = 150; + +let pendingUuids = new Set(); +let timer = null; + +/** + * Queues `uuid` for a batched GET /api/locations/marker-styles fetch, once its + * marker becomes individually visible (not folded into a cluster) - see + * lazy-load-marker-styling-plan.md. Fetches pin styling data (has_remark plus any + * marker_styles field values), so it's needed regardless of whether marker_styles + * is even configured - has_remark alone still drives the asterisk badge. Debounced + * so that markers becoming visible in quick succession (panning, zooming, a + * cluster spiderfying) share one request instead of firing one per marker. + * + * Scoped to client-side clustering for now - server-side clustering's own + * lazy-loading trigger is a separate follow-up (see the plan doc). + * + * @param {string} uuid - Location UUID whose marker just became individually visible + */ +const requestMarkerStyle = uuid => { + if (globalThis.FEATURE_FLAGS?.USE_SERVER_SIDE_CLUSTERING) { + return; + } + + const alreadyKnown = uuid in useMarkerStylesStore.getState().stylesByUuid; + if (alreadyKnown || pendingUuids.has(uuid)) { + return; + } + pendingUuids.add(uuid); + + if (timer) { + clearTimeout(timer); + } + timer = setTimeout(() => { + const uuids = [...pendingUuids]; + pendingUuids = new Set(); + timer = null; + + httpService + .getMarkerStyles(uuids) + .then(styles => { + // Every requested uuid is recorded, even with no matching styling + // ({}), so it isn't queued again on the next re-cluster. + const withDefaults = Object.fromEntries(uuids.map(u => [u, styles[u] ?? {}])); + useMarkerStylesStore.getState().mergeStyles(withDefaults); + }) + .catch(error => console.error('Failed to fetch marker styles:', error)); + }, BATCH_DEBOUNCE_MS); +}; + +export default requestMarkerStyle; 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/src/services/http/endpoints.js b/frontend/src/services/http/endpoints.js index 282b60fa..6f53f501 100644 --- a/frontend/src/services/http/endpoints.js +++ b/frontend/src/services/http/endpoints.js @@ -28,6 +28,14 @@ export const LOCATIONS = '/api/locations'; */ export const LOCATIONS_CLUSTERED = '/api/locations-clustered'; +/** + * API endpoint for lazily fetching marker styling field values (whatever + * marker_styles.icon_field/color_field point at) for specific locations, by uuid. + * Used once a location's marker becomes individually visible, instead of upfront + * for every location - see lazy-load-marker-styling-plan.md. + */ +export const LOCATIONS_MARKER_STYLES = '/api/locations/marker-styles'; + /** * External API endpoint for address search (forward geocoding) using OpenStreetMap Nominatim. * Converts addresses/place names to geographic coordinates. diff --git a/frontend/src/services/http/httpService.js b/frontend/src/services/http/httpService.js index 01999f3c..0962b8a6 100644 --- a/frontend/src/services/http/httpService.js +++ b/frontend/src/services/http/httpService.js @@ -5,6 +5,7 @@ import { LOCATIONS, SEARCH_ADDRESS, LOCATIONS_CLUSTERED, + LOCATIONS_MARKER_STYLES, } from './endpoints'; import useMapStore from '../../components/Map/store/map.store'; @@ -207,6 +208,32 @@ const httpService = { } }, + /** + * Fetches marker styling field values for specific locations, by uuid. + * Used to lazily fetch pin icon/color data once a marker becomes individually + * visible, instead of upfront for every location. + * + * @param {string[]} uuids - Location UUIDs to fetch styling for + * @returns {Promise>} Promise resolving to a map of + * uuid -> styling field values; uuids with no styling are simply absent + */ + getMarkerStyles: async uuids => { + if (!uuids.length) { + return {}; + } + const params = new URLSearchParams(); + for (const uuid of uuids) { + params.append('uuid', uuid); + } + const response = await fetch(`${LOCATIONS_MARKER_STYLES}?${params.toString()}`, { + method: 'GET', + headers: { + 'Content-Type': 'application/json', + }, + }); + return jsonOrThrow(response, 'marker styles'); + }, + /** * Searches for addresses using OpenStreetMap Nominatim API. * Returns up to 5 results with geocoded coordinates. diff --git a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx index 9689d6ca..79f2dfa2 100644 --- a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx @@ -4,6 +4,7 @@ import { render, screen, fireEvent, act, waitFor } from '@testing-library/react' import { MapContainer } from 'react-leaflet'; import MarkerPopup from '../../src/components/MarkerPopup/MarkerPopup'; import httpService from '../../src/services/http/httpService'; +import useMarkerStylesStore from '../../src/components/Map/store/markerStyles.store'; jest.mock('../../src/services/http/httpService'); @@ -35,6 +36,23 @@ const locationData = { }; httpService.getLocation.mockResolvedValue(locationData); +// Every mount now fires a lazy marker-styles request (see requestMarkerStyle.js) - +// a harmless default so it always resolves, even in tests that don't care about it. +httpService.getMarkerStyles.mockResolvedValue({}); + +/** + * requestMarkerStyle.js debounces/batches uuids through module-level state shared + * by every test in this file. Describes below that render with real timers must + * drain that debounce window before finishing, or its still-pending timer fires + * during a later (fake-timer) describe and merges its uuid into that batch. + */ +const flushMarkerStyleDebounce = () => + act( + () => + new Promise(resolve => { + setTimeout(resolve, 200); + }), + ); describe('MarkerPopup', () => { beforeEach(() => { @@ -54,8 +72,9 @@ describe('MarkerPopup', () => { ); }); - afterEach(() => { + afterEach(async () => { globalThis.fetch.mockRestore(); + await flushMarkerStyleDebounce(); }); it('should render marker without popup', () => { @@ -101,11 +120,12 @@ describe('MarkerPopup with remark', () => { }); }); - afterEach(() => { + afterEach(async () => { globalThis.fetch.mockRestore(); + await flushMarkerStyleDebounce(); }); - it('should render marker popup with asterisks when remark is true', () => { + it('should render our own pin with an asterisk badge when remark is true', () => { // eslint-disable-next-line camelcase -- matches backend API schema property name const locationWhenRemarkIsTrue = { ...location, has_remark: true }; act(() => { @@ -122,7 +142,9 @@ describe('MarkerPopup with remark', () => { , ); }); - 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', () => { @@ -140,15 +162,85 @@ describe('MarkerPopup with remark', () => { ); }); - 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'); + }); +}); + +describe('MarkerPopup lazy marker styling', () => { + // A uuid distinct from `location`'s (used by the describes above, which run with + // real timers) so a leftover real setTimeout from those can't resolve into this + // describe's store state mid-test and make "already known" skip our own request. + const lazyLocation = { + position: [51.2, 17.1], + uuid: 'lazy-marker-styling-uuid', + has_remark: false, // eslint-disable-line camelcase -- matches backend API schema property name + }; + + beforeEach(() => { + jest.useFakeTimers(); + useMarkerStylesStore.setState({ stylesByUuid: {} }); + globalThis.MARKER_STYLES = { + icon_field: 'pointType', // eslint-disable-line camelcase -- matches backend API schema property name + icons: { parcelLocker: 'https://cdn.example.com/parcel-locker.svg' }, + }; + httpService.getMarkerStyles.mockResolvedValue({ + [lazyLocation.uuid]: { pointType: 'parcelLocker' }, + }); + }); + + afterEach(() => { + jest.useRealTimers(); + delete globalThis.MARKER_STYLES; + }); + + // render() already wraps itself in act(), so there's nothing left for a caller + // to flush - wrapping it again is redundant (and duplicated across the two + // tests below, which is what this helper avoids). + const renderLazyLocationMarker = () => + render( + + + , + ); + + it('fetches marker styling once the marker becomes individually visible', async () => { + renderLazyLocationMarker(); + + expect(httpService.getMarkerStyles).not.toHaveBeenCalledWith([lazyLocation.uuid]); + + await act(async () => { + jest.advanceTimersByTime(200); + await Promise.resolve(); + }); + + expect(httpService.getMarkerStyles).toHaveBeenCalledWith([lazyLocation.uuid]); + }); + + it('re-renders the marker with the lazily-fetched icon once it arrives', async () => { + renderLazyLocationMarker(); + + // Nothing matched yet - default Leaflet icon, no custom pin + expect(document.querySelector('.custom-typed-marker-icon')).not.toBeInTheDocument(); + + await act(async () => { + jest.advanceTimersByTime(200); + await Promise.resolve(); + }); + + const marker = document.querySelector('.custom-typed-marker-icon'); + expect(marker).toBeInTheDocument(); + expect(marker.innerHTML).toContain('https://cdn.example.com/parcel-locker.svg'); }); }); diff --git a/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx b/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx new file mode 100644 index 00000000..abf79e98 --- /dev/null +++ b/frontend/tests/MarkerPopup/getTypedMarkerIcon.test.jsx @@ -0,0 +1,172 @@ +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 (icon_field, color_field, 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 the place value has no matching icon or color entry', () => { + setMarkerStyles(`{ + "icon_field": "pointType", + "color_field": "pointStatus", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "colors": { "open": "#2e7d32" } + }`); + + expect( + getTypedMarkerIcon({ uuid: '1', position: [50, 50], pointType: 'unknownType' }), + ).toBeNull(); + }); + + it('builds a DivIcon when the icon field matches a configured type icon', () => { + setMarkerStyles(`{ + "icon_field": "pointType", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + pointType: '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 color_field 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(`{ + "icon_field": "pointType", + "color_field": "pointStatus", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "colors": { "open": "#2e7d32" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + pointType: 'parcelLocker', + pointStatus: '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(`{ + "color_field": "pointStatus", + "colors": { "open": "#2e7d32" } + }`); + + const icon = getTypedMarkerIcon({ uuid: '1', position: [50, 50], pointStatus: 'open' }); + + expect(icon).not.toBeNull(); + expect(icon.options.html).toContain('#2e7d32'); + }); + + it('picks the color matching each value on a multi-tier color_field (e.g. speed-based coloring)', () => { + setMarkerStyles(`{ + "color_field": "speedLimit", + "colors": { "10": "#2e7d32", "30": "#ef6c00", "50": "#c62828" } + }`); + + const iconFor = speedLimit => + getTypedMarkerIcon({ uuid: '1', position: [50, 50], speedLimit }); + + 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 place.has_remark is set and a match was found', () => { + setMarkerStyles(`{ + "icon_field": "pointType", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + pointType: 'parcelLocker', + has_remark: 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 place.has_remark is not set', () => { + setMarkerStyles(`{ + "icon_field": "pointType", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" } + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + pointType: 'parcelLocker', + }); + + expect(icon.options.html).not.toContain('>*'); + }); + + it('returns our own pin in the fallback color with just the badge when has_remark is set but nothing matches', () => { + setMarkerStyles('{}'); + + const icon = getTypedMarkerIcon({ uuid: '1', position: [50, 50], has_remark: 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 remark 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(`{ + "icon_field": "pointType", + "icons": { "parcelLocker": "https://cdn.example.com/parcel-locker.svg" }, + "default_color": "#123456" + }`); + + const icon = getTypedMarkerIcon({ + uuid: '1', + position: [50, 50], + pointType: 'parcelLocker', + }); + + expect(icon.options.html).not.toContain('#123456'); + expect(icon.options.html).toContain('background-color:black'); + }); +}); diff --git a/frontend/tests/MarkerPopup/requestMarkerStyle.test.js b/frontend/tests/MarkerPopup/requestMarkerStyle.test.js new file mode 100644 index 00000000..7f2abebf --- /dev/null +++ b/frontend/tests/MarkerPopup/requestMarkerStyle.test.js @@ -0,0 +1,66 @@ +import requestMarkerStyle from '../../src/components/MarkerPopup/requestMarkerStyle'; +import httpService from '../../src/services/http/httpService'; +import useMarkerStylesStore from '../../src/components/Map/store/markerStyles.store'; + +jest.mock('../../src/services/http/httpService'); + +describe('requestMarkerStyle', () => { + beforeEach(() => { + jest.useFakeTimers(); + jest.clearAllMocks(); + useMarkerStylesStore.setState({ stylesByUuid: {} }); + globalThis.MARKER_STYLES = { icon_field: 'pointType' }; // eslint-disable-line camelcase -- matches backend API schema property name + delete globalThis.FEATURE_FLAGS; + }); + + afterEach(() => { + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + delete globalThis.MARKER_STYLES; + delete globalThis.FEATURE_FLAGS; + }); + + it('still fetches when marker styling is not configured, for has_remark', () => { + globalThis.MARKER_STYLES = {}; + httpService.getMarkerStyles.mockResolvedValue({ 'uuid-1': { has_remark: true } }); // eslint-disable-line camelcase -- matches backend API schema property name + requestMarkerStyle('uuid-1'); + jest.runAllTimers(); + expect(httpService.getMarkerStyles).toHaveBeenCalledWith(['uuid-1']); + }); + + it('does nothing when server-side clustering is enabled', () => { + globalThis.FEATURE_FLAGS = { USE_SERVER_SIDE_CLUSTERING: true }; + requestMarkerStyle('uuid-1'); + jest.runAllTimers(); + expect(httpService.getMarkerStyles).not.toHaveBeenCalled(); + }); + + it('batches uuids requested within the debounce window into one request', async () => { + httpService.getMarkerStyles.mockResolvedValue({ 'uuid-1': { pointType: 'a' } }); + requestMarkerStyle('uuid-1'); + requestMarkerStyle('uuid-2'); + jest.runAllTimers(); + await Promise.resolve(); + expect(httpService.getMarkerStyles).toHaveBeenCalledTimes(1); + expect(httpService.getMarkerStyles).toHaveBeenCalledWith(['uuid-1', 'uuid-2']); + }); + + it('merges results into the store, defaulting unmatched uuids to {}', async () => { + httpService.getMarkerStyles.mockResolvedValue({ 'uuid-1': { pointType: 'a' } }); + requestMarkerStyle('uuid-1'); + requestMarkerStyle('uuid-2'); + jest.runAllTimers(); + await Promise.resolve(); + expect(useMarkerStylesStore.getState().stylesByUuid).toEqual({ + 'uuid-1': { pointType: 'a' }, + 'uuid-2': {}, + }); + }); + + it('does not re-request a uuid already known, even with no matching styling', () => { + useMarkerStylesStore.setState({ stylesByUuid: { 'uuid-1': {} } }); + requestMarkerStyle('uuid-1'); + jest.runAllTimers(); + expect(httpService.getMarkerStyles).not.toHaveBeenCalled(); + }); +}); diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index e59b7915..eb5806ca 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -73,15 +73,42 @@ class LocationBasicInfo(BaseModel): 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" - ) class LocationList(RootModel[list[LocationBasicInfo]]): """List of points, each with identity and position only.""" +class LocationMarkerStyles(RootModel[dict[str, dict[str, Any]]]): + """Map of uuid -> pin styling data: has_remark (drives the asterisk badge) plus + whatever marker_styles.icon_field/color_field point at (drive icon/color) - for + lazily fetching it once a marker becomes individually visible instead of getting + it upfront for every location. Unknown/missing uuids are simply absent from the + response, not an error.""" + + +class MarkerStylesQueryParams(BaseModel): + """Query parameters of the marker styles lazy-loading endpoint.""" + + uuid: list[str] = Field(default_factory=list, description="Location UUIDs to fetch styling for") + + +def marker_style_values(location: BaseModel, style_fields: frozenset[str]) -> dict[str, Any]: + """Pin styling data for `location`, as /api/locations/marker-styles returns it. + + Always includes has_remark (drives the asterisk badge), plus the value of any + of `style_fields` this location actually has (drive icon/color). 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. + """ + data: dict[str, Any] = {"has_remark": bool(getattr(location, "remark", None))} + for field in sorted(style_fields): + value = getattr(location, field, None) + if value is not None: + data[field] = value + return data + + class ClusterInfo(BaseModel): """One entry of the clustered list: either a single point or a cluster of them.""" diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 09be7455..ac4285c1 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -25,12 +25,15 @@ LanguagesResponse, LocationDetail, LocationList, + LocationMarkerStyles, LocationQueryParams, LocationReportRequest, LocationReportResponse, LocationSchemaResponse, + MarkerStylesQueryParams, SuccessResponse, VersionResponse, + marker_style_values, ) from goodmap.clustering import ( MAX_ZOOM, @@ -195,6 +198,7 @@ def core_pages( photo_attachment_config: AttachmentConfig, feature_flags: FeatureFlagSet, shortcodes: dict[str, Shortcode], + pin_marker_fields: frozenset[str] = frozenset(), ) -> Blueprint: core_api_blueprint = Blueprint("api", __name__, url_prefix="/api") @@ -341,8 +345,9 @@ 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, showing only uuid and + position. Pin styling (has_remark, marker_styles field values) is fetched + separately, per-uuid, via /api/locations/marker-styles. """ locations = get_locations_from_request(database, request.args) return jsonify(locations) @@ -418,6 +423,30 @@ def get_location(location_id): formatted_data = prepare_pin(location.model_dump(), visible_data, meta_data, shortcodes) return jsonify(formatted_data) + @core_api_blueprint.route("/locations/marker-styles", methods=["GET"]) + @spec.validate( + tags=[TAG_MAP_DATA], + query=MarkerStylesQueryParams, + resp=Response(HTTP_200=LocationMarkerStyles), + ) + def get_locations_marker_styles(): + """Get pin styling data for specific locations, by uuid. + + For lazily fetching has_remark and marker_styles-relevant field values + only once a client-side-clustered marker becomes individually visible, + instead of the frontend getting them upfront for every location (see + lazy-load-marker-styling-plan.md). Unknown or missing uuids are + silently omitted from the response rather than erroring the whole + request - a marker that's re-clustered mid-flight isn't a client bug. + """ + result: dict[str, dict[str, Any]] = {} + for location_uuid in request.args.getlist("uuid"): + location = database.get_location(location_uuid) + if location is None: + continue + result[location_uuid] = marker_style_values(location, pin_marker_fields) + return jsonify(result) + @core_api_blueprint.route("/version", methods=["GET"]) @spec.validate(tags=[TAG_META], resp=Response(HTTP_200=VersionResponse)) def get_version(): 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..a1213650 100644 --- a/goodmap/db.py +++ b/goodmap/db.py @@ -562,6 +562,70 @@ 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 @@ -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", {}), @@ -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..9bca9f96 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -13,7 +13,6 @@ 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.core_api import core_pages @@ -21,9 +20,11 @@ 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.plugin import CAPABILITY_BASES, GoodmapPluginBase logger = logging.getLogger(__name__) @@ -110,34 +111,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 +168,28 @@ 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. + location_obligatory_fields = get_location_obligatory_fields(app.db) + categories = get_category_data(app.db)(app.db)["categories"] + marker_styles = get_marker_styles(app.db)(app.db) + marker_style_fields = { + field + for field in (marker_styles.get("icon_field"), marker_styles.get("color_field")) + if field is not None + } + + 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. + pin_marker_fields = frozenset(marker_style_fields) & obligatory_field_names app.extensions["goodmap"] = {"location_obligatory_fields": location_obligatory_fields} @@ -268,6 +257,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 +275,17 @@ def index(): Returns: Rendered map.html template with feature flags and the plugin manifest """ + try: + marker_styles = app.db.get_marker_styles() # type: ignore[attr-defined] + except (KeyError, AttributeError): + marker_styles = {} + 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..4852a8a9 100644 --- a/goodmap/templates/map.html +++ b/goodmap/templates/map.html @@ -116,11 +116,12 @@ 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, see 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..adbbf8db 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 - both has_remark and marker styling values are fetched + separately (see goodmap.api.api_models.marker_style_values and + lazy-load-marker-styling-plan.md), only once a marker is actually visible.""" + 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..2f5af344 --- /dev/null +++ b/tests/unit_tests/test_api_models.py @@ -0,0 +1,43 @@ +from typing import cast + +from goodmap.api.api_models import marker_style_values +from goodmap.data_models.location import LocationBase, create_location_model + + +def test_marker_style_values_includes_has_remark_and_configured_field_values(): + """marker_style_values() always includes has_remark (drives the asterisk + badge), plus the requested style_fields' values (drive icon/color) 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, frozenset({"type_of_place"})) == { + "has_remark": True, + "type_of_place": "parcel_locker", + } + + +def test_marker_style_values_has_remark_false_and_empty_when_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, frozenset()) == {"has_remark": False} + + +def test_marker_style_values_ignores_style_fields_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, frozenset({"nonexistent_field"})) == {"has_remark": False} diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index 6dbb03e5..d8a3aab8 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -272,12 +272,10 @@ def test_get_locations(test_app): { "uuid": "11111111-1111-1111-1111-111111111111", "position": [50, 50], - "has_remark": True, }, { "uuid": "22222222-2222-2222-2222-222222222222", "position": [60, 60], - "has_remark": False, }, ] @@ -315,6 +313,152 @@ def test_get_locations_accepts_valid_and_undeclared_parameters(test_app, query): assert response.status_code == 200 +# Fixture shared by the /api/locations and /api/locations/marker-styles tests +# below: point_type-categorized locker locations with a matching marker_styles +# config. Kept as data + a small factory, not one big db_overrides literal per +# test, so each test only states what it actually varies. +_LOCKER_LOCATIONS = [ + { + "name": "locker-1", + "position": [50, 50], + "point_type": "parcel_locker", + "uuid": "11111111-1111-1111-1111-111111111111", + }, + { + "name": "locker-2", + "position": [51, 51], + "point_type": "container", + "uuid": "22222222-2222-2222-2222-222222222222", + }, +] + + +def _create_marker_styles_test_app(data=_LOCKER_LOCATIONS, **db_overrides): + overrides = { + "categories": {"point_type": ["parcel_locker", "container"]}, + "location_obligatory_fields": [("point_type", "str"), ("name", "str")], + "marker_styles": {"icon_field": "point_type", "icons": {}, "colors": {}}, + "data": data, + "visible_data": ["name", "point_type"], + } + overrides.update(db_overrides) + return create_test_app(db_overrides=overrides) + + +def test_get_locations_omits_marker_style_field_values(): + """/api/locations should not surface the field marker_styles.icon_field + points at (e.g. a point-type category) - that value is fetched lazily via + /api/locations/marker-styles, only once a marker is individually visible, + instead of upfront for every location (see lazy-load-marker-styling-plan.md).""" + client = _create_marker_styles_test_app(data=_LOCKER_LOCATIONS[:1]) + + response = client.get("/api/locations") + + assert response.status_code == 200 + assert response.json == [ + { + "uuid": "11111111-1111-1111-1111-111111111111", + "position": [50, 50], + }, + ] + + +def test_get_locations_marker_styles_returns_requested_uuids_styling(): + """The lazy marker-styles endpoint returns just the marker_styles-relevant + field values for the requested uuids, not the full location.""" + client = _create_marker_styles_test_app() + + response = client.get("/api/locations/marker-styles?uuid=11111111-1111-1111-1111-111111111111") + + assert response.status_code == 200 + assert response.json == { + "11111111-1111-1111-1111-111111111111": { + "has_remark": False, + "point_type": "parcel_locker", + }, + } + + +def test_get_locations_marker_styles_supports_multiple_uuids(): + client = _create_marker_styles_test_app() + + response = client.get( + "/api/locations/marker-styles" + "?uuid=11111111-1111-1111-1111-111111111111" + "&uuid=22222222-2222-2222-2222-222222222222" + ) + + assert response.status_code == 200 + assert response.json == { + "11111111-1111-1111-1111-111111111111": { + "has_remark": False, + "point_type": "parcel_locker", + }, + "22222222-2222-2222-2222-222222222222": { + "has_remark": False, + "point_type": "container", + }, + } + + +def test_get_locations_marker_styles_omits_unknown_uuids(): + """An unknown/re-clustered-away uuid doesn't error the whole request - it's + just absent from the response.""" + client = _create_marker_styles_test_app( + data=_LOCKER_LOCATIONS[:1], categories={"point_type": ["parcel_locker"]} + ) + + response = client.get( + "/api/locations/marker-styles" + "?uuid=11111111-1111-1111-1111-111111111111" + "&uuid=99999999-9999-9999-9999-999999999999" + ) + + assert response.status_code == 200 + assert response.json == { + "11111111-1111-1111-1111-111111111111": { + "has_remark": False, + "point_type": "parcel_locker", + }, + } + + +def test_get_locations_marker_styles_includes_has_remark_without_marker_styles_config(): + """has_remark drives the asterisk badge independently of marker_styles - + deployments with no icon_field/color_field configured still need it fetched + lazily, the same as everyone else.""" + client = create_test_app( + db_overrides={ + "categories": {}, + "location_obligatory_fields": [("name", "str")], + "data": [ + { + "name": "test", + "position": [50, 50], + "uuid": "11111111-1111-1111-1111-111111111111", + "remark": "this is a remark", + }, + ], + } + ) + + response = client.get("/api/locations/marker-styles?uuid=11111111-1111-1111-1111-111111111111") + + assert response.status_code == 200 + assert response.json == { + "11111111-1111-1111-1111-111111111111": {"has_remark": True}, + } + + +def test_get_locations_marker_styles_empty_query_returns_empty_object(): + client = create_test_app(db_overrides={"categories": {}}) + + response = client.get("/api/locations/marker-styles") + + assert response.status_code == 200 + assert response.json == {} + + 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.""" @@ -1029,7 +1173,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) diff --git a/tests/unit_tests/test_db.py b/tests/unit_tests/test_db.py index d915ce2a..9759c97c 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, @@ -57,6 +58,7 @@ json_file_db_get_data, json_file_db_get_location_obligatory_fields, 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 +83,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 +299,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 +379,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 +1169,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() diff --git a/tests/unit_tests/test_goodmap.py b/tests/unit_tests/test_goodmap.py index 5fab3be2..aa7b79dd 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,51 @@ 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_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 +292,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 +312,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