#20702 closed Bug (fixed)
Using ModelAdmin.get_formsets() to filter inlines is broken.
Reported by: | Owned by: | Timothy Schilling | |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | admin inlines get_formsets |
Cc: | Timothy Schilling, Tim Graham | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi,
In the 1.4 admin documentation https://docs.djangoproject.com/en/1.4/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets it is said :
For example if you wanted to display a particular inline only in the change view, [...]
By extension, I tried to use that logic to display a particular inline only if the instance met a particular condition:
class BarInline(admin.TabularInline): model = Bar fields = ("name", "surname") class BazInline(admin.TabularInline): model = Baz fields = ("country", "zipcode") class FooAdmin(admin.ModelAdmin): inlines = [BarInline, BazInline] def get_formsets(self, request, obj=None): for inline in self.get_inline_instances(request): # Only in change view. if obj is None: continue if isinstance(inline, BarInline) and obj.condition(): yield inline.get_formset(request, obj) elif isinstance(inline, BazInline) and obj.another_condition(): yield inline.get_formset(request, obj)
Which raise an exception :
KeyError at /admin/app/foo/22/ "name" ... /Library/Python/2.7/site-packages/django/contrib/admin/helpers.py in fields 240. yield self.formset.form.base_fields[field]
The reason is because ModelAdmin.add_view() / .change_view() use zip(self.get_formsets(request), inline_instances) fonction to construct the formsets from the Formset class and the inline instance and so expects two ordered listes of matching Formset and Inline.
So if in get_formsets() you don't yield something you create a shift in the lists which explain the exception raised (use fields from BarInline to instanciate BazFormset).
A workaround is to override change_view() and change self.inline locally:
class FooAdmin(admin.ModelAdmin): inlines = [] def change_view(self, request, object_id, form_url='', extra_context=None): from django.contrib.admin.util import unquote obj = self.get_object(request, unquote(object_id)) if obj: if obj.condition(): self.inlines = [BarInline] elif obj.another_condition(): self.inlines = [BazInline] return super(FooAdmin, self).change_view(request, object_id, form_url, extra_context)
Is get_formsets() not supposed to be used like that?
Maybe this is implicitly fixed in 1.5 with the addition of the obj parameter in get_inline_instances()?
Thanks.
Change History (12)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Owner: | changed from | to
Status: | new → assigned |
I've created a patch that changes the ModelAdmin's get_formsets to return a tuple of the formset and the inline. This allows us to move away from using the zip function and avoids the problem of get_formsets not returning an inline that was including in the inlines property.
Patch: https://github.com/tim-schilling/django/commit/ea7c7dc856fd773d74214099f646f8c4e2c7f2f3
If this looks good, I'll add some additional unit tests around the changes in functionality (marking Needs tests so we don't forget). I did add the test that verified the bug in modeladmin.ModelAdminTests
comment:3 by , 11 years ago
Patch needs improvement: | set |
---|
Thanks, Tim. The patch looks like a good solution. Unfortunately, get_formsets
is a documented public API, so we cannot make this change without coming up with a solution that will be backwards compatible.
comment:4 by , 11 years ago
Ah, that makes sense. I'm not sure we can do that cleanly. The first solution that comes to mind for me is to pass in some value that indicates that it should return a tuple, otherwise just return the formset but that seems real hacky to me.
Another solution would be to create a new method to use instead of get_formsets. Maybe get_formset_and_inline_tuples? That way that old method would continue to work, as well as the new method.
comment:5 by , 11 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Alright so I re-added get_formsets and have it calling the renamed get_formsets_with_inlines so the logic (though as little as it is) remains the same for both calls.
Commit: https://github.com/tim-schilling/django/commit/bb00388b8a7feb14a6d51d4d8837405b8d031346
comment:6 by , 11 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
Unfortunately, that approach still won't work because if someone has overridden get_formsets()
their implementation will no longer be called. Perhaps we can use an approach like was done in ebb3e50243448545c7314a1932a9067ddca5960b which detects if the old method was overridden and uses it (but raises a DeprecationWarning
), otherwises uses the new method - get_formsets_with_inlines()
in this case.
Also, it looks like your commits may be against stable/1.4.x
. They should be on master
.
comment:7 by , 11 years ago
Patch needs improvement: | unset |
---|
Good point. I didn't think of that. I've got a solution, though I'm not sure of how to handle the naming of the methods. In order to get get_formsets()
to trigger a deprecation warning, I had to put the logic into the helper function _get_formsets()
and then call that with get_formsets()
where the warning is raised. Otherwise, the deprecation warning wouldn't be triggered until that generator had next()
called.
Then in order to use either the old get_formsets()
method or the new get_formsets_with_inlines()
method result, I had to zip the result of get_formsets()
with the inlines when it was returned. I brought the fetching of the inlines into the method since they were simply being passed around in the methods that were calling get_formsets_with_inlines()
. I can move them back out if needed.
Note that this does change the interface of _create_formsets()
but that's an internal method. The changes to it were removing of the inline_instances parameter and having it return a tuple containing the list of formsets and the list of used inline instances.
For the original bug, the solution's admin.py would now look like the following, overriding get_formsets_with_inlines()
:
from django.contrib import admin from .models import Bar, Baz, Foo class BarInline(admin.TabularInline): model = Bar fields = ("title", "surname") class BazInline(admin.TabularInline): model = Baz fields = ("country", "zipcode") class FooAdmin(admin.ModelAdmin): inlines = [BarInline, BazInline] def get_formsets_with_inlines(self, request, obj=None): for inline in self.get_inline_instances(request): # Only in change view. if obj is None: continue if isinstance(inline, BarInline) and obj.condition(): yield inline.get_formset(request, obj), inline elif isinstance(inline, BazInline) and obj.another_condition(): yield inline.get_formset(request, obj), inline admin.site.register(Bar) admin.site.register(Baz) admin.site.register(Foo, FooAdmin)
Commit (on master): https://github.com/tim-schilling/django/commit/5eac26f353533f6eb68779917d0885938ee2eebf
I've also updated the documentation. I'm not sure if I missed any steps, this is my first time working on a ticket.
comment:8 by , 11 years ago
Version: | 1.4 → master |
---|
This looks good Tim. I made a couple small edits and created a pull request in hopes of getting at least one more review.
comment:9 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
apollo13 also noticed this in #14580. We at least need a doc update pointing out that you'll need to drop the inline using
get_inline_instances()
. Possibly we can make the code incontrib.admin.options
a bit more sophisticated to detect this so it's not necessary to override both.