Opened 13 years ago

Closed 10 years ago

#16256 closed New feature (wontfix)

More class based views: formsets derived generic views

Reported by: rasca Owned by: Asif Saifuddin Auvi
Component: Generic views Version: dev
Severity: Normal Keywords:
Cc: rasca7@…, anton@…, GotenXiao, aaron@…, emyller@…, rh0dium, gerdemb, Trey Hunner, charette.s@…, hanne.moa@…, Markus Holtermann, dpwrussell, ville@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This ticket aims to add more class based views to the generic ones provided by django.

I've implemented a FormSetsView, a ModelFormSetsView and a InlineFormSetsView.

Related django-dev thread: http://groups.google.com/group/django-developers/browse_thread/thread/6964432300ca0c6f/9aa1f1ea1ad16d81?lnk=raot&pli=1

Attachments (3)

ticket16256.patch (24.8 KB ) - added by rasca 13 years ago.
ticket16256_2.patch (42.3 KB ) - added by rasca 13 years ago.
FormSets to FormSet and broke edit.py into a package
ticket16256-with-admin-InlineAdmin-refactor-with-EnhancedInlineFormSet.patch (45.8 KB ) - added by rasca 13 years ago.

Download all attachments as: .zip

Change History (40)

by rasca, 13 years ago

Attachment: ticket16256.patch added

comment:1 by rasca, 13 years ago

Has patch: set
Needs documentation: set
Owner: changed from nobody to rasca
Triage Stage: UnreviewedAccepted

Accepting based on a talk with Russell in #django-dev

Just attached a patch with code from django-enhanced-cbv.

You can also follow my django branch on github.

It doesn't have any docs yet.

comment:2 by rasca, 13 years ago

Summary: More class based viewsMore class based views: formsets derived generic views

comment:3 by anonymous, 13 years ago

Why your views are called FormSetsView, ModelFormSetsView and InlineFormSetsView, but not FormSetView, ModelFormSetView and InlineFormSetView?

comment:4 by Jannis Leidel, 13 years ago

Patch needs improvement: set

Agreed, using "FormSet" in the name is better.

by rasca, 13 years ago

Attachment: ticket16256_2.patch added

FormSets to FormSet and broke edit.py into a package

comment:5 by rasca, 13 years ago

Patch needs improvement: unset

comment:6 by rasca, 13 years ago

I have just closed #16215 in favor of this one. We should review the code in #16215 to look at what he may have done better in terms of API.

The idea is to keep it as close as possible to the forms API.

comment:7 by rasca, 13 years ago

Just for posterity, the 's' in FormSetsView et al was due to supporting multiple formsets in the same view. I can change it back if you like.

comment:8 by AndrewIngram, 13 years ago

With regards to handling multiple formsets in one view, I'd treat that as a separate problem. Ideally you'd want a single view that can handle forms *and* formsets. I've already attempted this (you can see it in the github repo mentioned in #16215, it's called MultiFormView), but I struggled with creating a decent API that made it easy to instantiate forms and formsets that have custom init arguments and so on.

in reply to:  8 comment:9 by rasca, 13 years ago

Replying to AndrewIngram:

With regards to handling multiple formsets in one view, I'd treat that as a separate problem. Ideally you'd want a single view that can handle forms *and* formsets. I've already attempted this (you can see it in the github repo mentioned in #16215, it's called MultiFormView)

Yeah.. my main goal was to replicate the admin's inlines. We could create another ticket for the 'MutiFormView'...

but I struggled with creating a decent API that made it easy to instantiate forms and formsets that have custom init arguments and so on.

What do you think about what I came up with?

comment:10 by rasca, 13 years ago

I've just made InlineAdmin a subclass of EnhancedInlineFormSet and all tests pass! Maybe the first approach on refactoring the admin with generic cbv?

One naming discrepancy I found was that I use formset_class and form_class and the admin uses formset and form. I fixed this by overriding get_formset_class and get_form_class.

comment:11 by AndrewIngram, 13 years ago

I think you may have introduced too much in one go, I think this can all be broken down into discrete things that need to be built. The way I see it is like this (I haven't achieved all of this with my own implementation yet):

  1. FormSetView and ModelFormSet view should mimic FormView and ModelFormView as closely as possible. In the same way that ModelFormMixin extends FormMixin and SingleObjectMixin, ModelFormSetView should extend FormSetMixin and MultipleObjectMixin, this way we get stuff like pagination for free (I haven't done this yet, but it was part of the plan).
  2. For InlineFormSetView, I think we need to respect the API for inline_formsets, ie it just simplifies building these formsets. Your implementation of this view includes the form for the owner model, which I think is too much functionality, it's part of the bigger problem of finding a mechanism to combine various forms and formsets into a single view.
  3. Once we have established the functionality for creating views with single formsets, we can start looking at ways to bridge the gap between (for example) the needs of the admin and the basic formset views.

I must confess that I've not got my head around all of what you've done. What is the purpose of the EnhancedFormset objects you've introduced?

comment:12 by rasca, 13 years ago

Hi Andrew, my EnhancedFormSet et al resembles the admin's InlineAdmin, where we can set some things needed to instantiate the formset. For example, you've put extra and can_order inside the Mixin class, but we need to have a different set of parameters for each formset, so I made another class encapsulating this parameters.

I don't like the naming though.

About splitting everything in steps... I'm not sure about it. An implementation of inlineformsets that doesn't allow more than one inline doesn't make much sense IMO. I might bring this up in django-developers, gonna try #django-dev first.

But surely, another ticket for multiple Forms or ModelForms would be great! I think they will be fairly easy to build from what we have so far.

comment:13 by AndrewIngram, 13 years ago

I had a go at my own implementation of what you've done with regards to editing inline formsets alongside a model (but I didn't touch the admin itself), the logic is nearly identical to your solution (largely coincidence, but I did borrow ideas for naming conventions), but I've done my best to mimic the structure of Django's existing class-based views for editing objects. The two new concrete views are:

  • CreateWithInlinesView
  • UpdateWithInlinesView

Each can take an inlines attribute which is a list of classes that extend InlineFormSet (which itself is just a bit of trickery around my InlineFormSetMixin which is used for the much simpler InlineFormSetView).

Code is here: https://github.com/AndrewIngram/django-extra-views/blob/master/extra_views/advanced.py

comment:14 by rasca, 13 years ago

I do think the splitting of the view into a Create and a Update views is more like the other generic cbv views, although I end up joining them most of the times, but that's another issue.

I still need a little more time to think about making views for 1 inline and for N inlines different. That way you end up having a formset_valid() in InlineFormSet which doesn't get used and might confuse people, for example.

Anyone care to weight in on this?

Being the logic the same in both implementations we might argue that the probability of it being correct is somewhat high =)

comment:15 by Jannis Leidel, 13 years ago

I agree with AndrewIngram's plan for staying close to the already existing form related mixins and I believe rasca's approach is basically trying to do the impossible (porting admin) to early in the API design.

So let's step back a bit and first implement the formset related views and then see what glue code or concept is needed to help concrete implementations like the admin to use the new API. We shouldn't carry around potentially bad API decisions in the admin into the generic views just to get it done quicker.

comment:16 by kd4ttc, 13 years ago

See ticket 16418. The addition of a generic view for another object type should just be done by Subclassing the generic view and then redefining get_object() to return the new object. The ticket 16418 points out a small bug in the approach, but a simple workaround is available until it gets fixed.

comment:17 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:18 by Victor Hooi, 13 years ago

heya,

Curious - is there any word on this ticket?

I'm guessing it's not going to hit 1.4, but any word when it might hit trunk? Would love to try this out for a project I'm working on =).

Cheers,
Victor

comment:19 by MichaelA, 13 years ago

If one (or more) of the core devs could give me some direction, I'd be happy to create an updated patch based on rasca's patch. The comments above leave more ambiguity than I feel comfortable dealing with on my own.

This feature alone, for those developing their own custom admin interfaces, could be the most excellent advancement in Django since newforms (IMHO).

Thanks
Michael

comment:20 by Anton Strogonoff, 13 years ago

Cc: anton@… added

comment:21 by GotenXiao, 13 years ago

Cc: GotenXiao added

comment:22 by Aaron C. de Bruyn, 13 years ago

Cc: aaron@… added

comment:23 by Evandro Myller, 13 years ago

Cc: emyller@… added

comment:24 by rh0dium, 13 years ago

Cc: rh0dium added

Can one of the core dev please reply to the originator. It appears he's asking the right questions - this has been sitting idle for over 4 months. I've added a separate ticket to update the docs as well. Andrew perhaps you could provide the docs patch as well?? Just a thought! In any event I'm pulling your code :)

Last edited 13 years ago by rh0dium (previous) (diff)

comment:25 by gerdemb, 13 years ago

Cc: gerdemb added

comment:26 by Trey Hunner, 13 years ago

Cc: Trey Hunner added

comment:27 by Simon Charette, 12 years ago

Cc: charette.s@… added

in reply to:  description comment:28 by hanne.moa@…, 12 years ago

Cc: hanne.moa@… added
Version: 1.3master

comment:29 by Markus Holtermann, 11 years ago

Cc: Markus Holtermann added

comment:30 by dpwrussell, 11 years ago

Cc: dpwrussell added

comment:31 by ville@…, 11 years ago

Cc: ville@… added

comment:32 by Fabio Caritas Barrionuevo da Luz <bnafta@…>, 11 years ago

What is the status of this?

comment:33 by Tom Christie, 10 years ago

Resolution: wontfix
Status: newclosed

There's really no reason we shouldn't be pushing for additional class based generic views to be implemented as third party packages.

See 'Django Extra Views' or 'Django Vanilla Views' as examples of these.

comment:34 by Asif Saifuddin Auvi, 10 years ago

Resolution: wontfix
Status: closednew

comment:35 by Asif Saifuddin Auvi, 10 years ago

Owner: changed from rasca to Asif Saifuddin Auvi
Status: newassigned

comment:36 by Asif Saifuddin Auvi, 10 years ago

considering this ticket as a part of gsoc proposal. so reopening. django-extra-views and django-vanila-views are great and their philosophy should be implemented in django contrib apps/ django proper.

comment:37 by Tim Graham, 10 years ago

Resolution: wontfix
Status: assignedclosed

You may reopen a ticket after there is consensus to do so on the DevelopersMailingList. Thanks.

Note: See TracTickets for help on using tickets.
Back to Top