Clean up duplicate code in admin formset handling

[Florian Apolloner]

See: ​http://groups.google.com/group/django-developers/browse_thread/thread/bfad2774ff7c357b#

The attached patch still might need some tests, though the current testsuite passes and should already cover all the cases.

[Alex Gaynor]

I don't think InlineAdminFormset is a public API, so I'd rather this method returned "normal" formset objs.

[Florian Apolloner]

Hmm, I uploaded a new page which returns formsets and inlines instead of the admin_inlines. I am still not really happy with it though. Especially since I am running into this problem now:

    def get_formset_instances(self, request, obj, change):
        f, i = super(RecipeAdmin, self).get_formset_instances(request, obj, change)
        return [f[0]], [i[0]]

That's a naive example for dropping inlines, but shows my problem… I have two inlines on the page and drop the second, this is fine, until I send a POST request. the super call instantiates the form and can't find the management data (obviously cause it's not there) before I get a chance to drop it. I am beginning to think that we shouldn't return the instances but the formclasses + inlines. (And maybe clean up the code by using an _construct_formsets function). Note: I am aware of the fact, that I could just c&p the code from get_formset_instances into my admin and modify it as needed, but that's not really nice… Any ideas?

[Florian Apolloner]

s/page/patch/g

[Jacob]

Milestone 1.3 deleted

[Florian Apolloner]

Fixed in r16934 via #8060

[Carl Meyer]

The fix for #8060 did touch some of this code, and would require minor updates to this patch, but unless I'm missing something I don't think it actually fixed the duplication this ticket was addressing. It added a get_inline_instances method, because the inline instances are now permissions-dependent rather than static; but the actual code that creates formsets based on those inline instances is still duplicated with minor differences several different places in add_view and change_view. I noticed that duplication in working on #8060 and I still think it'd be good to clean it up if we can, so I'd like to leave this ticket open until we do so.

[Tim Graham]

Adding a patch which is purely a code cleanup -- no change in behavior. I don't see any obvious benefit of making this new method a public API, but let me know.

[Florian Apolloner]

I see one downside in https://code.djangoproject.com/attachment/ticket/14580/14580.diff#L97 -- get_formsets calls get_inline_instances, but _create_fromsets could work with a different set of instances, the cleanest way might be to pass inline_instances down to get_formsets (maybe as inline_instances=None and call get_inline_instances only if it's indeed None for backwards compat)

[Florian Apolloner]

Okay, my comment about the backwards-compat was crap, we can't just add args to the signature of get_formsets :(. If you look at the docs: ​https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets your current patch will break, since the arguments to zip in https://code.djangoproject.com/attachment/ticket/14580/14580.diff#L97 will no longer have the same length! Essentially we'd need one method which returns the inline and the formset together... FWIW, even the current code is broken in that regard and the docs should drop the inline via get_inline_instances instead.

[Tim Graham]

Thanks for the feedback. When you say "I see one downside" is that in reference to my comment about not making this a public API?

I haven't taken the time to fully understand the issue you are describing, but it seems like it may at least be related to #20702. I can take a closer look at this tomorrow unless you are interested in working up a solution.

[Florian Apolloner]

I am fine with not making this a public API, #20702 seems to be exactly what I ment.

[Tim Graham]

In 402b4a7a20a4f00fce0f01cdc3f5f97967fdb935:

Fixed #14580 -- Cleaned up duplicate code in admin formset handling.

Thanks apollo13 for the report and review.