Opened 7 years ago

Closed 4 years ago

#8999 closed Cleanup/optimization (fixed)

ModelAdmin.get_form and get_formset will overwrite self.exclude with a kwargs exclude

Reported by: seanl Owned by: jkocherhans
Component: contrib.admin Version: 1.0
Severity: Normal Keywords:
Cc: brandon.konkle@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

In ModelAdmin.get_form and ModelAdmin.get_formset the intent is to combine self.exclude and kwargs['exclude'] but the defaults.update(kwargs) will overwrite the combined values. Fix is simply changing the kwargs.get('exclude', []) to kwargs.pop('exclude', []).

Change History (19)

comment:1 Changed 7 years ago by seanl

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

Patch modified to coerce self.exclude to list before concatenation, so any iterable can be used in the ModelAdmin subclass parameters.

comment:2 Changed 7 years ago by brosner

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 7 years ago by seanl

  • Needs tests unset

added regression tests for bugfixes

comment:4 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:5 Changed 6 years ago by jkocherhans

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

I don't think we should be combining anything here. If someone passes exclude to get_form, that value should override self.exclude. If someone wants different behavior, they can override the get_form method. I need to think about this a bit more in the context of a few other related issues though.

comment:6 Changed 6 years ago by seanl

OK, that also makes sense as behaviour, but the current code looks like it intends to combine the values, but due to a subtlety kwargs takes precedence. (that the 'exclude' kwarg if present is left in the kwargs dict, and thus overrides when you come to defaults.update(kwargs) ). If the current behaviour is what is intended, then the whole 'excludes' line in the defaults dictionary literal should be removed, and the behaviour will be the same, but with the code reflecting the intent.

comment:7 Changed 6 years ago by jkocherhans

Yeah, the apparent intention of the existing code is the wrong thing to do IMO. Just look at what a subtle bug it produced! ;)

comment:8 Changed 5 years ago by bkonkle

  • Cc brandon.konkle@… added

I just ran into this bug, myself. If the 'exclude' kwarg is intended to override self.exclude, then exclude.extend(kwargs.get("exclude", [])) should not be called. This implies that the kwarg will extend self.exclude.

My use case here is an ModelAdmin object where I'm overriding get_form to exclude two fields if this is a change as opposed to a create action. My ModelAdmin subclass has readonly_fields defined, which by default extend self.exclude in the ModelAdmin class's get_form method. I'm doing:

        if obj:
            kwargs['exclude'] = ['field', 'other_field']
        return super(MySubclass, self).get_form(request, obj,
                                                            **kwargs)

The end result is that the readonly_fields I have defined on my subclass get presented twice in the rendered admin view, since defaultsexclude? is overridden by the 'exclude' kwarg.

It's easy to fix, of course, but the existing code in ModelAdmin.get_form is confusing.

comment:9 Changed 4 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Bug

The tests would need to be rewritten using unittests since this is now Django's preferred way.

Changed 4 years ago by julien

comment:10 Changed 4 years ago by julien

  • Patch needs improvement unset
  • Type changed from Bug to Cleanup/optimization

I agree with jkocherhans that the exclude kwarg should completely override any other declarations rather than being combined to them. So, the current behaviour is actually correct.

The line "exclude.extend(kwargs.get("exclude", []))", which seems to suggest that the kwarg should be combined, is actually useless and it can be safely removed. The attached patch removes that line and verifies the current behaviour both for ModelAdmin.get_form() and InlineAdmin.get_formset().

comment:11 Changed 4 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

LGTM, although it needs a release note addition.

comment:12 Changed 4 years ago by jezdez

Actually, the test case could probably be split in two methods for further isolation.

comment:13 Changed 4 years ago by julien

Thanks Jannis. I don't think it needs a release note. The removed lines were actually useless. If exclude is passed via get_form()'s kwargs, then it is first combined to ModelAdmin.exclude but a bit further it overrides it anyway via defaults.update(kwargs).

The provided tests verify the current behaviour and they pass whether those two lines are present or not. This is basically just cleaning up some stale code. No one could have possibly relied on a different behaviour.

comment:14 Changed 4 years ago by ramiro

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

In [16422]:

Fixed #8999 -- Removed useless code in handling of exclude option in ModelAdmin and InlineModelAdmin custom form(set) hooks. Thanks goes to seanl for the report, patch and bkonkle and Julien for further work on final patch.

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