Opened 8 years ago

Closed 4 years ago

Last modified 16 months ago

#5863 closed Uncategorized (wontfix)

list_display does not allow functions of referenced objects

Reported by: Beat Bolli <me+django@…> Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: list_display
Cc: andy@…, mike.huynh+django@…, brillgen Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description (last modified by gwilson)

It is possible to include a model member function in the list_display Admin property. The same thing is not possible for ForeignKey models; at least not with any of the variations I have tried: foreign.method, foreign_method and foreign__method.

Attachments (2)

list_display_test.patch (591 bytes) - added by crippledcanary@… 7 years ago.
patch to regressiontests.modeladmin with a test that I would like to work
nested_list_display.diff (6.8 KB) - added by graham@… 7 years ago.
Allow double underscore style access to related object fields in list_display

Download all attachments as: .zip

Change History (46)

comment:1 Changed 8 years ago by Beat Bolli <me+django@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from list_display does not allow functions of references objects to list_display does not allow functions of referenced objects

comment:2 Changed 8 years ago by Beat Bolli <me+django@…>

Sorry; the third example should have two underline characters between "foreign" and "method".

comment:3 Changed 8 years ago by brosner

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

There is no need to support functions on ForeignKey models. You can encapsulate that in a function on the model.

comment:4 Changed 8 years ago by me+django@…

  • Resolution invalid deleted
  • Status changed from closed to reopened

Thanks for the quick reply, which I have implemented for now.

Still, your proposal violates DRY: I have to define the same function on each model that references the foreign model.

comment:5 Changed 8 years ago by gwilson

  • Description modified (diff)
  • Triage Stage changed from Unreviewed to Accepted

Seems like a reasonable addition to me. Do we even allow doing this for actual fields of a related model? This feature probably needs to be part of newforms-admin or wait until that branch is merged though. It would be helpful if you could attach a simple example, or even try your hand at creating a patch.

comment:6 Changed 7 years ago by zobbo

I just wanted to raise this again. I'm using the new forms admin branch and it's odd that in the following

class BlueEDIJobFileAdmin(admin.ModelAdmin):
    search_fields=('blueedijob__client__code',)
    list_display = ('id','blueedijob__client__code',)

The search_fields option is valid, but the list_display option is not.

comment:7 Changed 7 years ago by crippledcanary@…

I want to raise it once again since newforms-admin is now in...

Changed 7 years ago by crippledcanary@…

patch to regressiontests.modeladmin with a test that I would like to work

Changed 7 years ago by graham@…

Allow double underscore style access to related object fields in list_display

comment:8 Changed 7 years ago by graham@…

The patch uploaded shows the related objects field values in the same way as the object field values are shown (a boolean as an tick icon etc.) rather than as a string as a custom method would.

However the header of the field still shows the related object's verbose name. Ideally it would show something like (section -> title) to indicate its a related objects field, but I haven't worked out how to do that yet :)

comment:9 follow-up: Changed 7 years ago by pihentagy

What is the next step to see this patch in django?

thanks

comment:10 in reply to: ↑ 9 Changed 7 years ago by kmtracey

  • Has patch set
  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Accepted to Design decision needed

Replying to pihentagy:

What is the next step to see this patch in django?

Since this ticket was raised callables were added to the list of things allowed, in list_display, as were methods on ModelAdmin, in [8352]. Do we need yet another way to specify something in list_display? Can this case not be covered by callables?

I was going to experiment to see, but I can't. First, the patch doesn't apply cleanly to existing trunk. The actual apply failures are on the tests, not the code, so I thought I could experiment with the code, but I can't get that to work either. If I add something with the double underscore syntax to the list_display for one of the models in my app, I get an ImproperlyConfigured exception thrown during admin validation:

Traceback:
File "/home/kmt/django/trunk/django/core/handlers/base.py" in get_response
  77.                     request.path_info)
File "/home/kmt/django/trunk/django/core/urlresolvers.py" in resolve
  179.             for pattern in self.urlconf_module.urlpatterns:
File "/home/kmt/django/trunk/django/core/urlresolvers.py" in _get_urlconf_module
  198.             self._urlconf_module = __import__(self.urlconf_name, {}, {}, [''])
File "/home/kmt/software/web/xword/../xword/urls.py" in <module>
  9. admin.autodiscover()
File "/home/kmt/django/trunk/django/contrib/admin/__init__.py" in autodiscover
  40.         __import__("%s.admin" % app)
File "/home/kmt/software/web/xword/../xword/crossword/admin.py" in <module>
  61. admin.site.register(Puzzles, PuzzlesAdmin)
File "/home/kmt/django/trunk/django/contrib/admin/sites.py" in register
  76.             validate(admin_class, model)
File "/home/kmt/django/trunk/django/contrib/admin/validation.py" in validate
  38.                                 % (cls.__name__, idx, field, cls.__name__, model._meta.object_name))

Exception Type: ImproperlyConfigured at /admin/crossword/authors/
Exception Value: PuzzlesAdmin.list_display[4], 'AuthorID__Notes' is not a callable or an attribute of 'PuzzlesAdmin' or found in the model 'Puzzles'.

This may be validation code added since the patch was originally created, I don['t know -- I think I specified the new thing correctly but I'm not entirely sure because there are no docs for the new supported thing in list_display. The existing docs spell out pretty clearly what's allowed: http://docs.djangoproject.com/en/dev/ref/contrib/admin/#list-display and they would also need to be updated if it's decided this addition is still worthwhile.

So a working patch with docs that applies cleanly to trunk (and doesn't have the same problem with sorting that [8352] introduced, see [9212]), plus something that addresses graham's last remark about the title for the column being non-intuitive, are all missing for this to get in...but even before that please answer the first question I had: what does this provide that isn't already possible given the expanded options of things you can now specify in list_display?

comment:11 follow-up: Changed 7 years ago by pihentagy

Replying to kmtracey:

but even before that please answer the first question I had: what does this provide that isn't already possible given the expanded options of things you can now specify in list_display?

Consistency and DRY: so you shouldn't write boilerplate code. You shouldn't specify column name django alredy knows.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by kmtracey

Replying to pihentagy:

Consistency and DRY: so you shouldn't write boilerplate code. You shouldn't specify column name django alredy knows.

I understand wanting to avoid boilerplate code but I'm missing how this is a common enough case to warrant inclusion and balance the additional complexity (both for users -- the list of things you can put in list_display is getting a bit long -- and code maintainers). What's the common situation where user's would really like to see the value of a field in a related model instead of just the text representation of the related model? I'm just not seeing that as a very common need, meaning when you need it if you have to write a little extra code it's no big deal. It's not boilerplate if you only have to do it in rare situations. I'm looking for something a little more concrete as motivation than mom and apple pie goodness.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by pihentagy

Replying to kmtracey:

Replying to pihentagy:

Consistency and DRY: so you shouldn't write boilerplate code. You shouldn't specify column name django alredy knows.

I understand wanting to avoid boilerplate code but I'm missing how this is a common enough case to warrant inclusion and balance the additional complexity (both for users -- the list of things you can put in list_display is getting a bit long -- and code maintainers). What's the common situation where user's would really like to see the value of a field in a related model instead of just the text representation of the related model? I'm just not seeing that as a very common need, meaning when you need it if you have to write a little extra code it's no big deal. It's not boilerplate if you only have to do it in rare situations. I'm looking for something a little more concrete as motivation than mom and apple pie goodness.

If you have just 1 or 2 foreign keys, you have a lot of place, so it seems logical to display more interesting fields from a foreignkey. The unicode representation is ok, but it is just 1 column, you will find things quickly, if you have 2 or 3 fields from a foreign tabble (not to mention, you will be able to sort on that field for free!)

For example m2m tables with additional fields doesn't contain interesting information on their own. Maybe you want to display more columns from one FK and sort on them...

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by danros

Replying to pihentagy:

What's the common situation where user's would really like to see the value of a field in a related model instead of just the text representation of the related model? I'm just not seeing that as a very common need, meaning when you need it if you have to write a little extra code it's no big deal. It's not boilerplate if you only have to do it in rare situations. I'm looking for something a little more concrete as motivation than mom and apple pie goodness.

Can I add that I would very much like this functionality for my project. In fact the lack of this functionality led me to give up on the admin interface and create my own thing that did do that!

Being able to mix-in related model fields is very useful for creating a 'dashboard' style admin page which I would think is a very common requirement.

comment:15 in reply to: ↑ 14 Changed 7 years ago by kmtracey

Replying to danros:

Can I add that I would very much like this functionality for my project. In fact the lack of this functionality led me to give up on the admin interface and create my own thing that did do that!

It was easier to write your own admin than write some trivial ModelAdmin methods that returned the information you were interested in?

comment:16 follow-up: Changed 7 years ago by pihentagy

Khm, sorry for the noise guys, I think I misread this ticket.

All I want to do is:

list_display = ('foreign_key__related_fieldname1', 'foreign_key__related_fieldname2')

so displaying related fields.
Should I open a ticket for that, or is there one for it?

comment:17 in reply to: ↑ 16 Changed 7 years ago by kmtracey

Replying to pihentagy:

Should I open a ticket for that, or is there one for it?

I believe that is what this ticket is asking for, so no need for another ticket. In the absence of a working patch for this ticket, you can achieve what you want by adding ModelAdmin methods (or callables) that return the information you are looking for, and specifying these methods in your list_display. It is not particularly difficult and not a lot of code. Doc is here: http://docs.djangoproject.com/en/dev/ref/contrib/admin/#list-display

comment:18 Changed 6 years ago by ramiro

I'd say this ticket can be closed (I found it when looking for material related to #10230).

Using callables (bare functions for example, i.e. no model methods nor ModelAdmin subclasses methods) make the "It violates DRY: I have to define the same function on each model that references the foreign model" argument moot.

Also, when it comes to the HTML table headers:

  1. Seeing a bare 'Code' header (using the comment 6 example) that refers to a code field of a Client model that is located one or more FK-hops away isn't necessarily a good idea, it can be confusing because the user could assume it refers about a (non-existent) code field local to the model being displayed.
  2. In the case of more that one level of indirection, having an automatically-generated 'Hop1 Model > ... > HopN Model > Field' header wouldn´t be practical/scalable.

It's more practical in that case that the developer sets explicitely an appropiate, unambiguous header literal using the .short_description facility.

In other words, I'd say current functionality plus a bit of work satisfy the needs expressed in this ticket discussion for this arguably rare scenario.

comment:19 Changed 6 years ago by galund

Reasons not to close this ticket:

  • consistency between search_fields, list_display, and list_filter: it seems reasonable to want them all to work the same way. At the basic level, they are all a list of fields, after that, the limitations seem arbitrary (i.e. it seems at first glance as if the more advanced features of each could be available but just haven't been programmed yet).
  • this need isn't really so rare, particularly if you have models that relate one-to-one, where you naturally do want to include fields from across several tables.

comment:20 Changed 6 years ago by codekoala

  • Has patch unset
  • Needs tests set

Would someone like to review the snippet I've come up with as a "work around" of sorts? It would definitely need some work before it could be used in Django if it's deemed worthy, but I hope it's an option at least... I'm not sure I feel comfortable posting it as a patch because of the issues involved with my approach, but I still think it's worth looking at.

You can find it here, along with some details about the issues involved: http://www.codekoala.com/blog/2009/feb/27/model-relationships-and-list_display/

Thanks!

comment:21 Changed 6 years ago by codekoala

  • Has patch set
  • Needs tests unset

oops... didn't mean to adjust the ticket properties like that.

comment:22 follow-up: Changed 6 years ago by haffi67

One thing that I think no one has pointed out yet; if it were possible to set table__field in list_display, you could order the list by that field in the interface, which you can't do with callables.

comment:23 in reply to: ↑ 22 Changed 6 years ago by kmtracey

Replying to haffi67:

One thing that I think no one has pointed out yet; if it were possible to set table__field in list_display, you could order the list by that field in the interface, which you can't do with callables.

You can set admin_order_field on a callable to specify ordering; doc on that is the last bullet under http://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_display. If that doesn't currently support the double-underscore syntax to reference a ordering by a field in a related object, then I doubt it would 'just work' for things specified that way in list_display.

comment:24 Changed 6 years ago by subsume

Seems like discussion of this has petered out. Too bad, especially when more and more people are looking to subclass parts of admin for their own use.

This is so simple in many other ways in admin. I realize it leads to situations where what you get back isn't a field, but another queryset, but the developer gets what they deserve in that case. Its not hard (or even out of precedent) to simply ignore relations which are nonsense. Just take a look at the behavior of select_related! Having to create a custom method each time is tedious and this limitation prevents extensibility in situations where you've put those fields on the model yourself manually using extra() and other methods.

comment:25 Changed 6 years ago by andybak

  • Cc andy@… added

comment:26 Changed 5 years ago by mikexstudios

  • Cc mike.huynh+django@… added

comment:27 follow-up: Changed 4 years ago by Herman S

This needs attention.

I have the following models:

  • Applicant
  • Application, which has a field "applicant = models.ForeignKey(Applicant)"

In ApplicationAdmin I'm trying to request the applicants name, phone, email etc. with:
list_display = ['status', 'interview' ... 'applicantname', 'applicantemail']

As the parent/child relationship is reversed from what Django's ModelAdmin.inlines expect, there seems to not be a solution for my problem.

comment:28 in reply to: ↑ 27 Changed 4 years ago by kmtracey

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

Replying to Herman S:

As the parent/child relationship is reversed from what Django's ModelAdmin.inlines expect, there seems to not be a solution for my problem.

There's a trivially-easy solution you can code: methods on your model or model admin that return the information you desire, listed in your list_display.

I'm closing this wontfix; having it be open seems to be confusing people into thinking the requested function of allowing a specific field from a related model to be shown in list_display is not possible. It is possible, quite easily, with a feature (callables in list_display) added since this ticket was originally opened. That feature is more powerful than simply allowing traversal to a related model's field, so the syntax of how to do it is not exactly the same as it would be if just that one piece was implemented. But given the more powerful thing has been implemented, I don't believe the addition of another way of doing the same thing is necessary or worth the implementation/maintenance effort.

comment:29 follow-up: Changed 4 years ago by pihentagy

And what about sorting?

comment:30 in reply to: ↑ 29 Changed 4 years ago by kmtracey

Replying to pihentagy:

And what about sorting?

Sorting is supported for callables in in list_display, as noted in http://code.djangoproject.com/ticket/5863#comment:23

comment:31 Changed 4 years ago by brillgen

  • Cc brillgen added
  • Severity set to Normal
  • Type set to Uncategorized

@kmtracey:

We would have to use over a 100 extra boilerplate functions at last estimate since callables is being stated as the only supported means of getting direct fields from foreign keys...some of them are simply a text field that needs to be displayed as such..I have 2 questions for the community:

  1. Isn't there a performance impact of using a callable (select_related) benefit loss which could happen in case of simple FK fields required?

If there is no impact, its irrelevant because the boiler plate code is not thaat much trouble
However, if there is (and I believe there would be),

  1. Would the django admin accept a patch as long as it met the requirements of test cases, documentation etc etc.

I ask because there would be nothing more frustrating than developing a fully featured patch against trunk just to hear that it'll never get accepted due to contrary design ideas :(

comment:32 Changed 4 years ago by lukeplant

@brillgen:

It's difficult to believe there would be much difference between the two methods if you are using 'select_related' correctly (and we don't want to complicate the logic for select_related any further).

Also, note that you could easily write a function that would eliminate the boilerplate, if it is really boilerplate that conforms to a common pattern. Untested code:

def foreign_field(field_name):
    def accessor(obj):
        val = obj
        for part in field_name.split('__'):
            val = getattr(val, part)
        return val
    accessor.__name__ = field_name
    return accessor

ff = foreign_field

class MyAdmin(ModelAdmin):

    list_display = [ff('foreign_key__related_fieldname1'), 
                    ff('foreign_key__related_fieldname2')]

comment:33 Changed 4 years ago by brillgen

  • Easy pickings unset

@lukeplant: In the absence of any other solution your function is a lifesaver. I guess I'm not pythonic enough yet to have thought of it on my own...
Still it does have a few problems that can't be fixed...For instance, how to assign the short_description attribute to be the same as what has already been defined for the field? It can't be done the way I see it, without explicitly passing the model involved or the short description (which will have to be repeated here)

comment:34 Changed 4 years ago by andybak

I agree with @brillgen here.

It would be clearer and much more consistant to support the parent__child syntax for accessing related models wherever feasible.

I believe it's already been added for admin list_filter.

Last edited 4 years ago by andybak (previous) (diff)

comment:35 Changed 4 years ago by anonymous

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I am re-opening this ticket once more to get some more attention on this...the current proposed workarounds have serious deficiencies:

  1. Write functions everywhere (this involves a lot of repetition for every single foreign field)
  2. Write a wrapper like @lukeplant suggested (even though it is far superior solution to individual functions in every model)

are all HUGE violation of DRY principles on which sensible programming is based toady. Since the parent__child syntax is being used for list filter, search fields and everywhere else, why is the list_display so sacrosanct?

E.g. once a model field already has an attribute like verbose_name defined...why should a function or a wrapper function have to define it again. It is not feasible to copy that definition in the case of the wrapper.

My question is, will this be accepted as a patch if contributed on the trunk with test cases et all?

Last edited 4 years ago by jezdez (previous) (diff)

comment:36 Changed 4 years ago by jezdez

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

Please don't reopen tickets that were marked as wontfix by a core developer but raise the issue on the django-developers list instead.

comment:37 Changed 4 years ago by lukeplant

Also, in response to andybak and anonymous, please note:

list_display is fundamentally different from list_filter and search_fields. The other two are defining operations that must happen in the database, whereas list_display is defining an operation that must happen in Python code, and refers to Python functions/methods, and not to database fields. The confusion is that a name that of a database field is also the name of the corresponding attribute on the Python object, but that is where the similarity ends.

So, for instance, you can set '__unicode__' in list_display, and it will refer to the __unicode__ method, and not attempt any field lookups, despite the presence of double underscores. As it happens, this example makes it obvious that we cannot sensibly add the feature as proposed - how would you refer to the __unicode__ method on a related object? 'foreign_key____unicode__'? How would you parse that? What about other methods that people might choose to define, like __html__, etc.? The only way this feature could get in is we are happy with a bunch of special cases in the implementation, and a bunch of arbitrary limitations in functionality.

comment:38 Changed 4 years ago by brillgen

@lukeplant, the basic problem you've correctly raised is that list_display allows for callables and hence arbitrary names can be used which are simiar to the syntax for foreign key fields.
However, this problem exists for list_filter also: someone may define a field with the name class__field and try to use that in list_filter as a foreign key field. (i've just checked that you can indeed name a field like that)

So we can't really be compensating for dev stupidity at the cost of functionality as we haven't done in list_filter also.
Since all FKs always start with an alphabet are in the regex format (.+__.+.*), the __unicode__ etc cases can be easily and correctly filtered as is required to be done in list_filter.

Last edited 4 years ago by jezdez (previous) (diff)

comment:39 Changed 4 years ago by lukeplant

If we are allowing field names to contain double underscores, that is almost certainly a validation bug which should be fixed. We have restrictions on how fields can be named, and don't have restrictions on other methods or attributes, and I think we should keep it that way.

Adding attribute lookup into a syntax that has been designed for table joins is a bad idea. It will certainly require limitations on the names of attributes (e.g. 'private' attributes that start with a double underscore). Remembering that there is no reason why the attribute lookup should be at the end, it may not be possible at all - I'm not convinced you easily handle methods like __foo__ if it is followed by another 'join'.

However, using a dot lookup syntax would make sense here, because list_display is about object attributes and not field joins. This might be a sensible proposal, but not sure if would deal with all the problems - you still can't get the fields verbose_name easily etc.

comment:40 Changed 4 years ago by brillgen

I agree that its better to restrict the field names than have restrictions on other methods or attributes.

Based on the distinction between list_filter and list_display, I was just thinking that a . lookup syntax for attributes (class attribute would be closer to the idea that list_display is about object attributes and not database joins. However, why would retrieving attributes difficult? The admin has the model object and hence can span the model definitions to determine the attributes (using the . syntax to identify members and their attributes) ..In fact it would allow methods on FKs to be displayed as well which was not covered so well with the syntax.

comment:41 Changed 4 years ago by lukeplant

I gave a more full reply on the list: http://groups.google.com/group/django-developers/browse_thread/thread/790484bfbe1b421f

My apologies for bouncing around between Trac and the mailing list, I should have stuck to the list as is our policy.

comment:42 Changed 2 years ago by jcushman

  • UI/UX unset

If anyone else who wants to be able to refer to foreign keys with underscore syntax in list_display comes across this ticket, I put together a subclass of ModelAdmin to do that:

http://djangosnippets.org/snippets/2887/

So you can just do:

class FooAdmin(RelatedFieldAdmin):
    list_display = ('address__phone','address__country__country_code')

and it figures it out.

The benefit of using a subclass over luke's suggestion is we can override queryset() to automatically add the related models to select_related(), which prevents a database hit for each row. It also sets sensible defaults for short_description and admin_order_field. Would love to hear (over at djangosnippets) if anyone has suggested improvements.

comment:43 Changed 2 years ago by jcushman

  • UI/UX set

(Sorry, not sure why it unset UI/UX when I commented.)

comment:44 Changed 16 months ago by petr.dlouhy@…

I remade the Snippet 2996 in form of an application (for anyone interested in working around this issue): https://github.com/PetrDlouhy/django-related-admin

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