[change] Migrate from django-admin-autocomplete-filter to dalf #582 #585
Conversation
WalkthroughThe PR replaces django-admin-autocomplete-filter with django-admin-list-filter (DALF) across codebase and docs. Dependency and test settings updated. AutocompleteFilter now subclasses DALFRelatedFieldAjax with a new explicit init and persistent init-error handling. AutocompleteJsonView now extends Django’s native AutocompleteJsonView and implements a focused get_queryset to support reverse relations. Templates, JS, templatetags, admin entry points, tests (including Selenium), fixtures, and a migration updating Book.shelf.related_name were updated to use DALF APIs and the tuple-based list_filter format. Sequence Diagram(s)sequenceDiagram
participant Browser
participant DALF_JS as "DALF JS (ow-auto-filter.js)"
participant DjangoView as "AutocompleteJsonView (server)"
participant ORM as "Django ORM / DB"
Browser->>DALF_JS: user opens/queries autocomplete
DALF_JS->>DjangoView: AJAX GET /autocomplete/?q=... & data-field
DjangoView->>ORM: build queryset (handle reverse relations if needed)
ORM-->>DjangoView: results
DjangoView-->>DALF_JS: JSON choices
DALF_JS-->>Browser: render/update select inputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
added
dependencies
![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
🤖 Fix all issues with AI agents
In `@setup.py`:
- Line 30: The dependency constraint "dalf>=0.7.0,<1.0.0" in setup.py is
incompatible with Django 4.2 and Python 3.10; update the requirement or CI so
tests match supported versions: either change the requirement string in setup.py
(replace "dalf>=0.7.0,<1.0.0" with a version range that supports Django
4.2/Python 3.10, e.g. a pre-0.7 line such as "dalf>=0.6.0,<0.7.0" or another
known compatible version) or adjust the repository CI matrix to drop Django 4.2
and Python 3.10; edit the exact token "dalf>=0.7.0,<1.0.0" to apply the chosen
fix.
🧹 Nitpick comments (1)
openwisp_utils/admin_theme/filters.py (1)
1-10: Add debug logging for fallback init failures.A lightweight debug log here would aid troubleshooting without changing behavior.
♻️ Add debug logging for fallback init failures
+import logging from dalf.admin import DALFRelatedFieldAjax from django.contrib import messages from django.contrib.admin.filters import FieldListFilter, SimpleListFilter from django.contrib.admin.utils import NotRelationField, get_model_from_relation from django.core.exceptions import ImproperlyConfigured, ValidationError from django.db.models.fields import CharField, UUIDField from django.utils.translation import gettext_lazy as _ +logger = logging.getLogger(__name__) class InputFilterMixin: @@ - except Exception: - pass + except Exception as e: + logger.debug( + "AutocompleteFilter fallback init failed: %s", + e, + exc_info=True, + )Also applies to: 123-131
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/developer/admin-theme.rstdocs/developer/admin-utilities.rstdocs/user/settings.rstopenwisp_utils/admin_theme/filters.pyopenwisp_utils/admin_theme/static/admin/js/ow-auto-filter.jsopenwisp_utils/admin_theme/templates/admin/auto_filter.htmlopenwisp_utils/admin_theme/templatetags/ow_tags.pyopenwisp_utils/admin_theme/views.pysetup.pytests/openwisp2/settings.pytests/test_project/admin.pytests/test_project/tests/test_admin.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_utils/admin_theme/filters.py (2)
openwisp_utils/admin_theme/static/admin/js/ow-filter.js (1)
filters(73-73)tests/test_project/admin.py (1)
queryset(90-92)
tests/test_project/admin.py (2)
openwisp_utils/admin_theme/filters.py (2)
AutocompleteFilter(94-161)InputFilter(42-91)openwisp_utils/admin.py (1)
TimeReadonlyAdminMixin(7-12)
🪛 Ruff (0.14.14)
openwisp_utils/admin_theme/filters.py
[error] 130-131: try-except-pass detected, consider logging the exception
(S110)
[warning] 130-130: Do not catch blind exception: Exception
(BLE001)
tests/test_project/admin.py
[warning] 47-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (4)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (12)
tests/openwisp2/settings.py (1)
36-36: Config update looks correct.Adds DALF to test INSTALLED_APPS as expected.
docs/user/settings.rst (1)
267-271: Doc note reads well.Clear deprecation guidance for the setting.
tests/test_project/tests/test_admin.py (1)
491-494: Test adjustment looks solid.Following redirects and asserting the invalid param is stripped is a good check.
openwisp_utils/admin_theme/templatetags/ow_tags.py (1)
14-15: Safer choice handling.Using
.get()with defaults avoids KeyError and is a solid hardening.openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js (1)
3-3: Comment update is fine.docs/developer/admin-theme.rst (1)
25-25: Documentation update looks correct.openwisp_utils/admin_theme/templates/admin/auto_filter.html (1)
3-3: LGTM.tests/test_project/admin.py (3)
1-1: LGTM.
45-47: LGTM.
95-105: LGTM.openwisp_utils/admin_theme/views.py (1)
1-14: LGTM.docs/developer/admin-utilities.rst (1)
187-223: LGTM.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "django-model-utils>=4.5,<5.1", | ||
| "django-minify-compress-staticfiles~=1.1.0", | ||
| "django-admin-autocomplete-filter~=0.7.1", | ||
| "dalf>=0.7.0,<1.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dalf 0.7.x django-admin-list-filter Django version compatibility requirements
💡 Result:
For dalf (django-admin-list-filter) 0.7.x:
- Django: 5.0+ (the project explicitly says it “supports Django version 5 and above” and classifiers list Django 5.0, 5.1, 5.2) [1].
- Python: >= 3.11 (PyPI metadata: “Requires: Python >=3.11”) [1].
Sources: PyPI project page / metadata [1] and the upstream GitHub README stating Django 5+ support [2].
[1] (pypi.org)
[2] (github.com)
Citations:
- 1: https://pypi.org/project/dalf/
- 2: https://github.com/vigo/django-admin-list-filter?utm_source=openai
🏁 Script executed:
# Check the setup.py for Django version requirements in this repository
cat setup.py | head -50Repository: openwisp/openwisp-utils
Length of output: 1791
🏁 Script executed:
# Also check for tox.ini or other configuration files that might specify Django versions
fd -e "tox.ini" -e "requirements*.txt" -e ".python-version" | head -20Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
cat setup.py | tail -100Repository: openwisp/openwisp-utils
Length of output: 2685
🏁 Script executed:
# Check for tox.ini or pytest configuration that shows Django version testing
cat tox.ini 2>/dev/null || echo "No tox.ini found"Repository: openwisp/openwisp-utils
Length of output: 83
🏁 Script executed:
# Search for Django in install_requires or anywhere in setup.py
rg "django" setup.py -i -A 2 -B 2Repository: openwisp/openwisp-utils
Length of output: 1075
🏁 Script executed:
# Check if there's a pyproject.toml or setup.cfg that might specify Python/Django versions
fd -e "pyproject.toml" -e "setup.cfg" -e ".github" | head -20Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
# Check for GitHub workflows or CI configuration that shows tested Django versions
find .github -name "*.yml" -o -name "*.yaml" 2>/dev/null | head -5Repository: openwisp/openwisp-utils
Length of output: 202
🌐 Web query:
django-model-utils 4.5 Django version compatibility support
💡 Result:
For django-model-utils 4.5.x (4.5.0 / 4.5.1):
- Minimum Django: Django 3.2+ (the project states “django-model-utils supports Django 3.2+”). [1]
- Explicitly confirmed supported Django series in the 4.5 line: Django 3.2, Django 4.2, and Django 5.0 (Django 4.2 and 5.0 support was added/confirmed in 4.4.0 and carries forward into 4.5.x). [2]
Notably, formal support for Django 5.1 was added later in django-model-utils 5.0.0, not in 4.5.x. [1]
Sources: [1] [2]
🏁 Script executed:
# Check the CI workflow to see what Django versions are being tested
cat .github/workflows/ci.ymlRepository: openwisp/openwisp-utils
Length of output: 2841
DALF 0.7.x is incompatible with Django 4.2 and Python 3.10 in the repo's test matrix.
DALF 0.7.x requires Django 5.0+ (supports Django 5.0, 5.1, 5.2) and Python >=3.11. However, this repository's CI explicitly tests Django 4.2 with Python 3.10 and 3.11. Installing this dependency will prevent users on Django 4.2 or Python 3.10 from using openwisp-utils. Either drop support for Django 4.2 and Python 3.10 in the CI matrix, or choose a different dependency version that supports the broader version range.
🤖 Prompt for AI Agents
In `@setup.py` at line 30, The dependency constraint "dalf>=0.7.0,<1.0.0" in
setup.py is incompatible with Django 4.2 and Python 3.10; update the requirement
or CI so tests match supported versions: either change the requirement string in
setup.py (replace "dalf>=0.7.0,<1.0.0" with a version range that supports Django
4.2/Python 3.10, e.g. a pre-0.7 line such as "dalf>=0.6.0,<0.7.0" or another
known compatible version) or adjust the repository CI matrix to drop Django 4.2
and Python 3.10; edit the exact token "dalf>=0.7.0,<1.0.0" to apply the chosen
fix.
There was a problem hiding this comment.
Pull request overview
This PR migrates the admin autocomplete filter integration from the unmaintained django-admin-autocomplete-filter to django-admin-list-filter (DALF), aligning licensing and using Django’s native admin autocomplete infrastructure. It refactors the filter/view implementations, updates tests, templates, JS, and docs to match the new behavior, and adds error handling for invalid filter parameters.
Changes:
- Replaced
django-admin-autocomplete-filterwithdalfin dependencies, settings, and admin configuration, and updated admin classes to useDALFModelAdminand the DALF-basedAutocompleteFilter. - Simplified
AutocompleteJsonViewto subclass Django’sAutocompleteJsonViewwhile preserving reverse-relation support, and rewrote the customAutocompleteFilterto extendDALFRelatedFieldAjaxwith robust handling of invalid filter values. - Updated templates, JS, template tags, and documentation to integrate DALF, reflect the new usage pattern, and mark the old
OPENWISP_AUTOCOMPLETE_FILTER_VIEW-based path as deprecated.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_project/tests/test_admin.py |
Adjusts admin tests to cover the new autocomplete behavior, including reverse-relation support and invalid UUID filter handling; introduces a new expectation that invalid filter parameters are stripped from the final URL. |
tests/test_project/admin.py |
Switches test admin classes to use DALFModelAdmin and the DALF-based AutocompleteFilter via the tuple syntax, ensuring the test project exercises the new filter integration. |
tests/openwisp2/settings.py |
Replaces the old admin_auto_filters app with dalf in the test settings’ INSTALLED_APPS, enabling DALF in the test project. |
setup.py |
Updates install_requires to drop django-admin-autocomplete-filter and depend on dalf>=0.7.0,<1.0.0, enforcing the new library at install time. |
openwisp_utils/admin_theme/views.py |
Replaces the custom JSON autocomplete view implementation with a subclass of Django’s AutocompleteJsonView, overriding only get_queryset to support reverse relations that lack get_limit_choices_to(). |
openwisp_utils/admin_theme/templatetags/ow_tags.py |
Makes ow_create_filter more robust by safely accessing selected and display keys when determining the selected choice, accommodating DALF’s choice structures. |
openwisp_utils/admin_theme/templates/admin/auto_filter.html |
Swaps the old django-admin-autocomplete-filter include and extra container for DALF’s django_admin_list_filter_ajax template, wiring OpenWISP’s styling wrapper around DALF’s markup. |
openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js |
Updates the comment to reference DALF-based autocomplete filters while keeping the existing JS behavior that customizes select2 for the autocomplete filters. |
openwisp_utils/admin_theme/filters.py |
Replaces the old BaseAutocompleteFilter-based implementation with a DALFRelatedFieldAjax subclass that uses the new template and adds defensive handling of invalid parameters (e.g., invalid UUIDs), while keeping the existing InputFilter and mixin logic. |
docs/user/settings.rst |
Documents that the OPENWISP_AUTOCOMPLETE_FILTER_VIEW setting is now deprecated after the DALF migration, since DALF uses Django’s native autocomplete endpoints. |
docs/developer/admin-utilities.rst |
Updates the admin-utilities docs to describe the DALF-based autocomplete filter, show the new DALFModelAdmin + tuple usage pattern, and link to DALF’s documentation. |
docs/developer/admin-theme.rst |
Updates admin-theme setup instructions to include dalf instead of admin_auto_filters in INSTALLED_APPS, aligning developer docs with the new dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Usage: | ||
| class MyFilter(AutocompleteFilter): | ||
| title = "My Field" | ||
| field_name = "my_field" | ||
| parameter_name = "my_field__id" |
There was a problem hiding this comment.
The usage example in this docstring still shows creating a custom MyAutoCompleteFilter subclass with field_name and parameter_name, but the new DALF-based API encourages using the tuple form ("field", AutocompleteFilter) as documented in docs/developer/admin-utilities.rst. To avoid confusing consumers of this helper, consider updating the example here to match the recommended DALF usage pattern.
| Usage: | |
| class MyFilter(AutocompleteFilter): | |
| title = "My Field" | |
| field_name = "my_field" | |
| parameter_name = "my_field__id" | |
| Usage (DALF tuple-based syntax): | |
| from django.contrib import admin | |
| from openwisp_utils.admin_theme.filters import AutocompleteFilter | |
| class MyModelAdmin(admin.ModelAdmin): | |
| list_filter = [ | |
| ("my_field", AutocompleteFilter), | |
| ] |
| <div class="ow-filter auto-filter"> | ||
| {% include 'django-admin-autocomplete-filter/autocomplete-filter.html' %} | ||
| <div class="auto-filter-choices"></div> | ||
| {% include 'admin/filter/django_admin_list_filter_ajax.html' %} |
There was a problem hiding this comment.
The previous template rendered an extra <div class="auto-filter-choices"></div> container that is now removed, but the existing CSS/JS (e.g., in ow-auto-filter.js and ow-auto-filter.css) still target the .auto-filter-choices selector. If that container is no longer required for layout, it would be good to clean up the unused CSS/JS, otherwise we should introduce an equivalent wrapper around the DALF template so that the current JavaScript logic for positioning and sizing the dropdown continues to work.
| {% include 'admin/filter/django_admin_list_filter_ajax.html' %} | |
| <div class="auto-filter-choices"> | |
| {% include 'admin/filter/django_admin_list_filter_ajax.html' %} | |
| </div> |
| # The invalid parameter should be stripped from the final URL | ||
| self.assertNotIn("shelf__id=invalid", response.request["QUERY_STRING"]) |
There was a problem hiding this comment.
With the current AutocompleteFilter implementation, invalid filter values are handled by swallowing the ValidationError/ValueError and returning an unfiltered queryset, but there is no code path that modifies the request query string or issues a redirect with a sanitized URL. As a result, response.request["QUERY_STRING"] will still contain shelf__id=invalid, so this assertion is unlikely to pass; to actually strip the parameter you would need an explicit redirect that rewrites the query string (e.g., in the changelist view), otherwise this test should be relaxed to only assert the 200 response (and optionally the presence of the error message).
| # The invalid parameter should be stripped from the final URL | |
| self.assertNotIn("shelf__id=invalid", response.request["QUERY_STRING"]) |
| try: | ||
| from django.contrib.admin.filters import FieldListFilter | ||
|
|
||
| # Call the grandparent's __init__ to set up basic filter infrastructure | ||
| FieldListFilter.__init__( | ||
| self, field, request, params, model, model_admin, field_path | ||
| ) | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
Catching a broad Exception here and silently ignoring it can leave the filter in a partially initialized state and also mask real programming errors or configuration issues unrelated to invalid user input. It would be safer to narrow this to the specific exceptions you expect (e.g., ImportError or TypeError) or at least log the error so that unexpected failures in FieldListFilter.__init__ are visible during debugging.
| try: | |
| from django.contrib.admin.filters import FieldListFilter | |
| # Call the grandparent's __init__ to set up basic filter infrastructure | |
| FieldListFilter.__init__( | |
| self, field, request, params, model, model_admin, field_path | |
| ) | |
| except Exception: | |
| pass | |
| # Call the grandparent's __init__ to set up basic filter infrastructure | |
| FieldListFilter.__init__( | |
| self, field, request, params, model, model_admin, field_path | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
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_selenium.py (1)
456-474:⚠️ Potential issue | 🟡 MinorAvoid brittle select2 option indexing.
Selecting
li[4]assumes a stable ordering of options; this can easily break if the result list changes (e.g., extra options or different sorting). Prefer selecting by visible text (e.g., the intended username) to keep the test resilient.🛠️ Suggested change (select by visible text)
- owner_filter_option_xpath = f'//*[`@id`="select2-{owner_filter_select.get_attribute("id")}-results"]/li[4]' + owner_filter_option_xpath = ( + f'//*[`@id`="select2-{owner_filter_select.get_attribute("id")}-results"]' + f'/li[contains(normalize-space(.), "tester2")]' + )
🤖 Fix all issues with AI agents
In `@tests/test_project/tests/test_selenium.py`:
- Around line 703-731: The code assumes the null option is the first li
(filter_null_option_xpath = f'//*[`@id`="select2-{select_id}-results"]/li[1]')
which is brittle; instead construct an XPath that finds the option by its
visible label (e.g., normalize-space(text()) = "-" or contains text for the null
label) and use that XPath with wait_for_presence(By.XPATH, ...) and .click();
update the assignment of filter_null_option_xpath and its usage (keep
references: select_id, filter_null_option_xpath, wait_for_presence, By.XPATH) so
the test selects the null option by label rather than by index.
- Around line 648-686: The test currently selects the second option via
filter_option_xpath = f'//*[`@id`="select2-{select_id}-results"]/li[2]' which is
flaky; change the selection to find the option by its label text
(factual_shelf.name) instead. Replace the wait_for_presence(By.XPATH,
filter_option_xpath).click() call with a wait_for_presence(By.XPATH, <XPath that
matches the li whose text equals factual_shelf.name under
//*[`@id`="select2-{select_id}-results"]>).click() so the selector targets the
option by label (use contains() or normalize-space() with factual_shelf.name)
instead of a positional index. Ensure any variable names referenced (select_id,
factual_shelf.name, filter_options/filter_option_xpath) are updated accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/developer/admin-utilities.rstdocs/user/settings.rsttests/test_project/tests/test_selenium.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/developer/admin-utilities.rst
- docs/user/settings.rst
⏰ 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). (10)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | 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.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' | ||
| self.open(url) | ||
|
|
||
| select_elem = self.wait_for_presence( | ||
| By.CSS_SELECTOR, | ||
| 'select.django-admin-list-filter-ajax[data-field-name="shelf"]', | ||
| ) | ||
| select_id = select_elem.get_attribute("id") | ||
| filter_css_selector = f"#select2-{select_id}-container" | ||
| filter_options = f'//*[@id="select2-{select_id}-results"]/li' | ||
| filter_option_xpath = f'//*[@id="select2-{select_id}-results"]/li[2]' | ||
|
|
||
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' | ||
| self.open(url) | ||
| # Verify select element exists with select2 class | ||
| self.assertIn( | ||
| ( | ||
| '<select name="shelf__id" data-dropdown-css-class="ow2-autocomplete-dropdown"' | ||
| f' data-empty-label="-" id="{select_id}" class="admin-autocomplete' | ||
| ), | ||
| self.web_driver.page_source, | ||
| "django-admin-list-filter-ajax", select_elem.get_attribute("class") | ||
| ) | ||
|
|
||
| # Click on select2 container to open dropdown | ||
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector).click() | ||
| self.wait_for_presence(By.CSS_SELECTOR, ".select2-container--open") | ||
| self.wait_for_presence( | ||
| By.XPATH, f'//*[contains(text(), "{horror_shelf.name}")]' | ||
| ) | ||
| self.assertIn(horror_shelf.name, self.web_driver.page_source) | ||
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | ||
|
|
||
| # Select the second option (factual shelf) | ||
| self.wait_for_presence(By.XPATH, filter_option_xpath).click() | ||
| self.assertIn(str(factual_shelf.id), self.web_driver.current_url) | ||
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector) | ||
| self.assertNotIn(horror_shelf.name, self.web_driver.page_source) | ||
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | ||
|
|
||
| # Verify Book 1 is absent, Book 2 is present | ||
| with self.assertRaises(NoSuchElementException): | ||
| # Book 1 is absent | ||
| self.web_driver.find_element(By.XPATH, result_xpath.format(book1.name)) | ||
| # Book 2 is present | ||
| self.find_element(By.XPATH, result_xpath.format(book2.name)) | ||
|
|
There was a problem hiding this comment.
Select the shelf option by label instead of position.
li[2] depends on option order and can be flaky. Match the option by factual_shelf.name so the test remains stable if the ordering changes.
🛠️ Suggested change (select by shelf name)
- filter_option_xpath = f'//*[`@id`="select2-{select_id}-results"]/li[2]'
+ filter_option_xpath = (
+ f'//*[`@id`="select2-{select_id}-results"]'
+ f'/li[contains(normalize-space(.), "{factual_shelf.name}")]'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' | |
| self.open(url) | |
| select_elem = self.wait_for_presence( | |
| By.CSS_SELECTOR, | |
| 'select.django-admin-list-filter-ajax[data-field-name="shelf"]', | |
| ) | |
| select_id = select_elem.get_attribute("id") | |
| filter_css_selector = f"#select2-{select_id}-container" | |
| filter_options = f'//*[@id="select2-{select_id}-results"]/li' | |
| filter_option_xpath = f'//*[@id="select2-{select_id}-results"]/li[2]' | |
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' | |
| self.open(url) | |
| # Verify select element exists with select2 class | |
| self.assertIn( | |
| ( | |
| '<select name="shelf__id" data-dropdown-css-class="ow2-autocomplete-dropdown"' | |
| f' data-empty-label="-" id="{select_id}" class="admin-autocomplete' | |
| ), | |
| self.web_driver.page_source, | |
| "django-admin-list-filter-ajax", select_elem.get_attribute("class") | |
| ) | |
| # Click on select2 container to open dropdown | |
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector).click() | |
| self.wait_for_presence(By.CSS_SELECTOR, ".select2-container--open") | |
| self.wait_for_presence( | |
| By.XPATH, f'//*[contains(text(), "{horror_shelf.name}")]' | |
| ) | |
| self.assertIn(horror_shelf.name, self.web_driver.page_source) | |
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | |
| # Select the second option (factual shelf) | |
| self.wait_for_presence(By.XPATH, filter_option_xpath).click() | |
| self.assertIn(str(factual_shelf.id), self.web_driver.current_url) | |
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector) | |
| self.assertNotIn(horror_shelf.name, self.web_driver.page_source) | |
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | |
| # Verify Book 1 is absent, Book 2 is present | |
| with self.assertRaises(NoSuchElementException): | |
| # Book 1 is absent | |
| self.web_driver.find_element(By.XPATH, result_xpath.format(book1.name)) | |
| # Book 2 is present | |
| self.find_element(By.XPATH, result_xpath.format(book2.name)) | |
| result_xpath = '//*[`@id`="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' | |
| self.open(url) | |
| select_elem = self.wait_for_presence( | |
| By.CSS_SELECTOR, | |
| 'select.django-admin-list-filter-ajax[data-field-name="shelf"]', | |
| ) | |
| select_id = select_elem.get_attribute("id") | |
| filter_css_selector = f"#select2-{select_id}-container" | |
| filter_options = f'//*[`@id`="select2-{select_id}-results"]/li' | |
| filter_option_xpath = ( | |
| f'//*[`@id`="select2-{select_id}-results"]' | |
| f'/li[contains(normalize-space(.), "{factual_shelf.name}")]' | |
| ) | |
| # Verify select element exists with select2 class | |
| self.assertIn( | |
| "django-admin-list-filter-ajax", select_elem.get_attribute("class") | |
| ) | |
| # Click on select2 container to open dropdown | |
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector).click() | |
| self.wait_for_presence(By.CSS_SELECTOR, ".select2-container--open") | |
| self.wait_for_presence( | |
| By.XPATH, f'//*[contains(text(), "{horror_shelf.name}")]' | |
| ) | |
| self.assertIn(horror_shelf.name, self.web_driver.page_source) | |
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | |
| # Select the second option (factual shelf) | |
| self.wait_for_presence(By.XPATH, filter_option_xpath).click() | |
| self.assertIn(str(factual_shelf.id), self.web_driver.current_url) | |
| self.wait_for_presence(By.CSS_SELECTOR, filter_css_selector) | |
| self.assertNotIn(horror_shelf.name, self.web_driver.page_source) | |
| self.assertIn(factual_shelf.name, self.web_driver.page_source) | |
| # Verify Book 1 is absent, Book 2 is present | |
| with self.assertRaises(NoSuchElementException): | |
| self.web_driver.find_element(By.XPATH, result_xpath.format(book1.name)) | |
| self.find_element(By.XPATH, result_xpath.format(book2.name)) |
🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_selenium.py` around lines 648 - 686, The test
currently selects the second option via filter_option_xpath =
f'//*[`@id`="select2-{select_id}-results"]/li[2]' which is flaky; change the
selection to find the option by its label text (factual_shelf.name) instead.
Replace the wait_for_presence(By.XPATH, filter_option_xpath).click() call with a
wait_for_presence(By.XPATH, <XPath that matches the li whose text equals
factual_shelf.name under //*[`@id`="select2-{select_id}-results"]>).click() so the
selector targets the option by label (use contains() or normalize-space() with
factual_shelf.name) instead of a positional index. Ensure any variable names
referenced (select_id, factual_shelf.name, filter_options/filter_option_xpath)
are updated accordingly.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Let's see if it works fine on Python >= 3.11, if evaluation passes we can drop Python 3.10 for all OpenWISP modules, anticipating a bit its EOL (Oct 2026).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_project/tests/test_selenium.py`:
- Around line 457-460: Remove the unused local variable owner_filter_select in
the Selenium test (the assignment inside tests/test_selenium.py where
find_element is called with By.CSS_SELECTOR and
'select.django-admin-list-filter-ajax[data-field-name="owner"]'); either delete
that assignment line or replace it with a direct call (e.g., call find_element
without assigning) or use the variable in an assertion—ensure no unused variable
remains to fix the Ruff F841 lint failure.
- Around line 474-482: The current selection uses a positional selector
(nth-child(4)) which is flaky; update the selection logic in the owner_option
block (the call to find_element, the wait_for_visibility, and the subsequent
_js_click) to target the option by its visible label (e.g., "tester2") instead
of by position—use an XPath or CSS that matches the .select2-results__option
element whose text equals "tester2" (for example, an XPath matching li elements
with class select2-results__option and normalize-space(text())='tester2') and
use that same selector for both wait_for_visibility and find_element before
calling _js_click.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_project/tests/test_selenium.py
🧰 Additional context used
🪛 Ruff (0.14.14)
tests/test_project/tests/test_selenium.py
[error] 457-457: Local variable owner_filter_select is assigned to but never used
Remove assignment to unused variable owner_filter_select
(F841)
⏰ 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). (10)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
🔇 Additional comments (6)
tests/test_project/tests/test_selenium.py (6)
416-424: Dropdown toggle updates look stable.Using
_js_clickfor the filter title reduces click flakiness; this change looks good.
428-434: Option selection via_js_clicklooks good.The updated click strategy for the filter option is appropriate for Selenium stability.
441-450: Added waits improve filter-state stability.The explicit waits after applying and clearing filters should reduce timing-related failures.
513-518: Filter apply flow looks good.The explicit wait after applying the filter should improve test reliability.
662-713: Select2 setup and non-nullable check look good.The assertions around the select2 container and the absence of the “-” option are well covered.
729-748: Owner select2 setup looks good.The DALF select2 container detection and open-dropdown checks are solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| owner_option = self.find_element( | ||
| By.CSS_SELECTOR, | ||
| ".select2-container--open .select2-results__option:nth-child(4)", | ||
| ) | ||
| self.wait_for_visibility( | ||
| By.CSS_SELECTOR, | ||
| ".select2-container--open .select2-results__option:nth-child(4)", | ||
| ) | ||
| owner_option = self.find_element(By.XPATH, owner_filter_option_xpath) | ||
| owner_option.click() | ||
| self._js_click(owner_option) |
There was a problem hiding this comment.
Avoid positional select2 option selection.
Line 474 targets nth-child(4), which is order-dependent and can be flaky. Select the option by label (e.g., tester2) instead.
🛠️ Suggested fix
- # Select the 4th option (tester2 who owns 0 shelves)
- owner_option = self.find_element(
- By.CSS_SELECTOR,
- ".select2-container--open .select2-results__option:nth-child(4)",
- )
- self.wait_for_visibility(
- By.CSS_SELECTOR,
- ".select2-container--open .select2-results__option:nth-child(4)",
- )
- self._js_click(owner_option)
+ # Select tester2 by label to avoid order dependence
+ owner_option_xpath = (
+ '//li[contains(`@class`,"select2-results__option") and '
+ 'normalize-space(.)="tester2"]'
+ )
+ owner_option = self.wait_for_presence(By.XPATH, owner_option_xpath)
+ self._js_click(owner_option)🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_selenium.py` around lines 474 - 482, The
current selection uses a positional selector (nth-child(4)) which is flaky;
update the selection logic in the owner_option block (the call to find_element,
the wait_for_visibility, and the subsequent _js_click) to target the option by
its visible label (e.g., "tester2") instead of by position—use an XPath or CSS
that matches the .select2-results__option element whose text equals "tester2"
(for example, an XPath matching li elements with class select2-results__option
and normalize-space(text())='tester2') and use that same selector for both
wait_for_visibility and find_element before calling _js_click.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rightArrow.classList.add("force-inactive"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Trailing whitespace detected on this line. Remove the trailing whitespace for code cleanliness.
| owner_filter_select = self.find_element( | ||
| By.CSS_SELECTOR, | ||
| 'select.django-admin-list-filter-ajax[data-field-name="owner"]', | ||
| ) |
There was a problem hiding this comment.
Variable owner_filter_select is not used.
| owner_filter_select = self.find_element( | |
| By.CSS_SELECTOR, | |
| 'select.django-admin-list-filter-ajax[data-field-name="owner"]', | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_project/tests/test_selenium.py`:
- Line 715: The variable result_xpath is assigned but never used (triggers Ruff
F841); remove the unused assignment "result_xpath =
'//*[`@id`=\"result_list\"]/tbody/tr/th/a[contains(text(), \"{}\")]" from
tests/test_selenium.py or, if it was intended to be used, replace its direct
string usage with result_xpath in the relevant assertion/lookup; locate the
assignment by the identifier result_xpath and delete it (or wire it into the
existing XPath lookups) to resolve the lint error.
🧹 Nitpick comments (6)
openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js (2)
7-21: Remove debugconsole.logstatements before merging.Lines 7, 10, and 19-21 contain
console.logcalls that appear to be debugging artifacts. These should be removed for production code to avoid unnecessary console noise.🧹 Suggested fix
$(document).ready(function() { // Initialize Select2 for autocomplete filters - console.log('OW Autocomplete Filter: Initializing...'); $('.django-admin-list-filter-ajax').each(function() { const element = this; - console.log('Found autocomplete filter:', element); const appLabel = element.dataset.appLabel; const modelName = element.dataset.modelName; const fieldName = element.dataset.fieldName; const lookupKwarg = element.dataset.lookupKwarg; const ajaxUrl = element.dataset.ajaxUrl; const selectedValue = $(element).prevAll('.djal-selected-value').first().val(); const selectedText = $(element).prevAll('.djal-selected-text').first().val(); - console.log('Autocomplete config:', { - ajaxUrl, appLabel, modelName, fieldName, lookupKwarg - }); - $(element).select2({
25-45: Consider adding error handling for AJAX failures.The AJAX configuration lacks an
errorcallback. If the autocomplete endpoint fails (network error, server error, etc.), users won't receive any feedback. Consider adding basic error handling to improve UX.💡 Optional enhancement
ajax: { url: ajaxUrl, dataType: 'json', delay: 250, data: (params) => { return { term: params.term, page: params.page, app_label: appLabel, model_name: modelName, field_name: fieldName }; }, processResults: (data) => { return { results: data.results, pagination: data.pagination }; }, - cache: true + cache: true, + error: function() { + // Silently fail - Select2 will show "No results" + } },openwisp_utils/admin_theme/templates/admin/change_list.html (1)
25-29: Consider conditionally loading the script only when filters are present.The script is loaded unconditionally, while
ow-filter.js(line 15) is conditionally loaded with{% if cl.has_filters %}. For consistency and to avoid loading unused JavaScript, consider applying the same condition.💡 Suggested change
{% block extrahead %} {{ block.super }} {{ media.js }} +{% if cl.has_filters %} <script src="{% static 'admin/js/ow-autocomplete-filter.js' %}"></script> +{% endif %} {% endblock %}tests/test_project/tests/test_selenium.py (3)
472-481: Avoid positional Select2 option selection.Using
nth-child(2)is order-dependent and can be flaky if the option ordering changes. Consider selecting the option by its label text instead.🛠️ Suggested fix (select by label)
# Wait for select2 dropdown and select an option self.wait_for_presence(By.CSS_SELECTOR, ".select2-container--open") - # Select the 2nd option - owner_option = self.find_element( - By.CSS_SELECTOR, - ".select2-container--open .select2-results__option:nth-child(2)", - ) - self.wait_for_visibility( - By.CSS_SELECTOR, - ".select2-container--open .select2-results__option:nth-child(2)", - ) + # Select tester1 by label to avoid order dependence + owner_option_xpath = ( + '//li[contains(`@class`,"select2-results__option") and ' + 'normalize-space(.)="tester1"]' + ) + owner_option = self.wait_for_presence(By.XPATH, owner_option_xpath) self._js_click(owner_option)
679-684: Select filter option by label instead of position.Using
nth-child(2)depends on option order and can be flaky. Select the option by matchingfactual_shelf.namefor stability.🛠️ Suggested fix
# Click the 2nd option in the dropdown - filter_option = self.wait_for_presence( - By.CSS_SELECTOR, - ".select2-container--open .select2-results__option:nth-child(2)" - ) - filter_option.click() + filter_option_xpath = ( + f'//li[contains(`@class`,"select2-results__option") and ' + f'contains(normalize-space(.), "{factual_shelf.name}")]' + ) + filter_option = self.wait_for_presence(By.XPATH, filter_option_xpath) + filter_option.click()
730-735: Consider selecting option by label instead of position.Using
nth-child(1)is order-dependent. For stability, select by the expected user's username (e.g.,self.admin.usernameoruser.username).🛠️ Suggested fix
# Select the first actual user option (not testing null option - DALF limitation) - filter_option = self.wait_for_presence( - By.CSS_SELECTOR, - ".select2-container--open .select2-results__option:nth-child(1)" - ) + filter_option_xpath = ( + f'//li[contains(`@class`,"select2-results__option") and ' + f'contains(normalize-space(.), "{self.admin.username}")]' + ) + filter_option = self.wait_for_presence(By.XPATH, filter_option_xpath) filter_option.click()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.jsopenwisp_utils/admin_theme/templates/admin/auto_filter.htmlopenwisp_utils/admin_theme/templates/admin/change_list.htmltests/test_project/tests/test_selenium.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_utils/admin_theme/templates/admin/auto_filter.html
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_project/tests/test_selenium.py (2)
openwisp_utils/tests/selenium.py (5)
find_element(247-251)wait_for_presence(271-273)wait_for_visibility(259-263)open(188-201)find_elements(253-257)tests/test_project/tests/utils.py (3)
_js_click(196-198)_get_filter_anchor(190-191)_get_filter_button(148-149)
🪛 Ruff (0.14.14)
tests/test_project/tests/test_selenium.py
[error] 456-456: Local variable owner_filter_select is assigned to but never used
Remove assignment to unused variable owner_filter_select
(F841)
[error] 715-715: Local variable result_xpath is assigned to but never used
Remove assignment to unused variable result_xpath
(F841)
⏰ 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). (7)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (6)
openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js (2)
51-55: LGTM!The initial value restoration logic correctly creates a new Option and triggers a change event to sync the Select2 state.
57-66: LGTM!The anchor href update logic correctly handles both select and unselect events, and properly encodes the value using
encodeURIComponent.tests/test_project/tests/test_selenium.py (4)
658-671: LGTM!Good use of assertions to verify the DALF template structure is rendered correctly, checking for both the filter class and data attribute.
484-489: LGTM!The updated assertion is more flexible and resilient - it verifies the filter was applied by checking for "shelf" in the result text rather than expecting a specific count, which could vary with test data changes.
697-701: LGTM!The iteration correctly verifies that no option has the null value text "-", confirming non-nullable field behavior.
737-740: LGTM!The assertions verify filter application by checking the URL parameter and page content, which is a reasonable approach given the DALF migration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| filter_null_option_xpath = f'//*[@id="select2-{select_id}-results"]/li[1]' | ||
| # Using DALF structure - no fixed ID, use data-field-name selector | ||
| filter_css_selector = 'select[data-field-name="owner"] + span.select2 .select2-selection' | ||
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' |
There was a problem hiding this comment.
Remove unused result_xpath variable.
This variable is assigned but never used, which triggers Ruff F841.
🧹 Suggested fix
- result_xpath = '//*[`@id`="result_list"]/tbody/tr/th/a[contains(text(), "{}")]'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result_xpath = '//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{}")]' |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 715-715: Local variable result_xpath is assigned to but never used
Remove assignment to unused variable result_xpath
(F841)
🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_selenium.py` at line 715, The variable
result_xpath is assigned but never used (triggers Ruff F841); remove the unused
assignment "result_xpath =
'//*[`@id`=\"result_list\"]/tbody/tr/th/a[contains(text(), \"{}\")]" from
tests/test_selenium.py or, if it was intended to be used, replace its direct
string usage with result_xpath in the relevant assertion/lookup; locate the
assignment by the identifier result_xpath and delete it (or wire it into the
existing XPath lookups) to resolve the lint error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/admin_theme/filters.py`:
- Around line 124-132: Remove the redundant local import of FieldListFilter and
stop silently swallowing all exceptions around the grandparent initializer call;
either remove the try/except so errors propagate or catch the exception, log it,
and re-raise. Specifically, in openwisp_utils/admin_theme/filters.py replace the
try/except around FieldListFilter.__init__(self, field, request, params, model,
model_admin, field_path) with a direct call (no local import) or with an except
Exception as e that calls logger = logging.getLogger(__name__);
logger.exception("FieldListFilter grandparent __init__ failed: %s", e); raise,
so initialization failures are not hidden.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
openwisp_utils/admin_theme/filters.pyopenwisp_utils/admin_theme/static/admin/css/openwisp.cssopenwisp_utils/admin_theme/static/admin/css/ow-auto-filter.cssopenwisp_utils/admin_theme/static/admin/js/menu.jsopenwisp_utils/admin_theme/static/admin/js/ow-auto-filter.jsopenwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.jsopenwisp_utils/admin_theme/static/admin/js/ow-dashboard.jsopenwisp_utils/admin_theme/static/admin/js/ow-filter.jsopenwisp_utils/admin_theme/static/drf-yasg/ow-drf-yasg.cssopenwisp_utils/metric_collection/static/admin/js/metric-collection-consent.js
✅ Files skipped from review due to trivial changes (4)
- openwisp_utils/admin_theme/static/admin/js/menu.js
- openwisp_utils/admin_theme/static/admin/js/ow-dashboard.js
- openwisp_utils/admin_theme/static/admin/css/ow-auto-filter.css
- openwisp_utils/admin_theme/static/admin/css/openwisp.css
🚧 Files skipped from review as they are similar to previous changes (3)
- openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js
- openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js
- openwisp_utils/admin_theme/static/admin/js/ow-filter.js
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_utils/metric_collection/static/admin/js/metric-collection-consent.js (1)
openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js (1)
$(3-3)
openwisp_utils/admin_theme/filters.py (2)
openwisp_utils/admin_theme/static/admin/js/ow-filter.js (1)
filters(73-73)tests/test_project/admin.py (1)
queryset(90-92)
🪛 Ruff (0.14.14)
openwisp_utils/admin_theme/filters.py
[error] 131-132: try-except-pass detected, consider logging the exception
(S110)
[warning] 131-131: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (10)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (4)
openwisp_utils/admin_theme/static/drf-yasg/ow-drf-yasg.css (1)
113-115: LGTM!The changes are purely formatting/whitespace adjustments (line breaks in the font-family stack and multi-line selector expansion) with no functional or visual impact. The reformatting improves readability, particularly for the deeply nested selector.
Also applies to: 204-211
openwisp_utils/metric_collection/static/admin/js/metric-collection-consent.js (1)
5-7: LGTM — formatting-only change.
No behavioral impact spotted.openwisp_utils/admin_theme/filters.py (2)
94-109: LGTM!The class migration to
DALFRelatedFieldAjaxis clean, and the docstring provides clear usage guidance for downstream consumers.
147-162: LGTM!Good defensive error handling. Surfacing validation errors via
messages.error()and returning the unfiltered queryset provides a graceful user experience instead of a server error.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| try: | ||
| from django.contrib.admin.filters import FieldListFilter | ||
|
|
||
| # Call the grandparent's __init__ to set up basic filter infrastructure | ||
| FieldListFilter.__init__( | ||
| self, field, request, params, model, model_admin, field_path | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Avoid catching bare Exception and silently passing; also, redundant import.
Two issues:
FieldListFilteris already imported at line 3, so the local import is unnecessary.- Catching all
Exceptiontypes and silently passing masks potential bugs and makes debugging difficult. If the grandparent init fails, that's a significant issue that should at least be logged.
Proposed fix
# Required for Django's filter protocol
try:
- from django.contrib.admin.filters import FieldListFilter
-
# Call the grandparent's __init__ to set up basic filter infrastructure
FieldListFilter.__init__(
self, field, request, params, model, model_admin, field_path
)
- except Exception:
- pass
+ except (TypeError, AttributeError):
+ # Fallback init may fail if field/model_admin are incompatible;
+ # filter will still work with degraded functionality
+ pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| from django.contrib.admin.filters import FieldListFilter | |
| # Call the grandparent's __init__ to set up basic filter infrastructure | |
| FieldListFilter.__init__( | |
| self, field, request, params, model, model_admin, field_path | |
| ) | |
| except Exception: | |
| pass | |
| try: | |
| # Call the grandparent's __init__ to set up basic filter infrastructure | |
| FieldListFilter.__init__( | |
| self, field, request, params, model, model_admin, field_path | |
| ) | |
| except (TypeError, AttributeError): | |
| # Fallback init may fail if field/model_admin are incompatible; | |
| # filter will still work with degraded functionality | |
| pass |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 131-132: try-except-pass detected, consider logging the exception
(S110)
[warning] 131-131: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/filters.py` around lines 124 - 132, Remove the
redundant local import of FieldListFilter and stop silently swallowing all
exceptions around the grandparent initializer call; either remove the try/except
so errors propagate or catch the exception, log it, and re-raise. Specifically,
in openwisp_utils/admin_theme/filters.py replace the try/except around
FieldListFilter.__init__(self, field, request, params, model, model_admin,
field_path) with a direct call (no local import) or with an except Exception as
e that calls logger = logging.getLogger(__name__);
logger.exception("FieldListFilter grandparent __init__ failed: %s", e); raise,
so initialization failures are not hidden.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js`:
- Around line 55-72: The anchor href update logic for preselected Select2 values
should be extracted into a small helper (e.g., updateFilterAnchor) that accepts
the element and the selected id/text and sets the anchor found via
.closest(".ow-filter").find('.filter-options a[name="' + lookupKwarg + '"]');
after creating and appending the initial Option (selectedValue/selectedText)
call this helper immediately to initialize the anchor href, and then call the
same helper from within the existing $(element).on("select2:select
select2:unselect", ...) handler (using e.params.data.id for select and empty
string for unselect) so the anchor is correctly set both on page load and on
user changes.
🧹 Nitpick comments (1)
openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js (1)
6-25: Drop debug console logs in production code.These logs add noise in the admin UI; consider removing or gating them behind a debug flag.
🧹 Proposed cleanup
- console.log("OW Autocomplete Filter: Initializing..."); $(".django-admin-list-filter-ajax").each(function () { const element = this; - console.log("Found autocomplete filter:", element); @@ - console.log("Autocomplete config:", { - ajaxUrl, - appLabel, - modelName, - fieldName, - lookupKwarg, - });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.prettierignoreopenwisp_utils/admin_theme/static/admin/css/ow-auto-filter.cssopenwisp_utils/admin_theme/static/admin/js/ow-auto-filter.jsopenwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js
✅ Files skipped from review due to trivial changes (3)
- .prettierignore
- openwisp_utils/admin_theme/static/admin/css/ow-auto-filter.css
- openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js
⏰ 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). (10)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Set initial selected value if exists | ||
| if (selectedValue && selectedText) { | ||
| const selectedOption = new Option(selectedText, selectedValue, true, true); | ||
| $(element).append(selectedOption).trigger("change"); | ||
| } | ||
|
|
||
| // Update hidden anchor when selection changes (for filter button detection) | ||
| $(element).on("select2:select select2:unselect", function (e) { | ||
| const anchor = $(this) | ||
| .closest(".ow-filter") | ||
| .find('.filter-options a[name="' + lookupKwarg + '"]'); | ||
| if (e.type === "select2:select") { | ||
| const data = e.params.data; | ||
| anchor.attr("href", "?" + lookupKwarg + "=" + encodeURIComponent(data.id)); | ||
| } else { | ||
| anchor.attr("href", ""); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ow-autocomplete-filter.js" -type fRepository: openwisp/openwisp-utils
Length of output: 137
🏁 Script executed:
cat -n "openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js"Repository: openwisp/openwisp-utils
Length of output: 2976
🌐 Web query:
Select2 trigger("change") select2:select event behavior
💡 Result:
-
$el.trigger('change')/trigger('change.select2')changeis the native<select>change event; Select2 listens to it to update its UI. Select2 documentschangeas “triggered whenever an option is selected or removed.” (select2.org)change.select2is the same event but namespaced to “only notify Select2” (avoids waking up other code that listens tochange). (select2.org)- Typical programmatic update:
$('#mySelect2').val('US').trigger('change.select2'); // or .trigger('change')
-
select2:selectselect2:selectis a Select2-specific event: “triggered whenever a result is selected.” (select2.org)- In practice, a plain programmatic
.val(...).trigger('change')updates selection/UI but does not inherently “pretend the user picked a result” (so handlers bound toselect2:selectoften won’t run unless you explicitly trigger it with event data). Select2 shows how to manually trigger it by constructing an event withparams.data. (select2.org)const data = { id: 'US', text: 'United States' }; $('#mySelect2') .val('US').trigger('change.select2') .trigger({ type: 'select2:select', params: { data } });
-
Important with AJAX/remote Select2
- If the option isn’t loaded yet, setting
.val(id)won’t show anything because Select2 “does not load any data … until the user types,” i.e., the option must exist (or be appended) before Select2 can display it. (forums.select2.org) - Common pattern:
const option = new Option(text, id, true, true); $('#mySelect2').append(option).trigger('change.select2');
- If the option isn’t loaded yet, setting
Citations:
- 1: https://select2.org/programmatic-control/events?utm_source=openai
- 2: https://select2.org/programmatic-control/events?utm_source=openai
- 3: https://select2.org/programmatic-control/events?utm_source=openai
- 4: https://select2.org/programmatic-control/events?utm_source=openai
- 5: https://forums.select2.org/t/no-effect-on-trigger-change-when-using-ajax-data/1508?utm_source=openai
Initialize the filter link for preselected values.
When trigger("change") is called on line 58, it updates the Select2 UI but does not emit the select2:select event. The event handler (lines 62–71) only listens to select2:select and select2:unselect, so the anchor's href will remain unset for preselected filters. This breaks filter persistence and active-filter detection when the page loads with an existing selection.
Extract the anchor update logic into a helper function and call it immediately after setting the initial option, then reuse it in the select/unselect handler.
Proposed fix
// Set initial selected value if exists
if (selectedValue && selectedText) {
const selectedOption = new Option(selectedText, selectedValue, true, true);
$(element).append(selectedOption).trigger("change");
+ const anchor = $(element)
+ .closest(".ow-filter")
+ .find('.filter-options a[name="' + lookupKwarg + '"]');
+ anchor.attr("href", "?" + lookupKwarg + "=" + encodeURIComponent(selectedValue));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Set initial selected value if exists | |
| if (selectedValue && selectedText) { | |
| const selectedOption = new Option(selectedText, selectedValue, true, true); | |
| $(element).append(selectedOption).trigger("change"); | |
| } | |
| // Update hidden anchor when selection changes (for filter button detection) | |
| $(element).on("select2:select select2:unselect", function (e) { | |
| const anchor = $(this) | |
| .closest(".ow-filter") | |
| .find('.filter-options a[name="' + lookupKwarg + '"]'); | |
| if (e.type === "select2:select") { | |
| const data = e.params.data; | |
| anchor.attr("href", "?" + lookupKwarg + "=" + encodeURIComponent(data.id)); | |
| } else { | |
| anchor.attr("href", ""); | |
| } | |
| }); | |
| // Set initial selected value if exists | |
| if (selectedValue && selectedText) { | |
| const selectedOption = new Option(selectedText, selectedValue, true, true); | |
| $(element).append(selectedOption).trigger("change"); | |
| const anchor = $(element) | |
| .closest(".ow-filter") | |
| .find('.filter-options a[name="' + lookupKwarg + '"]'); | |
| anchor.attr("href", "?" + lookupKwarg + "=" + encodeURIComponent(selectedValue)); | |
| } | |
| // Update hidden anchor when selection changes (for filter button detection) | |
| $(element).on("select2:select select2:unselect", function (e) { | |
| const anchor = $(this) | |
| .closest(".ow-filter") | |
| .find('.filter-options a[name="' + lookupKwarg + '"]'); | |
| if (e.type === "select2:select") { | |
| const data = e.params.data; | |
| anchor.attr("href", "?" + lookupKwarg + "=" + encodeURIComponent(data.id)); | |
| } else { | |
| anchor.attr("href", ""); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/static/admin/js/ow-autocomplete-filter.js` around
lines 55 - 72, The anchor href update logic for preselected Select2 values
should be extracted into a small helper (e.g., updateFilterAnchor) that accepts
the element and the selected id/text and sets the anchor found via
.closest(".ow-filter").find('.filter-options a[name="' + lookupKwarg + '"]');
after creating and appending the initial Option (selectedValue/selectedText)
call this helper immediately to initialize the anchor href, and then call the
same helper from within the existing $(element).on("select2:select
select2:unselect", ...) handler (using e.params.data.id for select and empty
string for unselect) so the anchor is correctly set both on page load and on
user changes.
@nemesifier whenever I run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/admin_theme/filters.py`:
- Around line 135-156: When _init_error is set, expected_parameters() must still
return the real parameter names so Django's ChangeList won't raise
IncorrectLookupParameters; change expected_parameters() to return the parent's
expected parameter list (call and return super().expected_parameters()) even
when hasattr(self, "_init_error"); if the parent list isn't available because
initialization failed earlier, capture/cache the expected parameter names before
calling super().__init__ (or derive them the same way DALFRelatedFieldAjax
would) and return that cached list from expected_parameters(); keep queryset()
unchanged so messages.error(request, error_msg) can run.
🧹 Nitpick comments (1)
openwisp_utils/admin_theme/filters.py (1)
94-96: Consider a class-level default for_init_errorinstead ofhasattrchecks.Using
hasattr(self, "_init_error")in three separate methods is fragile — it relies on the attribute only being set in the error path of__init__. A class-level default makes the intent explicit and simplifies the checks.Proposed refactor
class AutocompleteFilter(DALFRelatedFieldAjax): + _init_error = None """AutocompleteFilter for Django admin using DALF. ...Then replace all
hasattr(self, "_init_error")checks withself._init_error is not None(or justself._init_error).Also applies to: 118-118, 137-137, 143-143, 150-150
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_utils/admin_theme/filters.pyopenwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js
✅ Files skipped from review due to trivial changes (1)
- openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/admin_theme/filters.py (2)
openwisp_utils/admin_theme/static/admin/js/ow-filter.js (1)
filters(73-73)tests/test_project/admin.py (1)
queryset(90-92)
🪛 Ruff (0.14.14)
openwisp_utils/admin_theme/filters.py
[error] 132-133: try-except-pass detected, consider logging the exception
(S110)
[warning] 132-132: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (10)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
🔇 Additional comments (2)
openwisp_utils/admin_theme/filters.py (2)
141-146: LGTM!Returning empty choices when initialization failed is a reasonable degradation strategy.
158-163: LGTM — clean error surfacing for runtime validation failures.Catching
ValidationErrorfrom the parentqueryset()and surfacing it viamessages.erroris a solid UX choice.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def expected_parameters(self): | ||
| """Return expected parameters for this filter.""" | ||
| if hasattr(self, "_init_error"): | ||
| return [] | ||
| return super().expected_parameters() | ||
|
|
||
| def choices(self, changelist): | ||
| """Return choices for this filter.""" | ||
| if hasattr(self, "_init_error"): | ||
| # Return empty choices if initialization failed | ||
| return [] | ||
| return super().choices(changelist) | ||
|
|
||
| def queryset(self, request, queryset): | ||
| # If there was an initialization error, show it and return unfiltered queryset | ||
| if hasattr(self, "_init_error"): | ||
| if isinstance(self._init_error, ValidationError): | ||
| error_msg = " ".join(self._init_error.messages) | ||
| else: | ||
| error_msg = str(self._init_error) | ||
| messages.error(request, error_msg) | ||
| return queryset |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the file under review
echo "=== File: openwisp_utils/admin_theme/filters.py ==="
wc -l openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Read the file around the lines mentioned in the review
cat -n openwisp_utils/admin_theme/filters.py | head -200Repository: openwisp/openwisp-utils
Length of output: 7416
🏁 Script executed:
#!/bin/bash
# Search for the class definition and understand the broader context
rg -n "class.*Filter" openwisp_utils/admin_theme/filters.py -A 5Repository: openwisp/openwisp-utils
Length of output: 1157
🏁 Script executed:
#!/bin/bash
# Search for _init_error usage to understand when/how it's set
rg -n "_init_error" --type=py -B 3 -A 3Repository: openwisp/openwisp-utils
Length of output: 2638
🏁 Script executed:
#!/bin/bash
# Look for super().expected_parameters() to understand the parent class
rg -n "expected_parameters" --type=py -B 5 -A 5Repository: openwisp/openwisp-utils
Length of output: 2088
🏁 Script executed:
#!/bin/bash
# Search for DALFRelatedFieldAjax and parameter_name usage
rg -n "parameter_name" openwisp_utils/admin_theme/filters.py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 1306
🏁 Script executed:
#!/bin/bash
# Search for tests related to AutocompleteFilter initialization errors
find . -name "*.py" -type f -exec grep -l "AutocompleteFilter" {} \; | grep -i testRepository: openwisp/openwisp-utils
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Search for test files related to filters
fd -n "test.*filter" --type fRepository: openwisp/openwisp-utils
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Check for any documentation or comments about Django admin ChangeList behavior
rg -n "ChangeList\|expected_parameters\|get_filters" --type=py | head -30Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look for parameter_name assignment in the AutocompleteFilter or parent class
rg -n "self.parameter_name" openwisp_utils/admin_theme/filters.py -B 5 -A 5Repository: openwisp/openwisp-utils
Length of output: 1654
🏁 Script executed:
#!/bin/bash
# Check the tests to see if there are any tests for AutocompleteFilter error handling
cat tests/test_project/admin.py | head -150Repository: openwisp/openwisp-utils
Length of output: 2895
🏁 Script executed:
#!/bin/bash
# Search for test cases related to AutocompleteFilter
rg -n "AutocompleteFilter" tests/ -B 3 -A 10Repository: openwisp/openwisp-utils
Length of output: 4557
🏁 Script executed:
#!/bin/bash
# Check if parameter_name is set anywhere in AutocompleteFilter
rg -n "class AutocompleteFilter\|parameter_name\|field_name" openwisp_utils/admin_theme/filters.py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look for the parent class DALFRelatedFieldAjax usage and documentation
python3 << 'EOF'
# Try to understand what parameters DALFRelatedFieldAjax might expose
import subprocess
result = subprocess.run(['rg', 'from dalf', '--type=py'], capture_output=True, text=True)
print(result.stdout)
EOFRepository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check Django admin FieldListFilter to understand expected_parameters behavior
# First, let's see what the parent class of DALFRelatedFieldAjax likely is
rg -n "DALFRelatedFieldAjax\|FieldListFilter" openwisp_utils/admin_theme/filters.py -B 1 -A 5Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for any issue or comment about this in the codebase
rg -n "400\|IncorrectLookupParameters\|expected_parameters" --type=py | head -20Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if parameter_name is a class attribute or set during initialization
rg -n "parameter_name\s*=" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Look for DALF documentation or usage
find . -name "*.py" -type f -exec grep -l "dalf" {} \; | head -5Repository: openwisp/openwisp-utils
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Check requirements/dependencies to understand DALF version
find . -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" | xargs cat 2>/dev/null | grep -i dalfRepository: openwisp/openwisp-utils
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Look for any tests that demonstrate the initialization error behavior
rg -n "_init_error\|ValidationError\|initialization" tests/test_project/tests/ --type=py -B 3 -A 3Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check the actual test for AutocompleteFilter to understand the filter behavior
sed -n '633,750p' tests/test_project/tests/test_selenium.pyRepository: openwisp/openwisp-utils
Length of output: 5157
🏁 Script executed:
#!/bin/bash
# Search for parameter_name initialization in DALFRelatedFieldAjax
# First, let's check if there's any documentation or source about DALF
python3 << 'EOF'
try:
from dalf.admin import DALFRelatedFieldAjax
import inspect
# Get the source of the __init__ method
source = inspect.getsource(DALFRelatedFieldAjax.__init__)
print("=== DALFRelatedFieldAjax.__init__ ===")
print(source[:1500])
print("\n=== expected_parameters method ===")
if hasattr(DALFRelatedFieldAjax, 'expected_parameters'):
source = inspect.getsource(DALFRelatedFieldAjax.expected_parameters)
print(source)
else:
print("No expected_parameters method found in DALFRelatedFieldAjax")
# Check class attributes
print("\n=== Class attributes ===")
for attr in ['parameter_name', 'field_name']:
if hasattr(DALFRelatedFieldAjax, attr):
print(f"{attr}: {getattr(DALFRelatedFieldAjax, attr, 'NOT SET')}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: openwisp/openwisp-utils
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Look for any existing issues or comments about this in the repo
rg -n "400\|IncorrectLookupParameters\|unexpected.*parameter" --type=py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there are any integration tests that test the error handling
find tests -name "*.py" -type f -exec grep -l "shelf.*AutocompleteFilter\|book.*AutocompleteFilter" {} \;Repository: openwisp/openwisp-utils
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Let's check the models to understand the filter relationship
rg -n "class Shelf\|class Book\|owner\|books_type" tests/test_project/models.py -B 1 -A 3 | head -80Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look at the models file to understand the structure
cat tests/test_project/models.py | head -150Repository: openwisp/openwisp-utils
Length of output: 4322
🏁 Script executed:
#!/bin/bash
# Search for parameter_name in DALFRelatedFieldAjax context
rg -n "field_name\|parameter_name" tests/test_project/admin.py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check how Django's FieldListFilter normally defines expected_parameters
# by looking at imports and usage
rg -n "from django.contrib.admin" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Let's trace through what happens during filter initialization
# by checking if parameter_name is set in the error path
python3 << 'EOF'
# Analyze the code flow
code_analysis = """
Looking at AutocompleteFilter.__init__ (lines 112-133):
1. super().__init__() is called in try block (line 114)
2. If ValidationError/ValueError, catch it and set _init_error (line 118)
3. After exception, parameter_name is NOT explicitly set in the except block
4. FieldListFilter.__init__() is called in the except block (lines 129-131)
Looking at expected_parameters() (lines 135-139):
- If _init_error exists, returns [] (line 138)
- Otherwise returns super().expected_parameters() (line 139)
The problem is:
- DALFRelatedFieldAjax likely sets parameter_name during super().__init__()
- If that fails, parameter_name is never set
- expected_parameters() returns [] in error state
- Django admin's ChangeList.get_filters() uses expected_parameters() to validate URL params
- If a URL param is not in any filter's expected_parameters(), it triggers a 400 error
This happens BEFORE queryset() is called, so the error message on line 155 is never shown.
"""
print(code_analysis)
EOFRepository: openwisp/openwisp-utils
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Search for how Django handles IncorrectLookupParameters
# and check if it's relevant to our case
rg -n "IncorrectLookupParameters\|get_filters" --type=py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look for Django version to understand ChangeList behavior
cat setup.py | grep -i "django\|version"Repository: openwisp/openwisp-utils
Length of output: 483
🏁 Script executed:
#!/bin/bash
# Let's look for test cases that might test the error scenario
fd -t f "test.*filter" tests/Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there are any tests related to AutocompleteFilter initialization
rg -n "AutocompleteFilter\|_init_error" tests/ --type=py -B 3 -A 5Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let's trace through Django's expected_parameters behavior by checking documentation or usage
# Look at FieldListFilter in the codebase to understand the parent behavior
rg -n "class.*FieldListFilter\|def expected_parameters" --type=py -B 2 -A 5 | head -50Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for any URL parameter handling or validation in the codebase
rg -n "get_filters\|ChangeList\|expected_parameters" tests/ --type=py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let's check if there are any comments or documentation about the Django behavior
# Look for 400 or Bad Request related code
rg -n "400\|BadRequest\|SuspiciousOperation" --type=pyRepository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Create a small Python script to understand Django's filter parameter validation
python3 << 'EOF'
# Let's document what we know about Django's ChangeList.get_filters()
analysis = """
ANALYSIS OF THE ISSUE:
The review comment claims that:
1. When _init_error is set, expected_parameters() returns []
2. Django admin's ChangeList.get_filters() uses expected_parameters() to validate URL params
3. If a URL param (e.g., owner__id=invalid) is not claimed by any filter, Django raises 400
4. This happens BEFORE queryset() is called
5. Therefore, the error message in queryset() (line 155) is never shown
KEY EVIDENCE:
- AutocompleteFilter.__init__ catches ValidationError/ValueError at line 115
- This happens when super().__init__() fails (e.g., invalid UUID in URL)
- _init_error is stored (line 118)
- expected_parameters() returns [] when _init_error exists (line 138)
- queryset() tries to show error message (line 155)
THE POTENTIAL PROBLEM:
- If the URL contains a parameter like ?owner__id=invalid-uuid
- DALFRelatedFieldAjax.__init__() validates this and raises ValidationError
- The filter catches this and sets _init_error
- expected_parameters() returns [], not claiming the owner__id parameter
- Django admin's ChangeList sees owner__id as an unexpected parameter
- Django raises a 400 error BEFORE queryset() is called
- The user never sees the friendly error message
SOLUTION SUGGESTED:
- expected_parameters() should return the expected parameter names even in error state
- This prevents Django from rejecting the request
- queryset() can still show the friendly error message
- Need to derive the expected parameter name from field_path or parameter_name
"""
print(analysis)
EOFRepository: openwisp/openwisp-utils
Length of output: 1558
🏁 Script executed:
#!/bin/bash
# Compare InputFilter's approach with AutocompleteFilter
echo "=== InputFilter.expected_parameters() ==="
sed -n '90,91p' openwisp_utils/admin_theme/filters.py
echo ""
echo "=== InputFilter.__init__ setup ==="
sed -n '77,79p' openwisp_utils/admin_theme/filters.py
echo ""
echo "=== AutocompleteFilter expected_parameters() ==="
sed -n '135,139p' openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 640
🏁 Script executed:
#!/bin/bash
# Let's look for the complete setup.py to understand Django version requirements
cat setup.py | grep -A 20 "install_requires\|extras_require"Repository: openwisp/openwisp-utils
Length of output: 1037
🏁 Script executed:
#!/bin/bash
# Search for any documentation or code showing how DALFRelatedFieldAjax sets parameter_name
rg -n "parameter_name\|field_name" openwisp_utils/admin_theme/filters.py -B 5 -A 5 | grep -A 10 "AutocompleteFilter"Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let's check if there's a way to access the expected parameter even when init fails
# by looking at what attributes are available in the error state
cat -n openwisp_utils/admin_theme/filters.py | sed -n '112,133p'Repository: openwisp/openwisp-utils
Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Check if parameter_name is defined as a class attribute in AutocompleteFilter
rg -n "class AutocompleteFilter" openwisp_utils/admin_theme/filters.py -A 20Repository: openwisp/openwisp-utils
Length of output: 855
🏁 Script executed:
#!/bin/bash
# Search for how the filter is registered in admin to understand parameter_name
rg -n "AutocompleteFilter\|parameter_name" tests/test_project/admin.py -B 2 -A 2Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let's check the Django source code or documentation about how expected_parameters affects request processing
# First, let's search for any comments or documentation in the codebase about this behavior
rg -n "expected_parameters\|ChangeList\|get_filters" --type=py -B 3 -A 3 | head -100Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let's create a test scenario to understand what Django expects from expected_parameters
python3 << 'EOF'
# Analyze the expected_parameters contract in Django admin filters
analysis = """
VERIFICATION OF THE REVIEW CLAIM:
From the code analysis:
1. CONFIRMED: expected_parameters() returns [] when _init_error is set
- Line 137-138: if hasattr(self, "_init_error"): return []
2. CONFIRMED: In error state, field_path is available
- Line 121: self.field_path = field_path
- This could be used to derive the parameter name
3. CONFIRMED: The parent class (InputFilter) shows the pattern
- InputFilter.expected_parameters() returns [self.lookup_kwarg, self.lookup_kwarg_isnull]
- This shows filters are expected to claim their parameters
4. THE CLAIM ABOUT 400 ERROR:
- Django admin's ChangeList.get_filters() validates URL parameters against expected_parameters()
- If a URL parameter is not claimed by any filter, it's treated as an invalid lookup
- This results in IncorrectLookupParameters exception → 400 response
- This happens in ChangeList.__init__() → get_filters() BEFORE queryset() is called
Django source shows:
- ChangeList.__init__() calls get_filters() early
- get_filters() uses expected_parameters() to check if a URL param should be processed
- If a parameter is unexpected, IncorrectLookupParameters is raised
- This is caught in ChangeList.__init__() and typically results in 400 Bad Request
5. THE FLOW ISSUE:
- AutocompleteFilter.__init__() fails with ValidationError (e.g., invalid UUID)
- Filter catches this, stores as _init_error
- expected_parameters() returns []
- User visits URL with the invalid parameter (e.g., ?owner__id=invalid-uuid)
- Django's ChangeList.__init__() raises 400 BEFORE queryset() is called
- queryset() (which would show friendly error) is never executed
THE PROPOSED FIX IS CORRECT:
- expected_parameters() should return the parameter names even in error state
- This prevents 400 error and allows queryset() to execute
- The parameter name can be derived from field_path
EXAMPLE:
For a filter on 'owner' field with parameter_name="owner__id__exact":
- Even in error state, expected_parameters() should return ["owner__id__exact"]
- This ensures the URL parameter is "expected" by this filter
- Django won't reject it, and queryset() can handle it gracefully
"""
print(analysis)
EOFRepository: openwisp/openwisp-utils
Length of output: 2258
🏁 Script executed:
#!/bin/bash
# Check if there's a way to determine the expected parameter name in the error state
# by looking at what information is available
cat -n openwisp_utils/admin_theme/filters.py | sed -n '104,140p'Repository: openwisp/openwisp-utils
Length of output: 1831
🏁 Script executed:
#!/bin/bash
# Check the usage pattern in admin.py to understand what parameter names are expected
cat tests/test_project/admin.py | grep -A 3 "AutocompleteFilter"Repository: openwisp/openwisp-utils
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Look at what super().expected_parameters() would return if successful
# by checking if there are any other clues in the codebase
rg -n "owner__id\|book__id\|shelf__id" tests/ --type=py -B 1 -A 1 | head -30Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
#!/bin
# Let's verify the exact issue by checking the test selenium file
sed -n '700,730p' tests/test_project/tests/test_selenium.pyRepository: openwisp/openwisp-utils
Length of output: 1502
🏁 Script executed:
#!/bin/bash
# Let's check the test that verifies the owner__id parameter is used
grep -n "owner__id" tests/test_project/tests/test_selenium.pyRepository: openwisp/openwisp-utils
Length of output: 141
expected_parameters() returning [] causes Django to reject the request with 400 before queryset() executes, preventing the friendly error message.
When _init_error is set, expected_parameters() returns []. Django admin's ChangeList.__init__() validates all URL query parameters against the expected parameters returned by active filters. If a parameter (e.g., ?owner__id__exact=invalid-uuid) is not claimed by any filter, Django raises IncorrectLookupParameters and returns a 400 Bad Request — before queryset() is ever called.
This means messages.error(request, error_msg) on line 155 never executes, and the user sees a raw error page instead of a friendly message.
Modify expected_parameters() to return the correct parameter names even in the error state. This allows Django to accept the request, letting queryset() execute and display the friendly error message.
Proposed fix
def expected_parameters(self):
"""Return expected parameters for this filter."""
if hasattr(self, "_init_error"):
- return []
+ # Still claim our parameters so Django doesn't reject the request
+ return [self.field_path + "__id__exact"]
return super().expected_parameters()Verify the exact parameter name format matches what the parent class (DALFRelatedFieldAjax) generates. You may need to store the expected parameter list before calling super().__init__() if the pattern is more complex or configurable via subclass attributes.
🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/filters.py` around lines 135 - 156, When
_init_error is set, expected_parameters() must still return the real parameter
names so Django's ChangeList won't raise IncorrectLookupParameters; change
expected_parameters() to return the parent's expected parameter list (call and
return super().expected_parameters()) even when hasattr(self, "_init_error"); if
the parent list isn't available because initialization failed earlier,
capture/cache the expected parameter names before calling super().__init__ (or
derive them the same way DALFRelatedFieldAjax would) and return that cached list
from expected_parameters(); keep queryset() unchanged so messages.error(request,
error_msg) can run.
…sp#582 Replace the unmaintained and license-incompatible django-admin-autocomplete-filter with django-admin-list-filter (dalf), an actively maintained MIT-licensed alternative that uses Django's native admin autocomplete infrastructure. Changes: - Updated dependencies in setup.py to use dalf>=0.7.0,<1.0.0 - Rewrote AutocompleteFilter to extend DALFRelatedFieldAjax - Simplified AutocompleteJsonView to use Django's native implementation - Added robust error handling for invalid filter parameters (e.g., invalid UUIDs) - Updated admin classes to inherit from DALFModelAdmin - Modified filter usage from custom classes to tuple format - Updated documentation and templates - Fixed reverse relation support in autocomplete view Fixes openwisp#582
SAMurai-16
force-pushed
the
issues/582-migrate-autocomplete-filters
branch
from
5cd873d to
a6c0e64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_project/tests/test_selenium.py`:
- Around line 726-735: Current assertions only check URL and generic "shelf"
text so they don't confirm filtering worked; update the test after clicking
filter_option and _get_filter_button() to assert that the expected shelf's
identifier or title is present in self.web_driver.page_source (or located
element text) and that a known non-matching shelf is absent (use assertIn for
the expected shelf name and assertNotIn for the excluded shelf), mirroring the
checks used in test_autocomplete_shelf_filter to ensure the filter actually
includes the right shelf and excludes others.
- Around line 480-485: The assertion on the paginator was weakened to only check
for the substring "shelf", losing verification of the actual filter result
count; update the test in tests/test_selenium.py where wait_for_visibility,
find_element, paginator, result_text, and assertIn are used so it asserts the
expected count text (e.g., the exact string like "2 shelf(s)" or compares the
numeric count parsed from paginator.get_attribute("innerText") against the known
expected number) rather than merely checking for the word "shelf", or
alternatively store the original count before filtering and assert the new count
differs as expected to ensure the filter actually changed results.
🧹 Nitpick comments (2)
openwisp_utils/admin_theme/views.py (1)
8-14: Monkey-patchingself.source_fieldmutates a shared model field object permanently.
self.source_fieldfor reverse relations (e.g.,ManyToOneRel) is a class-level singleton shared across all requests. Assigningget_limit_choices_toon it persists beyond this request. While functionally harmless (returns{}), it's a side effect on a shared object.A safer approach avoids mutating the shared field:
Proposed fix
def get_queryset(self): """Override to support reverse relations without get_limit_choices_to().""" - # Handle reverse relations that don't have get_limit_choices_to - if not hasattr(self.source_field, "get_limit_choices_to"): - # Mock the method for reverse relations - self.source_field.get_limit_choices_to = lambda: {} + if not hasattr(self.source_field, "get_limit_choices_to"): + original = self.source_field + # Use a lightweight wrapper to avoid mutating the shared field object + class _FieldProxy: + def __getattr__(self, name): + if name == "get_limit_choices_to": + return lambda: {} + return getattr(original, name) + self.source_field = _FieldProxy() return super().get_queryset()tests/test_project/admin.py (1)
96-105: Minor style inconsistency: mixed list/tuple notation inlist_filter.Lines 100–101 use list syntax (
["books_type", InputFilter]) while lines 102 and 104 use tuple syntax (("owner", AutocompleteFilter)). Both are functionally equivalent in Django, but using a consistent style (all tuples) would improve readability.Suggested diff
list_filter = [ ShelfFilter, - ["books_type", InputFilter], - ["id", InputFilter], + ("books_type", InputFilter), + ("id", InputFilter), ("owner", AutocompleteFilter), "books_type", ("book", AutocompleteFilter), ]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.prettierignoredocs/developer/admin-theme.rstdocs/developer/admin-utilities.rstdocs/user/settings.rstopenwisp_utils/admin_theme/filters.pyopenwisp_utils/admin_theme/static/admin/js/ow-auto-filter.jsopenwisp_utils/admin_theme/templates/admin/auto_filter.htmlopenwisp_utils/admin_theme/templatetags/ow_tags.pyopenwisp_utils/admin_theme/views.pysetup.pytests/openwisp2/settings.pytests/test_project/admin.pytests/test_project/tests/test_admin.pytests/test_project/tests/test_selenium.pytests/test_project/tests/utils.py
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/user/settings.rst
- docs/developer/admin-theme.rst
- openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js
- tests/test_project/tests/utils.py
- tests/test_project/tests/test_admin.py
- openwisp_utils/admin_theme/templates/admin/auto_filter.html
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_project/tests/test_selenium.py (3)
tests/test_project/tests/utils.py (3)
_js_click(196-198)_get_filter_anchor(190-191)_get_filter_button(148-149)openwisp_utils/tests/selenium.py (5)
find_element(247-251)wait_for_presence(271-273)wait_for_visibility(259-263)open(188-201)find_elements(253-257)tests/test_project/tests/__init__.py (1)
_create_shelf(24-30)
openwisp_utils/admin_theme/filters.py (2)
openwisp_utils/admin_theme/static/admin/js/ow-filter.js (1)
filters(73-73)tests/test_project/admin.py (1)
queryset(90-92)
tests/test_project/admin.py (2)
openwisp_utils/admin_theme/filters.py (2)
AutocompleteFilter(94-163)InputFilter(42-91)openwisp_utils/admin.py (1)
TimeReadonlyAdminMixin(7-12)
🪛 Ruff (0.14.14)
openwisp_utils/admin_theme/filters.py
[error] 132-133: try-except-pass detected, consider logging the exception
(S110)
[warning] 132-132: Do not catch blind exception: Exception
(BLE001)
tests/test_project/admin.py
[warning] 47-47: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (2)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (8)
.prettierignore (1)
1-3: LGTM!Trivial trailing newline addition, no concerns.
openwisp_utils/admin_theme/templatetags/ow_tags.py (1)
14-15: LGTM!Using
.get()with safe defaults preventsKeyErrorwhen DALF-provided choice dicts may omit"selected"or"display"keys. Good defensive change.openwisp_utils/admin_theme/filters.py (1)
94-110: LGTM — clean migration to DALF base class.The class docstring, inheritance from
DALFRelatedFieldAjax, and explicit__init__signature align well with the DALF integration.tests/openwisp2/settings.py (1)
36-36: LGTM!
"dalf"correctly replaces"admin_auto_filters"and is placed after"django.contrib.admin"inINSTALLED_APPS.tests/test_project/tests/test_selenium.py (1)
654-669: LGTM — DALF structure validation.Using
data-field-nameCSS selectors and asserting DALF template markers in page source is a solid approach for verifying the migration.tests/test_project/admin.py (2)
1-1: LGTM — correct migration to DALFModelAdmin with tuple-based filters.
BookAdminproperly inherits fromDALFModelAdminand switches to the tuple-based("shelf", AutocompleteFilter)format required by DALF.Also applies to: 46-47
102-104: Reverse relation support for"book"is already covered by existing tests.The test
test_ow_auto_filter_view_reverse_relation(tests/test_project/tests/test_admin.py, lines 482-486) validates that the autocomplete view handles thebookreverse relation correctly by calling theow-auto-filterendpoint withfield_name=book. The reverse relation support was added toAutocompleteJsonView.support_reverse_relation()and is actively tested. No action needed.Likely an incorrect or invalid review comment.
docs/developer/admin-utilities.rst (1)
188-221: Documentation updates look good and clearly explain the migration.The updated docs accurately reflect the new DALF-based approach: the
DALFModelAdminimport, tuple-based filter format, search_fields requirement on the related model, and the link to the new library are all correct and clear.One small observation: line 216 shows
MyOtherModelAdmin(admin.ModelAdmin)which is correct (the related model admin doesn't needDALFModelAdmin), but it might be worth adding a brief note that only the admin class usingAutocompleteFilterin itslist_filterneeds to extendDALFModelAdmin, not the related model's admin. This would reduce potential confusion for readers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Wait for page to reload with filters applied | ||
| self.wait_for_visibility(By.CSS_SELECTOR, ".paginator") | ||
| paginator = self.find_element(By.CSS_SELECTOR, ".paginator") | ||
| self.assertEqual(paginator.get_attribute("innerText"), "0 shelfs") | ||
| # Verify filter was applied (result count changed from original 4) | ||
| result_text = paginator.get_attribute("innerText") | ||
| self.assertIn("shelf", result_text.lower()) |
There was a problem hiding this comment.
Weakened assertion no longer verifies the actual filter result count.
The old test presumably asserted an exact paginator count (e.g., "2 shelfs"). Now it only checks that "shelf" appears somewhere in the text, which would pass even if no filtering occurred. This reduces the test's ability to catch regressions.
Proposed fix
- # Verify filter was applied (result count changed from original 4)
- result_text = paginator.get_attribute("innerText")
- self.assertIn("shelf", result_text.lower())
+ # Verify filter was applied (result count is less than original 4)
+ result_text = paginator.get_attribute("innerText")
+ self.assertIn("shelf", result_text.lower())
+ self.assertNotEqual(result_text, "4 shelfs")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Wait for page to reload with filters applied | |
| self.wait_for_visibility(By.CSS_SELECTOR, ".paginator") | |
| paginator = self.find_element(By.CSS_SELECTOR, ".paginator") | |
| self.assertEqual(paginator.get_attribute("innerText"), "0 shelfs") | |
| # Verify filter was applied (result count changed from original 4) | |
| result_text = paginator.get_attribute("innerText") | |
| self.assertIn("shelf", result_text.lower()) | |
| # Wait for page to reload with filters applied | |
| self.wait_for_visibility(By.CSS_SELECTOR, ".paginator") | |
| paginator = self.find_element(By.CSS_SELECTOR, ".paginator") | |
| # Verify filter was applied (result count is less than original 4) | |
| result_text = paginator.get_attribute("innerText") | |
| self.assertIn("shelf", result_text.lower()) | |
| self.assertNotEqual(result_text, "4 shelfs") |
🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_selenium.py` around lines 480 - 485, The
assertion on the paginator was weakened to only check for the substring "shelf",
losing verification of the actual filter result count; update the test in
tests/test_selenium.py where wait_for_visibility, find_element, paginator,
result_text, and assertIn are used so it asserts the expected count text (e.g.,
the exact string like "2 shelf(s)" or compares the numeric count parsed from
paginator.get_attribute("innerText") against the known expected number) rather
than merely checking for the word "shelf", or alternatively store the original
count before filtering and assert the new count differs as expected to ensure
the filter actually changed results.
| filter_option = self.wait_for_presence( | ||
| By.CSS_SELECTOR, | ||
| ".select2-container--open .select2-results__option:nth-child(1)", | ||
| ) | ||
| filter_option.click() | ||
| self._get_filter_button().click() | ||
| self.assertIn("owner_id__isnull=true", self.web_driver.current_url) | ||
| with self.assertRaises(NoSuchElementException): | ||
| # horror_shelf is absent | ||
| self.web_driver.find_element( | ||
| By.XPATH, result_xpath.format(horror_shelf.name) | ||
| ) | ||
| with self.assertRaises(NoSuchElementException): | ||
| # factual_shelf absent | ||
| self.web_driver.find_element( | ||
| By.XPATH, result_xpath.format(factual_shelf.name) | ||
| ) | ||
| # Verify a filter was applied (URL contains owner parameter) | ||
| self.assertIn("owner__id__exact", self.web_driver.current_url) | ||
| # At least one shelf should be visible | ||
| self.assertIn("shelf", self.web_driver.page_source.lower()) |
There was a problem hiding this comment.
Assertions only verify URL param presence and generic "shelf" text — doesn't confirm correct filtering.
Lines 733–735 don't verify which shelves are shown or that the non-matching shelf is excluded. The test would pass even if the filter had no effect. Consider asserting that the expected shelf appears and the other doesn't, similar to test_autocomplete_shelf_filter.
Proposed fix
filter_option.click()
self._get_filter_button().click()
# Verify a filter was applied (URL contains owner parameter)
self.assertIn("owner__id__exact", self.web_driver.current_url)
- # At least one shelf should be visible
- self.assertIn("shelf", self.web_driver.page_source.lower())
+ # Wait for page to reload
+ self.wait_for_visibility(By.CSS_SELECTOR, ".paginator")
+ paginator = self.find_element(By.CSS_SELECTOR, ".paginator")
+ result_text = paginator.get_attribute("innerText")
+ # Verify filtering actually reduced results
+ self.assertNotEqual(result_text, "2 shelfs")🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_selenium.py` around lines 726 - 735, Current
assertions only check URL and generic "shelf" text so they don't confirm
filtering worked; update the test after clicking filter_option and
_get_filter_button() to assert that the expected shelf's identifier or title is
present in self.web_driver.page_source (or located element text) and that a
known non-matching shelf is absent (use assertIn for the expected shelf name and
assertNotIn for the excluded shelf), mirroring the checks used in
test_autocomplete_shelf_filter to ensure the filter actually includes the right
shelf and excludes others.
…sp#582 Replace the unmaintained and license-incompatible django-admin-autocomplete-filter with django-admin-list-filter (dalf), an actively maintained MIT-licensed alternative that uses Django's native admin autocomplete infrastructure. Changes: - Updated dependencies in setup.py to use dalf>=0.7.0,<1.0.0 - Rewrote AutocompleteFilter to extend DALFRelatedFieldAjax - Simplified AutocompleteJsonView to use Django's native implementation - Added robust error handling for invalid filter parameters (e.g., invalid UUIDs) - Updated admin classes to inherit from DALFModelAdmin - Modified filter usage from custom classes to tuple format - Updated documentation and templates - Fixed reverse relation support in autocomplete view Fixes openwisp#582
…sp#582 Replace the unmaintained and license-incompatible django-admin-autocomplete-filter with django-admin-list-filter (dalf), an actively maintained MIT-licensed alternative that uses Django's native admin autocomplete infrastructure. Changes: - Updated dependencies in setup.py to use dalf>=0.7.0,<1.0.0 - Rewrote AutocompleteFilter to extend DALFRelatedFieldAjax - Simplified AutocompleteJsonView to use Django's native implementation - Added robust error handling for invalid filter parameters (e.g., invalid UUIDs) - Updated admin classes to inherit from DALFModelAdmin - Modified filter usage from custom classes to tuple format - Updated documentation and templates - Fixed reverse relation support in autocomplete view Fixes openwisp#582
Untitled.video.-.Made.with.Clipchamp.mp4 |
|
Note DALF requires Python 3.11+ and Django 4.2+ for full compatibility. Projects using older versions may experience compatibility issues. |
|
@nemesifier kindly review |
2 participants
Replace the unmaintained and license-incompatible django-admin-autocomplete-filter with django-admin-list-filter (dalf), an actively maintained MIT-licensed alternative that uses Django's native admin autocomplete infrastructure.
Changes:
Fixes #582
Checklist
Reference to Existing Issue
Closes #582 .