Code

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9053 closed (fixed)

Fix admin sortable when using callable functions in list_display

Reported by: rgl Owned by: nobody
Component: contrib.admin Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

We can use callable functions when we define the ModelAdmin.list_display property, but when we try to short then, an TypeError exception is raised:

Exception Type:		TypeError
Exception Value:	getattr(): attribute name must be string
Exception Location:	/home/rgl/Projects/django/django-trunk/django/contrib/admin/views/main.py in get_ordering, line 160

The attached patch fixes this.

This problem can be seen in the following ModelAdmin example; it uses two callables "admin_lat" and "admin_lng":

from django.contrib import admin
from airports.models import Airport

# See "callable" at http://docs.djangoproject.com/en/dev/ref/contrib/admin/#list-display
def admin_lat(airport):
    return '%.6f' % airport.lat
admin_lat.short_description = "Latitude"
admin_lat.admin_order_field = 'lat'
def admin_lng(airport):
    return '%.6f' % airport.lng
admin_lng.short_description = "Longitude"
admin_lng.admin_order_field = 'lng'

class AirportAdmin(admin.ModelAdmin):
    list_display = ('iata_code', 'name', admin_lat, admin_lng, 'tz', 'country')
    list_filter = ('country', )
    search_fields = ('=iata_code', )
admin.site.register(Airport, AirportAdmin)

Attachments (1)

admin_allow_callable_on_shorting.patch (1.0 KB) - added by rgl 6 years ago.

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by rgl

comment:1 Changed 6 years ago by julien

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

To my knowledge, you're not supposed to sort by a callable. Only database field sorting is supported. So I'd rather have it throw an error than just ignore it.

comment:2 Changed 6 years ago by kmtracey

  • Resolution set to invalid
  • Status changed from new to closed
  • Summary changed from Fix admin shortable when using callable functions in list_display to Fix admin sortable when using callable functions in list_display

Callables can be made to be sortable if you specify an admin_order_field for them, as described under list_display here:

http://docs.djangoproject.com/en/dev/ref/contrib/admin/#list-display

(scroll down to just above list_display_links)

The problem here is these "fields" are not enclosed in quotes in list_display. They should be, just like all the others and like the example in the doc. Put them in quotes and then it will work without any change needed in the admin code.

comment:3 Changed 6 years ago by rgl

  • Resolution invalid deleted
  • Status changed from closed to reopened

The docs say and show that I can use a function variable in list_display and even sort by it (by setting admin_order_field like I did)... but its not working; so, how come this was closed as invalid?

comment:4 Changed 6 years ago by kmtracey

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

It was closed as invalid because the way you have specified list_display is invalid. It should be:

list_display = ('iata_code', 'name', 'admin_lat', 'admin_lng', 'tz', 'country')

not your version which did not have quotes around admin_lat and admin_lng. This is consistent with what is in the docs, they do not show including callables unquoted in list_display.

comment:5 Changed 6 years ago by rgl

  • Resolution invalid deleted
  • Status changed from closed to reopened

Huh?

Please, check the docs:

"""

You have four possible values that can be used in list_display:
...
class PersonAdmin(admin.ModelAdmin):
    list_display = (upper_case_name,)

""" -- http://docs.djangoproject.com/en/dev/ref/contrib/admin/#list-display

comment:6 Changed 6 years ago by kmtracey

Look further down, where admin_order_field is described. That callable (colored_first_name) is included in quotes when specified in list_display:

class PersonAdmin(admin.ModelAdmin):
    list_display = ('first_name', 'colored_first_name')

I do not know without further investigation that I do not have time for at the moment if the instance you point out is a doc bug or a code bug. For right now, if you would just put your fields in quotes it will work. I have tested that.

comment:7 Changed 6 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

Oh, OK I see now. A callable that's not a model method, which was a late-added feature. Sorry to be dense.

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

The documentation actually says that the string version of the method names should be used in list_display when the callable is effectively a method (of the model or of the user-provided ModelAdmin subclass, it doesn't explicitly states the same is true for the non-method callables (bare functions that take one parameter).

It also doesn't explicitly states that it is possible to sort the admin display by such callables. It just says you can only use the model method kind of callables when they effectively represent a DB field (and in fact the sorting is performed using that DB field, that's what .admin_order_field is for).

Just my two cents, I don't know what's the problem (if any) here either: The documentation or the code implementation

comment:9 in reply to: ↑ 8 Changed 6 years ago by kmtracey

Replying to ramiro:

Just my two cents, I don't know what's the problem (if any) here either: The documentation or the code implementation

There is a code bug here, the admin makes the column sortable but then raises an exception if the user actually clicks on the link admin placed in the column header.

If these non-model-method callables aren't supposed to be allowed to be sorted, then admin shouldn't put the link to sort them in the column header, even if the user has specified the admin_order_field. But I suspect that the admin_order_field feature was just overlooked when non-model-method callables in list_display was implemented, rather than a conscious decision being made to not support sorting by them. From a brief look it seems easier to just complete the implementation of non-model-method callables and support sorting by them (there it is in the provided patch, after all) than to add the checks and doc required to properly disallow it instead of crashing and burning.

comment:10 Changed 6 years ago by jenan

That patch looks good, but why not fix the case of field_name referring to an attr of self.model_admin as well?

if callable(field_name):
    attr = field_name
elif hasattr(self.model_admin, field_name):
    attr = getattr(self.model_admin, field_name)
else:
    attr = getattr(self.model, field_name)

comment:11 Changed 6 years ago by kmtracey

Replying to jenan:

That patch looks good, but why not fix the case of field_name referring to an attr of self.model_admin as well?

Yes, some testing shows that while the admin does not crash and burn on sorting on a model_admin method, it also doesn't actually perform a sort on the specified field either. I plan to fix the code and add some tests that actually ensure these things work.

comment:12 Changed 6 years ago by kmtracey

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

(In [9211]) Fixed #9053 -- Allowed for sorting of callable and ModelAdmin methods specified in list_display (added in r8352). Previously attempting to sort on the former would raise an exception and the latter simply didn't sort. Also added tests for this function. Thanks rgl and jenan.

comment:13 Changed 6 years ago by kmtracey

(In [9212]) [1.0.X] Fixed #9053 -- Allowed for sorting of callable and ModelAdmin methods specified in list_display (added in r8352). Previously attempting to sort on the former would raise an exception and the latter simply didn't sort. Also added tests for this function. Thanks rgl and jenan.

Backport of [9211], also updated svnmerge.py metatdata.

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.