Skip to content

Commit

Permalink
[5.0.x] Fixed #35198 -- Fixed facet filters crash on querysets with n…
Browse files Browse the repository at this point in the history
…o primary key.

Thanks Simon Alef for the report.

Regression in 868e2fc.

Backport of a738281 from main
  • Loading branch information
adzshaf authored and felixxm committed Feb 29, 2024
1 parent 24de811 commit 80761c3
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 10 deletions.
2 changes: 1 addition & 1 deletion django/contrib/admin/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def get_facet_counts(self, pk_attname, filtered_qs):
if lookup_qs is not None:
counts[f"{i}__c"] = models.Count(
pk_attname,
filter=lookup_qs.query.where,
filter=models.Q(pk__in=lookup_qs),
)
self.used_parameters[self.parameter_name] = original_value
return counts
Expand Down
3 changes: 3 additions & 0 deletions docs/releases/5.0.3.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,6 @@ Bugfixes
* Fixed a regression in Django 5.0 that caused a crash when reloading a test
database and a base queryset for a base manager used ``prefetch_related()``
(:ticket:`35238`).

* Fixed a bug in Django 5.0 where facet filters in the admin would crash on a
``SimpleListFilter`` using a queryset without primary keys (:ticket:`35198`).
53 changes: 44 additions & 9 deletions tests/admin_filters/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from django.contrib.auth.admin import UserAdmin
from django.contrib.auth.models import User
from django.core.exceptions import ImproperlyConfigured
from django.db import connection
from django.db import connection, models
from django.test import RequestFactory, SimpleTestCase, TestCase, override_settings

from .models import Book, Bookmark, Department, Employee, ImprovedBook, TaggedItem
Expand Down Expand Up @@ -154,6 +154,30 @@ def expected_parameters(self):
return [self.lookup_kwarg]


class DepartmentOwnershipListFilter(SimpleListFilter):
title = "Department Ownership"
parameter_name = "department_ownership"

def lookups(self, request, model_admin):
return [
("DEV_OWNED", "Owned by Dev Department"),
("OTHER", "Other"),
]

def queryset(self, request, queryset):
queryset = queryset.annotate(
owned_book_count=models.Count(
"employee__department",
filter=models.Q(employee__department__code="DEV"),
),
)

if self.value() == "DEV_OWNED":
return queryset.filter(owned_book_count__gt=0)
elif self.value() == "OTHER":
return queryset.filter(owned_book_count=0)


class CustomUserAdmin(UserAdmin):
list_filter = ("books_authored", "books_contributed")

Expand Down Expand Up @@ -229,6 +253,7 @@ class DecadeFilterBookAdmin(ModelAdmin):
("author__email", AllValuesFieldListFilter),
("contributors", RelatedOnlyFieldListFilter),
("category", EmptyFieldListFilter),
DepartmentOwnershipListFilter,
)
ordering = ("-id",)

Expand Down Expand Up @@ -336,6 +361,14 @@ def setUpTestData(cls):
cls.bob = User.objects.create_user("bob", "bob@example.com")
cls.lisa = User.objects.create_user("lisa", "lisa@example.com")

# Departments
cls.dev = Department.objects.create(code="DEV", description="Development")
cls.design = Department.objects.create(code="DSN", description="Design")

# Employees
cls.john = Employee.objects.create(name="John Blue", department=cls.dev)
cls.jack = Employee.objects.create(name="Jack Red", department=cls.design)

# Books
cls.djangonaut_book = Book.objects.create(
title="Djangonaut: an art of living",
Expand All @@ -345,6 +378,7 @@ def setUpTestData(cls):
date_registered=cls.today,
availability=True,
category="non-fiction",
employee=cls.john,
)
cls.bio_book = Book.objects.create(
title="Django: a biography",
Expand All @@ -354,6 +388,7 @@ def setUpTestData(cls):
no=207,
availability=False,
category="fiction",
employee=cls.john,
)
cls.django_book = Book.objects.create(
title="The Django Book",
Expand All @@ -363,6 +398,7 @@ def setUpTestData(cls):
date_registered=cls.today,
no=103,
availability=True,
employee=cls.jack,
)
cls.guitar_book = Book.objects.create(
title="Guitar for dummies",
Expand All @@ -374,14 +410,6 @@ def setUpTestData(cls):
)
cls.guitar_book.contributors.set([cls.bob, cls.lisa])

# Departments
cls.dev = Department.objects.create(code="DEV", description="Development")
cls.design = Department.objects.create(code="DSN", description="Design")

# Employees
cls.john = Employee.objects.create(name="John Blue", department=cls.dev)
cls.jack = Employee.objects.create(name="Jack Red", department=cls.design)

def assertChoicesDisplay(self, choices, expected_displays):
for choice, expected_display in zip(choices, expected_displays, strict=True):
self.assertEqual(choice["display"], expected_display)
Expand Down Expand Up @@ -905,6 +933,7 @@ class EmployeeAdminReverseRelationship(ModelAdmin):
filterspec.lookup_choices,
[
(self.djangonaut_book.pk, "Djangonaut: an art of living"),
(self.bio_book.pk, "Django: a biography"),
(self.django_book.pk, "The Django Book"),
],
)
Expand Down Expand Up @@ -1407,6 +1436,8 @@ def _test_facets(self, modeladmin, request, query_string=None):
["All", "bob (1)", "lisa (1)", "??? (3)"],
# EmptyFieldListFilter.
["All", "Empty (2)", "Not empty (2)"],
# SimpleListFilter with join relations.
["All", "Owned by Dev Department (2)", "Other (2)"],
]
for filterspec, expected_displays in zip(filters, tests, strict=True):
with self.subTest(filterspec.__class__.__name__):
Expand Down Expand Up @@ -1482,6 +1513,8 @@ def test_facets_filter(self):
["All", "bob (0)", "lisa (0)", "??? (2)"],
# EmptyFieldListFilter.
["All", "Empty (0)", "Not empty (2)"],
# SimpleListFilter with join relations.
["All", "Owned by Dev Department (2)", "Other (0)"],
]
for filterspec, expected_displays in zip(filters, tests, strict=True):
with self.subTest(filterspec.__class__.__name__):
Expand Down Expand Up @@ -1525,6 +1558,8 @@ def test_facets_disallowed(self):
["All", "bob", "lisa", "???"],
# EmptyFieldListFilter.
["All", "Empty", "Not empty"],
# SimpleListFilter with join relations.
["All", "Owned by Dev Department", "Other"],
]
for filterspec, expected_displays in zip(filters, tests, strict=True):
with self.subTest(filterspec.__class__.__name__):
Expand Down

0 comments on commit 80761c3

Please sign in to comment.