Opened 2 months ago
Closed 2 months ago
#35747 closed Bug (fixed)
Admin change list doesn't redirect to default ordering when the last ordering field is removed from sorting
Reported by: | ldeluigi | Owned by: | ldeluigi |
---|---|---|---|
Component: | contrib.admin | Version: | 5.1 |
Severity: | Normal | Keywords: | admin change list ordering sorting sortremove |
Cc: | ldeluigi | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description (last modified by )
Basically, when an admin maually clicks on the sortremove link of the last remaining field in the sorting query variable, the redirect points to an empty string.
For example, if the query parameter is the default o
, the redirect points to /?o=
, which results in the change list not being ordered at all. Instead, I'm claiming that users would expect to see the same ordering they experience when landing on the change list page in the first place, which was the default ordering, only visible when the ordering parameter is absent from the sortremove query.
For this reason, I'd like the sortremove to redirect to /
instead of /?o=
when the last ordering field would be removed.
I'm opening a PR that *should* do the job: https://github.com/django/django/pull/18558
Change History (8)
comment:1 by , 2 months ago
Description: | modified (diff) |
---|
comment:2 by , 2 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
For example, if the query parameter is the default o, the redirect points to /?o=, which results in the change list not being ordered at all.
To me, the change list is ordered with the default ordering. The columns "appear" to have no ordering applied which might cause confusion. But I also think the suggested change makes the UI confusing as you are not able to "remove" the ordering in the UI when you have it ordered by one column, click to toggle it off, and the page reloads without any changes to the UI.
I don't think there is a clear bug here, and currently prefer the way it is.
Your PR would need a test which might help clarify the expected behavior
comment:3 by , 2 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
To me, the change list is ordered with the default ordering.
It's not: I've defined default ordering in my model's Meta class, like this:
... class Meta: .... ordering = [ models.Case( models.When(status='D', then=models.Value(0)), models.When(status='N', then=models.Value(1)), ..., default=models.Value(10), ), '-created' ]
If I navigate to the admin change list page, without query parameters in the URL, I see that the ordering is applied to the results.
Instead, if I navigate to .../?o=
I see a totally different ordering being applied.
By enabling the debugger, I can see that the latter sorts by id descending and doesn't honor the ordering of the model's meta class. This is caused by the behaviour of _get_deterministic_ordering
on the ChangeList class, which receives an empty list because the get_ordering
method overwrites the default ordering with an empty list whenever the ORDER_VAR is passed in params.
comment:4 by , 2 months ago
Another option would be to honor the default ordering even when /?o=
is passed. This would mean that changing the url of the "sortremove" link is not necessary anymore, as it would make that case behave the same as /?
comment:5 by , 2 months ago
Easy pickings: | unset |
---|---|
Needs tests: | set |
Owner: | set to |
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
Thank you for the clarification
Have a test and an alternative patch. Needs testing in the admin but I think I've replicated the behavior here
-
django/contrib/admin/views/main.py
a b class ChangeList: 395 395 ordering = list( 396 396 self.model_admin.get_ordering(request) or self._get_default_ordering() 397 397 ) 398 if ORDER_VAR in params :398 if ORDER_VAR in params and params[ORDER_VAR]: 399 399 # Clear ordering and used params 400 400 ordering = [] 401 401 order_params = params[ORDER_VAR].split(".") -
tests/admin_changelist/tests.py
diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 694f807781..5324f39364 100644
a b class ChangeListTests(TestCase): 1375 1375 OrderedObjectAdmin.ordering = ["id", "bool"] 1376 1376 check_results_order(ascending=True) 1377 1377 1378 def test_ordering_from_model_meta(self): 1379 Swallow.objects.create(origin="Swallow A", load=4, speed=2) 1380 Swallow.objects.create(origin="Swallow B", load=2, speed=1) 1381 Swallow.objects.create(origin="Swallow C", load=5, speed=1) 1382 m = SwallowAdmin(Swallow, custom_site) 1383 request = self._mocked_authenticated_request("/swallow/?o=", self.superuser) 1384 changelist = m.get_changelist_instance(request) 1385 queryset = changelist.get_queryset(request) 1386 self.assertQuerySetEqual( 1387 queryset, 1388 [(1.0, 2.0), (1.0, 5.0), (2.0, 4.0)], 1389 lambda s: (s.speed, s.load), 1390 ) 1391 1378 1392 @isolate_apps("admin_changelist") 1379 1393 def test_total_ordering_optimization(self): 1380 1394 class Related(models.Model):
comment:7 by , 2 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
added link to pr