Code

Opened 7 years ago

Closed 3 years ago

#4926 closed Bug (duplicate)

Ordering in admin listview ignores ordering in admin options

Reported by: Glin <glin@…> Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: nfa-someday nfa-changelist
Cc: smoonen@…, james.m.pearson+django@…, django@…, rob@…, patrickk Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When I specify ordering in admin options like this:

class ArticleOptions(admin.ModelAdmin):
    list_display = ('title', 'is_news', 'published_from_date')
    ordering = ('is_news', 'pub_date')

Ordering set default ordering column to 'is_news', but doesn't take 'pub_date' into account.

Generally all other fields except first specified in ordering tuple are ignored.

Attachments (4)

ordering_patch_main.py.diff (517 bytes) - added by Glin <glin@…> 7 years ago.
ticket_4926_changelist_multifield_ordering.diff (705 bytes) - added by Eric B <ebartels@…> 6 years ago.
ticket_4926_changelist_multifield_ordering_r12379.diff (705 bytes) - added by akaihola 4 years ago.
patch against current trunk, otherwise unmodified
ticket_4926_changelist_multifield_ordering_r12379.2.diff (1.3 KB) - added by akaihola 4 years ago.
also removes the related admonition from contrib.admin documentation

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by Glin <glin@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

It is because query.order_by rewrites previous ordering. In this case, it is on this line:

qs = qs.order_by(order_type + lookup_order_field)

So ordering from 'ordering' admin options are not taken in account (and 'ordering' from Meta, too).

I created a patch for this.

Changed 7 years ago by Glin <glin@…>

comment:2 Changed 7 years ago by Glin <glin@…>

  • Has patch set

comment:3 Changed 7 years ago by jkocherhans

(In [6232]) newforms-admin: Fixed ordering for ModelAdmin.queryset. Refs #4926, but that ticket still needs ModelAdmin.changelist_view to use ModelAdmin.queryset.

comment:4 Changed 7 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from new to closed

(In [6239]) newforms-admin: Fixed a problem with [6232] where an exception was raised if ModelAdmin didn't specify ordering. Also, fixed #4926.

comment:5 Changed 6 years ago by Glin <glin@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately this did not solve the problem.

Problem is that any order_by() call deletes ordering of previous order_by() call (e.g. qs.order_by('is_news').order_by('pub_date') is the same as qs.order_by('pub_date') ).

So problem must be fixed in code of ChangeList class, because there is the last call of order_by method. (as in my patch)

comment:6 Changed 6 years ago by ramiro

See ticket #5673 and this discussion http://groups.google.com/group/django-developers/browse_frm/thread/c74afae800fda0b9?tvc=1 for a discussion about multi-column ordering in the admin UI (it's not about the admin app ordering option bout about the model's Meta.ordering setting though).

comment:7 Changed 6 years ago by Glin <glin@…>

I looked at it, initial comment is nearly about the same issue as this ticket (later in discussion, there is mentioned multi column sorting in way that user can influence these sorting columns.)

My patch only solves first issue, so admin still sorts primary according to one column selected by user, and other sorting columns are immutable (they are specified in model_admin.ordering). So it is only small (one and half line) patch, so way don't use this?

comment:8 follow-up: Changed 6 years ago by brosner

  • Keywords nfa-someday added

This ticket isn't critical to the merge of newforms-admin since it is an enhancement. Tagging with nfa-someday.

comment:9 Changed 6 years ago by anonymous

  • Triage Stage changed from Unreviewed to Someday/Maybe

comment:10 Changed 6 years ago by ales_zoulek

  • Keywords nfa-changelist added

comment:11 Changed 6 years ago by Eric B <ebartels@…>

I realized this is considered not to be critical for merging, but I'm submitting a new patch against the latest code.

Here's the new behavior: In the case where multiple fields are specified, that ordering will be respected by default. In this case, the changelist won't show a single table column as being sorted until a column header has been clicked.

This seems much more consistent to me than the current behavior.
It's a little confusing that ModelAdmin requires ordering as a tuple/list of fields, and then ignores all but the first field.

Changed 6 years ago by Eric B <ebartels@…>

comment:12 Changed 6 years ago by smoonen@…

  • Cc smoonen@… added
  • Version changed from newforms-admin to SVN

Me-too (now that nfa has hit the trunk). Most of my models have multiple fields in their ordering. Changing the version of this ticket to 'SVN' now that nfa is on trunk.

comment:13 Changed 6 years ago by xiongchiamiov

  • Cc james.m.pearson+django@… added

comment:14 Changed 5 years ago by Jonas

  • Cc django@… added

comment:15 Changed 5 years ago by Xiong Chiamiov

This seems like an easy fix without many consequences to me. It would be nice to either get this merged in (or a "this should go in, but the patch needs work") or a wontfix close. If the previous is the case, I'm willing to work on it. If the latter, then it'll just be nice to have a resolution of some sort.

comment:16 Changed 5 years ago by fabiocorneti

This is a very useful feature (especially for django-mptt users) and the patch seems quite unobtrusive, +1 for merging it into trunk

comment:17 Changed 5 years ago by bendavis78

If anyone is interested, I've submitted a patch for a proposed UI for sorting by multiple fields. This would also fix this particular issue.

See here: #11868

Any feedback is appreaciated.

Changed 4 years ago by akaihola

patch against current trunk, otherwise unmodified

Changed 4 years ago by akaihola

also removes the related admonition from contrib.admin documentation

comment:18 in reply to: ↑ 8 Changed 4 years ago by akaihola

Replying to brosner:

This ticket isn't critical to the merge of newforms-admin since it is an enhancement. Tagging with nfa-someday.

I'd consider this a little more than an enhancement. As Eric B noted, it's confusing that a tuple/list of fields is required but all but the first field are ignored.

comment:19 Changed 4 years ago by Mogga <rob@…>

I agree.

This for me, is most certainly a bug...

For example, I have a set of teams and each team has a set of players with numbers.
I would like to order them by team and then by number but the following does not do that.

ordering=('team','number')

In fact any number of fields are irrelevant given it really only ever orders like this:

ordering=('team')

The behavior is just wrong so for me it's a bug, not an enhancement.

comment:20 Changed 4 years ago by Mogga <rob@…>

  • Cc rob@… added

comment:21 Changed 4 years ago by Mogga <rob@…>

that patch works perfectly here...

comment:22 Changed 3 years ago by patrickk

  • Cc patrickk added

comment:23 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:24 Changed 3 years ago by julien

  • Resolution set to duplicate
  • Status changed from reopened to closed

Closing as a duplicate of #11868 which, although newer, suggests a better approach UI-wise and has a more recent patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.