Opened 16 years ago

Closed 9 months ago

Last modified 9 months ago

#10743 closed New feature (fixed)

Support lookup separators in ModelAdmin.list_display

Reported by: mrts Owned by: Tom Carrick
Component: contrib.admin Version: dev
Severity: Normal Keywords: efficient-admin, list_display
Cc: ales.zoulek@…, zachborboa@…, olivier.dalang@…, Ofer Nave, Adam Johnson, Egor R, Nina Menezes, Tom Carrick Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As with #3400, supporting the __ lookup separator in ModelAdmin.list_display is useful as it avoids calling __unicode__ bluntly, giving users more control over the output:

  • display several fields from the same related model,
  • as listed in #3400, traverse several levels of relations.

One could argue that most of this can be achieved with __unicode__ by cramming the information to its output.

However, there's a larger problem with deferred fields (deferring is exemplified in this comment) -- namely, that couples admin code to the implementation of __unicode__ in related models. That is, knowledge of what fields are referred to in __unicode__ is required to write proper deferred statements.

A case to illustrate how changes in __unicode__ can have dramatic, unintentional impact:

  • assume the related objects are large and complex,
  • to keep the admin changelist view snappy, the developer looks which fields are referred to in their __unicode__ and writes the ModelAdmin.queryset() method correspondingly, selecting only these fields with only(),
  • a single query is all that is needed to display the data in changelist view, admin is snappy, there is much rejoicing,
  • a second person keeps hacking at the related models, unknowing the impact changes to __unicode__ could have to admin, adds another field to it,
  • suddenly rendering the changelist results in n queries, each of which pulls in the whole object.

All that can be avoided with explicit __ support.

Attachments (2)

patch.2.diff (4.9 KB ) - added by Rilt 15 years ago.
patch.diff (4.9 KB ) - added by Rilt 15 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Rilt, 15 years ago

Has patch: set
Keywords: list_display added

Added a patch, but would definitely like someone to look at it and comment. The basic jist of what I did was add an additional check for any strings with __ in them, indicating a field spanning a relationship, and then looping through each relationship until the proper attribute was grabbed. Then in the admin validation, there is a recursive function which accurately checks that the field exists and is not a ManyToMany.

Last edited 9 years ago by Aymeric Augustin (previous) (diff)

by Rilt, 15 years ago

Attachment: patch.2.diff added

by Rilt, 15 years ago

Attachment: patch.diff added

comment:3 by anonymous, 15 years ago

Cc: ales.zoulek@… added

comment:4 by James Bennett, 15 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:5 by anonymous, 14 years ago

milestone: 1.4

comment:6 by Chris Beaven, 14 years ago

Severity: Normal
Type: New feature

comment:7 by Julien Phalip, 14 years ago

Needs documentation: set
Needs tests: set

comment:8 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:2 by Aymeric Augustin, 13 years ago

UI/UX: unset

Added a patch, but would definitely like someone to look at it and comment. The basic jist of what I did was add an additional check for any strings with __ in them, indicating a field spanning a relationship, and then looping through each relationship until the proper attribute was grabbed. Then in the admin validation, there is a recursive function which accurately checks that the field exists and is not a ManyToMany.

Last edited 9 years ago by Aymeric Augustin (previous) (diff)

comment:3 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:4 by Aymeric Augustin, 10 years ago

Resolution: wontfix
Status: newclosed

If I understand correctly, the goal of this ticket is to enable Django to automatically optimize the queryset for building the admin changelist by adding appropriate fields to select_related() and only().

But that can't be done automatically as soon as someone puts a callable in display_list. So we're back to square 1, except Django pretended that it could handle the optimization automatically, and failed at that.

In my opinion that would be worse that just telling the user to write an appropriate get_queryset. For this reason (and also considering limited demand for this feature over the last five years) I'm going to reject this ticket.

comment:5 by Aymeric Augustin, 9 years ago

Resolution: wontfix
Status: closednew

I'm going to reopen this ticket because I disagree with my 19-month-ago-former-self.

My present-day-self keeps trying to write:

    list_display = ['foo__bar']

and being mad at Django that it doesn't work.

We could limit that syntax to relation fields and not support calling methods on the target model if that helps. It would still solve 95% of the problem.

comment:6 by Zach Borboa, 9 years ago

Cc: zachborboa@… added

comment:7 by Olivier Dalang, 8 years ago

Cc: olivier.dalang@… added

comment:8 by Ofer Nave, 3 years ago

I'm eager to have this, and am surprised that it hasn't happened after 13 years.

In the meantime, I'm relying on this very small package which implements a version of this feature:

https://github.com/PetrDlouhy/django-related-admin/

comment:9 by Ofer Nave, 3 years ago

Cc: Ofer Nave added

comment:10 by Adam Johnson, 3 years ago

Cc: Adam Johnson added

comment:11 by Egor R, 3 years ago

Cc: Egor R added

comment:12 by Jacob Walls, 20 months ago

Needs documentation: unset
Needs tests: unset

comment:13 by Jacob Walls, 20 months ago

Owner: changed from nobody to Alex Garcia Ruiz de Oteiza
Status: newassigned

comment:14 by Natalia Bidart, 19 months ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:15 by Tom Carrick, 13 months ago

Owner: changed from Alex Garcia Ruiz de Oteiza to Tom Carrick

I made a new PR from the old one.

PR

comment:16 by Natalia Bidart, 13 months ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Resetting patch flags so it gets shown in the review queue.

comment:17 by Natalia Bidart, 13 months ago

Needs tests: set
Patch needs improvement: set

comment:18 by Tom Carrick, 13 months ago

Needs tests: unset
Patch needs improvement: unset

comment:19 by Mariusz Felisiak, 13 months ago

Patch needs improvement: set

Marking as needs improvement per Natalia's comment.

comment:20 by Tom Carrick, 13 months ago

Patch needs improvement: unset

comment:21 by Natalia Bidart, 10 months ago

Needs tests: set
Patch needs improvement: set

Setting as patch needs improvement following this PR comment stating that selenium test would be needed for the list display column ordering.

comment:22 by Natalia Bidart, 10 months ago

Selenium test was added and it looks great, I just suggested two unit tests for the changes in the admin's utils module. Leaving flags as they are.

comment:23 by Nina Menezes, 9 months ago

Cc: Nina Menezes added
Needs tests: unset
Patch needs improvement: unset

comment:24 by Tom Carrick, 9 months ago

Cc: Tom Carrick added

comment:25 by Natalia Bidart, 9 months ago

Triage Stage: AcceptedReady for checkin

comment:26 by Natalia <124304+nessita@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In 4ade8386:

Fixed #10743 -- Allowed lookups for related fields in ModelAdmin.list_display.

Co-authored-by: Alex Garcia <me@…>
Co-authored-by: Natalia <124304+nessita@…>
Co-authored-by: Nina Menezes <https://github.com/nmenezes0>

comment:27 by Natalia <124304+nessita@…>, 9 months ago

In 9cefdfc:

Refs #10743 -- Enabled ordering for lookups in ModelAdmin.list_display.

Co-authored-by: Natalia <124304+nessita@…>
Co-authored-by: Nina Menezes <https://github.com/nmenezes0>

comment:28 by GitHub <noreply@…>, 9 months ago

In 3e820d10:

Refs #10743 -- Removed leftover comment in tests/admin_changelist/tests.py.

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