diff --git a/docs/api.rst b/docs/api.rst index 054d942c..856ad768 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -19,6 +19,27 @@ Database Module :undoc-members: :show-inheritance: +Query Module +------------ + +Sorting, limiting, and query orchestration for location data. + +.. automodule:: goodmap.core + :members: + :undoc-members: + :show-inheritance: + +Filtering Module +---------------- + +Category filter combination logic (see :ref:`categories-filter-mode` in +:doc:`quickstart` for the config-level guide). + +.. automodule:: goodmap.filtering + :members: + :undoc-members: + :show-inheritance: + API Endpoints ------------- diff --git a/docs/quickstart.rst b/docs/quickstart.rst index 3fa7f54e..18b90ffa 100644 --- a/docs/quickstart.rst +++ b/docs/quickstart.rst @@ -70,6 +70,8 @@ Example configuration in your data source: ] } +.. _categories-filter-mode: + Categories and Filtering ~~~~~~~~~~~~~~~~~~~~~~~~ @@ -92,14 +94,66 @@ checkboxes, one per allowed value. pre-checked in the filter panel when the app first loads, before the user has made any selection. -Example configuration in your data source: +``categories_filter_mode`` + Dict of category key to how *multiple selected values within that + category* are combined when filtering locations. Categories not listed + here default to ``"or"``. This only affects combination **within** one + category - across different categories, selections are always combined + with AND (a location must match every category that has an active + selection). + + ``"or"`` (default) + A location matches if it has **any** of the selected values. This is + the usual "check more boxes to broaden results" behavior - e.g. + checking both ``bikes`` and ``cars`` on ``accessible_by`` shows + locations that allow bikes *or* cars, not only locations that allow + both (which would often be zero results). + + ``"and"`` + A location matches only if it has **every** one of the selected + values - narrowing rather than broadening. Only meaningful for + list-valued categories (a location can have several simultaneous + values); for a single-valued category it behaves like ``"or"`` + restricted to one selection at a time. Still rendered as checkboxes + (it's still multi-select), but with a "(match all)" hint next to the + category title so it reads differently from the default "or" + behavior - e.g. an ``amenities`` field where checking ``lighting`` + and ``benches`` should show only bridges that have both, not either. + + ``"exclusive"`` + Single-select: the frontend renders the options as radio buttons + instead of checkboxes, so only one value can be active at a time. Use + this for categories with three or more mutually-exclusive states, + e.g. a toll tier: ``free`` / ``discounted`` / ``full_price``. + + ``"boolean"`` + For a field with exactly the two values ``"true"`` and ``"false"``. + Only the ``"true"`` option is rendered, as a single checkbox; leaving + it unchecked already means "show everything" (both true and false + locations), so there's no separate control for isolating ``"false"`` + alone. Use this when nobody would deliberately filter for the + negative case - e.g. a "free only" checkbox for an ``is_free`` field, + since drivers care about "free" or "all", not "paid only". + + ``"threshold"`` + For an ordered, numeric-valued category, e.g. a speed limit in km/h. + Selecting a value matches any location whose value is **at or below** + the highest selected value - e.g. selecting ``30`` also matches + locations with ``10`` or ``30``, but not ``50``. The frontend renders + this as radio buttons too, since selecting more than one option would + be redundant: the highest selection alone determines the cutoff. + +Example configuration combining all five modes: .. code-block:: json { "categories": { "accessible_by": ["bikes", "cars", "pedestrians"], - "type_of_place": ["big bridge", "small bridge"] + "type_of_place": ["big bridge", "small bridge"], + "is_free": ["true", "false"], + "speed_limit": ["10", "30", "50"], + "amenities": ["lighting", "benches", "toilets"] }, "categories_help": ["accessible_by"], "categories_options_help": { @@ -107,9 +161,20 @@ Example configuration in your data source: }, "categories_default_checked": { "accessible_by": ["cars"] + }, + "categories_filter_mode": { + "accessible_by": "or", + "type_of_place": "or", + "is_free": "boolean", + "speed_limit": "threshold", + "amenities": "and" } } +The active mode for each category is also exposed as ``filter_mode`` in the +``/api/categories-full`` response, so a custom frontend can render the +right control (checkbox vs. radio) without hardcoding category names. + .. _data-model-visible_data: Database Types diff --git a/e2e-tests/e2e_test_data_initial.json b/e2e-tests/e2e_test_data_initial.json index 37eead36..17e8b58d 100644 --- a/e2e-tests/e2e_test_data_initial.json +++ b/e2e-tests/e2e_test_data_initial.json @@ -4,7 +4,7 @@ { "name": "Grunwaldzki", "position": [ - 51.1095, + 51.109444, 17.0525 ], "accessible_by": [ @@ -12,6 +12,12 @@ "cars" ], "type_of_place": "big bridge", + "is_free": "true", + "speed_limit": "50", + "amenities": [ + "lighting", + "benches" + ], "uuid": "9264286a-5d33-4e38-ab11-c8e179a7754a", "CTA": { "type": "CTA", @@ -22,16 +28,166 @@ { "name": "Zwierzyniecka", "position": [ - 51.10655, - 17.0555 + 51.108056, + 17.07 ], "accessible_by": [ "bikes", "pedestrians" ], "type_of_place": "small bridge", + "is_free": "true", + "speed_limit": "10", + "amenities": [ + "lighting" + ], "uuid": "c8ecf476-5968-40da-ba5c-e810ad9ff203", "remark": "very old bridge" + }, + { + "name": "Milenijny", + "position": [ + 51.133692, + 16.993103 + ], + "accessible_by": [ + "cars" + ], + "type_of_place": "big bridge", + "is_free": "false", + "speed_limit": "30", + "amenities": [], + "uuid": "1a8f9a2e-4b6d-4a1a-9a8e-2f6c7d0b3e9a", + "remark": "toll bridge, cars only" + }, + { + "name": "Pokoju", + "position": [ + 51.111739, + 17.049236 + ], + "accessible_by": [ + "cars", + "pedestrians" + ], + "type_of_place": "big bridge", + "is_free": "true", + "speed_limit": "50", + "amenities": [ + "lighting", + "benches", + "toilets" + ], + "uuid": "f03afca0-4ef8-45bf-a176-dd477ce48d92" + }, + { + "name": "Tumski", + "position": [ + 51.114708, + 17.042319 + ], + "accessible_by": [ + "pedestrians" + ], + "type_of_place": "small bridge", + "is_free": "true", + "speed_limit": "10", + "amenities": [ + "lighting", + "benches" + ], + "uuid": "e8b8a5b2-91e1-49f9-821b-5c7fa9eceb4b", + "remark": "connects the cathedral island" + }, + { + "name": "Piaskowy", + "position": [ + 51.113542, + 17.039806 + ], + "accessible_by": [ + "pedestrians" + ], + "type_of_place": "small bridge", + "is_free": "true", + "speed_limit": "10", + "amenities": [ + "benches" + ], + "uuid": "5986e755-1eaa-4121-a01c-4fef1d5d1da1" + }, + { + "name": "Uniwersytecki", + "position": [ + 51.11525, + 17.033694 + ], + "accessible_by": [ + "pedestrians" + ], + "type_of_place": "small bridge", + "is_free": "true", + "speed_limit": "30", + "amenities": [ + "lighting" + ], + "uuid": "043dd6ed-1b70-40a1-a7ad-dbfa3424ed8e" + }, + { + "name": "Redzinski", + "position": [ + 51.155556, + 16.958889 + ], + "accessible_by": [ + "cars" + ], + "type_of_place": "big bridge", + "is_free": "false", + "speed_limit": "50", + "amenities": [ + "lighting" + ], + "uuid": "66847536-4ffb-452d-b03d-874831abb233", + "remark": "highway bridge, cars only" + }, + { + "name": "Osobowicki", + "position": [ + 51.131389, + 17.027778 + ], + "accessible_by": [ + "cars", + "pedestrians" + ], + "type_of_place": "big bridge", + "is_free": "true", + "speed_limit": "30", + "amenities": [ + "lighting", + "benches" + ], + "uuid": "1d7ff5c0-ea4d-4701-bf53-49295333e176" + }, + { + "name": "Warszawski", + "position": [ + 51.130256, + 17.059375 + ], + "accessible_by": [ + "cars", + "pedestrians" + ], + "type_of_place": "big bridge", + "is_free": "true", + "speed_limit": "50", + "amenities": [ + "lighting", + "toilets" + ], + "uuid": "e77cac7f-78ba-4a6d-8d99-8018c5057de6" } ], "location_obligatory_fields": [ @@ -46,6 +202,18 @@ [ "type_of_place", "str" + ], + [ + "is_free", + "str" + ], + [ + "speed_limit", + "str" + ], + [ + "amenities", + "list" ] ], "reported_issue_types": [ @@ -62,10 +230,33 @@ "type_of_place": [ "big bridge", "small bridge" + ], + "is_free": [ + "true", + "false" + ], + "speed_limit": [ + "10", + "30", + "50" + ], + "amenities": [ + "lighting", + "benches", + "toilets" ] }, + "categories_filter_mode": { + "accessible_by": "or", + "type_of_place": "or", + "is_free": "boolean", + "speed_limit": "threshold", + "amenities": "and" + }, "categories_help": [ - "accessible_by" + "accessible_by", + "speed_limit", + "amenities" ], "categories_options_help": { "type_of_place": [ @@ -85,6 +276,9 @@ "remark", "accessible_by", "type_of_place", + "is_free", + "speed_limit", + "amenities", "CTA" ], "meta_data": [ @@ -153,7 +347,6 @@ "url": "https://fonts.googleapis.com/css2?family=Poppins" }, "primary_color": "#FFFFFF", - "secondary_color": "#245466", - "left_bar_width": "300px" + "secondary_color": "#245466" } } diff --git a/e2e-tests/scripts/generate_stress_test_data.py b/e2e-tests/scripts/generate_stress_test_data.py index 7bfbe8e7..0470d2c8 100644 --- a/e2e-tests/scripts/generate_stress_test_data.py +++ b/e2e-tests/scripts/generate_stress_test_data.py @@ -83,7 +83,6 @@ def main(): }, "primary_color": "#FFFFFF", "secondary_color": "#245466", - "left_bar_width": "300px", }, } diff --git a/e2e-tests/tests/basic/test_accessibility_table.py b/e2e-tests/tests/basic/test_accessibility_table.py index 0c9255ea..2707c647 100644 --- a/e2e-tests/tests/basic/test_accessibility_table.py +++ b/e2e-tests/tests/basic/test_accessibility_table.py @@ -10,7 +10,13 @@ import pytest from playwright.sync_api import Page, expect -from tests.conftest import BASE_URL, MARKER_LOAD_TIMEOUT, TABLE_LOAD_TIMEOUT, TEST_LOCATIONS +from tests.conftest import ( + BASE_URL, + MARKER_LOAD_TIMEOUT, + SEEDED_LOCATION_COUNT, + TABLE_LOAD_TIMEOUT, + TEST_LOCATIONS, +) class TestAccessibilityTable: @@ -28,8 +34,8 @@ def setup(self, page: Page, geolocation): # "accessible_by: cars" is checked by default (see categories_default_checked # in the test data), which excludes Zwierzyniecka (bikes/pedestrians only, - # no cars). Uncheck it so both seeded locations are visible, matching what - # these tests assert. + # no cars). Uncheck it so all seeded locations are visible, matching + # what these tests assert. page.wait_for_selector("#filter-form", timeout=MARKER_LOAD_TIMEOUT) page.locator("#filter-form input#cars").uncheck() @@ -50,11 +56,11 @@ def setup(self, page: Page, geolocation): def test_should_properly_display_places(self, page: Page): """ - Verify table displays correct number of rows. - Should have 1 header row + 2 data rows = 3 total rows. + Verify table displays correct number of rows: 1 header row plus one + row per seeded location. """ rows = page.locator("tr") - expect(rows).to_have_count(3) + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) def test_zwierzyniecka_should_be_first_row(self, page: Page): """ diff --git a/e2e-tests/tests/basic/test_language.py b/e2e-tests/tests/basic/test_language.py index f536a4c6..b0460ba1 100644 --- a/e2e-tests/tests/basic/test_language.py +++ b/e2e-tests/tests/basic/test_language.py @@ -10,7 +10,7 @@ import pytest from playwright.sync_api import Page, expect -from tests.conftest import BASE_URL, MARKER_LOAD_TIMEOUT, clear_all_checkboxes +from tests.conftest import BASE_URL, open_test_popup def get_language_button(page: Page): @@ -71,35 +71,7 @@ def test_switch_to_polish_changes_popup_text(self, page: Page): page.get_by_role("link", name="polski").click() page.wait_for_load_state("domcontentloaded") - clear_all_checkboxes(page) - - # Click marker cluster to expand - page.locator(".leaflet-marker-icon").first.click() - - # Wait for markers to appear - markers = page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2, timeout=MARKER_LOAD_TIMEOUT) - - # Click rightmost marker to open popup - page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + open_test_popup(page) # Verify popup is visible popup = page.locator(".leaflet-popup-content") diff --git a/e2e-tests/tests/basic/test_map.py b/e2e-tests/tests/basic/test_map.py index 5a084286..98db6c8f 100644 --- a/e2e-tests/tests/basic/test_map.py +++ b/e2e-tests/tests/basic/test_map.py @@ -7,27 +7,75 @@ import pytest from playwright.sync_api import Page, expect -from tests.conftest import BASE_URL +from tests.conftest import BASE_URL, SEEDED_LOCATION_COUNT, TABLE_LOAD_TIMEOUT, TEST_LOCATIONS class TestMap: """Test suite for map functionality""" @pytest.fixture(autouse=True) - def setup(self, page: Page): - """Navigate to home page before each test""" + def setup(self, page: Page, geolocation): + """Navigate to home page before each test. + + Also grants geolocation (set to WROCLAW_CENTER), since several tests + below use the List View table to verify filter results - it's a + clustering-independent read of exactly what's currently filtered in, + unlike counting `.leaflet-marker-icon` elements on the map, which + depends on Leaflet.markercluster's zoom-dependent grouping and isn't a + reliable way to assert result counts once bridges are spread across + realistic real-world distances (a single cluster can require multiple + zoom-in clicks to fully expand, rather than one click). + """ + location = TEST_LOCATIONS["WROCLAW_CENTER"] + geolocation(location["lat"], location["lon"]) page.goto(BASE_URL, wait_until="domcontentloaded") return - def test_displays_filter_list_with_two_categories_with_5_items(self, page: Page): - """Verify filter list has correct number of checkboxes and category groups""" - # Check number of checkboxes (5 filter options) + def _open_list_view(self, page: Page): + """Switch to List View and return the results table locator.""" + list_view_button = page.locator('button[id="listViewButton"]') + expect(list_view_button).to_be_visible(timeout=5000) + list_view_button.click() + + table = page.locator("table") + expect(table).to_be_visible(timeout=TABLE_LOAD_TIMEOUT) + return table + + def test_displays_filter_list_with_four_categories(self, page: Page): + """Verify filter list has correct number of checkboxes/radios and category groups""" + # accessible_by (3) + type_of_place (2) + amenities (3, "and" mode is + # still multi-select) = 8 "or"/"and" checkboxes, plus is_free + # ("boolean") contributes 1 more checkbox (only its "true" option is + # rendered; "false" is hidden - see FiltersForm.jsx). checkboxes = page.get_by_role("checkbox") - expect(checkboxes).to_have_count(5) + expect(checkboxes).to_have_count(9) - # Check that both category groups are present (using translated names) + # speed_limit ("threshold") is single-select, rendered as radios. + radios = page.get_by_role("radio") + expect(radios).to_have_count(3) + + # Check that all category groups are present (using translated names). + # is_free doesn't get its own header - it's grouped into "Others" + # (see FiltersForm.jsx), labeled with its own translated name. expect(page.get_by_text("accessible by")).to_be_visible() expect(page.get_by_text("type of place")).to_be_visible() + expect(page.get_by_text("speed limit")).to_be_visible() + expect(page.get_by_text("Others")).to_be_visible() + expect(page.get_by_text("Free only")).to_be_visible() + expect(page.get_by_text("amenities")).to_be_visible() + + def mode_badge(symbol): + return page.get_by_label("Filter mode:", exact=False).filter(has_text=symbol) + + # check whether badges are there + expect(mode_badge("+")).to_have_count(2) # accessible_by, type_of_place + expect(mode_badge("&")).to_have_count(1) # amenities + expect(mode_badge("≤")).to_have_count(1) # speed_limit + expect(mode_badge("•")).to_have_count(1) # is_free + + # Badges are keyboard-focusable (not just hoverable), so their + # tooltip is reachable without a mouse. + expect(mode_badge("&")).to_have_attribute("tabindex", "0") def test_should_not_have_scrollbars(self, page: Page): """Verify the page has no horizontal or vertical scrollbars""" @@ -57,34 +105,114 @@ def test_should_not_have_scrollbars(self, page: Page): ) def test_filter_checkbox_filters_markers(self, page: Page): - """Verify clicking filter checkbox actually filters the markers on the map""" - # On desktop, filter panel is already visible (no toggle needed). + """Verify clicking filter checkbox actually filters the results""" # "accessible_by: cars" is checked by default (see categories_default_checked - # in the test data), so start by clearing it to see both seeded locations. + # in the test data), so start by clearing it to see all seeded locations. cars_checkbox = page.get_by_role("checkbox", name="cars", exact=False) expect(cars_checkbox).to_be_checked() cars_checkbox.click() - # Wait for markers to load - first_marker = page.locator(".leaflet-marker-icon").first - expect(first_marker).to_be_visible(timeout=5000) + table = self._open_list_view(page) + rows = table.locator("tr") + # 1 header + all seeded locations + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) - # Click marker cluster to expand it - first_marker.click() + # Re-check the "cars" filter checkbox - this should filter to only show + # the 6 bridges accessible by cars (1 header + 6 data rows) + cars_checkbox.click() + expect(rows).to_have_count(7) - # Wait for markers to expand - should be 2 markers after expansion - markers = page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2) + # Uncheck to restore all results + cars_checkbox.click() + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) - # Re-check the "cars" filter checkbox - this should filter to only show - # places accessible by cars (1 marker instead of 2) + def test_or_filter_within_category_broadens_results(self, page: Page): + """Selecting multiple checkboxes within one category (accessible_by) should + return the union of matches (OR semantics), not only bridges that satisfy + every selected option at once (which would incorrectly return nothing here, + since no bridge allows both bikes and cars).""" + cars_checkbox = page.get_by_role("checkbox", name="cars", exact=False) + expect(cars_checkbox).to_be_checked() + cars_checkbox.click() + + bikes_checkbox = page.get_by_role("checkbox", name="bikes", exact=False) + + table = self._open_list_view(page) + rows = table.locator("tr") + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) + + # bikes alone -> only Zwierzyniecka (1 header + 1 data row) + bikes_checkbox.click() + expect(rows).to_have_count(2) + + # bikes OR cars -> union of both (1 + 6, no overlap since no bridge + # allows both bikes and cars): 1 header + 7 data rows + cars_checkbox.click() + expect(rows).to_have_count(8) + + def test_is_free_boolean_filter_toggles_free_only(self, page: Page): + """is_free is a "boolean" filter: a single checkbox for "free only". + Unchecked shows both free and paid bridges (drivers care about "free" + or "all", not "paid only", so there's no separate option for that); + checking it narrows down to free bridges.""" + cars_checkbox = page.get_by_role("checkbox", name="cars", exact=False) + cars_checkbox.click() + + free_checkbox = page.get_by_role("checkbox", name="Free only", exact=False) + expect(free_checkbox).not_to_be_checked() + + table = self._open_list_view(page) + rows = table.locator("tr") + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) + + free_checkbox.click() + expect(free_checkbox).to_be_checked() + # 1 header + 8 free bridges + expect(rows).to_have_count(9) + + # Unchecking goes back to showing both free and paid bridges. + free_checkbox.click() + expect(free_checkbox).not_to_be_checked() + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) + + def test_speed_limit_threshold_filter_includes_lower_values(self, page: Page): + """Selecting a speed limit should also match bridges with a lower limit + (cumulative/threshold semantics), not only an exact match. speed_limit is + single-select (radio), since picking "30" already implies "30 or lower".""" + cars_checkbox = page.get_by_role("checkbox", name="cars", exact=False) cars_checkbox.click() - # After filtering, only 1 marker should be visible (the one accessible by cars) - expect(markers).to_have_count(1) + speed_30_radio = page.get_by_role("radio", name="30 km/h", exact=False) + + table = self._open_list_view(page) + rows = table.locator("tr") + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) - # Uncheck to restore all markers + # 30 km/h also matches the three 10 km/h bridges, but not the 50 km/h + # ones: 1 header + 6 data rows + speed_30_radio.click() + expect(rows).to_have_count(7) + + def test_and_filter_within_category_narrows_results(self, page: Page): + """Selecting multiple checkboxes within an "and" category (amenities) + should narrow results to locations that have every selected value, + the opposite of the default "or" behavior.""" + cars_checkbox = page.get_by_role("checkbox", name="cars", exact=False) + expect(cars_checkbox).to_be_checked() cars_checkbox.click() - # Both markers should be visible again - expect(markers).to_have_count(2) + lighting_checkbox = page.get_by_role("checkbox", name="lighting", exact=False) + benches_checkbox = page.get_by_role("checkbox", name="benches", exact=False) + + table = self._open_list_view(page) + rows = table.locator("tr") + expect(rows).to_have_count(SEEDED_LOCATION_COUNT + 1) + + # lighting alone -> 8 bridges (1 header + 8 data rows) + lighting_checkbox.click() + expect(rows).to_have_count(9) + + # lighting AND benches -> only bridges with both (4), fewer than + # either alone (8 and 5) - the opposite of OR's broadening. + benches_checkbox.click() + expect(rows).to_have_count(5) diff --git a/e2e-tests/tests/basic/test_mobile_box.py b/e2e-tests/tests/basic/test_mobile_box.py index 9c568efc..0cc43133 100644 --- a/e2e-tests/tests/basic/test_mobile_box.py +++ b/e2e-tests/tests/basic/test_mobile_box.py @@ -9,7 +9,7 @@ import pytest from playwright.sync_api import Page, expect -from tests.conftest import ALL_MOBILE_DEVICES, BASE_URL, clear_all_checkboxes +from tests.conftest import ALL_MOBILE_DEVICES, BASE_URL, open_test_popup from tests.helpers import EXPECTED_PLACE_ZWIERZYNIECKA, verify_popup_content, verify_problem_form @@ -32,37 +32,7 @@ def test_displays_title_and_subtitle_in_popup( # Navigate to the page (device emulation already configured by mobile_page fixture) mobile_page.goto(BASE_URL, wait_until="domcontentloaded") - clear_all_checkboxes(mobile_page) - - # Click first marker to expand cluster - # Use JavaScript click to bypass webpack overlay that may intercept clicks on CI - first_marker = mobile_page.locator(".leaflet-marker-icon").first - first_marker.evaluate("el => el.click()") - - # Wait for markers to appear (should be 2 after expansion) - markers = mobile_page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2) - - # Click the rightmost marker - mobile_page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + open_test_popup(mobile_page) # On mobile, popup appears as Material-UI Dialog (bottom sheet) dialog_content = mobile_page.locator(".MuiDialogContent-root") diff --git a/e2e-tests/tests/basic/test_popup.py b/e2e-tests/tests/basic/test_popup.py index cafb4b0f..abff7da6 100644 --- a/e2e-tests/tests/basic/test_popup.py +++ b/e2e-tests/tests/basic/test_popup.py @@ -6,7 +6,7 @@ from playwright.sync_api import Page, expect -from tests.conftest import BASE_URL, MARKER_LOAD_TIMEOUT, clear_all_checkboxes +from tests.conftest import BASE_URL, open_test_popup from tests.helpers import EXPECTED_PLACE_ZWIERZYNIECKA, verify_popup_content, verify_problem_form @@ -22,37 +22,9 @@ def test_displays_popup_title_subtitle_categories_and_cta(self, page: Page, wind """ page.goto(BASE_URL, wait_until="domcontentloaded") - clear_all_checkboxes(page) - - # Click first marker to trigger cluster expansion - first_marker = page.locator(".leaflet-marker-icon").first - first_marker.click() - - # Wait for markers to appear (should be 2 after cluster expansion) - markers = page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2, timeout=MARKER_LOAD_TIMEOUT) - - # Click the rightmost marker - # Note: Using evaluate to find rightmost marker since we don't have data-testid - page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + # Isolate Zwierzyniecka's marker and click it directly, rather than + # expanding a multi-marker cluster and guessing at its layout. + open_test_popup(page) # Verify popup content popup = page.locator(".leaflet-popup-content") @@ -80,36 +52,7 @@ def test_problem_form_on_desktop(self, page: Page, window_open_stub): """ page.goto(BASE_URL, wait_until="domcontentloaded") - clear_all_checkboxes(page) - - # Click first marker to trigger cluster expansion - first_marker = page.locator(".leaflet-marker-icon").first - first_marker.click() - - # Wait for markers to appear (should be 2 after cluster expansion) - markers = page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2, timeout=MARKER_LOAD_TIMEOUT) - - # Click the rightmost marker - page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + open_test_popup(page) # Verify popup is visible popup = page.locator(".leaflet-popup-content") diff --git a/e2e-tests/tests/basic/test_share.py b/e2e-tests/tests/basic/test_share.py index 43911107..674528cb 100644 --- a/e2e-tests/tests/basic/test_share.py +++ b/e2e-tests/tests/basic/test_share.py @@ -15,6 +15,7 @@ BASE_URL, MARKER_LOAD_TIMEOUT, clear_all_checkboxes, + open_test_popup, ) @@ -31,36 +32,7 @@ def test_share_button_copies_link_to_clipboard(self, page: Page): # Grant clipboard permissions page.context.grant_permissions(["clipboard-read", "clipboard-write"]) - clear_all_checkboxes(page) - - # Click first marker to trigger cluster expansion - first_marker = page.locator(".leaflet-marker-icon").first - first_marker.click() - - # Wait for markers to appear (should be 2 after cluster expansion) - markers = page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2, timeout=MARKER_LOAD_TIMEOUT) - - # Click the rightmost marker - page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + open_test_popup(page) # Verify popup is visible popup = page.locator(".leaflet-popup-content") @@ -83,6 +55,13 @@ def test_shared_link_opens_popup_with_correct_content(self, page: Page): """ Verify navigating to a URL with ?locationId= auto-opens the popup with the correct location content. + + Note: this passes because Zwierzyniecka's seeded coordinates keep it + far enough from its nearest neighbor to render as a standalone marker + at the zoom level GoToLocation.jsx navigates to. There is a known, + separate app bug (see TODO in MarkerPopup.jsx) where this same flow + silently fails to open the popup if the target happens to be clustered + under the viewer's current filters/zoom - not exercised by this test. """ page.goto( f"{BASE_URL}/?locationId=c8ecf476-5968-40da-ba5c-e810ad9ff203", @@ -124,36 +103,7 @@ def test_share_button_triggers_native_share(self, mobile_page: Page, device_name mobile_page.goto(BASE_URL, wait_until="domcontentloaded") - clear_all_checkboxes(mobile_page) - - # Click first marker to expand cluster - first_marker = mobile_page.locator(".leaflet-marker-icon").first - first_marker.evaluate("el => el.click()") - - # Wait for markers to appear (should be 2 after expansion) - markers = mobile_page.locator(".leaflet-marker-icon") - expect(markers).to_have_count(2, timeout=MARKER_LOAD_TIMEOUT) - - # Click the rightmost marker - mobile_page.evaluate(""" - () => { - const markers = document.querySelectorAll('.leaflet-marker-icon'); - let rightmostMarker = null; - let maxX = -Infinity; - - markers.forEach(marker => { - const rect = marker.getBoundingClientRect(); - if (rect.x > maxX) { - maxX = rect.x; - rightmostMarker = marker; - } - }); - - if (rightmostMarker) { - rightmostMarker.click(); - } - } - """) + open_test_popup(mobile_page) # On mobile, popup appears as Material-UI Dialog dialog_content = mobile_page.locator(".MuiDialogContent-root") diff --git a/e2e-tests/tests/conftest.py b/e2e-tests/tests/conftest.py index 544819b5..51d5f840 100644 --- a/e2e-tests/tests/conftest.py +++ b/e2e-tests/tests/conftest.py @@ -13,7 +13,7 @@ from pathlib import Path import pytest -from playwright.sync_api import BrowserContext, Page +from playwright.sync_api import BrowserContext, Page, expect BASE_URL = "http://localhost:5000" @@ -21,6 +21,11 @@ TABLE_LOAD_TIMEOUT = 5000 FLY_TO_TIMEOUT = 20000 +# Number of bridges in e2e_test_data_initial.json's "data" array. Kept as a +# single constant so tests don't hardcode this count (and its +1-for-header +# variant) in multiple places. +SEEDED_LOCATION_COUNT = 10 + MOBILE_DEVICES = { "iphone-x": { "viewport": {"width": 375, "height": 812}, @@ -66,7 +71,9 @@ "lon": 20.088, "tile_pattern": r"https://[abc]\.tile\.openstreetmap\.org/1[3-6]/\d+/\d+\.png", }, - "WROCLAW_CENTER": {"lat": 51.10655, "lon": 17.0555}, + # Matches Zwierzyniecka's seeded position exactly, so it's guaranteed to + # sort first by distance in tests that rely on that ordering. + "WROCLAW_CENTER": {"lat": 51.108056, "lon": 17.07}, } @@ -100,6 +107,39 @@ def clear_all_checkboxes(page: Page) -> None: page.locator('button[aria-label="Close left panel"]').evaluate("el => el.click()") +def open_test_popup(page: Page) -> None: + """ + Filter down to exactly one known seeded location (Zwierzyniecka) and + click its marker to open its popup. + + Clicking into a multi-marker cluster spiderfies it into a layout whose + on-screen ordering isn't guaranteed to match geographic position + (especially on narrow mobile viewports) or to be stable as more + locations are seeded, so tests that need a specific, known location's + popup should isolate it first rather than reaching into an expanded + cluster by pixel position (e.g. "rightmost marker"). + + "bikes" uniquely identifies Zwierzyniecka among the seeded e2e bridges + (the only accessible_by option no other bridge has), so checking it + leaves exactly one marker on the map. + """ + toggle_button = page.locator('button[aria-label="Toggle left panel"]') + opened_dialog = toggle_button.is_visible() + if opened_dialog: + toggle_button.click() + + page.wait_for_selector("#filter-form", timeout=MARKER_LOAD_TIMEOUT) + page.locator("#clear-filters-button").click() + page.locator("#filter-form input#bikes").check() + + if opened_dialog: + page.locator('button[aria-label="Close left panel"]').evaluate("el => el.click()") + + markers = page.locator(".leaflet-marker-icon") + expect(markers).to_have_count(1, timeout=MARKER_LOAD_TIMEOUT) + markers.first.click() + + def _block_hmr(page: Page) -> None: """Block HMR/hot reload requests to prevent page refreshes during tests.""" page.route("**/ws", lambda route: route.abort()) diff --git a/e2e-tests/translations/en/LC_MESSAGES/messages.po b/e2e-tests/translations/en/LC_MESSAGES/messages.po index bcd185f6..a070c492 100644 --- a/e2e-tests/translations/en/LC_MESSAGES/messages.po +++ b/e2e-tests/translations/en/LC_MESSAGES/messages.po @@ -21,6 +21,15 @@ msgstr "accessible by" msgid "type_of_place" msgstr "type of place" +msgid "is_free" +msgstr "Free only" + +msgid "speed_limit" +msgstr "speed limit" + +msgid "amenities" +msgstr "amenities" + msgid "remark" msgstr "remark" @@ -41,6 +50,33 @@ msgstr "big bridge" msgid "small bridge" msgstr "small bridge" +# Category values - is_free +msgid "true" +msgstr "yes" + +msgid "false" +msgstr "no" + +# Category values - speed_limit (km/h, left untranslated on purpose) +msgid "10" +msgstr "10 km/h" + +msgid "30" +msgstr "30 km/h" + +msgid "50" +msgstr "50 km/h" + +# Category values - amenities +msgid "lighting" +msgstr "lighting" + +msgid "benches" +msgstr "benches" + +msgid "toilets" +msgstr "toilets" + # Reported issue types msgid "under construction" msgstr "under construction" @@ -52,6 +88,12 @@ msgstr "has a hole" msgid "categories_help_accessible_by" msgstr "Who can use this bridge" +msgid "categories_help_speed_limit" +msgstr "Selecting a speed limit also shows bridges with a lower limit" + +msgid "categories_help_amenities" +msgstr "Selecting several shows only bridges that have all of them" + # Help texts - category options msgid "categories_options_help_small bridge" msgstr "A smaller pedestrian or bike bridge" diff --git a/e2e-tests/translations/pl/LC_MESSAGES/messages.po b/e2e-tests/translations/pl/LC_MESSAGES/messages.po index c76a5fb1..ed6418c7 100644 --- a/e2e-tests/translations/pl/LC_MESSAGES/messages.po +++ b/e2e-tests/translations/pl/LC_MESSAGES/messages.po @@ -21,6 +21,15 @@ msgstr "dostępny dla" msgid "type_of_place" msgstr "typ miejsca" +msgid "is_free" +msgstr "tylko bezpłatne" + +msgid "speed_limit" +msgstr "ograniczenie prędkości" + +msgid "amenities" +msgstr "udogodnienia" + msgid "remark" msgstr "uwaga" @@ -41,6 +50,33 @@ msgstr "duży most" msgid "small bridge" msgstr "mały most" +# Category values - is_free +msgid "true" +msgstr "tak" + +msgid "false" +msgstr "nie" + +# Category values - speed_limit (km/h) +msgid "10" +msgstr "10 km/h" + +msgid "30" +msgstr "30 km/h" + +msgid "50" +msgstr "50 km/h" + +# Category values - amenities +msgid "lighting" +msgstr "oświetlenie" + +msgid "benches" +msgstr "ławki" + +msgid "toilets" +msgstr "toalety" + # Reported issue types msgid "under construction" msgstr "w budowie" @@ -52,6 +88,12 @@ msgstr "ma dziurę" msgid "categories_help_accessible_by" msgstr "Kto może korzystać z tego mostu" +msgid "categories_help_speed_limit" +msgstr "Wybranie limitu prędkości pokazuje też mosty z niższym limitem" + +msgid "categories_help_amenities" +msgstr "Wybranie kilku pokazuje tylko mosty, które mają je wszystkie" + # Help texts - category options msgid "categories_options_help_small bridge" msgstr "Mniejszy most dla pieszych lub rowerzystów" diff --git a/examples/e2e_test_data.json b/examples/e2e_test_data.json index f435d0ff..60fc6126 100644 --- a/examples/e2e_test_data.json +++ b/examples/e2e_test_data.json @@ -149,7 +149,6 @@ "url": "https://fonts.googleapis.com/css2?family=Poppins" }, "primary_color": "#FFFFFF", - "secondary_color": "#245466", - "left_bar_width": "300px" + "secondary_color": "#245466" } } diff --git a/frontend/src/components/FiltersForm/FiltersForm.jsx b/frontend/src/components/FiltersForm/FiltersForm.jsx index 6255f04a..6667064e 100644 --- a/frontend/src/components/FiltersForm/FiltersForm.jsx +++ b/frontend/src/components/FiltersForm/FiltersForm.jsx @@ -1,6 +1,7 @@ import React, { useState, useEffect } from 'react'; import styled, { keyframes } from 'styled-components'; import { useTranslation } from 'react-i18next'; +import { Tooltip } from '@mui/material'; import { useCategories } from '../Categories/CategoriesContext'; import { httpService } from '../../services/http/httpService'; import FiltersTooltip from './FiltersTooltip'; @@ -68,12 +69,43 @@ const FilterHeader = styled.div` `; const FilterTitle = styled.span` + flex: 1; font-size: 13px; font-weight: 600; letter-spacing: 0.3px; line-height: 16px; `; +// Every filter mode gets a small badge next to its category title, so "or" +// and "and" (both checkboxes, otherwise visually identical at rest) are as +// distinguishable as "exclusive"/"threshold" already are via their radio +// shape. Kept to a single subtle character rather than a word, with the +// full explanation one hover/focus away in the tooltip. +const MODE_BADGES = { + or: { badge: 'filterModeOrBadge', tooltip: 'filterModeOrTooltip' }, + and: { badge: 'filterModeAndBadge', tooltip: 'filterModeAndTooltip' }, + exclusive: { badge: 'filterModeExclusiveBadge', tooltip: 'filterModeExclusiveTooltip' }, + boolean: { badge: 'filterModeBooleanBadge', tooltip: 'filterModeBooleanTooltip' }, + threshold: { badge: 'filterModeThresholdBadge', tooltip: 'filterModeThresholdTooltip' }, +}; + +const ModeBadge = styled.span` + display: inline-flex; + align-items: center; + justify-content: center; + min-width: 15px; + height: 15px; + padding: 0 3px; + border-radius: 50%; + font-size: 10px; + font-weight: 700; + line-height: 1; + background-color: rgba(79, 195, 247, 0.18); + color: #4fc3f7; + border: 1px solid rgba(79, 195, 247, 0.45); + cursor: help; +`; + const FilterOption = styled.label` display: flex; align-items: center; @@ -123,6 +155,21 @@ const StyledCheckbox = styled.input` outline: none; box-shadow: 0 0 0 2px rgba(79, 195, 247, 0.4); } + + &[type='radio'] { + border-radius: 50%; + } + + &[type='radio']:checked::after { + left: 4px; + top: 4px; + width: 6px; + height: 6px; + border: none; + border-radius: 50%; + background-color: white; + transform: none; + } `; const OptionText = styled.span` @@ -175,7 +222,7 @@ const ClearFiltersButton = styled.button` * Fetches category data from the API and renders checkboxes for each filter option. * Manages filter state through the Categories context. * - * @returns {React.ReactElement} Form element containing categorized filter checkboxes with optional tooltips + * @return {React.ReactElement} Form element containing categorized filter checkboxes with optional tooltips */ const LoadingSkeleton = () => ( <> @@ -202,7 +249,7 @@ export const FiltersForm = () => { const handleCheckboxChange = event => { const { value, checked } = event.target; - const category = event.target.dataset.category; + const { category } = event.target.dataset; setCategories(prevSelectedFilters => { const newSelectedFilters = { ...prevSelectedFilters }; @@ -219,6 +266,18 @@ export const FiltersForm = () => { }); }; + // "exclusive" categories are single-select (radio buttons): picking one + // option replaces any previous selection instead of toggling it. + const handleRadioChange = event => { + const { value } = event.target; + const { category } = event.target.dataset; + + setCategories(prevSelectedFilters => ({ + ...prevSelectedFilters, + [category]: [value], + })); + }; + const handleClearFilters = () => { setCategories({}); }; @@ -248,20 +307,58 @@ export const FiltersForm = () => { fetchCategories(); }, []); - const renderFilterOptions = (filters, category) => { - return filters[1].map(([name, translation]) => { + const renderModeBadge = mode => { + const keys = MODE_BADGES[mode] ?? MODE_BADGES.or; + const tooltipText = t(keys.tooltip); + return ( + + + {t(keys.badge)} + + + ); + }; + + const renderFilterOptions = category => { + // "exclusive" (pick one) and "threshold" (pick a cumulative upper bound, + // e.g. speed limit) are both single-select: rendered as radio buttons so + // only one option can be active at a time. + // + // TODO: "threshold" categories would read more naturally as an MUI + // with discrete `marks` at each option value - the filled + // track from min to the thumb is a direct visual match for "everything + // up to here is included," better than a radio group. Needs a design + // decision first: sliders have no natural "nothing selected" state (the + // thumb always sits somewhere), but "no filter" must stay distinct from + // "lowest value selected" - e.g. an explicit "Any" mark left of the + // lowest real value, or relying on the existing "Clear filters" button + // as the only way back to unset. Also needs keyboard/screen-reader + // slider accessibility and updated frontend/e2e tests. + const { categoryKey, options, optionsHelp, filterMode } = category; + const isSingleSelect = filterMode === 'exclusive' || filterMode === 'threshold'; + return options.map(([name, translation]) => { const tooltipData = globalThis.FEATURE_FLAGS?.CATEGORIES_HELP - ? filters[3].find(it => it[name]) + ? optionsHelp.find(it => it[name]) : ''; return ( - + {translation} {tooltipData && ( @@ -274,24 +371,80 @@ export const FiltersForm = () => { }); }; - const sections = categoriesData.map(filtersData => { - const [categoryKey, categoryName] = filtersData[0]; - const sectionKey = `${categoryKey}-${categoryName}`; + // "boolean" categories (e.g. "free only") have exactly one meaningful filter + // state - leaving them unchecked already means "show everything" - so rather + // than giving each its own titled section, they're grouped into one shared + // "Others" section as plain checkboxes labeled with the category's own name. + // This keeps the panel compact as more true/false-style filters are added. + const isBooleanCategory = category => category.filterMode === 'boolean'; + const booleanCategories = categoriesData.filter(isBooleanCategory); + const otherCategories = categoriesData.filter(f => !isBooleanCategory(f)); + + const renderBooleanFilterOption = category => { + const { categoryKey, categoryName, options, optionsHelp } = category; + const trueOption = options.find(([optionValue]) => optionValue === 'true'); + if (!trueOption) { + return null; + } + const [name] = trueOption; + const tooltipData = globalThis.FEATURE_FLAGS?.CATEGORIES_HELP + ? optionsHelp.find(it => it[name]) + : ''; + return ( + + + {categoryName} + {renderModeBadge('boolean')} + {tooltipData && ( + + + + )} + + ); + }; + + const sections = otherCategories.map(category => { + const { categoryKey, categoryName, categoriesHelp, filterMode } = category; + // Built from categoryKey alone (not categoryName): aria-labelledby + // values are parsed as space-separated ID references, so an id built + // from a category's translated name (e.g. "accessible by") would + // silently break the association for any name containing whitespace. + const sectionId = `filter-label-${categoryKey}`; const categoryTooltip = globalThis.FEATURE_FLAGS?.CATEGORIES_HELP - ? filtersData[2].find(it => it[categoryKey]) + ? categoriesHelp.find(it => it[categoryKey]) : null; return ( - + - {categoryName} + {categoryName} + {renderModeBadge(filterMode)} {categoryTooltip && } - {renderFilterOptions(filtersData, categoryKey)} + {renderFilterOptions(category)} ); }); + if (booleanCategories.length > 0) { + sections.push( + + + {t('otherFilters')} + + {booleanCategories.map(renderBooleanFilterOption)} + , + ); + } + if (isLoading) { return (
diff --git a/frontend/src/components/MarkerPopup/MarkerPopup.jsx b/frontend/src/components/MarkerPopup/MarkerPopup.jsx index 96d30ead..e0c5156f 100644 --- a/frontend/src/components/MarkerPopup/MarkerPopup.jsx +++ b/frontend/src/components/MarkerPopup/MarkerPopup.jsx @@ -89,6 +89,14 @@ export const MarkerPopup = ({ place }) => { const setSelectedLocationId = useMapStore(state => state.setSelectedLocationId); const [isClicked, setIsClicked] = useState(false); + // TODO: this only opens the popup if `place`'s Marker is actually attached to + // the map. Leaflet.markercluster detaches individual markers while they sit + // inside an unexpanded cluster bubble, so a shared ?locationId= link to a + // clustered location silently fails to open its popup on desktop (mobile's + // MobilePopup is unaffected - it's a state-driven MUI Dialog, not tied to the + // Leaflet marker). Fix: call the cluster group's zoomToShowLayer(marker, cb) + // before/instead of setIsClicked when the marker is clustered. Covered by the + // xfail'd test_shared_link_opens_popup_with_correct_content in e2e-tests. useEffect(() => { if (selectedLocationId === place.uuid) { setIsClicked(true); diff --git a/frontend/src/locales/en/map.json b/frontend/src/locales/en/map.json index 04752ff4..8679593b 100644 --- a/frontend/src/locales/en/map.json +++ b/frontend/src/locales/en/map.json @@ -38,5 +38,17 @@ "loadFiltersError": "Failed to load filters.", "retry": "Retry", "clearFilters": "Clear filters", - "clearAllFiltersAriaLabel": "Clear all filters" + "clearAllFiltersAriaLabel": "Clear all filters", + "otherFilters": "Others", + "filterModeOrBadge": "+", + "filterModeOrTooltip": "Selecting several options here shows results that match any of them", + "filterModeAndBadge": "&", + "filterModeAndTooltip": "Selecting several options here shows only results that have all of them, not just any of them", + "filterModeExclusiveBadge": "1", + "filterModeExclusiveTooltip": "Only one option can be selected at a time", + "filterModeBooleanBadge": "•", + "filterModeBooleanTooltip": "Unchecked shows everything; checking it narrows the results to just this", + "filterModeThresholdBadge": "≤", + "filterModeThresholdTooltip": "Selecting a value also includes every lower value", + "filterModeHelpAriaLabel": "Filter mode: {{description}}" } diff --git a/frontend/src/locales/pl/map.json b/frontend/src/locales/pl/map.json index 69747f4c..7c33f971 100644 --- a/frontend/src/locales/pl/map.json +++ b/frontend/src/locales/pl/map.json @@ -38,5 +38,17 @@ "loadFiltersError": "Nie udało się załadować filtrów.", "retry": "Spróbuj ponownie", "clearFilters": "Wyczyść filtry", - "clearAllFiltersAriaLabel": "Wyczyść wszystkie filtry" + "clearAllFiltersAriaLabel": "Wyczyść wszystkie filtry", + "otherFilters": "Inne", + "filterModeOrBadge": "+", + "filterModeOrTooltip": "Wybranie kilku opcji tutaj pokazuje wyniki pasujące do dowolnej z nich", + "filterModeAndBadge": "&", + "filterModeAndTooltip": "Wybranie kilku opcji tutaj pokazuje tylko wyniki, które mają je wszystkie, a nie dowolne", + "filterModeExclusiveBadge": "1", + "filterModeExclusiveTooltip": "Można wybrać tylko jedną opcję naraz", + "filterModeBooleanBadge": "•", + "filterModeBooleanTooltip": "Odznaczone pokazuje wszystko; zaznaczenie zawęża wyniki tylko do tego", + "filterModeThresholdBadge": "≤", + "filterModeThresholdTooltip": "Wybranie wartości obejmuje też każdą niższą wartość", + "filterModeHelpAriaLabel": "Tryb filtra: {{description}}" } diff --git a/frontend/src/locales/ua/map.json b/frontend/src/locales/ua/map.json index bcadc554..dc890d1a 100644 --- a/frontend/src/locales/ua/map.json +++ b/frontend/src/locales/ua/map.json @@ -37,5 +37,17 @@ "loadFiltersError": "Не вдалося завантажити фільтри.", "retry": "Спробувати ще раз", "clearFilters": "Очистити фільтри", - "clearAllFiltersAriaLabel": "Очистити всі фільтри" + "clearAllFiltersAriaLabel": "Очистити всі фільтри", + "otherFilters": "Інше", + "filterModeOrBadge": "+", + "filterModeOrTooltip": "Вибір кількох варіантів тут показує результати, що відповідають будь-якому з них", + "filterModeAndBadge": "&", + "filterModeAndTooltip": "Вибір кількох варіантів тут показує лише результати, що мають їх усі, а не будь-який", + "filterModeExclusiveBadge": "1", + "filterModeExclusiveTooltip": "Можна вибрати лише один варіант одночасно", + "filterModeBooleanBadge": "•", + "filterModeBooleanTooltip": "Не позначено - показує все; позначення звужує результати лише до цього", + "filterModeThresholdBadge": "≤", + "filterModeThresholdTooltip": "Вибір значення також включає кожне нижче значення", + "filterModeHelpAriaLabel": "Режим фільтра: {{description}}" } diff --git a/frontend/src/services/http/httpService.js b/frontend/src/services/http/httpService.js index 974f928c..e734563d 100644 --- a/frontend/src/services/http/httpService.js +++ b/frontend/src/services/http/httpService.js @@ -47,28 +47,24 @@ export const httpService = { * Fetches complete categories data including subcategories in a single request. * Uses the /api/categories-full endpoint to avoid waterfall requests. * - * @returns {Promise<{categories: Array, defaultChecked: Object}>} Promise resolving to - * the array of category data tuples plus a map of category key to the option values - * that should be pre-checked by default. + * @returns {Promise<{categories: Array<{categoryKey: string, categoryName: string, + * options: Array<[string, string]>, categoriesHelp: Array, optionsHelp: Array, + * filterMode: string}>, defaultChecked: Object}>} Promise resolving to the array of + * category data plus a map of category key to the option values that should be + * pre-checked by default. */ getCategoriesData: async () => { const response = await fetch(CATEGORIES_FULL).then(res => res.json()); - - // Transform to expected format: [[key, name], options, help?, optionsHelp?] - const categories = response.categories.map(category => { - const categoryTuple = [category.key, category.name]; - const options = category.options; - - if (globalThis.FEATURE_FLAGS?.CATEGORIES_HELP) { - return [ - categoryTuple, - category.options_with_help ?? options, - response.categories_help ?? [], - category.options_help ?? [], - ]; - } - return [categoryTuple, options]; - }); + const useCategoriesHelp = Boolean(globalThis.FEATURE_FLAGS?.CATEGORIES_HELP); + + const categories = response.categories.map(category => ({ + categoryKey: category.key, + categoryName: category.name, + options: (useCategoriesHelp ? category.options_with_help : null) ?? category.options, + categoriesHelp: useCategoriesHelp ? response.categories_help ?? [] : [], + optionsHelp: useCategoriesHelp ? category.options_help ?? [] : [], + filterMode: category.filter_mode ?? 'or', + })); const defaultChecked = Object.fromEntries( response.categories diff --git a/frontend/tests/FiltersForm.test.jsx b/frontend/tests/FiltersForm.test.jsx index d983365e..6eae3bbf 100644 --- a/frontend/tests/FiltersForm.test.jsx +++ b/frontend/tests/FiltersForm.test.jsx @@ -1,6 +1,6 @@ import React from 'react'; import '@testing-library/jest-dom'; -import { render, waitFor, within } from '@testing-library/react'; +import { fireEvent, render, waitFor, within } from '@testing-library/react'; import { FiltersForm } from '../src/components/FiltersForm/FiltersForm'; import { CategoriesProvider } from '../src/components/Categories/CategoriesContext'; import { httpService } from '../src/services/http/httpService'; @@ -8,15 +8,17 @@ import { httpService } from '../src/services/http/httpService'; jest.mock('../src/services/http/httpService'); const categories = [ - [ - ['types', 'typy'], - [ + { + categoryKey: 'types', + categoryName: 'typy', + options: [ ['clothes', 'ciuchy'], ['shoes', 'buty'], ], - [{ types: 'Inaczej rodzaje' }], - [{ shoes: 'Kozaki też' }], - ], + categoriesHelp: [{ types: 'Inaczej rodzaje' }], + optionsHelp: [{ shoes: 'Kozaki też' }], + filterMode: 'or', + }, ]; httpService.getCategoriesData.mockResolvedValue({ categories, defaultChecked: {} }); @@ -34,9 +36,7 @@ describe('Creates good filter_form box', () => { , ); - await waitFor(() => - expect(document.querySelector('#filter-label-types-typy')).not.toBeNull(), - ); + await waitFor(() => expect(document.querySelector('#filter-label-types')).not.toBeNull()); }); afterEach(() => { @@ -47,7 +47,7 @@ describe('Creates good filter_form box', () => { const form = document.querySelector('form'); expect(form).not.toBeNull(); - const filterLabel = form.querySelector('#filter-label-types-typy'); + const filterLabel = form.querySelector('#filter-label-types'); expect(filterLabel).not.toBeNull(); expect(filterLabel.textContent).toBe('typy'); @@ -86,7 +86,7 @@ describe('Creates good filter_form box', () => { // Category help tooltip is now in FilterHeader, not FilterTitle // Look for it in the parent FilterHeader element - const filterHeader = form.querySelector('#filter-label-types-typy').parentElement; + const filterHeader = form.querySelector('#filter-label-types').parentElement; const { queryByLabelText } = within(filterHeader); expect(queryByLabelText(/Help: Inaczej rodzaje/i)).toBeInTheDocument(); }); @@ -116,3 +116,226 @@ describe('Pre-checks options configured as default-checked', () => { expect(clothesCheckbox.checked).toBe(false); }); }); + +describe('Renders exclusive (single-select) categories as radio buttons', () => { + // "exclusive" is for categories with 3+ mutually-exclusive options, e.g. a + // hypothetical toll tier. Boolean yes/no categories use "boolean" mode + // instead (see below), which offers a single checkbox and no separate + // radio-deselection problem. + const exclusiveCategories = [ + { + categoryKey: 'payment_status', + categoryName: 'payment status', + options: [ + ['free', 'free'], + ['discounted', 'discounted'], + ['full_price', 'full price'], + ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'exclusive', + }, + ]; + + beforeEach(async () => { + httpService.getCategoriesData.mockResolvedValueOnce({ + categories: exclusiveCategories, + defaultChecked: {}, + }); + render( + + + , + ); + await waitFor(() => expect(document.querySelector('#free')).not.toBeNull()); + }); + + it('renders options as radio inputs sharing the category name', () => { + const freeInput = document.querySelector('#free'); + const discountedInput = document.querySelector('#discounted'); + expect(freeInput.type).toBe('radio'); + expect(discountedInput.type).toBe('radio'); + expect(freeInput.name).toBe('payment_status'); + expect(discountedInput.name).toBe('payment_status'); + }); + + it('selecting one option replaces rather than adds to the selection', () => { + const freeInput = document.querySelector('#free'); + const discountedInput = document.querySelector('#discounted'); + + fireEvent.click(freeInput); + expect(freeInput.checked).toBe(true); + + fireEvent.click(discountedInput); + expect(discountedInput.checked).toBe(true); + expect(freeInput.checked).toBe(false); + }); +}); + +describe('Groups boolean categories into a shared "Others" section', () => { + const mixedCategories = [ + { + categoryKey: 'types', + categoryName: 'typy', + options: [ + ['clothes', 'ciuchy'], + ['shoes', 'buty'], + ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'or', + }, + { + categoryKey: 'is_free', + categoryName: 'Free only', + options: [ + ['true', 'yes'], + ['false', 'no'], + ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'boolean', + }, + ]; + + beforeEach(async () => { + httpService.getCategoriesData.mockResolvedValueOnce({ + categories: mixedCategories, + defaultChecked: {}, + }); + render( + + + , + ); + await waitFor(() => expect(document.querySelector('#is_free')).not.toBeNull()); + }); + + it('keeps non-boolean categories in their own titled section', () => { + expect(document.querySelector('#filter-label-types')).not.toBeNull(); + expect(document.querySelector('#clothes')).not.toBeNull(); + }); + + it('renders the boolean category as a single checkbox under "Others", labeled with the category name', () => { + expect(document.getElementById('filter-label-others').textContent).toBe('Others'); + + const freeCheckbox = document.querySelector('#is_free'); + expect(freeCheckbox.type).toBe('checkbox'); + expect(freeCheckbox.value).toBe('true'); + + const label = document.querySelector('label[for="is_free"]'); + expect(label.textContent).toContain('Free only'); + + // "false" is never rendered - unchecked already means "show everything". + expect(document.querySelector('#false')).toBeNull(); + expect(document.querySelector('#true')).toBeNull(); + }); + + it('unchecked by default (shows everything); checking narrows the results', () => { + const freeCheckbox = document.querySelector('#is_free'); + expect(freeCheckbox.checked).toBe(false); + + fireEvent.click(freeCheckbox); + expect(freeCheckbox.checked).toBe(true); + + fireEvent.click(freeCheckbox); + expect(freeCheckbox.checked).toBe(false); + }); +}); + +describe('Renders threshold categories as radio buttons too', () => { + const thresholdCategories = [ + { + categoryKey: 'speed_limit', + categoryName: 'speed limit', + options: [ + ['10', '10 km/h'], + ['30', '30 km/h'], + ['50', '50 km/h'], + ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'threshold', + }, + ]; + + beforeEach(async () => { + httpService.getCategoriesData.mockResolvedValueOnce({ + categories: thresholdCategories, + defaultChecked: {}, + }); + render( + + + , + ); + await waitFor(() => expect(document.getElementById('10')).not.toBeNull()); + }); + + it('renders a single-select radio group rather than independent checkboxes', () => { + const low = document.getElementById('10'); + const mid = document.getElementById('30'); + const high = document.getElementById('50'); + expect(low.type).toBe('radio'); + expect(mid.type).toBe('radio'); + expect(high.type).toBe('radio'); + expect(low.name).toBe('speed_limit'); + + fireEvent.click(mid); + expect(mid.checked).toBe(true); + + fireEvent.click(high); + expect(high.checked).toBe(true); + expect(mid.checked).toBe(false); + }); +}); + +describe('Distinguishes "and" categories with a visible hint, but keeps checkboxes', () => { + const andCategories = [ + { + categoryKey: 'amenities', + categoryName: 'amenities', + options: [ + ['lighting', 'lighting'], + ['benches', 'benches'], + ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'and', + }, + ]; + + beforeEach(async () => { + httpService.getCategoriesData.mockResolvedValueOnce({ + categories: andCategories, + defaultChecked: {}, + }); + render( + + + , + ); + await waitFor(() => expect(document.querySelector('#lighting')).not.toBeNull()); + }); + + it('still renders checkboxes (multi-select), unlike exclusive/threshold', () => { + const lighting = document.querySelector('#lighting'); + const benches = document.querySelector('#benches'); + expect(lighting.type).toBe('checkbox'); + expect(benches.type).toBe('checkbox'); + + fireEvent.click(lighting); + fireEvent.click(benches); + expect(lighting.checked).toBe(true); + expect(benches.checked).toBe(true); + }); + + it('shows an "&" badge with a keyboard-focusable, localized tooltip', () => { + const header = document.querySelector('#filter-label-amenities').parentElement; + expect(header.textContent).toContain('&'); + + const badge = within(header).getByLabelText(/Filter mode:/i); + expect(badge).toBeInTheDocument(); + expect(badge).toHaveAttribute('tabIndex', '0'); + }); +}); diff --git a/frontend/tests/Map/MapComponent.test.jsx b/frontend/tests/Map/MapComponent.test.jsx index c8bb0320..7886aafd 100644 --- a/frontend/tests/Map/MapComponent.test.jsx +++ b/frontend/tests/Map/MapComponent.test.jsx @@ -9,13 +9,17 @@ import { httpService } from '../../src/services/http/httpService'; jest.mock('../../src/services/http/httpService'); const categories = [ - [ - ['types', 'typy'], - [ + { + categoryKey: 'types', + categoryName: 'typy', + options: [ ['clothes', 'ciuchy'], ['shoes', 'buty'], ], - ], + categoriesHelp: [], + optionsHelp: [], + filterMode: 'or', + }, ]; const locations = [ @@ -27,7 +31,7 @@ const locations = [ ]; httpService.getLocations.mockResolvedValue(locations); -httpService.getCategoriesData.mockResolvedValue(categories); +httpService.getCategoriesData.mockResolvedValue({ categories, defaultChecked: {} }); describe('MapComponent', () => { beforeAll(() => { diff --git a/goodmap/core.py b/goodmap/core.py index c3ef7baa..c323560c 100644 --- a/goodmap/core.py +++ b/goodmap/core.py @@ -1,26 +1,10 @@ -"""Core data filtering and sorting utilities for location queries.""" +"""Core data sorting, limiting, and query orchestration for location queries.""" -from typing import Any, Dict, List +from typing import Any, Dict, List, Mapping -# TODO move filtering to db site - - -def does_fulfill_requirement(entry, requirements): - """Check if an entry fulfills all category requirements. - - Args: - entry: Location data entry to check - requirements: List of (category, values) tuples to match +from goodmap.filtering import NO_FILTER_MODES, does_fulfill_requirement - Returns: - bool: True if entry matches all non-empty requirements - """ - matches = [] - for category, values in requirements: - if not values: - continue - matches.append(all(entry_value in entry[category] for entry_value in values)) - return all(matches) +# TODO move filtering to db site def sort_by_distance(data: List[Dict[str, Any]], query_params: Dict[str, List[str]]): @@ -64,13 +48,18 @@ def limit(data, query_params): return data -def get_queried_data(all_data, categories, query_params): +def get_queried_data( + all_data, categories, query_params, filter_modes: Mapping[str, str] = NO_FILTER_MODES +): """Filter, sort, and limit location data based on query parameters. Args: all_data: Complete list of location data categories: Available categories for filtering query_params: Query parameters for filtering, sorting, and limiting + filter_modes: Dict mapping category name to combination mode ("or", + "and", "exclusive", "boolean", or "threshold"), see + goodmap.filtering.does_fulfill_requirement. Returns: Filtered, sorted, and limited location data @@ -79,7 +68,7 @@ def get_queried_data(all_data, categories, query_params): for key in categories.keys(): requirements.append((key, query_params.get(key))) - filtered_data = [x for x in all_data if does_fulfill_requirement(x, requirements)] + filtered_data = [x for x in all_data if does_fulfill_requirement(x, requirements, filter_modes)] final_data = sort_by_distance(filtered_data, query_params) final_data = limit(final_data, query_params) return final_data diff --git a/goodmap/core_api.py b/goodmap/core_api.py index 13ddb360..c21fcfcf 100644 --- a/goodmap/core_api.py +++ b/goodmap/core_api.py @@ -441,6 +441,7 @@ def get_categories_full(): categories_options_help = categories_data.get("categories_options_help", {}) categories_default_checked = categories_data.get("categories_default_checked", {}) + categories_filter_mode = categories_data.get("categories_filter_mode", {}) for key, options in categories_data["categories"].items(): category_entry = { @@ -452,6 +453,7 @@ def get_categories_full(): for option in categories_default_checked.get(key, []) if option in options ], + "filter_mode": categories_filter_mode.get(key, "or"), } if CategoriesHelp in feature_flags: diff --git a/goodmap/db.py b/goodmap/db.py index 3b745381..a059f831 100644 --- a/goodmap/db.py +++ b/goodmap/db.py @@ -613,12 +613,16 @@ def json_db_get_category_data(self, category_type=None): category_type, [] ) }, + "categories_filter_mode": { + category_type: self.data.get("categories_filter_mode", {}).get(category_type, "or") + }, } return { "categories": self.data["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", {}), + "categories_filter_mode": self.data.get("categories_filter_mode", {}), } @@ -636,12 +640,16 @@ def json_file_db_get_category_data(self, category_type=None): "categories_default_checked": { category_type: data.get("categories_default_checked", {}).get(category_type, []) }, + "categories_filter_mode": { + category_type: data.get("categories_filter_mode", {}).get(category_type, "or") + }, } return { "categories": data["categories"], "categories_help": data.get("categories_help", []), "categories_options_help": data.get("categories_options_help", {}), "categories_default_checked": data.get("categories_default_checked", {}), + "categories_filter_mode": data.get("categories_filter_mode", {}), } @@ -658,12 +666,16 @@ def google_json_db_get_category_data(self, category_type=None): "categories_default_checked": { category_type: data.get("categories_default_checked", {}).get(category_type, []) }, + "categories_filter_mode": { + category_type: data.get("categories_filter_mode", {}).get(category_type, "or") + }, } return { "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", {}), + "categories_filter_mode": data.get("categories_filter_mode", {}), } @@ -687,18 +699,25 @@ def mongodb_db_get_category_data(self, category_type=None): category_type, [] ) }, + "categories_filter_mode": { + category_type: config_doc.get("categories_filter_mode", {}).get( + category_type, "or" + ) + }, } return { "categories": config_doc.get("categories", {}), "categories_help": config_doc.get("categories_help", []), "categories_options_help": config_doc.get("categories_options_help", {}), "categories_default_checked": config_doc.get("categories_default_checked", {}), + "categories_filter_mode": config_doc.get("categories_filter_mode", {}), } return { "categories": {}, "categories_help": [], "categories_options_help": {}, "categories_default_checked": {}, + "categories_filter_mode": {}, } @@ -769,7 +788,12 @@ def get_locations_list_from_raw_data(map_data, query, location_model): Returns: List of validated location model instances. """ - filtered_locations = get_queried_data(map_data["data"], map_data["categories"], query) + filtered_locations = get_queried_data( + map_data["data"], + map_data["categories"], + query, + map_data.get("categories_filter_mode", {}), + ) return [location_model.model_validate(point) for point in filtered_locations] @@ -791,9 +815,30 @@ def json_db_get_locations(self, query, location_model): def mongodb_db_get_locations(self, query, location_model): """Retrieve filtered locations from MongoDB.""" + config_doc = self.db.config.find_one({"_id": "map_config"}) or {} + filter_modes = config_doc.get("categories_filter_mode", {}) + mongo_query = {} for key, values in query.items(): - if values: + if not values: + continue + mode = filter_modes.get(key, "or") + if mode == "threshold": + # Threshold categories (e.g. speed limits) are numeric and ordered: + # selecting a value also matches any stored value at or below it. + # Assumes the field is stored numerically in MongoDB. + try: + mongo_query[key] = {"$lte": max(float(value) for value in values)} + except (TypeError, ValueError): + # Match nothing for this category, same as goodmap.filtering's + # _matches_threshold (which returns False on the same error), + # rather than silently dropping the filter and matching everything. + mongo_query[key] = {"$in": []} + elif mode == "and": + # Entry must have every selected value, not just any of them. + mongo_query[key] = {"$all": values} + else: + # "or", "exclusive", and "boolean" all match any of the selected values. mongo_query[key] = {"$in": values} projection = {"_id": 0, "uuid": 1, "position": 1, "remark": 1} diff --git a/goodmap/filtering.py b/goodmap/filtering.py new file mode 100644 index 00000000..351d39e1 --- /dev/null +++ b/goodmap/filtering.py @@ -0,0 +1,93 @@ +"""Category filter combination logic for location queries. + +See the "Categories and Filtering" section of the docs (categories_filter_mode) +for the config-level guide to the modes handled here: "or", "and", "exclusive", +"boolean", and "threshold". +""" + +from types import MappingProxyType +from typing import Any, Mapping + +# Python has no builtin "frozendict" (the way frozenset mirrors set); +# MappingProxyType is the standard-library equivalent - a read-only view that +# raises TypeError on mutation. Used as the filter_modes default below (and in +# goodmap.core.get_queried_data) so a single shared instance is safe to reuse +# across calls instead of a plain {}. +NO_FILTER_MODES: Mapping[str, str] = MappingProxyType({}) + + +def _as_list(value: Any) -> list[str]: + """Wrap a scalar field value in a list, leaving list values untouched.""" + if value is None: + return [] + if isinstance(value, list): + return value + return [value] + + +def _matches_or(entry_values: list[str], selected_values: list[str]) -> bool: + """Match if the entry has at least one of the selected values (any-of).""" + return any(value in entry_values for value in selected_values) + + +def _matches_and(entry_values: list[str], selected_values: list[str]) -> bool: + """Match only if the entry has every selected value (all-of). + + Only meaningful for list-valued categories (an entry can have multiple + simultaneous values), e.g. narrowing down to locations that have both + "lighting" and "benches" among their amenities. For a single-valued + category this can only match when a single value is selected. + """ + return all(value in entry_values for value in selected_values) + + +def _matches_threshold(entry_values: list[str], selected_values: list[str]) -> bool: + """Match if any entry value is numerically <= the highest selected value. + + Used for ordered numeric categories (e.g. speed limits) where selecting a + value implies "this value or lower" (e.g. selecting 50 also matches 10 and 30). + """ + try: + entry_numbers = [float(value) for value in entry_values] + max_selected = max(float(value) for value in selected_values) + except (TypeError, ValueError): + return False + return any(number <= max_selected for number in entry_numbers) + + +# "exclusive" (radio group) and "boolean" (single checkbox, e.g. "free only") +# categories only ever send a single selected value, so matching is the same +# as "or". +_FILTER_MATCHERS = { + "or": _matches_or, + "and": _matches_and, + "exclusive": _matches_or, + "boolean": _matches_or, + "threshold": _matches_threshold, +} + + +def does_fulfill_requirement( + entry, requirements, filter_modes: Mapping[str, str] = NO_FILTER_MODES +): + """Check if an entry fulfills all category requirements. + + Args: + entry: Location data entry to check + requirements: List of (category, values) tuples to match + filter_modes: Dict mapping category name to combination mode ("or", + "and", "exclusive", "boolean", or "threshold"). Categories not + present default to "or" (entry matches if it has any of the + selected values). + + Returns: + bool: True if entry matches all non-empty requirements + """ + matches = [] + for category, values in requirements: + if not values: + continue + entry_values = _as_list(entry.get(category)) + matcher = _FILTER_MATCHERS.get(filter_modes.get(category, "or"), _matches_or) + matches.append(matcher(entry_values, values)) + return all(matches) diff --git a/tests/unit_tests/test_core.py b/tests/unit_tests/test_core.py index e5631470..bcc429be 100644 --- a/tests/unit_tests/test_core.py +++ b/tests/unit_tests/test_core.py @@ -1,4 +1,4 @@ -from goodmap.core import does_fulfill_requirement, get_queried_data, limit, sort_by_distance +from goodmap.core import get_queried_data, limit, sort_by_distance test_data = [ { @@ -31,38 +31,14 @@ def test_query(): assert get_queried_data(test_data, categories, query) == expected_data -def test_filtering(): - requirements = [("types", ["clothes"]), ("gender", ["male"])] - expected_data = [ - { - "name": "PCK", - "position": [51.1, 17.05], - "types": ["clothes"], - "gender": ["male"], - } - ] - filtered_data = list(filter(lambda x: does_fulfill_requirement(x, requirements), test_data)) - assert filtered_data == expected_data +def test_get_queried_data_applies_filter_modes(): + categories = {"gender": ["male", "female"]} + query = {"gender": ["female"]} + filter_modes = {"gender": "exclusive"} + result = get_queried_data(test_data, categories, query, filter_modes) -def test_category_match_if_not_specified(): - requirements = [("types", []), ("gender", ["male"])] - expected_data = [ - { - "name": "LASSO", - "position": [51.113, 17.06], - "types": ["shoes"], - "gender": ["male", "female"], - }, - { - "name": "PCK", - "position": [51.1, 17.05], - "types": ["clothes"], - "gender": ["male"], - }, - ] - filtered_data = list(filter(lambda x: does_fulfill_requirement(x, requirements), test_data)) - assert filtered_data == expected_data + assert result == [test_data[0]] def test_that_limit_works_properly(): diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index cd090fa4..76ad9cd0 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -173,6 +173,24 @@ def test_categories_full_endpoint(test_app): # No default-checked options configured for this category assert category["default_checked"] == [] + # Categories without an explicit filter mode default to "or" + assert category["filter_mode"] == "or" + + +@mock.patch("goodmap.core_api.gettext", fake_translation) +def test_categories_full_endpoint_reports_configured_filter_mode(): + test_app = create_test_app( + db_overrides={ + "categories": {"test-category": ["opt1", "opt2"]}, + "categories_filter_mode": {"test-category": "exclusive"}, + } + ) + response = test_app.get("/api/categories-full") + assert response.status_code == 200 + assert response.json is not None + category = response.json["categories"][0] + assert category["filter_mode"] == "exclusive" + @mock.patch("goodmap.core_api.gettext", fake_translation) def test_categories_full_endpoint_with_default_checked(): @@ -304,6 +322,131 @@ def test_get_locations(test_app): ] +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.""" + client = create_test_app( + db_overrides={ + "categories": {"tags": ["red", "blue", "green"]}, + "location_obligatory_fields": [("tags", "list"), ("name", "str")], + "data": [ + { + "name": "red-only", + "position": [50, 50], + "tags": ["red"], + "uuid": "11111111-1111-1111-1111-111111111111", + }, + { + "name": "blue-only", + "position": [60, 60], + "tags": ["blue"], + "uuid": "22222222-2222-2222-2222-222222222222", + }, + { + "name": "green-only", + "position": [70, 70], + "tags": ["green"], + "uuid": "33333333-3333-3333-3333-333333333333", + }, + ], + "visible_data": ["name", "tags"], + } + ) + + response = client.get("/api/locations?tags=red&tags=blue") + + assert response.status_code == 200 + assert response.json is not None + uuids = {loc["uuid"] for loc in response.json} + assert uuids == { + "11111111-1111-1111-1111-111111111111", + "22222222-2222-2222-2222-222222222222", + } + + +def test_get_locations_and_filter_mode_requires_every_selected_value(): + """An "and" category (e.g. amenities) narrows to entries that have every + selected value, not just any of them - the opposite of "or".""" + client = create_test_app( + db_overrides={ + "categories": {"amenities": ["lighting", "benches", "toilets"]}, + "categories_filter_mode": {"amenities": "and"}, + "location_obligatory_fields": [("amenities", "list"), ("name", "str")], + "data": [ + { + "name": "lighting-and-benches", + "position": [50, 50], + "amenities": ["lighting", "benches"], + "uuid": "11111111-1111-1111-1111-111111111111", + }, + { + "name": "lighting-only", + "position": [60, 60], + "amenities": ["lighting"], + "uuid": "22222222-2222-2222-2222-222222222222", + }, + { + "name": "benches-only", + "position": [70, 70], + "amenities": ["benches"], + "uuid": "33333333-3333-3333-3333-333333333333", + }, + ], + "visible_data": ["name", "amenities"], + } + ) + + response = client.get("/api/locations?amenities=lighting&amenities=benches") + + assert response.status_code == 200 + assert response.json is not None + uuids = {loc["uuid"] for loc in response.json} + assert uuids == {"11111111-1111-1111-1111-111111111111"} + + +def test_get_locations_threshold_filter_mode(): + """A "threshold" category (e.g. speed limit) matches any stored value at or + below the highest selected value.""" + client = create_test_app( + db_overrides={ + "categories": {"speed_limit": ["10", "30", "50"]}, + "categories_filter_mode": {"speed_limit": "threshold"}, + "location_obligatory_fields": [("speed_limit", "str"), ("name", "str")], + "data": [ + { + "name": "slow", + "position": [50, 50], + "speed_limit": "10", + "uuid": "11111111-1111-1111-1111-111111111111", + }, + { + "name": "medium", + "position": [60, 60], + "speed_limit": "30", + "uuid": "22222222-2222-2222-2222-222222222222", + }, + { + "name": "fast", + "position": [70, 70], + "speed_limit": "50", + "uuid": "33333333-3333-3333-3333-333333333333", + }, + ], + "visible_data": ["name", "speed_limit"], + } + ) + + response = client.get("/api/locations?speed_limit=30") + + assert response.status_code == 200 + assert response.json is not None + uuids = {loc["uuid"] for loc in response.json} + assert uuids == { + "11111111-1111-1111-1111-111111111111", + "22222222-2222-2222-2222-222222222222", + } + + @mock.patch("goodmap.core_api.gettext", fake_translation) @mock.patch("goodmap.formatter.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) diff --git a/tests/unit_tests/test_db.py b/tests/unit_tests/test_db.py index 6243166d..67e66fec 100644 --- a/tests/unit_tests/test_db.py +++ b/tests/unit_tests/test_db.py @@ -122,13 +122,15 @@ def initialize_and_assert_db(db, data): location_model = create_location_model(location_obligatory_fields, {}) extend_db_with_goodmap_queries(db, location_model) - query = {"test-category": "searchable"} + # Query values arrive as lists in production (request.args.to_dict(flat=False)); + # a single selected value exactly matches one entry's scalar category field. + query = {"test-category": ["searchable"]} location = db.get_location("1") assert location.position == (50, 50) assert location.uuid == "1" - assert len(db.get_locations(query)) == 2 + assert len(db.get_locations(query)) == 1 assert db.get_data() == data @@ -1424,6 +1426,7 @@ def test_json_db_get_category_data(): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1437,6 +1440,7 @@ def test_json_db_get_category_data_specific_category(): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {"test-category": "or"}, } assert category_data == expected @@ -1453,6 +1457,7 @@ def test_json_file_db_get_category_data(tmp_path): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1469,6 +1474,7 @@ def test_json_file_db_get_category_data_specific_category(tmp_path): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {"test-category": "or"}, } assert category_data == expected @@ -1486,6 +1492,7 @@ def test_google_json_db_get_category_data(mock_cli): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1503,6 +1510,7 @@ def test_google_json_db_get_category_data_specific_category(mock_cli): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {"test-category": "or"}, } assert category_data == expected @@ -1527,6 +1535,7 @@ def test_mongodb_db_get_category_data(mock_client): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1551,6 +1560,7 @@ def test_mongodb_db_get_category_data_specific_category(mock_client): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {"test-category": "or"}, } assert category_data == expected @@ -1569,6 +1579,7 @@ def test_mongodb_db_get_category_data_no_config(mock_client): "categories_help": [], "categories_options_help": {}, "categories_default_checked": {}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1582,6 +1593,7 @@ def test_get_category_data(): "categories_help": ["Help text for categories"], "categories_options_help": {"test-category": ["Help for test category"]}, "categories_default_checked": {"test-category": ["searchable"]}, + "categories_filter_mode": {}, } assert category_data == expected @@ -1613,6 +1625,56 @@ def test_mongodb_db_get_locations(mock_client): ) +@mock.patch("platzky.db.mongodb_db.MongoClient") +def test_mongodb_db_get_locations_and_filter_mode(mock_client): + """An "and" category must use $all (every selected value), not $in (any).""" + mock_db = mock.Mock() + mock_client.return_value.__getitem__.return_value = mock_db + mock_db.config.find_one.return_value = { + "_id": "map_config", + "categories_filter_mode": {"amenities": "and"}, + } + mock_db.locations.find.return_value = [{"uuid": "1", "position": [50, 50]}] + + db = MongoDB("mongodb://localhost:27017", "test_db") + extend_db_with_goodmap_queries(db, LocationBase) + + query = {"amenities": ["lighting", "benches"]} + locations = list(mongodb_db_get_locations(db, query, LocationBase)) + + assert len(locations) == 1 + mock_db.locations.find.assert_called_once_with( + {"amenities": {"$all": ["lighting", "benches"]}}, + {"_id": 0, "uuid": 1, "position": 1, "remark": 1}, + ) + + +@mock.patch("platzky.db.mongodb_db.MongoClient") +def test_mongodb_db_get_locations_threshold_parse_failure_matches_nothing(mock_client): + """A non-numeric threshold value should match nothing for that category, + the same as goodmap.filtering's _matches_threshold, rather than silently + dropping the filter (which would match everything).""" + mock_db = mock.Mock() + mock_client.return_value.__getitem__.return_value = mock_db + mock_db.config.find_one.return_value = { + "_id": "map_config", + "categories_filter_mode": {"speed_limit": "threshold"}, + } + mock_db.locations.find.return_value = [] + + db = MongoDB("mongodb://localhost:27017", "test_db") + extend_db_with_goodmap_queries(db, LocationBase) + + query = {"speed_limit": ["not-a-number"]} + locations = list(mongodb_db_get_locations(db, query, LocationBase)) + + assert locations == [] + mock_db.locations.find.assert_called_once_with( + {"speed_limit": {"$in": []}}, + {"_id": 0, "uuid": 1, "position": 1, "remark": 1}, + ) + + @mock.patch("platzky.db.mongodb_db.MongoClient") def test_mongodb_db_get_locations_empty_query(mock_client): mock_db = mock.Mock() diff --git a/tests/unit_tests/test_filtering.py b/tests/unit_tests/test_filtering.py new file mode 100644 index 00000000..92fd7090 --- /dev/null +++ b/tests/unit_tests/test_filtering.py @@ -0,0 +1,133 @@ +from goodmap.filtering import does_fulfill_requirement + +test_data = [ + { + "name": "LASSO", + "position": [51.113, 17.06], + "types": ["shoes"], + "gender": ["male", "female"], + }, + { + "name": "PCK", + "position": [51.1, 17.05], + "types": ["clothes"], + "gender": ["male"], + }, +] + + +def test_filtering(): + requirements = [("types", ["clothes"]), ("gender", ["male"])] + expected_data = [ + { + "name": "PCK", + "position": [51.1, 17.05], + "types": ["clothes"], + "gender": ["male"], + } + ] + filtered_data = list(filter(lambda x: does_fulfill_requirement(x, requirements), test_data)) + assert filtered_data == expected_data + + +def test_category_match_if_not_specified(): + requirements = [("types", []), ("gender", ["male"])] + expected_data = [ + { + "name": "LASSO", + "position": [51.113, 17.06], + "types": ["shoes"], + "gender": ["male", "female"], + }, + { + "name": "PCK", + "position": [51.1, 17.05], + "types": ["clothes"], + "gender": ["male"], + }, + ] + filtered_data = list(filter(lambda x: does_fulfill_requirement(x, requirements), test_data)) + assert filtered_data == expected_data + + +def test_multiple_selected_values_in_same_category_are_or_by_default(): + """Selecting several checkboxes in one category should broaden results (any-of), + not narrow them to entries containing every selected value.""" + requirements = [("gender", ["male", "female"])] + filtered_data = list(filter(lambda x: does_fulfill_requirement(x, requirements), test_data)) + assert filtered_data == test_data + + +def test_or_mode_is_explicit_default(): + requirements = [("gender", ["female"])] + filter_modes = {"gender": "or"} + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), test_data) + ) + assert filtered_data == [test_data[0]] + + +def test_and_mode_requires_every_selected_value(): + """ "and" narrows results: an entry must have ALL selected values, useful + for list-valued categories like amenities ("lighting" AND "benches").""" + requirements = [("gender", ["male", "female"])] + filter_modes = {"gender": "and"} + + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), test_data) + ) + + # Only LASSO has both "male" and "female"; PCK (["male"]) doesn't match. + assert filtered_data == [test_data[0]] + + +def test_and_mode_matches_a_single_selected_value_like_or(): + requirements = [("gender", ["male"])] + filter_modes = {"gender": "and"} + + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), test_data) + ) + + assert filtered_data == test_data + + +def test_exclusive_mode_matches_the_single_selected_value(): + requirements = [("gender", ["female"])] + filter_modes = {"gender": "exclusive"} + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), test_data) + ) + assert filtered_data == [test_data[0]] + + +def test_threshold_mode_matches_values_at_or_below_the_highest_selected(): + speed_data = [ + {"name": "Zwierzyniecka", "speed_limit": "10"}, + {"name": "Milenijny", "speed_limit": "30"}, + {"name": "Grunwaldzki", "speed_limit": "50"}, + ] + requirements = [("speed_limit", ["30"])] + filter_modes = {"speed_limit": "threshold"} + + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), speed_data) + ) + + assert filtered_data == [speed_data[0], speed_data[1]] + + +def test_threshold_mode_uses_the_max_of_multiple_selected_values(): + speed_data = [ + {"name": "Zwierzyniecka", "speed_limit": "10"}, + {"name": "Milenijny", "speed_limit": "30"}, + {"name": "Grunwaldzki", "speed_limit": "50"}, + ] + requirements = [("speed_limit", ["10", "50"])] + filter_modes = {"speed_limit": "threshold"} + + filtered_data = list( + filter(lambda x: does_fulfill_requirement(x, requirements, filter_modes), speed_data) + ) + + assert filtered_data == speed_data