[Django] #20702: Using ModelAdmin.get_formsets() to filter inlines is broken.
Django
----------------------------------+----------------------------------------
Reporter: | Owner: nobody
stanislas.guerra@… | Status: new
Type: Bug | Version: 1.4
Component: contrib.admin | Keywords: admin inlines get_formsets
Severity: Normal | Has patch: 0
Triage Stage: Unreviewed | UI/UX: 0
Easy pickings: 0 |
----------------------------------+----------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: stanislas.guerra@… | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Severity: Normal | Resolution:
Keywords: admin inlines | Triage Stage: Accepted
get_formsets | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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 in
`contrib.admin.options` a bit more sophisticated to detect this so it's
not necessary to override both.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:1>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by CodenameTim):
* owner: nobody => CodenameTim
* status: new => assigned
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:2>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:3>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by CodenameTim):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:4>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: CodenameTim (added)
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:5>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: timo (added)
* needs_better_patch: 0 => 1
Comment:
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:6>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.4
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:7>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Component: contrib.admin | Status: assigned
Keywords: admin inlines | Resolution:
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.4 => master
Comment:
This looks good Tim. I made a couple small edits and created a
[https://github.com/django/django/pull/1634 pull request] in hopes of
getting at least one more review.
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:8>
Django
Reporter: stanislas.guerra@… | Owner:
Type: Bug | CodenameTim
Severity: Normal | Version: master
Keywords: admin inlines | Resolution: fixed
get_formsets | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0d1ba84d13eb6000c9f6e54b03d52863fcd31f27"]:
{{{
#!CommitTicketReference repository=""
revision="0d1ba84d13eb6000c9f6e54b03d52863fcd31f27"
Fixed #20702 -- Deprecated get_formsets in favor of
get_formsets_with_inlines.
Thanks stanislas.guerra at gmail.com for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:9>
Django
Reporter: stanislas.guerra@… | Owner:
| CodenameTim
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin inlines | Triage Stage: Accepted
get_formsets |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"2c9e95639e5a353f9fe1b81ecd3fdc5e2212781e"]:
{{{
#!CommitTicketReference repository=""
revision="2c9e95639e5a353f9fe1b81ecd3fdc5e2212781e"
Removed ModelAdmin.get_formsets() per deprecation timeline; refs #20702.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:10>
Django
Reporter: stanislas.guerra@… | Owner:
| CodenameTim
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin inlines | Triage Stage: Accepted
get_formsets |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3"]:
{{{
#!CommitTicketReference repository=""
revision="7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3"
[1.8.x] Added versionadded to ModelAdmin.get_formsets_with_inlines(); refs
#20702.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:11>
Django
Reporter: stanislas.guerra@… | Owner:
| CodenameTim
Type: Bug | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin inlines | Triage Stage: Accepted
get_formsets |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"ecbe20fe20ddc54aa034530b38a73195ee3f598d"]:
{{{
#!CommitTicketReference repository=""
revision="ecbe20fe20ddc54aa034530b38a73195ee3f598d"
[1.7.x] Added versionadded to ModelAdmin.get_formsets_with_inlines(); refs
#20702.
Backport of 7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3 from stable/1.8.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20702#comment:12>