Opened 3 months ago

Closed 3 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 ldeluigi)

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 ldeluigi, 3 months ago

Description: modified (diff)

added link to pr

comment:2 by Sarah Boyce, 3 months ago

Resolution: invalid
Status: newclosed

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 ldeluigi, 3 months ago

Resolution: invalid
Status: closednew

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.

Reference: https://github.com/django/django/blob/cdbd31960e0cf41063b3efac97292ee0ccc262bb/django/contrib/admin/views/main.py#L385

comment:4 by ldeluigi, 3 months ago

Another option would be to keep 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 /?

Version 0, edited 3 months ago by ldeluigi (next)

comment:5 by Sarah Boyce, 3 months ago

Easy pickings: unset
Needs tests: set
Owner: set to ldeluigi
Status: newassigned
Triage Stage: UnreviewedAccepted

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:  
    395395        ordering = list(
    396396            self.model_admin.get_ordering(request) or self._get_default_ordering()
    397397        )
    398         if ORDER_VAR in params:
     398        if ORDER_VAR in params and params[ORDER_VAR]:
    399399            # Clear ordering and used params
    400400            ordering = []
    401401            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):  
    13751375        OrderedObjectAdmin.ordering = ["id", "bool"]
    13761376        check_results_order(ascending=True)
    13771377
     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
    13781392    @isolate_apps("admin_changelist")
    13791393    def test_total_ordering_optimization(self):
    13801394        class Related(models.Model):

comment:6 by ldeluigi, 3 months ago

Needs tests: unset

Implemented the requested patch at PR

comment:7 by Sarah Boyce, 3 months ago

Triage Stage: AcceptedReady for checkin

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 3 months ago

Resolution: fixed
Status: assignedclosed

In 2a4321ba:

Fixed #35747 -- Used default ordering when the ORDER_VAR param is blank in the admin changelist.

Note: See TracTickets for help on using tickets.
Back to Top