From d2b688b966f5d30414899549412d370e1317ddb8 Mon Sep 17 00:00:00 2001 From: sarahboyce Date: Mon, 6 Mar 2023 15:24:39 +0100 Subject: [PATCH] Fixed #1873 -- Handled multi-valued query parameters in admin changelist filters. --- django/contrib/admin/filters.py | 18 ++-- django/contrib/admin/utils.py | 11 ++- django/contrib/admin/views/main.py | 15 +-- docs/releases/5.0.txt | 5 + tests/admin_changelist/test_date_hierarchy.py | 4 +- tests/admin_filters/tests.py | 94 +++++++++++++++++++ tests/admin_utils/tests.py | 15 +++ 7 files changed, 146 insertions(+), 16 deletions(-) diff --git a/django/contrib/admin/filters.py b/django/contrib/admin/filters.py index 3ae7805dc2483..f6f6d0dd6fd24 100644 --- a/django/contrib/admin/filters.py +++ b/django/contrib/admin/filters.py @@ -9,6 +9,7 @@ from django.contrib.admin.options import IncorrectLookupParameters from django.contrib.admin.utils import ( + build_q_object_from_lookup_parameters, get_last_value_from_parameters, get_model_from_relation, prepare_lookup_value, @@ -187,7 +188,8 @@ def has_output(self): def queryset(self, request, queryset): try: - return queryset.filter(**self.used_parameters) + q_object = build_q_object_from_lookup_parameters(self.used_parameters) + return queryset.filter(q_object) except (ValueError, ValidationError) as e: # Fields may raise a ValueError or ValidationError when converting # the parameters to the correct type. @@ -220,7 +222,7 @@ def __init__(self, field, request, params, model, model_admin, field_path): other_model = get_model_from_relation(field) self.lookup_kwarg = "%s__%s__exact" % (field_path, field.target_field.name) self.lookup_kwarg_isnull = "%s__isnull" % field_path - self.lookup_val = get_last_value_from_parameters(params, self.lookup_kwarg) + self.lookup_val = params.get(self.lookup_kwarg) self.lookup_val_isnull = get_last_value_from_parameters( params, self.lookup_kwarg_isnull ) @@ -293,7 +295,8 @@ def choices(self, changelist): count = facet_counts[f"{pk_val}__c"] val = f"{val} ({count})" yield { - "selected": self.lookup_val == str(pk_val), + "selected": self.lookup_val is not None + and str(pk_val) in self.lookup_val, "query_string": changelist.get_query_string( {self.lookup_kwarg: pk_val}, [self.lookup_kwarg_isnull] ), @@ -391,7 +394,7 @@ class ChoicesFieldListFilter(FieldListFilter): def __init__(self, field, request, params, model, model_admin, field_path): self.lookup_kwarg = "%s__exact" % field_path self.lookup_kwarg_isnull = "%s__isnull" % field_path - self.lookup_val = get_last_value_from_parameters(params, self.lookup_kwarg) + self.lookup_val = params.get(self.lookup_kwarg) self.lookup_val_isnull = get_last_value_from_parameters( params, self.lookup_kwarg_isnull ) @@ -432,7 +435,8 @@ def choices(self, changelist): none_title = title continue yield { - "selected": str(lookup) == self.lookup_val, + "selected": self.lookup_val is not None + and str(lookup) in self.lookup_val, "query_string": changelist.get_query_string( {self.lookup_kwarg: lookup}, [self.lookup_kwarg_isnull] ), @@ -555,7 +559,7 @@ class AllValuesFieldListFilter(FieldListFilter): def __init__(self, field, request, params, model, model_admin, field_path): self.lookup_kwarg = field_path self.lookup_kwarg_isnull = "%s__isnull" % field_path - self.lookup_val = get_last_value_from_parameters(params, self.lookup_kwarg) + self.lookup_val = params.get(self.lookup_kwarg) self.lookup_val_isnull = get_last_value_from_parameters( params, self.lookup_kwarg_isnull ) @@ -609,7 +613,7 @@ def choices(self, changelist): continue val = str(val) yield { - "selected": self.lookup_val == val, + "selected": self.lookup_val is not None and val in self.lookup_val, "query_string": changelist.get_query_string( {self.lookup_kwarg: val}, [self.lookup_kwarg_isnull] ), diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py index 90442788c9e0a..5e6a400b6cd78 100644 --- a/django/contrib/admin/utils.py +++ b/django/contrib/admin/utils.py @@ -2,6 +2,8 @@ import decimal import json from collections import defaultdict +from functools import reduce +from operator import or_ from django.core.exceptions import FieldDoesNotExist from django.db import models, router @@ -64,7 +66,7 @@ def prepare_lookup_value(key, value, separator=","): Return a lookup value prepared to be used in queryset filtering. """ if isinstance(value, list): - value = value[-1] + return [prepare_lookup_value(key, v, separator=separator) for v in value] # if key ends with __in, split parameter into separate values if key.endswith("__in"): value = value.split(separator) @@ -74,6 +76,13 @@ def prepare_lookup_value(key, value, separator=","): return value +def build_q_object_from_lookup_parameters(parameters): + q_object = models.Q() + for param, param_item_list in parameters.items(): + q_object &= reduce(or_, (models.Q((param, item)) for item in param_item_list)) + return q_object + + def quote(s): """ Ensure that primary key values do not confuse the admin URLs by escaping diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index c0c03819f38e9..9a130ae8a7c4d 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -16,6 +16,7 @@ ShowFacets, ) from django.contrib.admin.utils import ( + build_q_object_from_lookup_parameters, get_fields_from_path, lookup_spawns_duplicates, prepare_lookup_value, @@ -173,9 +174,10 @@ def get_filters(self, request): may_have_duplicates = False has_active_filters = False - for key, value in lookup_params.items(): - if not self.model_admin.lookup_allowed(key, value[-1]): - raise DisallowedModelAdminLookup("Filtering by %s not allowed" % key) + for key, value_list in lookup_params.items(): + for value in value_list: + if not self.model_admin.lookup_allowed(key, value): + raise DisallowedModelAdminLookup(f"Filtering by {key} not allowed") filter_specs = [] for list_filter in self.list_filter: @@ -246,8 +248,8 @@ def get_filters(self, request): to_date = make_aware(to_date) lookup_params.update( { - "%s__gte" % self.date_hierarchy: from_date, - "%s__lt" % self.date_hierarchy: to_date, + "%s__gte" % self.date_hierarchy: [from_date], + "%s__lt" % self.date_hierarchy: [to_date], } ) @@ -534,7 +536,8 @@ def get_queryset(self, request, exclude_parameters=None): # Finally, we apply the remaining lookup parameters from the query # string (i.e. those that haven't already been processed by the # filters). - qs = qs.filter(**remaining_lookup_params) + q_object = build_q_object_from_lookup_parameters(remaining_lookup_params) + qs = qs.filter(q_object) except (SuspiciousOperation, ImproperlyConfigured): # Allow certain types of errors to be re-raised as-is so that the # caller can treat them in a special way. diff --git a/docs/releases/5.0.txt b/docs/releases/5.0.txt index 53ceb5a0dd7a9..7dee6c4575a11 100644 --- a/docs/releases/5.0.txt +++ b/docs/releases/5.0.txt @@ -54,6 +54,11 @@ Minor features * The new :meth:`.AdminSite.get_log_entries` method allows customizing the queryset for the site's listed log entries. +* The ``django.contrib.admin.AllValuesFieldListFilter``, + ``ChoicesFieldListFilter``, ``RelatedFieldListFilter``, and + ``RelatedOnlyFieldListFilter`` admin filters now handle multi-valued query + parameters. + :mod:`django.contrib.admindocs` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/admin_changelist/test_date_hierarchy.py b/tests/admin_changelist/test_date_hierarchy.py index a8c10f7cd8105..03afaa33d9170 100644 --- a/tests/admin_changelist/test_date_hierarchy.py +++ b/tests/admin_changelist/test_date_hierarchy.py @@ -25,8 +25,8 @@ def assertDateParams(self, query, expected_from_date, expected_to_date): request.user = self.superuser changelist = EventAdmin(Event, custom_site).get_changelist_instance(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) + self.assertEqual(lookup_params["date__gte"], [expected_from_date]) + self.assertEqual(lookup_params["date__lt"], [expected_to_date]) def test_bounded_params(self): tests = ( diff --git a/tests/admin_filters/tests.py b/tests/admin_filters/tests.py index cd71ed8a9f55d..a3af966005bd4 100644 --- a/tests/admin_filters/tests.py +++ b/tests/admin_filters/tests.py @@ -1533,6 +1533,100 @@ def test_facets_disallowed(self): expected_displays, ) + def test_multi_related_field_filter(self): + modeladmin = DecadeFilterBookAdmin(Book, site) + request = self.request_factory.get( + "/", + [("author__id__exact", self.alfred.pk), ("author__id__exact", self.bob.pk)], + ) + request.user = self.alfred + changelist = modeladmin.get_changelist_instance(request) + queryset = changelist.get_queryset(request) + self.assertSequenceEqual( + queryset, + list( + Book.objects.filter( + author__pk__in=[self.alfred.pk, self.bob.pk] + ).order_by("-id") + ), + ) + filterspec = changelist.get_filters(request)[0][0] + choices = list(filterspec.choices(changelist)) + expected_choice_values = [ + ("All", False, "?"), + ("alfred", True, f"?author__id__exact={self.alfred.pk}"), + ("bob", True, f"?author__id__exact={self.bob.pk}"), + ("lisa", False, f"?author__id__exact={self.lisa.pk}"), + ] + for i, (display, selected, query_string) in enumerate(expected_choice_values): + self.assertEqual(choices[i]["display"], display) + self.assertIs(choices[i]["selected"], selected) + self.assertEqual(choices[i]["query_string"], query_string) + + def test_multi_choice_field_filter(self): + modeladmin = DecadeFilterBookAdmin(Book, site) + request = self.request_factory.get( + "/", + [("category__exact", "non-fiction"), ("category__exact", "fiction")], + ) + request.user = self.alfred + changelist = modeladmin.get_changelist_instance(request) + queryset = changelist.get_queryset(request) + self.assertSequenceEqual( + queryset, + list( + Book.objects.filter(category__in=["non-fiction", "fiction"]).order_by( + "-id" + ) + ), + ) + filterspec = changelist.get_filters(request)[0][3] + choices = list(filterspec.choices(changelist)) + expected_choice_values = [ + ("All", False, "?"), + ("Non-Fictional", True, "?category__exact=non-fiction"), + ("Fictional", True, "?category__exact=fiction"), + ("We don't know", False, "?category__exact="), + ("Not categorized", False, "?category__isnull=True"), + ] + for i, (display, selected, query_string) in enumerate(expected_choice_values): + self.assertEqual(choices[i]["display"], display) + self.assertIs(choices[i]["selected"], selected) + self.assertEqual(choices[i]["query_string"], query_string) + + def test_multi_all_values_field_filter(self): + modeladmin = DecadeFilterBookAdmin(Book, site) + request = self.request_factory.get( + "/", + [ + ("author__email", "bob@example.com"), + ("author__email", "lisa@example.com"), + ], + ) + request.user = self.alfred + changelist = modeladmin.get_changelist_instance(request) + queryset = changelist.get_queryset(request) + self.assertSequenceEqual( + queryset, + list( + Book.objects.filter( + author__email__in=["bob@example.com", "lisa@example.com"] + ).order_by("-id") + ), + ) + filterspec = changelist.get_filters(request)[0][5] + choices = list(filterspec.choices(changelist)) + expected_choice_values = [ + ("All", False, "?"), + ("alfred@example.com", False, "?author__email=alfred%40example.com"), + ("bob@example.com", True, "?author__email=bob%40example.com"), + ("lisa@example.com", True, "?author__email=lisa%40example.com"), + ] + for i, (display, selected, query_string) in enumerate(expected_choice_values): + self.assertEqual(choices[i]["display"], display) + self.assertIs(choices[i]["selected"], selected) + self.assertEqual(choices[i]["query_string"], query_string) + def test_two_characters_long_field(self): """ list_filter works with two-characters long field names (#16080). diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py index 113f5f7024bd5..cbe6b8bb1d5dc 100644 --- a/tests/admin_utils/tests.py +++ b/tests/admin_utils/tests.py @@ -7,6 +7,7 @@ from django.contrib.admin import helpers from django.contrib.admin.utils import ( NestedObjects, + build_q_object_from_lookup_parameters, display_for_field, display_for_value, flatten, @@ -424,3 +425,17 @@ def test_flatten_fieldsets(self): def test_quote(self): self.assertEqual(quote("something\nor\nother"), "something_0Aor_0Aother") + + def test_build_q_object_from_lookup_parameters(self): + parameters = { + "title__in": [["Article 1", "Article 2"]], + "hist__iexact": ["history"], + "site__pk": [1, 2], + } + q_obj = build_q_object_from_lookup_parameters(parameters) + self.assertEqual( + q_obj, + models.Q(title__in=["Article 1", "Article 2"]) + & models.Q(hist__iexact="history") + & (models.Q(site__pk=1) | models.Q(site__pk=2)), + )