Code

Opened 7 years ago

Closed 3 years ago

#3397 closed (fixed)

Add tests for 'Allow DB-level ordering by non-fields in changelist view' admin functionality

Reported by: marcink@… Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: nfa-changelist
Cc: djangotrac@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

The attached patch lets you specify a field to sort a non-field
column in the admin list view. I created it for the django-multilingual
library, but there are more cases where this could prove useful.
See the example in docs/model-api.txt diff.

Attachments (3)

patch_allow_ordering_by_nonfields.diff (5.2 KB) - added by marcink@… 7 years ago.
Diff against trunk
admin_list.diff (1.4 KB) - added by kent37@… 7 years ago.
Patch to allow descending sort of non-DB fields
patch_allow_ordering_by_nonfields_fix.diff (610 bytes) - added by anonymous 7 years ago.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by marcink@…

Diff against trunk

comment:1 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

Seems useful.

comment:2 Changed 7 years ago by marcink@…

  • Summary changed from [patch] Allow ordering by non-fields in admin list view to [patch] Allow DB-level ordering by non-fields in admin list view

Renamed the ticket to clarify it.

comment:3 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Ready for checkin

This seems reasonable. It's a little bit hard to understand until you read the docs, but the example gets the point across. The documentation paragraph probably needs a rewrite -- it feels a bit awkward at the moment. But I'm too tired to get it right now; we just need to remember to tweak that before checking in. Other than that, this looks fine.

comment:4 Changed 7 years ago by jacob

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

(In [4596]) Fixed #3397: You can now order by non-DB fields in the admin by telling Django which field to actually order by. Thanks, marcink@…

comment:5 Changed 7 years ago by kent@…

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Unreviewed

This doesn't quite work correctly. It allows clicking on the column title to sort in ascending order but it doesn't display the triangle and a second click will sort ascending rather than descending.

The fix is for admin_list.py line 104 to also check if the admin_order_field equals cl.order_field.

Patch attached.

Changed 7 years ago by kent37@…

Patch to allow descending sort of non-DB fields

comment:6 Changed 7 years ago by jacob

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

(In [5318]) Fixed #3397 (again): admin_order_field is now displayed correctly in the admin views. Thanks, kent37@…

Changed 7 years ago by anonymous

comment:7 Changed 7 years ago by glin@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unless I'm mistaken, in django/contrib/admin/views/main.py in section added in this patch, there(see my patch patch_allow_ordering_by_nonfields_fix.diff) should be AttributeError instead of IndexError (IndexError cannot raise there and IMO author meant there AttributeError)

comment:8 Changed 7 years ago by marcin@…

glin: you are right, it should be an AttributeError. I missed it because usually this code path is not executed at all, but it is a bug and should be fixed.

comment:9 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

The file "patch_allow_ordering_by_nonfields_fix.diff" should be checked in following the last two comments (i.e. should be AttributeError not IndexError.

comment:10 Changed 7 years ago by gwilson

(In [5799]) Refs #3397 -- Corrected the Exception that is caught when ordering by non-fields (added in [4596]), thanks glin@….

comment:11 Changed 7 years ago by gwilson

  • Has patch unset
  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

Just because the other parts of the admin don't have tests doesn't mean that we should continue the trend.

comment:13 Changed 6 years ago by jakub_vysoky

  • Keywords nfa-changelist added

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

Can someone verify if this broken on trunk? It sounds likely it will work just fine, but needs really needs a test case.

comment:15 in reply to: ↑ 14 Changed 6 years ago by Karen Tracey <kmtracey@…>

Replying to brosner:

Can someone verify if this broken on trunk? It sounds likely it will work just fine, but needs really needs a test case.

Verified the function works correctly on trunk [8169]. I guess the ticket was just reopened because the function lacks a test.

comment:16 Changed 5 years ago by visik7

  • Cc djangotrac@… added

comment:17 Changed 3 years ago by ramiro

  • Summary changed from [patch] Allow DB-level ordering by non-fields in admin list view to Add tests for 'Allow DB-level ordering by non-fields in changelist view' admin functionality

comment:18 Changed 3 years ago by kmtracey

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

In the intervening years since this was reopened, tests have been added for admin_order_field, see for example: http://code.djangoproject.com/browser/django/tags/releases/1.2.5/tests/regressiontests/admin_views/tests.py#L192

I think it's OK to close this one as fixed now.

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.