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)
Change History (9)
by , 13 years ago
Attachment: | 16257_r16381.diff added |
---|
comment:1 by , 13 years ago
Has patch: | set |
---|
Implemented the change with this patch, as well as provided a test and documentation.
comment:2 by , 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 , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 13 years ago
Attachment: | 16257_r16400.diff added |
---|
Updated the patch to build list_diplay_links outside the invocation of ChangeList
by , 13 years ago
Attachment: | 16257.get_list_display_links.diff added |
---|
comment:4 by , 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 , 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.
Patch for the proposed change