From 4484bc1b2f84da6442c9c2bfd95d3f1f7d8f96f7 Mon Sep 17 00:00:00 2001 From: Fran Hrzenjak Date: Tue, 19 May 2020 09:03:04 +0200 Subject: [PATCH] Fixed #31597, #31603 -- Corrected admin clear all filters link behavior. - Show "Clear all filters" only when any filter is selected. - Preserve query string not related with filters. Co-Authored-By: Hasan Ramezani --- .../admin/templates/admin/change_list.html | 4 +- django/contrib/admin/views/main.py | 27 ++++++-- tests/admin_changelist/admin.py | 27 +++++++- tests/admin_changelist/test_date_hierarchy.py | 2 +- tests/admin_changelist/tests.py | 66 +++++++++++++++++-- 5 files changed, 110 insertions(+), 16 deletions(-) diff --git a/django/contrib/admin/templates/admin/change_list.html b/django/contrib/admin/templates/admin/change_list.html index aebd12b957..18cd60101e 100644 --- a/django/contrib/admin/templates/admin/change_list.html +++ b/django/contrib/admin/templates/admin/change_list.html @@ -60,8 +60,8 @@ {% if cl.has_filters %}

{% translate 'Filter' %}

- {% if cl.preserved_filters %}

- ✖ {% translate "Clear all filters" %} + {% if cl.has_active_filters %}

+ ✖ {% translate "Clear all filters" %}

{% endif %} {% for spec in cl.filter_specs %}{% admin_list_filter cl spec %}{% endfor %}
diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 5bc3f736f1..6df80e3627 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -59,6 +59,8 @@ class ChangeList: self.list_display_links = list_display_links self.list_filter = list_filter self.has_filters = None + self.has_active_filters = None + self.clear_all_filters_qs = None self.date_hierarchy = date_hierarchy self.search_fields = search_fields self.list_select_related = list_select_related @@ -121,6 +123,7 @@ class ChangeList: def get_filters(self, request): lookup_params = self.get_filters_params() use_distinct = False + has_active_filters = False for key, value in lookup_params.items(): if not self.model_admin.lookup_allowed(key, value): @@ -128,6 +131,7 @@ class ChangeList: filter_specs = [] for list_filter in self.list_filter: + lookup_params_count = len(lookup_params) if callable(list_filter): # This is simply a custom list filter class. spec = list_filter(request, lookup_params, self.model, self.model_admin) @@ -145,7 +149,6 @@ class ChangeList: field_path = field field = get_fields_from_path(self.model, field_path)[-1] - lookup_params_count = len(lookup_params) spec = field_list_filter_class( field, request, lookup_params, self.model, self.model_admin, field_path=field_path, @@ -157,6 +160,8 @@ class ChangeList: use_distinct = use_distinct or lookup_needs_distinct(self.lookup_opts, field_path) if spec and spec.has_output(): filter_specs.append(spec) + if lookup_params_count > len(lookup_params): + has_active_filters = True if self.date_hierarchy: # Create bounded lookup parameters so that the query is more @@ -199,7 +204,10 @@ class ChangeList: for key, value in lookup_params.items(): lookup_params[key] = prepare_lookup_value(key, value) use_distinct = use_distinct or lookup_needs_distinct(self.lookup_opts, key) - return filter_specs, bool(filter_specs), lookup_params, use_distinct + return ( + filter_specs, bool(filter_specs), lookup_params, use_distinct, + has_active_filters, + ) except FieldDoesNotExist as e: raise IncorrectLookupParameters(e) from e @@ -433,9 +441,13 @@ class ChangeList: def get_queryset(self, request): # First, we collect all the declared list filters. - (self.filter_specs, self.has_filters, remaining_lookup_params, - filters_use_distinct) = self.get_filters(request) - + ( + self.filter_specs, + self.has_filters, + remaining_lookup_params, + filters_use_distinct, + self.has_active_filters, + ) = self.get_filters(request) # Then, we let every list filter modify the queryset to its liking. qs = self.root_queryset for filter_spec in self.filter_specs: @@ -470,6 +482,11 @@ class ChangeList: # Apply search results qs, search_use_distinct = self.model_admin.get_search_results(request, qs, self.query) + # Set query string for clearing all filters. + self.clear_all_filters_qs = self.get_query_string( + new_params=remaining_lookup_params, + remove=self.get_filters_params(), + ) # Remove duplicates from results, if necessary if filters_use_distinct | search_use_distinct: return qs.distinct() diff --git a/tests/admin_changelist/admin.py b/tests/admin_changelist/admin.py index 1194f26dd4..890919557e 100644 --- a/tests/admin_changelist/admin.py +++ b/tests/admin_changelist/admin.py @@ -3,7 +3,7 @@ from django.contrib.auth.admin import UserAdmin from django.contrib.auth.models import User from django.core.paginator import Paginator -from .models import Child, Event, Parent, Swallow +from .models import Band, Child, Event, Parent, Swallow site = admin.AdminSite(name="admin") @@ -59,6 +59,31 @@ class BandAdmin(admin.ModelAdmin): list_filter = ['genres'] +class NrOfMembersFilter(admin.SimpleListFilter): + title = 'number of members' + parameter_name = 'nr_of_members_partition' + + def lookups(self, request, model_admin): + return [ + ('5', '0 - 5'), + ('more', 'more than 5'), + ] + + def queryset(self, request, queryset): + value = self.value() + if value == '5': + return queryset.filter(nr_of_members__lte=5) + if value == 'more': + return queryset.filter(nr_of_members__gt=5) + + +class BandCallableFilterAdmin(admin.ModelAdmin): + list_filter = [NrOfMembersFilter] + + +site.register(Band, BandCallableFilterAdmin) + + class GroupAdmin(admin.ModelAdmin): list_filter = ['members'] diff --git a/tests/admin_changelist/test_date_hierarchy.py b/tests/admin_changelist/test_date_hierarchy.py index 65f42ecd05..a321650b32 100644 --- a/tests/admin_changelist/test_date_hierarchy.py +++ b/tests/admin_changelist/test_date_hierarchy.py @@ -21,7 +21,7 @@ class DateHierarchyTests(TestCase): request = self.factory.get('/', query) request.user = self.superuser changelist = EventAdmin(Event, custom_site).get_changelist_instance(request) - _, _, lookup_params, _ = changelist.get_filters(request) + _, _, lookup_params, *_ = changelist.get_filters(request) self.assertEqual(lookup_params['date__gte'], expected_from_date) self.assertEqual(lookup_params['date__lt'], expected_to_date) diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 53871e1a5b..38143bf592 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -5,7 +5,9 @@ from django.contrib.admin.models import LogEntry from django.contrib.admin.options import IncorrectLookupParameters from django.contrib.admin.templatetags.admin_list import pagination from django.contrib.admin.tests import AdminSeleniumTestCase -from django.contrib.admin.views.main import ALL_VAR, SEARCH_VAR +from django.contrib.admin.views.main import ( + ALL_VAR, IS_POPUP_VAR, ORDER_VAR, PAGE_VAR, SEARCH_VAR, TO_FIELD_VAR, +) from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.contrib.messages.storage.cookie import CookieStorage @@ -708,15 +710,65 @@ class ChangeListTests(TestCase): def test_clear_all_filters_link(self): self.client.force_login(self.superuser) - link = '✖ Clear all filters' - response = self.client.get(reverse('admin:auth_user_changelist')) - self.assertNotContains(response, link) + url = reverse('admin:auth_user_changelist') + response = self.client.get(url) + self.assertNotContains(response, '✖ Clear all filters') + link = '✖ Clear all filters' + for data, href in ( + ({'is_staff__exact': '0'}, '?'), + ( + {'is_staff__exact': '0', 'username__startswith': 'test'}, + '?username__startswith=test', + ), + ( + {'is_staff__exact': '0', SEARCH_VAR: 'test'}, + '?%s=test' % SEARCH_VAR, + ), + ( + {'is_staff__exact': '0', IS_POPUP_VAR: 'id'}, + '?%s=id' % IS_POPUP_VAR, + ), + ): + with self.subTest(data=data): + response = self.client.get(url, data=data) + self.assertContains(response, link % href) + + def test_clear_all_filters_link_callable_filter(self): + self.client.force_login(self.superuser) + url = reverse('admin:admin_changelist_band_changelist') + response = self.client.get(url) + self.assertNotContains(response, '✖ Clear all filters') + link = '✖ Clear all filters' + for data, href in ( + ({'nr_of_members_partition': '5'}, '?'), + ( + {'nr_of_members_partition': 'more', 'name__startswith': 'test'}, + '?name__startswith=test', + ), + ( + {'nr_of_members_partition': '5', IS_POPUP_VAR: 'id'}, + '?%s=id' % IS_POPUP_VAR, + ), + ): + with self.subTest(data=data): + response = self.client.get(url, data=data) + self.assertContains(response, link % href) + + def test_no_clear_all_filters_link(self): + self.client.force_login(self.superuser) + url = reverse('admin:auth_user_changelist') + link = '>✖ Clear all filters' for data in ( {SEARCH_VAR: 'test'}, - {'is_staff__exact': '0'}, + {ORDER_VAR: '-1'}, + {TO_FIELD_VAR: 'id'}, + {PAGE_VAR: '1'}, + {IS_POPUP_VAR: '1'}, + {'username__startswith': 'test'}, ): - response = self.client.get(reverse('admin:auth_user_changelist'), data=data) - self.assertContains(response, link) + with self.subTest(data=data): + response = self.client.get(url, data=data) + self.assertNotContains(response, link) def test_tuple_list_display(self): swallow = Swallow.objects.create(origin='Africa', load='12.34', speed='22.2')