Code

Opened 6 years ago

Closed 3 years ago

#9200 closed New feature (fixed)

Add a session based form wizard

Reported by: ddurham Owned by: ddurham
Component: contrib.formtools Version: master
Severity: Normal Keywords: session wizard forms
Cc: edcrypt@…, andrehcampos@…, donspauldingii@…, nate@…, shaun@…, domen@…, ckd, flosch@…, dougal85@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

The provided patch adds a class that can be extended to create form wizards that:

1) Use GETs for form displays (except in the case of validation errors) and POSTs for form submissions.
2) Successful POSTs are redirected to GET the next page in the wizard. This removes the POST from browser history so that back and refresh pretty much just work.

I have provided plenty of methods that can be overridden for customization purposes. Right now, I think I'm probably storing too much info in the session scope, in particular request.POST data, cleaned_data, and other things. I have not tested with multipart form data. I do not have documentation or tests, just wanted to see if you're interested in having this in contrib.

Attachments (8)

wizard_tests.patch (6.8 KB) - added by ddurham 6 years ago.
wizard_patch.txt (16.7 KB) - added by ddurham 6 years ago.
formtools_patch.txt (41.5 KB) - added by ddurham 6 years ago.
includes SessionWizard class, tests and documentation
session_wizard_patch.txt (42.1 KB) - added by ddurham 5 years ago.
did some more testing, fixed a couple bugs, fixed a small problem with the docs
session_wizard_patch.diff (42.1 KB) - added by ElliottM 5 years ago.
Same patch but in diff format
ticket9200-1.diff (138.4 KB) - added by jezdez 3 years ago.
Updated patch (including changes for secure cookie from #12417)
ticket9200-2.diff (140.3 KB) - added by jezdez 3 years ago.
9200-3.diff (156.8 KB) - added by jezdez 3 years ago.
Updates as requested.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 6 years ago by edcrypt

  • Cc edcrypt@… added
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 6 years ago by hanne.moa@…

I'm certainly interested; I've hacked the normal form wizard a bit to allow previews by putting the final POST into the session and redirecting to a new page with a submit-button and a dump of the data. (Oh and wizards which can take ModelForms with instances of course... can't have a wizard to change anything without that.)

What would be really nice is to allow each page/slide of the wizard to have its own url, then supporting forward/back would be trivially easy. Not needed for simple survey-things of course but handy when the wizard is "windows"-style, used to configure something.

I've yet to see a need to dynamically change a wizard too.

comment:3 Changed 6 years ago by ddurham

Each form page does have its own URL. Currently the URL has to be something like r'wizard/(?P<page0>\d+)/$' but it allows me to link directly to /wizard/0/, /wizard/1/ and so forth. In fact, even if I had a "back" button on the form that did nothing but go back, it should use a GET rather than POSTing the form.

comment:4 Changed 6 years ago by hanne.moa@…

Brilliant! So, now we wait for the design decision? Could just release it as an app/library too though.

Changed 6 years ago by ddurham

Changed 6 years ago by ddurham

comment:5 Changed 6 years ago by edcrypt

  • Needs tests unset
  • Patch needs improvement set

Instead of passing a list of forms (the form_list argument on SessionWizard.init()), would be nice if we could pass a sequence in the format:

(('step_slug', Form), ...)

Instead of r'wizard/(?P<page0>\d+)/$', that'd be r'wizard/(?P<step_slug>\w+)/$'.
The form_list can be tuned on a dict ( {'step_slug': Form, ...} ) doing form_dict = dict(form_list) , as you need to get the form given the url argument.

This change would allow us to have wizards with named pages, not just numbered.

comment:6 Changed 6 years ago by ddurham

That's a nice feature, so I'm working on adding it along side the current list init.

comment:7 Changed 6 years ago by andrehcampos@…

  • Cc andrehcampos@… added

comment:8 Changed 6 years ago by edcrypt

  • Needs documentation unset
  • Patch needs improvement unset

comment:9 Changed 6 years ago by edcrypt

  • Keywords forms added

Changed 6 years ago by ddurham

includes SessionWizard class, tests and documentation

comment:10 Changed 6 years ago by donspaulding

  • Cc donspauldingii@… added

I haven't fully looked over the approach you've taken here, but I thought I might share what I've done here at work along these lines. I've put my SessionFormWizard up on DjangoSnippets.org, in case there are any ideas to be taken from it. I'll give this a more thorough look soon though and leave comments when appropriate.

comment:11 follow-up: Changed 5 years ago by jacob

There's a fair bit of duplicate code here. A better approach would be an abstract wizard base class and concrete hidden-form and session-based subclasses.

comment:12 in reply to: ↑ 11 Changed 5 years ago by ddurham

Replying to jacob:

There's a fair bit of duplicate code here. A better approach would be an abstract wizard base class and concrete hidden-form and session-based subclasses.

Can you give me some more pointers to any methods you have in mind. I'm happy to make the changes, just don't immediately specific enough commonality.

Changed 5 years ago by ddurham

did some more testing, fixed a couple bugs, fixed a small problem with the docs

comment:13 Changed 5 years ago by anonymous

It fails with PicklingError if there are any formsets in the forms

Changed 5 years ago by ElliottM

Same patch but in diff format

comment:14 Changed 5 years ago by Nate

  • Cc nate@… added

I rolled one of these for myself a while back, and noticed that it was on the 1.1 wishlist. I see there's already a patch here, but you guys may find this implementation helpful. You can find a copy here. I'd upload it here, but I'm not sure it will be useful and I don't want to add clutter.

comment:15 Changed 5 years ago by marcob

In doc sessionwizard.txt:

form_data = self._get_all_cleaned_data(request.session)
self._clear_wizard_data_from_session(request.session)

Should be:

form_data = self._get_all_cleaned_data(request)
self._clear_wizard_data_from_session(request)

Twice :-)

comment:16 Changed 4 years ago by jezdez

  • milestone set to 1.2
  • Triage Stage changed from Design decision needed to Accepted

comment:17 Changed 4 years ago by iElectric

Note about my usage of Nate's Session based Wizard:

I needed option to not fill all forms until the last one, so I had to change the following parts:

  • in finish() form validation, force is_bound=True to override django default behaviour making empty forms not valid
  • add request parameter to is_last() function in order to inspect unexpected wizard finish

Also note the following, which is very imporatant:

  • django upload handlers use cStringIO object which is NOT pickle-able. Use StringIO.StringIO instead.

comment:18 Changed 4 years ago by iElectric

  • Cc domen@… added

comment:19 Changed 4 years ago by steph

I started working on a rewrite of the formwizard with pluggable storages. I think it would be easier to work on the wizard in an third party module because there are many approaches to meet the requirements. Feed free to fork or ask for commit rights. Maybe we could get some stuff done to contribute the module to django in 1.3 - I don't think we'll get the module stable until the 1.2 freeze.

http://github.com/stephrdev/django-formwizard

comment:20 follow-up: Changed 4 years ago by danaspiegel

Seeing as 1.2 is almost ready to be released, is there any chance that this patch will see the light of day in 1.2? I know its on the low-priority list, but I would love to know whether I should wait for it, or just forge ahead with my own solution.

Also, is there any indication about solving this issue for ModelForms, and not just regular Forms?

comment:21 in reply to: ↑ 20 Changed 4 years ago by ddurham

Replying to danaspiegel:

Seeing as 1.2 is almost ready to be released, is there any chance that this patch will see the light of day in 1.2? I know its on the low-priority list, but I would love to know whether I should wait for it, or just forge ahead with my own solution.

You can just take the code and use it outside of the django namespace, since this feature won't make it into the 1.2 release. Or forge ahead with your own of course.

Also, is there any indication about solving this issue for ModelForms, and not just regular Forms?

The patch that I submitted works with ModelForms. At least the model forms I tested it with. Someone else had a pickling error with field sets, but I was not able to reproduce that problem.

comment:22 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:23 follow-up: Changed 4 years ago by danaspiegel

In implementing this code (ddurham, thanks for providing it!), I ran into an issue where the usage of ModelForms doesn't work properly when stepping back to previous pages. Specifically, when using either ForeignKey or ManyToMany relationships, you cannot use the cleaned data to re-create new versions of the model instance since the representation of the form data for these two field types is different from the way you set init data. I've fixed the code to work for my usage, and will try to post the changes (though I don't know if they cover all cases or are implemented appropriately.

I believe this issue must be fixed before this patch is accepted.

comment:24 in reply to: ↑ 23 Changed 4 years ago by ddurham

  • Owner changed from nobody to ddurham
  • Status changed from new to assigned

Replying to danaspiegel:

In implementing this code (ddurham, thanks for providing it!), I ran into an issue where the usage of ModelForms doesn't work properly when stepping back to previous pages. Specifically, when using either ForeignKey or ManyToMany relationships, you cannot use the cleaned data to re-create new versions of the model instance since the representation of the form data for these two field types is different from the way you set init data. I've fixed the code to work for my usage, and will try to post the changes (though I don't know if they cover all cases or are implemented appropriately.

I believe this issue must be fixed before this patch is accepted.

I think I ran into this issue as well. You can see in the GET method where I check if the form is a model form and do things a little differently.

333 if issubclass(form_class, forms.ModelForm):

334 form = form_class(instance=form_class.Meta.model(page_data))
335 else:
336 form = form_class(initial=page_data)

Where page_data is the cleaned_data. If you had to do something different, could you post your code?

comment:25 Changed 4 years ago by danaspiegel

I haven't had time to put together a patch (nor am I sure this is the best way to do this) but here's the code:

    def GET(self, request, page_key):
        """
        Initialize a form if necessary, and display the form/page identified by 
        page_key.
        """
        page_data = self._get_cleaned_data(request, page_key)
        if page_data is None:
            form = self._get_form_classes(request)[page_key]()
        else:
            form_class = self._get_form_classes(request)[page_key]
            if issubclass(form_class, forms.ModelForm):
                # We need to manually create a new instance of the form's model, since we need to specially handle ManyToMany fields
                instance = form_class.Meta.model()
                for attribute_name, attribute_value in page_data.items():
                    if hasattr(instance, attribute_name):
                        setattr(instance, attribute_name, attribute_value)
                        if isinstance(attribute_value, base.Model):
                            # when the related object is a model object, we need to explicitly set the form field's value
                            page_data[attribute_name] = attribute_value.pk
                    elif isinstance(getattr(form_class.Meta.model, attribute_name), fields.related.ReverseSingleRelatedObjectDescriptor):
                        page_data[attribute_name] = attribute_value.pk
                    elif isinstance(getattr(form_class.Meta.model, attribute_name), fields.related.ReverseManyRelatedObjectsDescriptor):
                        page_data[attribute_name] = [value.pk for value in attribute_value]
                form = form_class(instance=instance, initial=page_data)
            else:
                form = form_class(initial=page_data)
        
        return self._show_form(request, page_key, form)

comment:26 Changed 4 years ago by ckd

  • Cc ckd added
  • Resolution set to fixed
  • Status changed from assigned to closed

comment:27 Changed 4 years ago by ckd

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:28 Changed 4 years ago by ckd

  • Status changed from reopened to new

comment:29 Changed 4 years ago by danaspiegel

This code unfortunately also doesn't support ManyToMany relationships where you need to present an inline FormSet. I think that actually, the functionality of this code isn't much beyond the existing FormWizard shipping with 1.2, and though its cleaner, if there's going to be an upgrade to the FormWizard functionality, I think that there needs to be some rethinking about how it should work.

At the very least, it should provide over-rideable methods for getting the forms for a step, so that you can provide multiple forms (to support inline formsets) and their related instances, and actually support multiple forms per step.

If someone takes this on, I'd be happy to provide some complex use-cases and code to help figure out the right architecture.

I vote for "Patch Needs Improvement" (though I won't change the setting on this ticket since I'm not a core developer for Django)

comment:30 Changed 4 years ago by steph

my implementation works with ManyToMany and FormSet too. you may give it a try. I'm working on a proposal to get it into django 1.3 too.. django-formwizard docs are missing (basicly same as current formwizard), but feel free to contact me or open issues on github.

comment:31 Changed 4 years ago by shauncutts

  • Cc shaun@… added

comment:32 Changed 4 years ago by anonymous

  • Cc flosch@… added

comment:33 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Uncategorized

session_wizard_patch.diff fails to apply cleanly on to trunk

comment:34 Changed 3 years ago by jezdez

  • Type changed from Uncategorized to New feature

comment:35 Changed 3 years ago by d0ugal

  • Cc dougal85@… added

Changed 3 years ago by jezdez

Updated patch (including changes for secure cookie from #12417)

comment:37 Changed 3 years ago by jezdez

FYI, https://github.com/jezdez/django/compare/feature/formwizard has the latest code (similar to the newly attached patch).

comment:38 Changed 3 years ago by jezdez

  • Patch needs improvement unset

Changed 3 years ago by jezdez

Changed 3 years ago by jezdez

Updates as requested.

comment:39 Changed 3 years ago by jezdez

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

In [16307]:

Fixed #9200 -- Added new form wizard to formtools based on class based views. Many thanks to Stephan Jäkel, ddurham and ElliottM for their work.

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.