[feature] Implemented sub-filter functionality in Django admin#700
[feature] Implemented sub-filter functionality in Django admin#700pandafy wants to merge 2 commits into
Conversation
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
📝 WalkthroughWalkthroughThis PR implements conditional admin "sub-filters": a SubFilterMixin enforces parent-dependent filter application on the backend; template tag logic groups parent filters with their sub-filters and emits metadata; JS/CSS control sub-filter visibility and layout; a test-project CreatedSubFilter demonstrates usage; unit and Selenium tests cover behavior and rendering; and Sphinx docs were added. Sequence Diagram(s)sequenceDiagram
participant Browser
participant JS as ow-filter.js
participant Template as ow_render_filters
participant DjangoAdmin
participant DB
Browser->>JS: DOMContentLoaded -> initSubFilterVisibility()
JS->>Template: read data-sub-filter-parent / data-sub-filter-active-values
Browser->>JS: user selects parent filter
JS->>JS: updateSubFilters() -> show/hide .ow-sub-filter
Browser->>DjangoAdmin: submit changelist with parent(+optional sub) query params
DjangoAdmin->>DjangoAdmin: SubFilterMixin.is_parent_active checks request params
DjangoAdmin->>DjangoAdmin: SubFilterMixin.filter_queryset() applies when parent active
DjangoAdmin->>DB: execute filtered queryset
DB-->>DjangoAdmin: results
DjangoAdmin-->>Browser: render changelist (templates include sub-filter metadata)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@openwisp_utils/admin_theme/filters.py`:
- Around line 154-156: Add a unit test that exercises the is_parent_active
branch where request.GET contains the "{parent_parameter_name}__exact" key:
create a mock request with GET including f"{parent_parameter_name}__exact"
(matching a parent id/value used by the test), instantiate or subclass
SubFilterMixin (as other tests do), call is_parent_active and assert the
expected boolean result; name the test similar to existing ones (e.g.,
test_sub_filter_is_parent_active_exact_parent_parameter_name) and follow the
setup pattern from test_sub_filter_is_parent_active_no_parent to ensure it hits
the "__exact" branch.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e928198-7da3-407c-b0c2-3534437284fe
📒 Files selected for processing (11)
docs/developer/admin-utilities.rstdocs/user/admin-filters.rstopenwisp_utils/admin_theme/filters.pyopenwisp_utils/admin_theme/static/admin/css/ow-filters.cssopenwisp_utils/admin_theme/static/admin/js/ow-filter.jsopenwisp_utils/admin_theme/templates/admin/change_list.htmlopenwisp_utils/admin_theme/templates/admin/filter.htmlopenwisp_utils/admin_theme/templatetags/ow_tags.pytests/test_project/admin.pytests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_utils/admin_theme/templates/admin/change_list.htmlopenwisp_utils/admin_theme/templates/admin/filter.htmltests/test_project/tests/test_selenium.pyopenwisp_utils/admin_theme/filters.pytests/test_project/admin.pytests/test_project/tests/test_admin.pyopenwisp_utils/admin_theme/templatetags/ow_tags.py
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (AGENTS.md)
Update documentation when behavior, settings, public APIs, setup steps, QA rules, or supported versions change
Files:
docs/user/admin-filters.rstdocs/developer/admin-utilities.rst
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Add or update tests for every behavior change
Runopenwisp-qa-formatafter editing
Prefer in-process tests so coverage tools can measure changed code
When checking coverage for a changed module, usepython -m pytest <test_path> --cov=<dotted.module.path> --cov-report=term-missing
Watch for unsafe file paths, unsafe subprocess usage, token or secret exposure, and changes that could weaken QA or release safeguards
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside it
Files:
tests/test_project/tests/test_selenium.pyopenwisp_utils/admin_theme/filters.pytests/test_project/admin.pytests/test_project/tests/test_admin.pyopenwisp_utils/admin_theme/templatetags/ow_tags.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix
Files:
tests/test_project/tests/test_selenium.pytests/test_project/admin.pytests/test_project/tests/test_admin.py
🪛 HTMLHint (1.9.2)
openwisp_utils/admin_theme/templates/admin/filter.html
[error] 3-3: Special characters must be escaped : [ < ].
(spec-char-escape)
[error] 3-3: Special characters must be escaped : [ > ].
(spec-char-escape)
🪛 OpenGrep (1.22.0)
openwisp_utils/admin_theme/templatetags/ow_tags.py
[WARNING] 96-96: Django mark_safe() with dynamic content can lead to XSS. Only use mark_safe() with trusted, pre-escaped content.
(coderabbit.xss.python-mark-safe)
🔇 Additional comments (19)
docs/developer/admin-utilities.rst (1)
228-279: LGTM!docs/user/admin-filters.rst (1)
20-28: LGTM!tests/test_project/admin.py (1)
1-1: LGTM!Also applies to: 4-4, 7-7, 21-21, 55-94
tests/test_project/tests/test_admin.py (1)
3-3: LGTM!Also applies to: 8-8, 11-23, 41-42, 628-776
tests/test_project/tests/test_selenium.py (1)
721-803: LGTM!openwisp_utils/admin_theme/filters.py (2)
165-170: LGTM!
172-173: LGTM!openwisp_utils/admin_theme/templatetags/ow_tags.py (5)
1-1: LGTM!Also applies to: 6-6, 8-8
12-27: LGTM!
30-32: LGTM!
65-67: Well-designed parent-child matching logic.The matching accommodates both exact parameter names and Django's
__exactsuffix pattern, ensuring compatibility with FieldListFilter behavior.
86-96: Reviewmark_safe()safety inow_render_filters()
openwisp_utils/admin_theme/templatetags/ow_tags.pymarks the final concatenated HTML as safe (mark_safe("".join(output))). That string includes both hardcoded wrapper<div>tags and the HTML returned by_render_single_filter()(which renderstpl = get_template(spec.template)with context liketitle,selected_choice/choice["display"],choices, andspec). This is only safe if all filter templates referenced byspec.templaterely on Django auto-escaping and never disable it or mark user-controlled values as safe.Ensure the filter templates (including
admin/filter.html,admin/input_filter.html,admin/auto_filter.html, and any other templates reachable viaspec.template) do not use{% autoescape off %}or|safe, and thatspec.title/choice["display"]are not pre-wrapped asSafeString/mark_safecontent.openwisp_utils/admin_theme/templates/admin/change_list.html (1)
56-56: LGTM!openwisp_utils/admin_theme/static/admin/css/ow-filters.css (1)
243-254: LGTM!openwisp_utils/admin_theme/static/admin/js/ow-filter.js (4)
18-18: LGTM!
134-134: LGTM!
229-255: Sub-filter visibility logic is well-structured.The functions correctly traverse the DOM to find parent-child relationships and apply visibility rules. However, the correctness depends on
getFilterValue()returning parameter values rather than display text (see previous comment).
220-227: Reconsider: getFilterValue() usesselected.titlefor link-based filters, but current tests suggest values match forshelf__books_type
openwisp_utils/admin_theme/static/admin/js/ow-filter.js(lines 220-227) returnsselected.titlefor non-selectfilters, and the filter template sets thattitlefromchoice.display. Ifchoice.displaycan differ from the actual query-parameter value, theparent_active_valuescomparison could break.However, the existing Selenium coverage for
CreatedSubFilter(test_sub_filter_visible_when_parent_activeusing?shelf__books_type=HORRORandtest_sub_filter_hidden_when_parent_inactiveusing?shelf__books_type=FANTASY) indicatesgetFilterValue()matches"HORROR"/"FANTASY"correctly for the test fixture, so the reported issue doesn’t manifest here. Confirm that this holds for any cases wherechoice.display≠ option value; otherwise updategetFilterValue()to read the underlying value (e.g., from the option’s query string or a data attribute).openwisp_utils/admin_theme/templates/admin/filter.html (1)
3-3:data-sub-filter-*values are rendered through Django auto-escaping (no request-driven injection)
data-sub-filter-parentanddata-sub-filter-active-valuesinopenwisp_utils/admin_theme/templates/admin/filter.htmlare output via normal Django{{ ... }}expressions (spec.parent_parameter_nameandspec.parent_active_values|join:','). The values originate from filter class attributes (SubFilterMixin.parent_parameter_name/parent_active_values, and examples in tests use simple literals), not fromrequest.GET(which is used only to computespec.is_parent_active). Django’s escaping should prevent breaking out of the attribute and injecting HTML/JS via quotes/markup.If a filter class is populated with attacker-controlled input (i.e., filter authors aren’t trusted), validate/sanitize
parent_parameter_nameandparent_active_valuesto match the expected formats even though Django will escape them in the template.
ReST Formatting and Test FailuresHello @pandafy,
|
This change introduces ``SubFilterMixin``, a reusable mixin that allows admin filters to be displayed and applied only when a parent filter has specific values. To support this functionality, the filter rendering system has been enhanced to group parent filters with their sub-filters, rendering sub-filters vertically below their parent. Client-side logic has been added to dynamically show or hide sub-filters as parent filter values change. Invalid sub-filter usage is now rejected by raising ``IncorrectLookupParameters`` when a sub-filter value is supplied while its parent filter is inactive. Misconfigured orphaned sub-filters are not rendered and are logged as errors to help identify configuration issues. Comprehensive unit and Selenium tests have been added to verify filter visibility, queryset filtering, invalid parameter handling, rendering behavior, and orphaned sub-filter detection.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_project/tests/test_admin.py (1)
688-704: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMisleading test name and missing coverage for
parent_parameter_name=None.
test_sub_filter_is_parent_active_no_parent(line 688) usesCreatedSubFilter, which HASparent_parameter_name="shelf__books_type". The test name implies testing a filter without a parent, but it actually tests therequest=Nonecase. Additionally,test_sub_filter_is_parent_active_no_request(line 694) covers the same scenario, making these tests largely duplicate.More importantly, the branch at
openwisp_utils/admin_theme/filters.py:156-157(if not self.parent_parameter_name: return True) is never exercised by any test. This branch handles filters that legitimately have no parent dependency.🧪 Recommended changes
Rename
test_sub_filter_is_parent_active_no_parenttotest_sub_filter_is_parent_active_with_parent_and_no_requestor consolidate with the test at line 694.Add a new test that actually exercises the
parent_parameter_name=Nonebranch:def test_sub_filter_is_parent_active_no_parent_parameter(self): class NoParentParameterSubFilter(SubFilterMixin, SimpleInputFilter): title = "Test" parameter_name = "test" parent_parameter_name = None # or "" parent_active_values = () request = HttpRequest() request.GET = {"some_filter": "value"} filter = NoParentParameterSubFilter( request=request, params={}, model=Book, model_admin=BookAdmin ) self.assertEqual(filter.is_parent_active, True)🤖 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 `@tests/test_project/tests/test_admin.py` around lines 688 - 704, Rename or consolidate the misleading test test_sub_filter_is_parent_active_no_parent (which actually uses CreatedSubFilter with parent_parameter_name set) to a name like test_sub_filter_is_parent_active_with_parent_and_no_request or merge it with test_sub_filter_is_parent_active_no_request, and add a new unit test that instantiates a filter class (subclassing SubFilterMixin and SimpleInputFilter, e.g., NoParentParameterSubFilter) with parent_parameter_name set to None (or empty string) and verifies its is_parent_active property returns True to exercise the branch in SubFilterMixin.is_parent_active that checks if not self.parent_parameter_name.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/test_project/tests/test_admin.py`:
- Around line 688-704: Rename or consolidate the misleading test
test_sub_filter_is_parent_active_no_parent (which actually uses CreatedSubFilter
with parent_parameter_name set) to a name like
test_sub_filter_is_parent_active_with_parent_and_no_request or merge it with
test_sub_filter_is_parent_active_no_request, and add a new unit test that
instantiates a filter class (subclassing SubFilterMixin and SimpleInputFilter,
e.g., NoParentParameterSubFilter) with parent_parameter_name set to None (or
empty string) and verifies its is_parent_active property returns True to
exercise the branch in SubFilterMixin.is_parent_active that checks if not
self.parent_parameter_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c875e7c3-c084-4c6a-b6b2-8e01f4116d91
📒 Files selected for processing (3)
openwisp_utils/admin_theme/filters.pytests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,html,txt}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_utils/admin_theme/filters.pytests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place imports at the top of the file; only defer imports when necessary (e.g., Django model imports inside functions or methods where the app registry is not yet ready)
Avoid unnecessary blank lines inside function and method bodies
Add or update tests for every behavior change
Runopenwisp-qa-formatafter editing
Prefer in-process tests so coverage tools can measure changed code
When checking coverage for a changed module, usepython -m pytest <test_path> --cov=<dotted.module.path> --cov-report=term-missing
Watch for unsafe file paths, unsafe subprocess usage, token or secret exposure, and changes that could weaken QA or release safeguards
Write comments and docstrings only when they explain why code is shaped a certain way; place comments before the relevant code block instead of scattering them inside it
Files:
openwisp_utils/admin_theme/filters.pytests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For bug fixes, write the regression test first, run it against the unfixed code, confirm it fails for the expected reason, then implement the fix
Files:
tests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.py
🔇 Additional comments (10)
openwisp_utils/admin_theme/filters.py (2)
150-152: LGTM!
170-170: LGTM!tests/test_project/tests/test_admin.py (7)
628-647: LGTM!
649-665: LGTM!
667-686: LGTM!
706-718: LGTM!
720-747: LGTM!
749-757: LGTM!
759-789: LGTM!tests/test_project/tests/test_selenium.py (1)
721-802: LGTM!The
TestSubFilterclass provides comprehensive coverage of the sub-filter feature. The tests properly verify:
- Default hidden state
- Conditional visibility based on parent filter values (HORROR vs FANTASY)
- Backend queryset filtering when both parent and sub-filter are active
- Proper use of Selenium waits and assertions
As per coding guidelines, please ensure you run
openwisp-qa-formatafter editing this file.Source: Coding guidelines
|
coverage: 97.619% (-0.01%) from 97.629% — sub-filter into master |
2 participants
Checklist
Description of Changes
This change introduces
SubFilterMixin, a reusable mixin that allows admin filters to be displayed and applied only when a parent filter has specific values.To support this functionality, the filter rendering system has been enhanced to group parent filters with their sub-filters, rendering sub-filters vertically below their parent. Client-side logic has been added to dynamically show or hide sub-filters as parent filter values change.
Invalid sub-filter usage is now rejected by raising
IncorrectLookupParameterswhen a sub-filter value is supplied while its parent filter is inactive. Misconfigured orphaned sub-filters are not rendered and are logged as errors to help identify configuration issues.Comprehensive unit and Selenium tests have been added to verify filter visibility, queryset filtering, invalid parameter handling, rendering behavior, and orphaned sub-filter detection.
Screenshot
The sub-filter is shown when the "Type of Book" is set to "HORROR"
