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)
Change History (19)
by , 16 years ago
Attachment: | modeladmin-getform-and-getformset-exclude-fix.diff added |
---|
by , 16 years ago
Attachment: | modeladmin-getform-and-getformset-exclude-fix-2.diff added |
---|
comment:1 by , 16 years ago
comment:2 by , 16 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 16 years ago
Attachment: | modeladmin-getform-and-getformset-exclude-fix-with-tests.diff added |
---|
comment:5 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 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 , 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 , 14 years ago
Cc: | 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.
by , 14 years ago
Attachment: | modeladmin-getform-and-getformset-exclude-will-overwrite.diff added |
---|
comment:9 by , 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 , 13 years ago
Attachment: | 8999.getform-exclude-kwarg.diff added |
---|
comment:10 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Type: | Bug → 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 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
LGTM, although it needs a release note addition.
comment:12 by , 13 years ago
Actually, the test case could probably be split in two methods for further isolation.
comment:13 by , 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.
Patch modified to coerce self.exclude to list before concatenation, so any iterable can be used in the ModelAdmin subclass parameters.