Django

Code

Ticket #9053 (closed: fixed)

Opened 4 months ago

Last modified 3 months ago

Fix admin sortable when using callable functions in list_display

Reported by: rgl Assigned to: nobody
Milestone: Component: django.contrib.admin
Version: 1.0 Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

admin_allow_callable_on_shorting.patch (1.0 kB) - added by rgl on 09/12/08 02:24:48.

Change History

09/12/08 02:24:48 changed by rgl

  • attachment admin_allow_callable_on_shorting.patch added.

09/12/08 02:38:33 changed by julien

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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.

09/12/08 07:34:02 changed by kmtracey

  • status changed from new to closed.
  • resolution set to invalid.
  • 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.

09/12/08 09:33:58 changed by rgl

  • status changed from closed to reopened.
  • resolution deleted.

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?

09/12/08 09:39:27 changed by kmtracey

  • status changed from reopened to closed.
  • resolution set to invalid.

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.

09/12/08 09:47:10 changed by rgl

  • status changed from closed to reopened.
  • resolution deleted.

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

09/12/08 09:56:45 changed 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.

09/12/08 10:16:05 changed by kmtracey

  • 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.

(follow-up: ↓ 9 ) 09/12/08 10:24:07 changed 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

(in reply to: ↑ 8 ) 09/12/08 10:53:41 changed 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.

09/19/08 15:53:51 changed 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)

10/07/08 12:13:19 changed 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.

10/08/08 09:47:02 changed by kmtracey

  • status changed from reopened to closed.
  • resolution set to fixed.

(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.

10/08/08 09:57:34 changed 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/Change #9053 (Fix admin sortable when using callable functions in list_display)




Change Properties
Action