[Django] #35747: Admin change list doesn't redirect to default ordering when the last ordering field is removed from sorting

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Type: Bug
Status: new | Component:
| contrib.admin
Version: 5.1 | Severity: Normal
Keywords: admin change list | Triage Stage:
ordering sorting sortremove | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35747>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------

Reporter: ldeluigi | Owner: (none)

Type: Bug | Status: new
Component: contrib.admin | Version: 5.1

Severity: Normal | Resolution:

Keywords: admin change list | Triage Stage:
ordering sorting sortremove | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------

Old description:

> 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.

New description:

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

--
Comment (by ldeluigi):

added link to pr
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:1>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: (none)

Type: Bug | Status: closed
Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution: invalid

Keywords: admin change list | Triage Stage:
ordering sorting sortremove | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* resolution: => invalid
* status: new => closed

Comment:

> 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
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:2>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: (none)

Type: Bug | Status: new

Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution:

Keywords: admin change list | Triage Stage:
ordering sorting sortremove | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------

Changes (by ldeluigi):

* resolution: invalid =>
* status: closed => new

Comment:

> 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
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:3>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution:
Keywords: admin change list | Triage Stage:
ordering sorting sortremove | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------

Comment (by ldeluigi):

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 `/?`
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:4>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------

Reporter: ldeluigi | Owner: ldeluigi
Type: Bug | Status: assigned

Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution:

Keywords: admin change list | Triage Stage: Accepted
ordering sorting sortremove |

Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* easy: 1 => 0
* needs_tests: 0 => 1
* owner: (none) => ldeluigi
* stage: Unreviewed => Accepted
* status: new => assigned

Comment:

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
{{{#!diff
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -395,7 +395,7 @@ class ChangeList:
ordering = list(
self.model_admin.get_ordering(request) or
self._get_default_ordering()
)
- if ORDER_VAR in params:
+ if ORDER_VAR in params and params[ORDER_VAR]:
# Clear ordering and used params
ordering = []
order_params = params[ORDER_VAR].split(".")
diff --git a/tests/admin_changelist/tests.py
b/tests/admin_changelist/tests.py
index 694f807781..5324f39364 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -1375,6 +1375,20 @@ class ChangeListTests(TestCase):
OrderedObjectAdmin.ordering = ["id", "bool"]
check_results_order(ascending=True)

+ def test_ordering_from_model_meta(self):
+ Swallow.objects.create(origin="Swallow A", load=4, speed=2)
+ Swallow.objects.create(origin="Swallow B", load=2, speed=1)
+ Swallow.objects.create(origin="Swallow C", load=5, speed=1)
+ m = SwallowAdmin(Swallow, custom_site)
+ request = self._mocked_authenticated_request("/swallow/?o=",
self.superuser)
+ changelist = m.get_changelist_instance(request)
+ queryset = changelist.get_queryset(request)
+ self.assertQuerySetEqual(
+ queryset,
+ [(1.0, 2.0), (1.0, 5.0), (2.0, 4.0)],
+ lambda s: (s.speed, s.load),
+ )
+
@isolate_apps("admin_changelist")
def test_total_ordering_optimization(self):
class Related(models.Model):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:5>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: ldeluigi
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution:
Keywords: admin change list | Triage Stage: Accepted
ordering sorting sortremove |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------

Changes (by ldeluigi):

* needs_tests: 1 => 0

Comment:

Implemented the requested patch at
[https://github.com/django/django/pull/18558 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:6>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: ldeluigi
Type: Bug | Status: assigned
Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution:

Keywords: admin change list | Triage Stage: Ready for
ordering sorting sortremove | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:7>

[Django]

#35747: Admin change list doesn't redirect to default ordering when the last
ordering field is removed from sorting
-------------------------------------+-------------------------------------
Reporter: ldeluigi | Owner: ldeluigi

Type: Bug | Status: closed
Component: contrib.admin | Version: 5.1
Severity: Normal | Resolution: fixed

Keywords: admin change list | Triage Stage: Ready for
ordering sorting sortremove | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"2a4321ba23042c962f8dc966779239244c6e4402" 2a4321ba]:
{{{#!CommitTicketReference repository=""
revision="2a4321ba23042c962f8dc966779239244c6e4402"
Fixed #35747 -- Used default ordering when the ORDER_VAR param is blank in
the admin changelist.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35747#comment:8>