Code

#20702 closed Bug (fixed)

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

Reported by: stanislas.guerra@… Owned by: CodenameTim
Component: contrib.admin Version: master
Severity: Normal Keywords: admin inlines get_formsets
Cc: CodenameTim, timo 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.

Attachments (0)

Change History (9)

comment:1 Changed 12 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 11 months ago by CodenameTim

  • Has patch set
  • Needs tests set
  • Owner changed from nobody to CodenameTim
  • Status changed from new to 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 Changed 11 months ago by timo

  • 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 Changed 11 months ago 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.

comment:5 Changed 11 months ago by CodenameTim

  • Cc CodenameTim 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 Changed 11 months ago by timo

  • Cc timo 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 Changed 10 months ago by CodenameTim

  • 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 Changed 10 months ago by timo

  • Version changed from 1.4 to 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 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 0d1ba84d13eb6000c9f6e54b03d52863fcd31f27:

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.