Code

Opened 3 years ago

Closed 2 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 3 years ago.
Patch for the proposed change
16257_r16400.diff (5.1 KB) - added by graveyboat 3 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 3 years ago.

Download all attachments as: .zip

Change History (9)

Changed 3 years ago by graveyboat

Patch for the proposed change

comment:1 Changed 3 years ago by graveyboat

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by aaugustin

  • 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 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by graveyboat

Updated the patch to build list_diplay_links outside the invocation of ChangeList

Changed 3 years ago by julien

comment:4 Changed 3 years ago by julien

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 Changed 2 years ago by julien

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 Changed 2 years ago by julien

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

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.

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.