Opened 4 years ago

Closed 4 months ago

#16256 closed New feature (wontfix)

More class based views: formsets derived generic views

Reported by: rasca Owned by: auvipy
Component: Generic views Version: master
Severity: Normal Keywords:
Cc: rasca7@…, anton@…, GotenXiao, aaron@…, emyller@…, rh0dium, gerdemb, treyhunner, charette.s@…, hanne.moa@…, MarkusH, 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 4 years ago.
ticket16256_2.patch (42.3 KB) - added by rasca 4 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 4 years ago.

Download all attachments as: .zip

Change History (40)

Changed 4 years ago by rasca

comment:1 Changed 4 years ago by rasca

  • Has patch set
  • Needs documentation set
  • Needs tests unset
  • Owner changed from nobody to rasca
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

  • Summary changed from More class based views to More class based views: formsets derived generic views

comment:3 Changed 4 years ago by anonymous

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

comment:4 Changed 4 years ago by jezdez

  • Patch needs improvement set

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

Changed 4 years ago by rasca

FormSets to FormSet and broke edit.py into a package

comment:5 Changed 4 years ago by rasca

  • Patch needs improvement unset

comment:6 Changed 4 years ago by rasca

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

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 follow-up: Changed 4 years ago by 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), 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.

comment:9 in reply to: ↑ 8 Changed 4 years ago by rasca

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

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

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

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

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

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

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

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

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:18 Changed 4 years ago by victorhooi

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

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

  • Cc anton@… added

comment:21 Changed 3 years ago by GotenXiao

  • Cc GotenXiao added

comment:22 Changed 3 years ago by darkpixel

  • Cc aaron@… added

comment:23 Changed 3 years ago by emyller

  • Cc emyller@… added

comment:24 Changed 3 years ago by rh0dium

  • 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 3 years ago by rh0dium (previous) (diff)

comment:25 Changed 3 years ago by gerdemb

  • Cc gerdemb added

comment:26 Changed 3 years ago by treyhunner

  • Cc treyhunner added

comment:27 Changed 3 years ago by charettes

  • Cc charette.s@… added

comment:28 in reply to: ↑ description Changed 2 years ago by hanne.moa@…

  • Cc hanne.moa@… added
  • Version changed from 1.3 to master

comment:29 Changed 2 years ago by MarkusH

  • Cc MarkusH added

comment:30 Changed 19 months ago by dpwrussell

  • Cc dpwrussell added

comment:31 Changed 15 months ago by ville@…

  • Cc ville@… added

comment:32 Changed 15 months ago by Fabio Caritas Barrionuevo da Luz <bnafta@…>

What is the status of this?

comment:33 Changed 8 months ago by tomchristie

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

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 Changed 4 months ago by auvipy

  • Resolution wontfix deleted
  • Status changed from closed to new

comment:35 Changed 4 months ago by auvipy

  • Owner changed from rasca to auvipy
  • Status changed from new to assigned

comment:36 Changed 4 months ago by auvipy

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 Changed 4 months ago by timgraham

  • Resolution set to wontfix
  • Status changed from assigned to closed

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