Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#20702 closed Bug (fixed)

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

Reported by: stanislas.guerra@… 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 Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by Timothy Schilling, 11 years ago

Has patch: set
Needs tests: set
Owner: changed from nobody to Timothy Schilling
Status: newassigned

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 Tim Graham, 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.

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

comment:4 by Timothy Schilling, 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 Timothy Schilling, 11 years ago

Cc: Timothy Schilling 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 Tim Graham, 11 years ago

Cc: Tim Graham 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 Timothy Schilling, 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 Tim Graham, 11 years ago

Version: 1.4master

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 Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 0d1ba84d13eb6000c9f6e54b03d52863fcd31f27:

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

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

comment:10 by Tim Graham <timograham@…>, 9 years ago

In 2c9e95639e5a353f9fe1b81ecd3fdc5e2212781e:

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

comment:11 by Tim Graham <timograham@…>, 9 years ago

In 7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3:

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

comment:12 by Tim Graham <timograham@…>, 9 years ago

In ecbe20fe20ddc54aa034530b38a73195ee3f598d:

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

Backport of 7cfcdd98dcf0dbbde7cfd656e450c52342dbe6f3 from stable/1.8.x

Note: See TracTickets for help on using tickets.
Back to Top