Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#11058 closed (fixed)

list_display_links doesn't allow callables not defined in the model

Reported by: dvine Owned by: acdha
Component: contrib.admin Version: master
Severity: Keywords: list_display_links
Cc: mail@…, chris@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (7)

validation.diff (705 bytes) - added by cmbeelby 5 years ago.
tests.2.diff (2.8 KB) - added by cmbeelby 5 years ago.
Test cases to ensure fix doesn't break again later
tests.diff (2.8 KB) - added by cmbeelby 5 years ago.
Test cases to ensure fix doesn't break again later
1.2.3...ticket-11058-list_display_links.txt (7.1 KB) - added by acdha 4 years ago.
New patch against 1.2.3
django-11058-list_display_links.patch (5.9 KB) - added by acdha 4 years ago.
ticket-11058-list_display_links-1.3.patch (7.1 KB) - added by acdha 4 years ago.
11058_list_display_links_validation.diff (4.8 KB) - added by julien 4 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 6 years ago by dvine

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

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.

comment:2 Changed 6 years ago by akaihola

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

comment:3 Changed 6 years ago by 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.

comment:4 Changed 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by 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.

comment:6 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:8 Changed 5 years ago by cmbeelby

  • Owner changed from anonymous to cmbeelby
  • Status changed from assigned to new

comment:9 Changed 5 years ago by cmbeelby

  • Keywords list_display_links added

comment:10 Changed 5 years ago by cmbeelby

  • Status changed from new to assigned

comment:11 Changed 5 years ago by cmbeelby

  • Has patch set

comment:12 Changed 5 years ago by ramiro

  • Needs documentation set
  • Needs tests set

comment:13 Changed 5 years ago by cmbeelby

  • Needs documentation unset

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.

Changed 5 years ago by cmbeelby

Changed 5 years ago by cmbeelby

Test cases to ensure fix doesn't break again later

Changed 5 years ago by cmbeelby

Test cases to ensure fix doesn't break again later

comment:14 Changed 5 years ago by cmbeelby

  • Needs tests unset

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.

comment:15 Changed 5 years ago by cmbeelby

  • Has patch unset

comment:16 Changed 5 years ago by cmbeelby

  • Has patch set

comment:17 Changed 5 years ago by cmbeelby

  • Triage Stage changed from Accepted to Ready for checkin

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.

comment:18 Changed 5 years ago by cmbeelby

  • Owner cmbeelby deleted
  • Status changed from assigned to new

comment:19 Changed 5 years ago by cmbeelby

  • Version changed from 1.0 to SVN

comment:20 Changed 5 years ago by mtredinnick

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

comment:21 follow-up: Changed 5 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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.

comment:22 Changed 4 years ago by acdha

  • Cc chris@… added

comment:23 Changed 4 years ago by acdha

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.

comment:24 Changed 4 years ago by acdha

  • Owner set to acdha
  • Status changed from new to assigned

Changed 4 years ago by acdha

New patch against 1.2.3

comment:25 Changed 4 years ago by acdha

  • milestone set to 1.3
  • Patch needs improvement unset
  • Version changed from SVN to 1.2

Changed 4 years ago by acdha

comment:26 Changed 4 years ago by acdha

  • Version changed from 1.2 to SVN

Changed 4 years ago by acdha

Changed 4 years ago by julien

comment:27 in reply to: ↑ 21 Changed 4 years ago by julien

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.

comment:28 Changed 4 years ago by DrMeers

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:29 Changed 4 years ago by lukeplant

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

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.

comment:30 Changed 4 years ago by lukeplant

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.

comment:31 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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