Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#14206 closed New feature (fixed)

contrib:admin dynamic list_display support

Reported by: Gabe Jackson 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 13 years ago.
14206_r16337.diff (11.5 KB ) - added by cyrus 13 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by Yeago, 13 years ago

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 by Alex Gaynor, 13 years ago

Triage Stage: UnreviewedAccepted

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

comment:3 by Julien Phalip, 13 years ago

This could be implemented in a similar way as get_readonly_fields.

comment:4 by Graham King, 13 years ago

Severity: Normal
Type: New feature

comment:5 by aviv.by@…, 13 years ago

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

comment:6 by cyrus, 13 years ago

Owner: changed from nobody to cyrus
Status: newassigned

by cyrus, 13 years ago

Attachment: 14206_r16204.diff added

comment:7 by cyrus, 13 years ago

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 by cyrus, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Julien Phalip, 13 years ago

Triage Stage: Ready for checkinAccepted

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 by Luke Plant, 13 years ago

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.

in reply to:  9 ; comment:11 by cyrus, 13 years ago

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!

in reply to:  11 comment:12 by Julien Phalip, 13 years ago

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.

by cyrus, 13 years ago

Attachment: 14206_r16337.diff added

comment:13 by cyrus, 13 years ago

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 by Luke Plant, 13 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

LGTM.

comment:15 by Luke Plant, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16340]:

Fixed #14206 - dynamic list_display support in admin

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

comment:16 by graveyboat, 13 years ago

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 by Julien Phalip, 13 years ago

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