Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#14206 closed New feature (fixed)

contrib:admin dynamic list_display support

Reported by: gabejackson Owned by: cyrus
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: list_display override dynamic admin
Cc: aviv.by@…, delinhabit@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was trying to add dynamic list_display support for the following use-case:
The admin area of a shop should have separate views for the owner and for the supplier. the supplier should not see all the fields that the owner can see (i.e cannot see WHO ordered the products which are being requested from the supplier). For this, I wanted to have a dynamic list_display based on request.user.

We need a decent way to do this. Currently I'm using the following hacky approach which seems to work:

class MyModelAdmin(ModelAdmin):
    list_display = ('dynamicfield', 'permanent 1', 'permanent 2')
    def preprocess_list_display(self, request):
        if 'dynamicfield' not in self.list_display:
            self.list_display.insert(1, 'dynamicfield')
        if not request.user.is_superuser:
            if 'myfield' in self.list_display:
                self.list_display.remove('dynamicfield')

    def changelist_view(self, request, extra_context=None):
        self.preprocess_list_display(request)
        return super(MyModelAdmin, self).changelist_view(request)

The re-insertion of the 'dynamicfield' value is neccessary because ModelAdmin instances seem to persist. so once you log in as a non-superuser and the field is gone, the field will always be gone (even if you log back in as a superuser).

I'm not sure if it is possible, but a better appraoch would be to instanciate the ModelAdmin classes with the request, like this, one could simply overwrite init and set list_display accordingly.

Attachments (2)

14206_r16204.diff (4.7 KB) - added by cyrus 4 years ago.
14206_r16337.diff (11.5 KB) - added by cyrus 4 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by subsume

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

This could be solved without a patch using proxy models, since there may be a whole world of difference between what your two audiences see (not only in list_display, but also in the form itself).

You create a proxy model of your original model and you assign permissions to whoever based on what's appropriate.

comment:2 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

Marking as accepted, because right now the only way to solve this isn't thread safe.

comment:3 Changed 5 years ago by julien

This could be implemented in a similar way as get_readonly_fields.

comment:4 Changed 4 years ago by graham_king

  • Severity set to Normal
  • Type set to New feature

comment:5 Changed 4 years ago by aviv.by@…

  • Cc aviv.by@… added
  • Easy pickings unset

comment:6 Changed 4 years ago by cyrus

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

Changed 4 years ago by cyrus

comment:7 Changed 4 years ago by cyrus

  • Cc delinhabit@… added
  • Has patch set

Implemented the get_list_display method on the ModelAdmin class. It can be used to change the list_display configuration at runtime in the same manner as get_readonly_fields.
The provided patch includes also some tests and updated documentation.

comment:8 Changed 4 years ago by cyrus

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 follow-up: Changed 4 years ago by julien

  • Triage Stage changed from Ready for checkin to Accepted

Thanks a lot for your work. However, you shouldn't mark your own patch as Ready For Checkin and instead get (or wait for) someone to review it.

By the way, after a very brief look at the patch, the versionadded clause should be 1.4, not 1.2.6, as the maintenance release (third-level number) is for bug fixes only and does not introduce new features. Also, version 1.3 is already out.

comment:10 Changed 4 years ago by lukeplant

  • Patch needs improvement set

The patch has various problems:

  • patch needs to be against trunk, it is currently way out of date, and tests fail etc.
  • tests should use RequestFactory, not another mock request object. (The existing MockRequest was almost certainly created before we had RequestFactory. Feel free to update that code with RequestFactory while you are at it).
  • DynamicListDisplayChildAdmin in the tests has some code that is rather bad form - it appears to mutate the class attribute 'list_display'. In fact it doesn't, because of another implementation detail, but that shouldn't be relied on.

comment:11 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by cyrus

Replying to julien:

Thanks a lot for your work. However, you shouldn't mark your own patch as Ready For Checkin and instead get (or wait for) someone to review it.

By the way, after a very brief look at the patch, the versionadded clause should be 1.4, not 1.2.6, as the maintenance release (third-level number) is for bug fixes only and does not introduce new features. Also, version 1.3 is already out.

Hey,

Thanks for the clarifications. I didn't see any progress on this issue, so I changed the stage to get some attention; and it seemed to work :) Sorry if I misunderstood the way the releases are getting out (I think I missed that part somehow). I will make the appropriate changes also based on the feedback from lukeplant and will update the patch. One question though, should I add a new patch or should I replace the existing one? Thanks!

comment:12 in reply to: ↑ 11 Changed 4 years ago by julien

Replying to cyrus:

One question though, should I add a new patch or should I replace the existing one? Thanks!

Add a new one, so that the old one can still be referred to.

Changed 4 years ago by cyrus

comment:13 Changed 4 years ago by cyrus

I updated the patch to be against trunk. Also based on the feedback from lukeplant I made the following changes:

  • Updated all the tests to use RequestFactory
  • Improved the DynamicListDisplayChildAdmin implementation to make the code more clear in terms of what it does

comment:14 Changed 4 years ago by lukeplant

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

LGTM.

comment:15 Changed 4 years ago by lukeplant

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

In [16340]:

Fixed #14206 - dynamic list_display support in admin

Thanks to gabejackson for the suggestion, and to cyrus for the patch.

comment:16 Changed 4 years ago by graveyboat

  • UI/UX unset

I ran across this ticket today, but I've created some similar functionality in the past. One thing I ran across which doesn't seem to be handled here is the case where the field removed from list_display also happens to be in list_display_links. For instance, in the test, list_display = ('name','parent'). In this case, list_display_links would be [ 'name' ]. The test case covers removing 'parent', but not 'name'. If you were to remove 'name', your change_list screen would be left with a list of results with no link.

I don't want to be "that guy" that re-opens tickets, but if it needs to be re-opened, I'd be glad to submit a patch to fix this.

comment:17 Changed 4 years ago by julien

Good point. Feel free to open a new ticket with a clear description of the problem, and even a patch if possible ;)

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