Opened 16 years ago

Closed 13 years ago

#8999 closed Cleanup/optimization (fixed)

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

Reported by: Sean Legassick 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: no

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', []).

Attachments (5)

modeladmin-getform-and-getformset-exclude-fix.diff (928 bytes ) - added by Sean Legassick 16 years ago.
modeladmin-getform-and-getformset-exclude-fix-2.diff (1.3 KB ) - added by Sean Legassick 16 years ago.
modeladmin-getform-and-getformset-exclude-fix-with-tests.diff (4.6 KB ) - added by Sean Legassick 16 years ago.
modeladmin-getform-and-getformset-exclude-will-overwrite.diff (2.2 KB ) - added by Brandon Konkle 14 years ago.
8999.getform-exclude-kwarg.diff (3.3 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by Sean Legassick, 16 years ago

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

comment:2 by Brian Rosner, 16 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 by Sean Legassick, 16 years ago

Needs tests: unset

added regression tests for bugfixes

comment:4 by (none), 16 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:5 by jkocherhans, 16 years ago

Owner: changed from nobody to jkocherhans
Status: newassigned

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 by Sean Legassick, 16 years ago

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 by jkocherhans, 16 years ago

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 by Brandon Konkle, 14 years ago

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

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: Bug

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

by Julien Phalip, 13 years ago

comment:10 by Julien Phalip, 13 years ago

Patch needs improvement: unset
Type: BugCleanup/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 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

LGTM, although it needs a release note addition.

comment:12 by Jannis Leidel, 13 years ago

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

comment:13 by Julien Phalip, 13 years ago

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

Resolution: fixed
Status: assignedclosed

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