Opened 16 years ago
Closed 14 years ago
#9200 closed New feature (fixed)
Add a session based form wizard
Reported by: | David Durham | Owned by: | David Durham |
---|---|---|---|
Component: | contrib.formtools | Version: | dev |
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: | no |
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)
Change History (47)
comment:1 by , 16 years ago
Cc: | added |
---|---|
Needs documentation: | set |
Needs tests: | set |
Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 16 years ago
comment:3 by , 16 years ago
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 by , 16 years ago
Brilliant! So, now we wait for the design decision? Could just release it as an app/library too though.
by , 16 years ago
Attachment: | wizard_tests.patch added |
---|
by , 16 years ago
Attachment: | wizard_patch.txt added |
---|
comment:5 by , 16 years ago
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 by , 16 years ago
That's a nice feature, so I'm working on adding it along side the current list init.
comment:7 by , 16 years ago
Cc: | added |
---|
comment:8 by , 16 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:9 by , 16 years ago
Keywords: | forms added |
---|
by , 16 years ago
Attachment: | formtools_patch.txt added |
---|
includes SessionWizard class, tests and documentation
comment:10 by , 16 years ago
Cc: | 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.
follow-up: 12 comment:11 by , 16 years ago
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 by , 16 years ago
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.
by , 16 years ago
Attachment: | session_wizard_patch.txt added |
---|
did some more testing, fixed a couple bugs, fixed a small problem with the docs
comment:14 by , 16 years ago
Cc: | 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 by , 16 years ago
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 by , 15 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Design decision needed → Accepted |
comment:17 by , 15 years ago
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 by , 15 years ago
Cc: | added |
---|
comment:19 by , 15 years ago
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.
follow-up: 21 comment:20 by , 15 years ago
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 by , 15 years ago
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 by , 15 years ago
milestone: | 1.2 |
---|
1.2 is feature-frozen, moving this feature request off the milestone.
follow-up: 24 comment:23 by , 15 years ago
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 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 15 years ago
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 by , 15 years ago
Cc: | added |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
comment:27 by , 15 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:28 by , 15 years ago
Status: | reopened → new |
---|
comment:29 by , 15 years ago
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 by , 15 years ago
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 by , 15 years ago
Cc: | added |
---|
comment:32 by , 15 years ago
Cc: | added |
---|
comment:33 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
Severity: | → Normal |
Type: | → Uncategorized |
session_wizard_patch.diff fails to apply cleanly on to trunk
comment:34 by , 14 years ago
Type: | Uncategorized → New feature |
---|
comment:35 by , 14 years ago
Cc: | added |
---|
comment:36 by , 14 years ago
Note that work on this ticket is happening in a branch : https://github.com/stephrdev/django-formwizard/commits/feature%2Fcbv_refactor
There is a discussion on the mailing list : https://groups.google.com/group/django-developers/browse_thread/thread/185be65b47bd304b?pli=1
by , 14 years ago
Attachment: | ticket9200-1.diff added |
---|
Updated patch (including changes for secure cookie from #12417)
comment:37 by , 14 years ago
FYI, https://github.com/jezdez/django/compare/feature/formwizard has the latest code (similar to the newly attached patch).
comment:38 by , 14 years ago
Patch needs improvement: | unset |
---|
by , 14 years ago
Attachment: | ticket9200-2.diff added |
---|
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.