Using ModelAdmin.get_formsets() to filter inlines is broken.

[stanislas.guerra@…]

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.

[Tim Graham]

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.

[Timothy Schilling]

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

[Tim Graham]

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.

​https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets

[Timothy Schilling]

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.

[Timothy Schilling]

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

[Tim Graham]

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.

[Timothy Schilling]

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.

[Tim Graham]

This looks good Tim. I made a couple small edits and created a ​pull request in hopes of getting at least one more review.

[Tim Graham <timograham@…>]

In 0d1ba84d13eb6000c9f6e54b03d52863fcd31f27:

Fixed #20702 -- Deprecated get_formsets in favor of get_formsets_with_inlines.

Thanks stanislas.guerra at gmail.com for the report.

[Tim Graham <timograham@…>]

In 2c9e95639e5a353f9fe1b81ecd3fdc5e2212781e:

Removed ModelAdmin.get_formsets() per deprecation timeline; refs #20702.

[Tim Graham <timograham@…>]

In 7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3:

[1.8.x] Added versionadded to ModelAdmin.get_formsets_with_inlines(); refs #20702.

[Tim Graham <timograham@…>]

In ecbe20fe20ddc54aa034530b38a73195ee3f598d:

[1.7.x] Added versionadded to ModelAdmin.get_formsets_with_inlines(); refs #20702.

Backport of 7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3 from stable/1.8.x