feat: added filter types - #388
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable per-category filtering modes across backend queries, API metadata, frontend controls, documentation, seeded E2E data, and unit/E2E tests. Supported behaviors include OR, AND, exclusive, boolean, and threshold matching. ChangesCategory filter modes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MapConfig
participant BackendAPI
participant FiltersForm
participant LocationsAPI
MapConfig->>BackendAPI: provide categories_filter_mode
BackendAPI-->>FiltersForm: return category filter_mode
FiltersForm->>LocationsAPI: submit selected category values
LocationsAPI->>BackendAPI: apply configured matcher
BackendAPI-->>LocationsAPI: return filtered locations
LocationsAPI-->>FiltersForm: render filtered results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs/quickstart.rst (1)
97-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocumentation omits the
"and"filter mode; the "four modes" example doesn't cover all four listed modes.
core.pyimplements and tests"and"as a fully supportedcategories_filter_modevalue (see_matches_andand its dedicated tests), but this section only documents"or","exclusive","boolean", and"threshold"—"and"is undiscoverable from the docs. Separately, the example claims to combine "all four modes" but only shows"or","or","boolean","threshold"("exclusive"is missing from the example).Suggest adding an
"and"subsection (mirroring the docstring in_matches_and, e.g. narrowing amenities to entries with both"lighting"AND"benches") and updating the example to actually demonstrate all modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/quickstart.rst` around lines 97 - 159, Add an ``"and"`` subsection to the ``categories_filter_mode`` documentation, describing that a location must contain every selected value within the category and illustrating it with the ``lighting`` and ``benches`` example. Update the example configuration to include an ``"exclusive"`` category and use ``"and"`` for another category so it demonstrates all supported modes: ``or``, ``and``, ``exclusive``, ``boolean``, and ``threshold``.goodmap/core.py (1)
59-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefault multi-select combination flips from AND to OR — flag as a breaking behavior change.
Per the summary, this replaces "the prior fixed all-of values present in entry[category] logic" with a default
"or"matcher for any category without an explicitfilter_mode. For any existing deployment where a category currently relies on multiple selected values narrowing results (old AND semantics), this silently broadens result sets after upgrade — with no config change required to trigger it and no changelog/upgrade note included in this diff.The new default lines up with the deliberate rationale documented in the new tests (e.g.
test_multiple_selected_values_in_same_category_are_or_by_default), so this looks intentional, but it's still a user-visible behavior change on upgrade for anyone currently depending on the old default.Please confirm this is intentional and consider adding a changelog/upgrade note advising operators that categories needing the old "match all selected values" semantics must now set
categories_filter_mode: "and"explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@goodmap/core.py` around lines 59 - 81, Document the intentional default change in the project’s changelog or upgrade notes: categories without an explicit filter mode now use OR instead of the previous AND behavior. Advise operators who need the old match-all semantics to configure categories_filter_mode to "and", referencing does_fulfill_requirement and its filter_modes default.e2e-tests/tests/conftest.py (1)
105-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the toggle-panel open/close logic.
The
toggle_button/opened_dialogopen-close pattern here (Lines 121-131) is identical to the one inclear_all_checkboxes(Lines 93-102). Extracting a shared context manager would avoid drift as more helpers are added.♻️ Proposed extraction
+from contextlib import contextmanager + +@contextmanager +def _mobile_panel_open(page: Page): + 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) + try: + yield + finally: + if opened_dialog: + page.locator('button[aria-label="Close left panel"]').evaluate("el => el.click()") + def open_zwierzyniecka_popup(page: Page) -> None: ... - 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()") + with _mobile_panel_open(page): + page.locator("`#clear-filters-button`").click() + page.locator("`#filter-form` input#bikes").check() markers = page.locator(".leaflet-marker-icon") expect(markers).to_have_count(1, timeout=MARKER_LOAD_TIMEOUT) markers.first.click()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-tests/tests/conftest.py` around lines 105 - 137, Deduplicate the repeated left-panel open/close handling used by open_zwierzyniecka_popup and clear_all_checkboxes by extracting a shared context manager. Have both helpers use it while preserving the existing visibility checks, selector interactions, and cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e-tests/tests/basic/test_map.py`:
- Around line 44-63: Update test_displays_filter_list_with_four_categories to
expect 9 checkboxes, reflecting the three amenities options alongside the
existing filters. Also assert that the translated “amenities” category header is
visible, while preserving the existing radio count and other category
assertions.
In `@goodmap/db.py`:
- Around line 818-838: Update mongodb_db_get_locations to handle the "and"
filter mode explicitly, using MongoDB’s all-elements matching semantics ($all)
so every selected value must be present. Keep "threshold" handling and the
existing "$in" behavior for "or" and "exclusive" modes unchanged. Also align
threshold parse failures with core.py by making invalid threshold values produce
no matches rather than silently dropping that category filter.
---
Nitpick comments:
In `@docs/quickstart.rst`:
- Around line 97-159: Add an ``"and"`` subsection to the
``categories_filter_mode`` documentation, describing that a location must
contain every selected value within the category and illustrating it with the
``lighting`` and ``benches`` example. Update the example configuration to
include an ``"exclusive"`` category and use ``"and"`` for another category so it
demonstrates all supported modes: ``or``, ``and``, ``exclusive``, ``boolean``,
and ``threshold``.
In `@e2e-tests/tests/conftest.py`:
- Around line 105-137: Deduplicate the repeated left-panel open/close handling
used by open_zwierzyniecka_popup and clear_all_checkboxes by extracting a shared
context manager. Have both helpers use it while preserving the existing
visibility checks, selector interactions, and cleanup behavior.
In `@goodmap/core.py`:
- Around line 59-81: Document the intentional default change in the project’s
changelog or upgrade notes: categories without an explicit filter mode now use
OR instead of the previous AND behavior. Advise operators who need the old
match-all semantics to configure categories_filter_mode to "and", referencing
does_fulfill_requirement and its filter_modes default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e3ed9c6-9e45-41fa-8fd2-8135c48b7303
📒 Files selected for processing (25)
docs/api.rstdocs/quickstart.rste2e-tests/e2e_test_data_initial.jsone2e-tests/tests/basic/test_accessibility_table.pye2e-tests/tests/basic/test_language.pye2e-tests/tests/basic/test_map.pye2e-tests/tests/basic/test_mobile_box.pye2e-tests/tests/basic/test_popup.pye2e-tests/tests/basic/test_share.pye2e-tests/tests/conftest.pye2e-tests/translations/en/LC_MESSAGES/messages.poe2e-tests/translations/pl/LC_MESSAGES/messages.pofrontend/src/components/FiltersForm/FiltersForm.jsxfrontend/src/components/MarkerPopup/MarkerPopup.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/src/services/http/httpService.jsfrontend/tests/FiltersForm.test.jsxgoodmap/core.pygoodmap/core_api.pygoodmap/db.pytests/unit_tests/test_core.pytests/unit_tests/test_core_api.pytests/unit_tests/test_db.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/FiltersForm/FiltersForm.jsx`:
- Around line 310-324: Update renderModeBadge in FiltersForm.jsx so the tooltip
trigger is keyboard-focusable, allowing keyboard users to reveal the mode help.
Replace the hardcoded “Help:” aria-label prefix with the localized
filterModeHelpAriaLabel translation, passing tooltipText as its description
interpolation, and add this key to every locale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a309194a-456c-4649-beb3-158c6d391472
📒 Files selected for processing (10)
docs/quickstart.rste2e-tests/e2e_test_data_initial.jsone2e-tests/scripts/generate_stress_test_data.pye2e-tests/tests/basic/test_map.pyexamples/e2e_test_data.jsonfrontend/src/components/FiltersForm/FiltersForm.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/tests/FiltersForm.test.jsx
💤 Files with no reviewable changes (1)
- e2e-tests/scripts/generate_stress_test_data.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/quickstart.rst
- frontend/tests/FiltersForm.test.jsx
- e2e-tests/e2e_test_data_initial.json
- e2e-tests/tests/basic/test_map.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/FiltersForm/FiltersForm.jsx`:
- Around line 411-419: Update the `sectionKey` and related
`aria-labelledby`/`FilterTitle` ID construction in the category rendering flow
to use the stable `categoryKey` rather than localized `categoryName`. Preserve
unique IDs and ensure `FilterSection` references the matching `FilterTitle` ID
without whitespace-separated values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa809098-695b-4c21-af36-6a77c2750140
📒 Files selected for processing (4)
frontend/src/components/FiltersForm/FiltersForm.jsxfrontend/src/services/http/httpService.jsfrontend/tests/FiltersForm.test.jsxfrontend/tests/Map/MapComponent.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/FiltersForm.test.jsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@goodmap/filtering.py`:
- Around line 19-55: Update the type annotations in _as_list, _matches_or,
_matches_and, and _matches_threshold to use parameterized list types such as
list[Any] instead of bare list annotations, and import Any for the _as_list
value parameter. Preserve the existing runtime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75fee784-28ee-419c-b7e5-36c9f865d78f
📒 Files selected for processing (13)
docs/api.rste2e-tests/tests/basic/test_map.pyfrontend/src/components/FiltersForm/FiltersForm.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/tests/FiltersForm.test.jsxgoodmap/core.pygoodmap/db.pygoodmap/filtering.pytests/unit_tests/test_core.pytests/unit_tests/test_db.pytests/unit_tests/test_filtering.py
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/locales/pl/map.json
- frontend/src/locales/en/map.json
- frontend/tests/FiltersForm.test.jsx
- tests/unit_tests/test_db.py
- goodmap/db.py
- e2e-tests/tests/basic/test_map.py
|
1 participant
Summary by CodeRabbit
/api/categories-fulland category data now expose each category’s activefilter_mode.categories_filter_modeand detailed combination behavior.