Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#9360 closed (invalid)

Admin interface method get_form does not call get_fieldsets to get fieldsets

Reported by: david_christiansen Owned by: nobody
Component: contrib.admin Version: 1.1
Severity: Keywords: fieldsets form modelform permissions
Cc: domen@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I'm presently limiting the fields that some users can edit in our admin interface by defining two fieldsets variables and then overriding get_fieldsets on the admin object to check for a user permission and return the right fieldsets. It looks like this:

def get_fieldsets(self, request, obj=None):
    if request.user.has_perm('page.can_change_structure'):
        return self.fieldsets
    else:
        return self.fieldsets_content_only

However, the exluded fields were being set to blank whenever the form was submitted by a user with fewer privileges. I tracked it down to the get_form method of ModelAdmin. It gets the fieldsets as such: (starting on line 777)

if self.declared_fieldsets:
    fields = flatten_fieldsets(self.declared_fieldsets)
else:
    fields = None

This ignores the get_fieldsets method. I'm not quite sure where the right place to fix this is, so I haven't written a patch. For our use case, it worked to simply override the get_form method and replace the four lines above with:

fields = flatten_fieldsets(self.get_fieldsets(request, obj=obj))

Perhaps something similar would be the right solution overall?

We're running Django 1.0.

Change History (5)

comment:1 follow-up: Changed 6 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

get_fieldsets() is indeed called, at line 517 of contrib/admin/options.py, as part of instantiating the AdminForm (a wrapper around the form which helps with some of the admin niceties). And I've personally got code in production which relies on get_fieldsets() working as advertised, so I know it does.

If your real issue is that you want to change not only the fieldsets but also which fields are made available, you'll need to do a bit more work (e.g., in get_form() to swap out form classes on the fly).

comment:2 in reply to: ↑ 1 Changed 6 years ago by edcrypt

Replying to ubernostrum:

If your real issue is that you want to change not only the fieldsets but also which fields are made available, you'll need to do a bit more work (e.g., in get_form() to swap out form classes on the fly).

Why not just use get_fieldsets() on get_form() too? If get_formsets() gives the possibility of changing which fields appear to some users, it should be used to determine what gets saved too, IMHO.

comment:3 Changed 5 years ago by iElectric

  • Cc domen@… added
  • milestone set to 1.2
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from 1.0 to 1.1

I have also stumbled upon this issue.

get_fieldsets is not used in get_form, which means it can not be used to populate fieldsets dynamically from request information.

I had to override get_form with following snippet:

        self.fieldsets = self.get_fieldsets(request, obj)

It is not about how much is there to change Django. But the issue is, forward compatibility and how much time user spent for fixing this issue. Took me 2 unproductive hours.

I vote this should be patched.

comment:4 Changed 5 years ago by russellm

  • Resolution set to invalid
  • Status changed from reopened to closed

Already closed by a core developer. Start a discussion on django-dev if you disagree.

comment:5 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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