list_display_links doesn't allow callables not defined in the model

[dvine]

In a custom ModelAdmin class you can use class methods of this class as change_list fields via the list_display property. But if you wan't to use this callable as link with the ist_display_links property you get an error

Example:

'CustomPortfolioAdmin.list_display_links[0]' refers to 'change_list_status' that is neither a field, method or property of model 'CustomPortfolio'.

There is a ticket that introduced the error message (#2578), but I think this is a new issue, that's why I open a new ticket.

[dvine]

Oh, and while we are at it, I would have a feature request regarding list_display_links, that could be implented in the same go. Setting list_display_links to an empty tuple/list should not result in default behavior (choosing the first column). Instead there should be no automatic display links at all, so that one can handle them individually in the callables.
Thanks a lot and keep up the absolutely amazing work you are doing with django.

[Antti Kaihola]

If all (or most of) fields displayed are editable, it makes sense to have an "Edit" link.

Here's how we can display an initial "Edit" column with links to change forms for each row:

def list_display_column_factory(value, title=None):
    column = lambda result: value
    column.short_description = title or value
    return column

class PollAdmin(admin.ModelAdmin):
    model = Poll
    edit_column = list_display_column_factory(_(u'Edit'))
    list_display = (edit_column, 'question', 'pub_date')
    list_editable = ('question', 'pub_date')

But if we want to move the "Edit" column to another position or introduce another link column, this won't work:

    list_display = ('question', 'pub_date', edit_column)
    list_editable = ('pub_date', )
    list_display_links = ('question', edit_column,)

It throws this exception:

  [...]
  File "polls/admin.py", line 16, in <module>
    admin.site.register(Poll, PollAdmin)
  File "django/contrib/admin/sites.py", line 92, in register
    validate(admin_class, model)
  File "django/contrib/admin/validation.py", line 50, in validate
    fetch_attr(cls, model, opts, 'list_display_links[%d]' % idx, field)
  File "django/contrib/admin/validation.py", line 308, in fetch_attr
    return getattr(model, field)
TypeError: getattr(): attribute name must be string

[sehmaschine@…]

getting the same error here. it should at least be mentioned in the docs that list_display_links is different from list_display when it comes to callables and attributes.

[philomat]

Even more strangely, but a workaround: In your model, define a method 'change_list_status' that does nothing at all. Now you can add 'change_list_status', which is a callable of the admin class, to list_display_links and it works as.

[cmbeelby@…]

I got the same error and I have figured out the problem. Line 48 of /django/contrib/admin/validation.py needs to be removed. (I am using revision 12872 of the file as my base)

1. It is a call to fetch_attr but the return value is not even being used or stored. If the call is simply for side-effects this still does not make sense as it will always fail in cases where the field is not part of the model and this is not what the documentation says is possible. If we must keep this call then we should have a similar pre-check as in the preceding block of code which first checks to see that the field is not a callable and not an attribute of the Admin class (if it is then no further checking is done).
2. The call itself doesn't seem to make sense to me. According to the documentation you should be able to have anything in the list_display_links that is already in the list_display list, and by removing the call to fetch_attr the code will perform this check correctly. It is the responsibility of the preceding code block which checks the actual list_display list to see if the items in that list are valid and this is where the fetch_attr is appropriate and the return value is used.

I have tested by commenting out the offending line and now I can have my list_display_links refer to any field in the list_display and it works as expected and documented.

[cmbeelby]

Not sure what you mean by needs docs. This is not a new feature and the existing documentation is correct. This fixes a bug in the source which makes the admin site produce an error when following the guidelines given in the documentation. Specifically the documentation at ​http://docs.djangoproject.com/en/1.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_display_links says that list_display_links can be anything that is in list_display.

As far as needing test's that is certainly something I can look into, but will probably take a few more days to get something setup. I can see a simple test would be to create four items in the list_display of the four types that are allowed based on the documentation and then verify that the list_display_links can also refer to them all.

[cmbeelby]

Test cases to ensure fix doesn't break again later

[cmbeelby]

Test cases to ensure fix doesn't break again later

[cmbeelby]

Finally figured out a good way to test. Have attached the test files and verified that the tests pass for me (if you apply the fix in this ticket. Without the fix one test will fail as expected). I also updated the patch itself to be relative to the base of the project as is the preferred method of attaching patches. Sorry the "tests" shows up there twice, can't seem to find a way to remove the second one, they should be the same. Please identify me as "Christopher M. Beelby" in your check-in notes. Thanks.

[cmbeelby]

Even though it is not recommended I am going to change this to "ready for checkin" in hope that someone will notice this fix and at least look at or check it in. It has been sitting out here for a while and I am not seeing any indication that there is any activity going on.

[Malcolm Tredinnick]

For next time: ready for checkin means there is a single patch that can be applied. Not three patches. (Also, generally a poor idea to "ready for checkin" your own work -- it's like editing your own writing: invariably hard because you're too close to it).

[Malcolm Tredinnick]

Ok, I like the idea, but I can't make the tests pass (not the new tests, but it breaks existing tests). The modeladmin tests show why just removing that single line in admin/validate.py isn't a good idea -- it provides a misleading error message in another case (and possibly others that aren't even tested). So a more logical approach to validation is needed there, rather than just removing it.

Also, as a second point for the next (single!) patch to this ticket: there's no need to create contrib/admin/tests/. We already have tests/regressiontests/admin_views, ../admin_widgets, etc for admin testing.

[Chris Adams]

I have a github branch against Django 1.2.3 which includes working tests and a doc update to clarify that anything which you put in list_display can be used in list_display_links:

​https://github.com/acdha/django/tree/ticket-11058-list_display_links

Here's the ​comparison and ​patch against Django 1.2.3.

One note regarding tests: the old doctest hard-coded the error message you'd get for a field which is not defined on the model, whether or not it was otherwise valid. I believe ImproperlyConfigured: … list_display_links[0]'refers to 'non_existent_field' which is not defined in 'list_display' is now more appropriate and have updated the doctest to match.

[Chris Adams]

New patch against 1.2.3

[Chris Adams]

Updated patch against 1.2.X branch:

​https://github.com/acdha/django/compare/django-1.2.X...ticket-11058-list_display_links

[Chris Adams]

Updated branch and patch against current trunk:

​https://github.com/acdha/django/compare/master...ticket-11058-list_display_links-1.3

[Julien Phalip]

Replying to mtredinnick:

Ok, I like the idea, but I can't make the tests pass (not the new tests, but it breaks existing tests). The modeladmin tests show why just removing that single line in admin/validate.py isn't a good idea -- it provides a misleading error message in another case (and possibly others that aren't even tested). So a more logical approach to validation is needed there, rather than just removing it.

There might have been a mistake in the tests at that time, as all tests are passing now. I think that the proper fix is in fact to simply remove that line, which makes the wrong assumption that the fields declared in list_display_linksmust belong to the model. The only requirement should be that fields declared in list_display_links are also declared in list_display.

The attached patch adds more robust tests for list_display_links as well as for list_display.

[Simon Meers]

This makes sense to me; list_display is already validated; performing separate validation on list_display_links seems redundant. Nice simple solution Julien.

[Luke Plant]

In [15619]:

Fixed #11058 - list_display_links doesn't allow callables not defined in the model

Thanks to dvine for the report and julien for the patch.

[Luke Plant]

In [15621]:

[1.2.X] Fixed #11058 - list_display_links doesn't allow callables not defined in the model

Thanks to dvine for the report and julien for the patch.

Backport of [15619] from trunk.

[Jacob]

Milestone 1.3 deleted