Opened 14 years ago

Closed 11 years ago

#14580 closed Cleanup/optimization (fixed)

Clean up duplicate code in admin formset handling

Reported by: Florian Apolloner Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: sehmaschine@…, timograham@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (3)

admin_get_formset_instances.diff (8.7 KB ) - added by Florian Apolloner 14 years ago.
admin_get_formset_instances2.diff (8.2 KB ) - added by Florian Apolloner 14 years ago.
14580.diff (5.8 KB ) - added by Tim Graham 11 years ago.

Download all attachments as: .zip

Change History (20)

by Florian Apolloner, 14 years ago

comment:1 by Alex Gaynor, 14 years ago

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

by Florian Apolloner, 14 years ago

comment:2 by Florian Apolloner, 14 years ago

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?

comment:3 by Florian Apolloner, 14 years ago

s/page/patch/g

comment:4 by anonymous, 14 years ago

Cc: sehmaschine@… added

comment:5 by Julien Phalip, 14 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:6 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:7 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:8 by Florian Apolloner, 13 years ago

Easy pickings: unset
Resolution: fixed
Status: newclosed
UI/UX: unset

Fixed in r16934 via #8060

comment:9 by Carl Meyer, 13 years ago

Resolution: fixed
Status: closedreopened

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.

comment:10 by Aymeric Augustin, 12 years ago

Status: reopenednew

by Tim Graham, 11 years ago

Attachment: 14580.diff added

comment:11 by Tim Graham, 11 years ago

Cc: timograham@… added
Needs tests: unset
Summary: Cleaning up duplicate code in the admin, add get_formsets_instancesClean up duplicate code in admin formset handling

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.

comment:12 by Florian Apolloner, 11 years ago

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)

comment:13 by Florian Apolloner, 11 years ago

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.

comment:14 by Tim Graham, 11 years ago

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.

comment:15 by Florian Apolloner, 11 years ago

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

comment:16 by Tim Graham, 11 years ago

Triage Stage: AcceptedReady for checkin

In 402b4a7a20a4f00fce0f01cdc3f5f97967fdb935:

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

Thanks apollo13 for the report and review.

comment:17 by Tim Graham, 11 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top