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

[Sean Legassick]

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

[Sean Legassick]

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

[Sean Legassick]

added regression tests for bugfixes

[(none)]

Milestone post-1.0 deleted

[jkocherhans]

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.

[Sean Legassick]

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.

[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! ;)

[Brandon Konkle]

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.

[Julien Phalip]

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

[Julien Phalip]

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().

[Jannis Leidel]

LGTM, although it needs a release note addition.

[Jannis Leidel]

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

[Julien Phalip]

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.

[Ramiro Morales]

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.