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)
Change History (20)
by , 14 years ago
Attachment: | admin_get_formset_instances.diff added |
---|
comment:1 by , 14 years ago
by , 14 years ago
Attachment: | admin_get_formset_instances2.diff added |
---|
comment:2 by , 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:4 by , 14 years ago
Cc: | added |
---|
comment:5 by , 14 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:6 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
Resolution: | → fixed |
Status: | new → closed |
UI/UX: | unset |
comment:9 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 , 12 years ago
Status: | reopened → new |
---|
by , 11 years ago
Attachment: | 14580.diff added |
---|
comment:11 by , 11 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Summary: | Cleaning up duplicate code in the admin, add get_formsets_instances → Clean 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 , 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 , 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 , 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 , 11 years ago
I am fine with not making this a public API, #20702 seems to be exactly what I ment.
comment:16 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
In 402b4a7a20a4f00fce0f01cdc3f5f97967fdb935:
Fixed #14580 -- Cleaned up duplicate code in admin formset handling.
Thanks apollo13 for the report and review.
comment:17 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I don't think InlineAdminFormset is a public API, so I'd rather this method returned "normal" formset objs.