Migrate AutocompleteFilter to dalf (vigo/django-admin-list-filter)#589
Migrate AutocompleteFilter to dalf (vigo/django-admin-list-filter)#589Siddhazntx wants to merge 2 commits intoopenwisp:masterfrom
Conversation
- Removed deprecated django-admin-autocomplete-filter dependency. - Integrated DALF to ensure compatibility with Django 5.0+. - Implemented custom choices() and queryset() methods in filters.py to maintain OpenWISP template compatibility. - Added TestAutocompleteFilter unit tests to verify backend logic and choice generation. - Updated Media class with correct static paths for the new library. Note: Backend logic is fully verified by unit tests; currently investigating a minor JavaScript initialization conflict with the Foreign Key arrow.
WalkthroughThe pull request migrates the autocomplete filter implementation from the unmaintained Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_utils/admin_theme/static/admin/js/ow-auto-filter.js (1)
35-48:⚠️ Potential issue | 🟠 Major
select2:close,select2:select, andselect2:clearhandlers not extended to.dalf-filter.Lines 32 and 35 correctly extend
setAllPlaceholderandselect2:opento.dalf-filter, but the handlers on Lines 50, 89, and 93 still only bind to.auto-filter. This means.dalf-filterelements will open the dropdown correctly but won't actually apply filters or reset the placeholder on clear/close.This is likely the root cause of the known "minor JS initialization issue" mentioned in the PR description.
Proposed fix
- django.jQuery(".auto-filter").on("select2:close", function () { + django.jQuery(".dalf-filter,.auto-filter").on("select2:close", function () { django.jQuery(".auto-filter-choices").css("min-height", ""); });- django.jQuery(".auto-filter").on("select2:select", function (event) { + django.jQuery(".dalf-filter,.auto-filter").on("select2:select", function (event) { applyFilter(event.target); }); - django.jQuery(".auto-filter").on("select2:clear", function (event) { + django.jQuery(".dalf-filter,.auto-filter").on("select2:clear", function (event) { applyFilter(event.target);
🤖 Fix all issues with AI agents
In `@openwisp_utils/admin_theme/filters.py`:
- Around line 124-133: The try/except in the Filter class __init__ swallows
ValidationError/ValueError leaving the instance half-initialized; remove the
try/except around super().__init__(...) so exceptions propagate (preferred) or,
if you must catch them, set the parent-required attributes (used_parameters,
lookup_kwarg, title, field, etc.) to safe defaults and still set self.ajax_url =
reverse("admin:ow-auto-filter") so that subsequent calls to choices() and
queryset() don't raise AttributeError; update __init__ accordingly to ensure a
consistent object state.
- Around line 142-155: The choices() method in AutocompleteFilter uses
self.parameter_name which may not exist (parent DALFRelatedFieldAjax defines
lookup_kwarg instead), causing AttributeError; update choices() to use
getattr(self, "parameter_name", self.lookup_kwarg) wherever parameter_name is
referenced (e.g., the "selected" check and when calling get_query_string/remove)
and correct the dictionary closing indentation to match
InputFilterMixin.choices(); reference the AutocompleteFilter.choices(),
DALFRelatedFieldAjax, InputFilterMixin, and lookup_kwarg symbols when making the
change.
In `@openwisp_utils/admin_theme/static/admin/css/ow-auto-filter.css`:
- Around line 36-38: The CSS selector uses .is-open but the JS
(showFilterOptions/hideFilterOptions in ow-filter.js) toggles .ow-active, so the
arrow rotation never fires; update the CSS rules for the arrow (currently
targeting .auto-filter.is-open and .dalf-filter.is-open) to target .ow-active
instead, or alternatively update the showFilterOptions/hideFilterOptions
functions to toggle the .is-open class consistently; pick one approach and make
sure the selector and the toggled class names match (refer to .auto-filter,
.dalf-filter and the showFilterOptions/hideFilterOptions functions).
- Around line 1-3: The CSS selector `.auto-filter,.dalf-filter h3` incorrectly
targets the whole `.auto-filter` element and only `h3` inside `.dalf-filter`;
update the selector to target both container elements consistently by changing
it to `.auto-filter, .dalf-filter` (or, if the intent is to target headings
only, make both `.auto-filter h3, .dalf-filter h3`) so `pointer-events: all;`
applies to the intended elements; locate the rule using selectors `.auto-filter`
and `.dalf-filter h3` and replace accordingly.
In `@tests/openwisp2/settings.py`:
- Around line 36-37: INSTALLED_APPS still lists "admin_auto_filters" while
openwisp_utils/admin_theme/views.py imports AutocompleteJsonView from
admin_auto_filters.views; either remove the INSTALLED_APPS entry or eliminate
the import/use in views.py. Fix by updating openwisp_utils/admin_theme/views.py
to replace the admin_auto_filters dependency (remove the import of
AutocompleteJsonView and implement or use an alternative autocomplete view
consistent with the new dalf-based filters in filters.py), or if you cannot
change views.py now, keep "admin_auto_filters" in INSTALLED_APPS so the import
remains valid; make sure the change references INSTALLED_APPS,
openwisp_utils/admin_theme/views.py, AutocompleteJsonView,
admin_auto_filters.views, and filters.py.
In `@tests/test_project/tests/test_admin.py`:
- Around line 14-17: Remove the duplicate imports that shadow existing symbols:
eliminate the re-imports of TestCase, AdminSite, and Shelf and instead add only
the new symbols RequestFactory, AutocompleteFilter, and Book to the pre-existing
import lines so you consolidate imports; update the existing import statements
where TestCase, AdminSite, and Shelf were originally imported to include
RequestFactory, AutocompleteFilter, and Book rather than keeping the duplicated
import block.
- Around line 601-638: Move the TestAutocompleteFilter class out of the
enclosing TestAdmin so it is a top-level TestCase in the module (so Django test
discovery picks it up); in the tests that call filter_inst.choices(None) replace
the None changelist with a small mock object that implements get_query_string
and get_filters_params (or use RequestFactory/HttpRequest wrapper) so
choices(...) can call those methods without raising AttributeError; keep
references to the same symbols (TestAutocompleteFilter, AutocompleteFilter,
filter_inst.choices(...), filter_inst.queryset(...)) so the tests still
initialize the AutocompleteFilter and exercise queryset filtering with the
provided shelf id.
🧹 Nitpick comments (2)
tests/test_project/admin.py (1)
52-52: Tuple-form filter declarations look correct for dalf.Minor nit: inconsistent spacing in tuples —
("shelf",AutoShelfFilter)vs("shelf", AutoShelfFilter). Consider adding spaces after commas for consistency with PEP 8.Formatting fix
- list_filter = [("shelf",AutoShelfFilter), "name"] + list_filter = [("shelf", AutoShelfFilter), "name"]- ("owner",AutoOwnerFilter), + ("owner", AutoOwnerFilter), "books_type", - ("book",ReverseBookFilter), + ("book", ReverseBookFilter),Also applies to: 119-121
openwisp_utils/admin_theme/filters.py (1)
142-155: Optional:choices()logic is near-duplicate ofInputFilterMixin.choices().The only addition here is the
"parameter_name"key. If the filter templates can be unified to expect that key consistently, the dict-building could be shared via a small helper to avoid the copy-paste across the two hierarchies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
openwisp_utils/admin_theme/filters.pyopenwisp_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-filter.jsopenwisp_utils/admin_theme/templates/admin/auto_filter.htmltests/openwisp2/settings.pytests/test_project/admin.pytests/test_project/tests/test_admin.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_project/tests/test_admin.py (2)
openwisp_utils/admin_theme/filters.py (5)
AutocompleteFilter(95-172)choices(18-28)choices(142-155)queryset(39-40)queryset(158-172)tests/test_project/models.py (2)
Book(68-78)Shelf(16-65)
🪛 Ruff (0.14.14)
tests/test_project/admin.py
[warning] 52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
tests/test_project/tests/test_admin.py
[error] 14-14: Redefinition of unused TestCase from line 7: TestCase redefined here
Remove definition: TestCase
(F811)
[error] 15-15: Redefinition of unused AdminSite from line 3: AdminSite redefined here
Remove definition: AdminSite
(F811)
🔇 Additional comments (5)
openwisp_utils/admin_theme/templates/admin/auto_filter.html (1)
3-4: Template include updated to dalf's AJAX filter template — looks correct.The switch from the old
django-admin-autocomplete-filter/autocomplete-filter.htmltoadmin/filter/django_admin_list_filter_ajax.htmlaligns with the dalf migration.openwisp_utils/admin_theme/static/admin/js/ow-filter.js (1)
8-32: CorrectreadyStateguard for deferred initialization.The
initAllextraction and thereadyStatecheck ensure the initialization runs correctly regardless of when the script is loaded. This is a sound pattern that prevents a missedDOMContentLoadedevent.openwisp_utils/admin_theme/filters.py (3)
102-122: Well-documented media asset loading order.The numbered comments clarifying JS load order are helpful for maintainability.
135-140:value()implementation looks correct.Clean delegation to
used_parameters. Note that it depends on a successful__init__(addressed above).
158-172: Good error handling pattern inqueryset().Gracefully catches validation errors, surfaces them via the messages framework, and falls back to the unfiltered queryset. The
hasattr(e, 'messages')check correctly distinguishesValidationErrorfrom plainValueError.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def __init__(self, field, request, params, model, model_admin, field_path): | ||
| # We manually use the full arguments instead of *args, **kwargs | ||
| # so we can manipulate self.ajax_url after initialization | ||
| try: | ||
| return super().__init__(*args, **kwargs) | ||
| except ValidationError: | ||
| None | ||
| super().__init__(field, request, params, model, model_admin, field_path) | ||
| # Override dalf's default URL to use OpenWISP's custom view | ||
| self.ajax_url = reverse("admin:ow-auto-filter") | ||
| except (ValidationError, ValueError): | ||
| # Handle invalid UUIDs passed in params during init | ||
| pass |
There was a problem hiding this comment.
Bare pass in __init__ exception handler leaves the filter half-initialized.
If super().__init__() raises, parent-class attributes (used_parameters, lookup_kwarg, title, field, etc.) may not be set. Django will still invoke choices() and queryset() on this object, which will crash with an AttributeError—a different 500 error than the one you intended to prevent.
Either let the exception propagate (since queryset() already handles these errors gracefully), or initialize all required fallback attributes in the handler.
Option A (preferred): Remove the try/except from __init__ entirely
def __init__(self, field, request, params, model, model_admin, field_path):
- # We manually use the full arguments instead of *args, **kwargs
- # so we can manipulate self.ajax_url after initialization
- try:
- super().__init__(field, request, params, model, model_admin, field_path)
- # Override dalf's default URL to use OpenWISP's custom view
- self.ajax_url = reverse("admin:ow-auto-filter")
- except (ValidationError, ValueError):
- # Handle invalid UUIDs passed in params during init
- pass
+ super().__init__(field, request, params, model, model_admin, field_path)
+ # Override dalf's default URL to use OpenWISP's custom view
+ self.ajax_url = reverse("admin:ow-auto-filter")If Django's ChangeList.get_filters can't tolerate an exception here, use Option B instead.
Option B: Catch but ensure consistent state
def __init__(self, field, request, params, model, model_admin, field_path):
- # We manually use the full arguments instead of *args, **kwargs
- # so we can manipulate self.ajax_url after initialization
try:
super().__init__(field, request, params, model, model_admin, field_path)
- # Override dalf's default URL to use OpenWISP's custom view
- self.ajax_url = reverse("admin:ow-auto-filter")
except (ValidationError, ValueError):
- # Handle invalid UUIDs passed in params during init
- pass
+ # Ensure minimum required state so choices()/queryset() don't crash
+ self.field = field
+ self.field_path = field_path
+ self.title = getattr(field, 'verbose_name', field_path)
+ self.lookup_kwarg = field_path
+ self.used_parameters = {}
+ self.ajax_url = reverse("admin:ow-auto-filter")🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/filters.py` around lines 124 - 133, The try/except
in the Filter class __init__ swallows ValidationError/ValueError leaving the
instance half-initialized; remove the try/except around super().__init__(...) so
exceptions propagate (preferred) or, if you must catch them, set the
parent-required attributes (used_parameters, lookup_kwarg, title, field, etc.)
to safe defaults and still set self.ajax_url = reverse("admin:ow-auto-filter")
so that subsequent calls to choices() and queryset() don't raise AttributeError;
update __init__ accordingly to ensure a consistent object state.
| def choices(self, changelist): | ||
| all_choice = { | ||
| "selected": self.value() is None, | ||
| "query_string": changelist.get_query_string(remove=[self.parameter_name]), | ||
| "display": _("All"), | ||
| "query_parts": [], | ||
| "parameter_name": self.lookup_kwarg, | ||
| } | ||
|
|
||
| for key, value in changelist.get_filters_params().items(): | ||
| if key != self.parameter_name: | ||
| all_choice["query_parts"].append((key, value)) | ||
|
|
||
| yield all_choice |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file in question
find . -name "filters.py" -path "*/admin_theme/*" -type fRepository: openwisp/openwisp-utils
Length of output: 106
🏁 Script executed:
# Read the file to see the exact code
cat -n openwisp_utils/admin_theme/filters.py | sed -n '130,165p'Repository: openwisp/openwisp-utils
Length of output: 1563
🏁 Script executed:
# Search for DALFRelatedFieldAjax definition in the codebase
rg -n "class DALFRelatedFieldAjax" --type pyRepository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
# Check imports to understand parent classes
head -30 openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 1208
🏁 Script executed:
# Search for parameter_name usage throughout the file
rg -n "parameter_name" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 580
🏁 Script executed:
# Find the class that contains the choices() method at line 142-155
cat -n openwisp_utils/admin_theme/filters.py | sed -n '95,160p'Repository: openwisp/openwisp-utils
Length of output: 2801
🏁 Script executed:
# Check if parameter_name is set anywhere in __init__ or class definition after line 44
rg -n "class.*AutocompleteFilter" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 119
🏁 Script executed:
# Check what other classes inherit from BaseAutocompleteFilter or InputFilterMixin
rg -n "class.*BaseAutocompleteFilter|class.*InputFilterMixin" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 267
🏁 Script executed:
# Get full context of InputFilterMixin to understand parameter_name definition
cat -n openwisp_utils/admin_theme/filters.py | sed -n '11,90p'Repository: openwisp/openwisp-utils
Length of output: 3582
🏁 Script executed:
# Check if there are any other usages or patterns that show parameter_name initialization
rg -B3 -A3 "parameter_name\s*=" openwisp_utils/admin_theme/filters.pyRepository: openwisp/openwisp-utils
Length of output: 578
🏁 Script executed:
# Check what SimpleListFilter (from Django) defines
python3 << 'EOF'
import inspect
try:
from django.contrib.admin.filters import SimpleListFilter
# Check if SimpleListFilter has parameter_name attribute
if hasattr(SimpleListFilter, 'parameter_name'):
print("SimpleListFilter.parameter_name exists")
print(f"Value: {getattr(SimpleListFilter, 'parameter_name', 'N/A')}")
else:
print("SimpleListFilter does NOT have parameter_name as class attribute")
# Get all attributes
attrs = [a for a in dir(SimpleListFilter) if not a.startswith('_')]
print(f"\nSimpleListFilter attributes: {attrs[:20]}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: openwisp/openwisp-utils
Length of output: 98
🏁 Script executed:
# Search online documentation or check if dalf defines parameter_name
# Let's check what BaseAutocompleteFilter (DALFRelatedFieldAjax) might inherit from
python3 << 'EOF'
try:
from dalf.admin import DALFRelatedFieldAjax
import inspect
print("DALFRelatedFieldAjax parent classes:")
print(DALFRelatedFieldAjax.__bases__)
print("\nDALFRelatedFieldAjax attributes related to 'parameter':")
attrs = [a for a in dir(DALFRelatedFieldAjax) if 'parameter' in a.lower()]
print(attrs)
print("\nLooking for parameter_name:")
if hasattr(DALFRelatedFieldAjax, 'parameter_name'):
print(f"Found parameter_name: {getattr(DALFRelatedFieldAjax, 'parameter_name', 'N/A')}")
else:
print("parameter_name NOT found in DALFRelatedFieldAjax")
except ImportError as e:
print(f"dalf not installed: {e}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: openwisp/openwisp-utils
Length of output: 109
🏁 Script executed:
# Check if the choices() method in AutocompleteFilter actually gets parameter_name from parent
# Let's trace the MRO and parent class structure
python3 << 'EOF'
try:
from dalf.admin import DALFRelatedFieldAjax
print("Method Resolution Order (MRO) for DALFRelatedFieldAjax:")
for i, cls in enumerate(DALFRelatedFieldAjax.__mro__):
print(f" {i}: {cls}")
# Check which class defines parameter_name
print("\nSearching for parameter_name in MRO:")
for cls in DALFRelatedFieldAjax.__mro__:
if 'parameter_name' in cls.__dict__:
print(f" Found in {cls}: {cls.__dict__['parameter_name']}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: openwisp/openwisp-utils
Length of output: 96
🌐 Web query:
django-admin-list-filter DALFRelatedFieldAjax parameter_name
💡 Result:
DALFRelatedFieldAjax does not use a parameter_name attribute (that’s a SimpleListFilter concept). It inherits Django’s RelatedFieldListFilter behavior, so the querystring key (“parameter name”) is derived from the field path. In practice it’s:
<field_path>__id__exact(the selected related object’s PK)- and also
<field_path>__isnull(for the “(None)” choice, when applicable)
Example: for ('category', DALFRelatedFieldAjax) the URL parameter is typically category__id__exact=<pk>.
If you need a custom parameter name, you’d have to write your own filter class (subclass FieldListFilter/RelatedFieldListFilter), not configure it on DALFRelatedFieldAjax. [1] [2] [3]
Sources: DALF docs/README for inheritance, Django admin filter docs for parameter_name being for SimpleListFilter, and related-filter usage patterns showing __id__exact. [1] [2] [3]
🌐 Web query:
Django SimpleListFilter parameter_name attribute
💡 Result:
In a custom Django admin SimpleListFilter, the parameter_name attribute is the name of the GET query-string parameter Django will use in the changelist URL to store the selected filter value (i.e., what shows up in ?parameter_name=value). It’s required (along with title) when subclassing SimpleListFilter. [1]
Typical usage:
- Set
parameter_nameto a short, unique string (e.g."decade"). - In
queryset(), read the chosen option viaself.value()(which comes from that GET parameter) and filter accordingly. [1]
Example from the Django docs: parameter_name = 'decade' (so URLs look like ?decade=80s). [1]
Source(s): Django docs — ModelAdmin List Filters → Using a SimpleListFilter [1]
References:
[1] https://docs.djangoproject.com/en/4.1/ref/contrib/admin/filters/
🏁 Script executed:
# Check if there are any test files or documentation that shows usage
find . -name "*.py" -type f -exec grep -l "AutocompleteFilter\|DALFRelatedFieldAjax" {} \; | head -20Repository: openwisp/openwisp-utils
Length of output: 221
🏁 Script executed:
# Check the test files to see how AutocompleteFilter is actually used
cat -n ./tests/test_project/admin.py | head -100Repository: openwisp/openwisp-utils
Length of output: 3246
🏁 Script executed:
# Check if there are any tests that would catch this AttributeError
rg -n "AutocompleteFilter\|choices" ./tests/test_project/tests/test_admin.py | head -20Repository: openwisp/openwisp-utils
Length of output: 49
🏁 Script executed:
# Search for any place where parameter_name might be set on AutocompleteFilter
rg -n "AutocompleteFilter" openwisp_utils/admin_theme/filters.py -A10 -B5Repository: openwisp/openwisp-utils
Length of output: 1079
Fix missing parameter_name attribute and indentation inconsistency in choices() method.
The AutocompleteFilter.choices() method references self.parameter_name on lines 145 and 152, but DALFRelatedFieldAjax (the parent class) doesn't define it. Unlike SimpleListFilter, RelatedFieldListFilter doesn't have this attribute. While subclasses can set it explicitly (as shown in the test admin with AutoShelfFilter), it's not guaranteed. If not set, this raises AttributeError at runtime.
Additionally, the closing } on line 149 is indented at 4 spaces instead of 8, inconsistent with the identical pattern in InputFilterMixin.choices() at line 24.
Use getattr(self, 'parameter_name', self.lookup_kwarg) as a safe fallback, and fix the indentation:
Diff
def choices(self, changelist):
+ param = getattr(self, 'parameter_name', self.lookup_kwarg)
all_choice = {
"selected": self.value() is None,
- "query_string": changelist.get_query_string(remove=[self.parameter_name]),
+ "query_string": changelist.get_query_string(remove=[param]),
"display": _("All"),
"query_parts": [],
"parameter_name": self.lookup_kwarg,
- }
+ }
for key, value in changelist.get_filters_params().items():
- if key != self.parameter_name:
+ if key != param:
all_choice["query_parts"].append((key, value))
yield all_choice🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/filters.py` around lines 142 - 155, The choices()
method in AutocompleteFilter uses self.parameter_name which may not exist
(parent DALFRelatedFieldAjax defines lookup_kwarg instead), causing
AttributeError; update choices() to use getattr(self, "parameter_name",
self.lookup_kwarg) wherever parameter_name is referenced (e.g., the "selected"
check and when calling get_query_string/remove) and correct the dictionary
closing indentation to match InputFilterMixin.choices(); reference the
AutocompleteFilter.choices(), DALFRelatedFieldAjax, InputFilterMixin, and
lookup_kwarg symbols when making the change.
| .auto-filter,.dalf-filter h3 { | ||
| pointer-events: all; | ||
| } |
There was a problem hiding this comment.
CSS selector precedence issue: .auto-filter,.dalf-filter h3 is parsed as two selectors.
This is parsed as .auto-filter (the whole element) OR .dalf-filter h3 (only the h3 child). If the intent is to apply pointer-events: all to both containers, it should be .auto-filter, .dalf-filter. If it should only target h3 in both, it should mirror line 11.
Proposed fix (assuming intent is both containers)
-.auto-filter,.dalf-filter h3 {
+.auto-filter, .dalf-filter {
pointer-events: all;
}🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/static/admin/css/ow-auto-filter.css` around lines
1 - 3, The CSS selector `.auto-filter,.dalf-filter h3` incorrectly targets the
whole `.auto-filter` element and only `h3` inside `.dalf-filter`; update the
selector to target both container elements consistently by changing it to
`.auto-filter, .dalf-filter` (or, if the intent is to target headings only, make
both `.auto-filter h3, .dalf-filter h3`) so `pointer-events: all;` applies to
the intended elements; locate the rule using selectors `.auto-filter` and
`.dalf-filter h3` and replace accordingly.
| .auto-filter.is-open h3::after, .dalf-filter.is-open h3::after { | ||
| transform: rotate(180deg); | ||
| } |
There was a problem hiding this comment.
.is-open class is never toggled — arrow rotation will not work.
The JS in ow-filter.js toggles the ow-active class (via showFilterOptions/hideFilterOptions), not is-open. This means the h3::after arrow rotation defined here will never trigger.
Proposed fix
-/* Rotation when the filter is open */
-.auto-filter.is-open h3::after, .dalf-filter.is-open h3::after {
+/* Rotation when the filter is open */
+.auto-filter.ow-active h3::after, .dalf-filter.ow-active h3::after {
transform: rotate(180deg);
}📝 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.
| .auto-filter.is-open h3::after, .dalf-filter.is-open h3::after { | |
| transform: rotate(180deg); | |
| } | |
| .auto-filter.ow-active h3::after, .dalf-filter.ow-active h3::after { | |
| transform: rotate(180deg); | |
| } |
🤖 Prompt for AI Agents
In `@openwisp_utils/admin_theme/static/admin/css/ow-auto-filter.css` around lines
36 - 38, The CSS selector uses .is-open but the JS
(showFilterOptions/hideFilterOptions in ow-filter.js) toggles .ow-active, so the
arrow rotation never fires; update the CSS rules for the arrow (currently
targeting .auto-filter.is-open and .dalf-filter.is-open) to target .ow-active
instead, or alternatively update the showFilterOptions/hideFilterOptions
functions to toggle the .is-open class consistently; pick one approach and make
sure the selector and the toggled class names match (refer to .auto-filter,
.dalf-filter and the showFilterOptions/hideFilterOptions functions).
| "dalf", | ||
| "admin_auto_filters", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all references to admin_auto_filters in the codebase
rg -n "admin_auto_filters" --type=pyRepository: openwisp/openwisp-utils
Length of output: 247
🏁 Script executed:
# Check the full context of the import and usage in views.py
cat -n openwisp_utils/admin_theme/views.py | head -20Repository: openwisp/openwisp-utils
Length of output: 716
🏁 Script executed:
# Check if views.py is modified in this PR - look for more context
cat -n openwisp_utils/admin_theme/views.pyRepository: openwisp/openwisp-utils
Length of output: 2592
🏁 Script executed:
# Check what dalf provides - search for dalf in the codebase
rg -n "dalf" --type=py | head -20Repository: openwisp/openwisp-utils
Length of output: 333
🏁 Script executed:
# Check how dalf is being used in filters.py
cat -n openwisp_utils/admin_theme/filters.py | head -40Repository: openwisp/openwisp-utils
Length of output: 1811
🏁 Script executed:
# Check if dalf provides AutocompleteJsonView or similar
rg -n "AutocompleteJsonView" --type=pyRepository: openwisp/openwisp-utils
Length of output: 385
🏁 Script executed:
# Let's check if there are any recent changes to views.py that might indicate if this should have been updated
git log --oneline openwisp_utils/admin_theme/views.py | head -5Repository: openwisp/openwisp-utils
Length of output: 108
🏁 Script executed:
# Search for what dalf provides - check if it has AutocompleteJsonView
rg -n "from dalf" --type=py -A 1Repository: openwisp/openwisp-utils
Length of output: 252
admin_auto_filters dependency removal incomplete.
The PR objective is to remove the django-admin-autocomplete-filter dependency, but it's still referenced in the codebase. admin_auto_filters remains in INSTALLED_APPS at line 37, and the code in openwisp_utils/admin_theme/views.py still imports and uses AutocompleteJsonView from admin_auto_filters.views. The migration to dalf covers only the filter functionality (in filters.py), not the views component. Either views.py needs to be updated to remove the dependency on admin_auto_filters, or if that's out of scope, the INSTALLED_APPS entry should remain.
🤖 Prompt for AI Agents
In `@tests/openwisp2/settings.py` around lines 36 - 37, INSTALLED_APPS still lists
"admin_auto_filters" while openwisp_utils/admin_theme/views.py imports
AutocompleteJsonView from admin_auto_filters.views; either remove the
INSTALLED_APPS entry or eliminate the import/use in views.py. Fix by updating
openwisp_utils/admin_theme/views.py to replace the admin_auto_filters dependency
(remove the import of AutocompleteJsonView and implement or use an alternative
autocomplete view consistent with the new dalf-based filters in filters.py), or
if you cannot change views.py now, keep "admin_auto_filters" in INSTALLED_APPS
so the import remains valid; make sure the change references INSTALLED_APPS,
openwisp_utils/admin_theme/views.py, AutocompleteJsonView,
admin_auto_filters.views, and filters.py.
| from django.test import TestCase, RequestFactory | ||
| from django.contrib.admin.sites import AdminSite | ||
| from openwisp_utils.admin_theme.filters import AutocompleteFilter | ||
| from ..models import Book, Shelf |
There was a problem hiding this comment.
Duplicate imports — flagged by Ruff (F811).
TestCase (line 7), AdminSite (line 3), and Shelf (line 25) are already imported above. These re-imports shadow the originals. Consolidate into the existing import statements and add only the new symbols (RequestFactory, AutocompleteFilter, Book).
Proposed fix
Remove lines 14–17 and merge into the existing imports:
-from django.test import TestCase
+from django.test import TestCase, RequestFactory
...
-from openwisp_utils.admin_theme.filters import InputFilter, SimpleInputFilter
+from openwisp_utils.admin_theme.filters import AutocompleteFilter, InputFilter, SimpleInputFilter
...
-from django.test import TestCase, RequestFactory
-from django.contrib.admin.sites import AdminSite
-from openwisp_utils.admin_theme.filters import AutocompleteFilter
-from ..models import Book, Shelf
+from ..models import (
+ Book,
+ Operator,
+ ...
+ Shelf,
+)🧰 Tools
🪛 Ruff (0.14.14)
[error] 14-14: Redefinition of unused TestCase from line 7: TestCase redefined here
Remove definition: TestCase
(F811)
[error] 15-15: Redefinition of unused AdminSite from line 3: AdminSite redefined here
Remove definition: AdminSite
(F811)
🤖 Prompt for AI Agents
In `@tests/test_project/tests/test_admin.py` around lines 14 - 17, Remove the
duplicate imports that shadow existing symbols: eliminate the re-imports of
TestCase, AdminSite, and Shelf and instead add only the new symbols
RequestFactory, AutocompleteFilter, and Book to the pre-existing import lines so
you consolidate imports; update the existing import statements where TestCase,
AdminSite, and Shelf were originally imported to include RequestFactory,
AutocompleteFilter, and Book rather than keeping the duplicated import block.
|
hi, please do the following:
|
2 participants
Changes Made
Testing
Known Issues
Currently investigating a minor JavaScript initialization issue affecting the ForeignKey filter toggle.
Checklist
Reference to Existing Issue
Closes #582 .
Please open a new issue if there isn't an existing issue yet.
Description of Changes
I have analyzed the requirements for migrating from the deprecated django-admin-autocomplete-filter and have implemented a migration to vigo/django-admin-list-filter (dalf). This library was selected because it is actively maintained and supports Django 5.0+, which is essential for the project's long-term stability.
Screenshot