Skip to content

Commit

Permalink
Fixed #1873 -- Handled multi-valued query parameters in admin changel…
Browse files Browse the repository at this point in the history
…ist filters.
  • Loading branch information
sarahboyce authored and felixxm committed Mar 16, 2023
1 parent d03dc63 commit d2b688b
Show file tree
Hide file tree
Showing 7 changed files with 146 additions and 16 deletions.
18 changes: 11 additions & 7 deletions django/contrib/admin/filters.py
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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]
),
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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]
),
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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]
),
Expand Down
11 changes: 10 additions & 1 deletion django/contrib/admin/utils.py
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
15 changes: 9 additions & 6 deletions django/contrib/admin/views/main.py
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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],
}
)

Expand Down Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions docs/releases/5.0.txt
Expand Up @@ -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`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
4 changes: 2 additions & 2 deletions tests/admin_changelist/test_date_hierarchy.py
Expand Up @@ -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 = (
Expand Down
94 changes: 94 additions & 0 deletions tests/admin_filters/tests.py
Expand Up @@ -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).
Expand Down
15 changes: 15 additions & 0 deletions tests/admin_utils/tests.py
Expand Up @@ -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,
Expand Down Expand Up @@ -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)),
)

0 comments on commit d2b688b

Please sign in to comment.