Opened 13 years ago

Closed 13 years ago

#16257 closed New feature (fixed)

contrib:admin dynamic list_display_links support

Reported by: graveyboat Owned by: nobody
Component: contrib.admin Version: 1.3
Severity: Normal Keywords: list_display override dynamic admin
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Ticket #14206 created a very useful way to allow programmers to override list_display at runtime by using a new function get_list_display instead of accessing self.list_display. Unfortunately, it potentially causes an issue with list_display_links.

If list_display_links is not set in the ModelAdmin instance, currently the first field after 'action_checkbox' in list_display becomes list_display_links. Since get_list_display happens after list_display_links is set, there is a potential that what is returned from get_list_display no longer contains the field in list_display_links. Take the following for example:

class DynamicListDisplayChildAdmin(admin.ModelAdmin):
    list_display = ('name', 'parent')

    def get_list_display(self, request):
        my_list_display = list(self.list_display)
        if request.user.username == 'noparents':
            my_list_display.remove('name')

        return my_list_display

In this case, list_display_links would be name, but 'name' is no longer in the list_display passed to the ChangeList instance. Basically, the fix is to create a get_list_display_links method that is capable of checking list_display if necessary.

Attachments (3)

16257_r16381.diff (5.0 KB ) - added by graveyboat 13 years ago.
Patch for the proposed change
16257_r16400.diff (5.1 KB ) - added by graveyboat 13 years ago.
Updated the patch to build list_diplay_links outside the invocation of ChangeList
16257.get_list_display_links.diff (10.3 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (9)

by graveyboat, 13 years ago

Attachment: 16257_r16381.diff added

Patch for the proposed change

comment:1 by graveyboat, 13 years ago

Has patch: set

Implemented the change with this patch, as well as provided a test and documentation.

comment:2 by Aymeric Augustin, 13 years ago

Patch needs improvement: set

This looks useful.

In the patch, it would be more readable to build list_display_links = list(self.get_list_display_links(request, list_display)) outside the invocation of ChangeList(...), just after building list_display.

comment:3 by Aymeric Augustin, 13 years ago

Triage Stage: UnreviewedAccepted

by graveyboat, 13 years ago

Attachment: 16257_r16400.diff added

Updated the patch to build list_diplay_links outside the invocation of ChangeList

by Julien Phalip, 13 years ago

comment:4 by Julien Phalip, 13 years ago

Definitely a good idea, however the suggested approach could be made more robust. In particular, it's a shame having to override get_list_display_links() nearly every time get_list_display() is overridden to avoid any missing field. I've posted a new patch which moves the initialisation of the list display fields and links right into the hooks, which means that by default get_list_display_links() will always pick the first field returned by get_list_display().

This would introduce a slight backwards incompatible change as the initialisation would be done at runtime instead of when ModelAdmin itself is initialised. I haven't had enough time to think about whether this would be an issue that we need to care about -- so some feedback would be welcome.

The test could also be improved to make the result of get_list_display_links() vary based on the request and list_display parameters.

comment:5 by Julien Phalip, 13 years ago

While working on this I've realised that there were some issues with get_list_display() which I think should be addressed beforehand. Those issues are described in #17090.

comment:6 by Julien Phalip, 13 years ago

Resolution: fixed
Status: newclosed

In [17037]:

Fixed #16257 -- Added new ModelAdmin.get_list_display_links() method to allow for the dynamic display of links on the admin changelist. Thanks to graveyboat for the suggestion and initial patch.

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