Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#8367 closed (fixed)

Small bug in ModelAdmin.get_fieldsets()

Reported by: jarrow Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I think this line here:

form = self.get_form(request)

should pass obj to get_form():

form = self.get_form(request, obj)

http://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py?rev=8396#L334

Attachments (0)

Change History (7)

comment:1 Changed 6 years ago by mtredinnick

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

You've proposed a solution without describing the problem you're trying to solve. So we can't assess if the solution is appropriate.

The change you mention looks useful, since it allows overriding the form on a per-model basis, but is that the problem that motivated you to file the ticket? If not, what's the real problem at issue here?

comment:2 Changed 6 years ago by jarrow

I didn't post the problem because I just discovered this while looking over the code and thought it's trivial :) But here we go:

If I'm thinking straight this could lead to get_form() (overridden) beeing called with obj=None even though there is an object. It could than return a different form which get_fieldsets() (not overriden) would base it's return value on.

I could probably come up with a test case but I think it's enough to argue that if get_form() get's the object it should pass it on to any other function that also takes the object.

comment:3 Changed 6 years ago by jarrow

When reading your comment again I figure a simple yes to your question would have been enough :) But anyway I actually ran into the problem today:

    class MyModelAdminForm(forms.ModelForm):
        class Meta:
            model = MyModel
        extra_option = forms.BooleanField()
    
    class MyModelAdmin(admin.ModelAdmin):
        def get_form(self, request, obj=None, **kwargs):
            if obj:
                return MyModelAdminForm
            return super(MyModelAdmin, self).get_form(request, obj, **kwargs)

This will not display the extra field as get_fieldsets() gets the wrong form when calling get_form().

comment:4 Changed 6 years ago by brosner

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by buriy

duplicate of #7703 and #8400

comment:6 Changed 6 years ago by brosner

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

(In [8479]) Fixed #8367 -- Pass the object to get_form from get_fieldsets in ModelAdmin. Thanks jarrow for catching this.

comment:7 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.