Code

Opened 4 years ago

Closed 12 months ago

#14580 closed Cleanup/optimization (fixed)

Clean up duplicate code in admin formset handling

Reported by: apollo13 Owned by: nobody
Component: contrib.admin Version: master
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 apollo13 4 years ago.
admin_get_formset_instances2.diff (8.2 KB) - added by apollo13 4 years ago.
14580.diff (5.8 KB) - added by timo 12 months ago.

Download all attachments as: .zip

Change History (20)

Changed 4 years ago by apollo13

comment:1 Changed 4 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

Changed 4 years ago by apollo13

comment:2 Changed 4 years ago by apollo13

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 Changed 4 years ago by apollo13

s/page/patch/g

comment:4 Changed 4 years ago by anonymous

  • Cc sehmaschine@… added

comment:5 Changed 4 years ago by julien

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:8 Changed 3 years ago by apollo13

  • Easy pickings unset
  • Resolution set to fixed
  • Status changed from new to closed
  • UI/UX unset

Fixed in r16934 via #8060

comment:9 Changed 3 years ago by carljm

  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

Changed 12 months ago by timo

comment:11 Changed 12 months ago by timo

  • Cc timograham@… added
  • Needs tests unset
  • Summary changed from Cleaning up duplicate code in the admin, add get_formsets_instances to 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 Changed 12 months ago by apollo13

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 Changed 12 months ago by apollo13

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

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 Changed 12 months ago by apollo13

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

comment:16 Changed 12 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

In 402b4a7a20a4f00fce0f01cdc3f5f97967fdb935:

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

Thanks apollo13 for the report and review.

comment:17 Changed 12 months ago by timo

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

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.