SessionWizard

8 views
Skip to first unread message

David Cramer

unread,
Dec 3, 2008, 8:50:39 PM12/3/08
to Django developers
So I needed the ability to specify initial data for a wizard, and I
also liked the idea of storing it in the session (vs the POST). I was
pointed to the SessionWizard patch, and I believe this was a 1.1
possibility as well, so I figured I'd bring up the numerous issues.

First and foremost, the latest patch doesn't even work. There's
several naming conflicts, and mismatches in the docs. There's a few
things I like about this, that I think would work well in the base
wizard class (which needs to be created):

- A few more hooks such as get_page_title
- The possibility to interact with the GET request to get form data (I
want to sometimes skip a step if they come from a location which
provides that information).

And a few of the obvious issues:

- The API is *completely* different.
- Much of the code is specifically written for sessions (which is good
and bad).

I've got it working locally myself, but I've already begun making
tweaks (to make it work in our environment, and fix the bugs). I'm
going to refactor a BaseWizard class, and try to keep all the original
FormWizard methods, such as render_template, done, etc..

Most of the original base methods are still possible without tweaking
the design, but one of the very obvious differences is the done
method. This may be up for debate, but I believe the done method
should be what it says, done. This means that the session data should
automatically wipe once the done method has been called. There is also
the issue that the forms are not passed via the done method, and this
needs changed so that it is the same API.

The API (even private) also needs cleaned IMO to be a bit closer to
PEP 8.

Anyways, I'm going to work up some changes, and I'll submit another
patch. If anyone has any recommendations on API for this please let me
know as I can commit the time nescesary (since we need this) to finish
the code.

Also, is this feature still slated for 1.1?

David Cramer

unread,
Dec 3, 2008, 9:34:09 PM12/3/08
to Django developers
So far I've refactored a bunch of the methods to store less in the
session, and generate more on demand (otherwise you could change the
method and then session data represents inaccurate information). I've
completedly redesigned the pages storage so it's just storing valid
and verified in the session. I'm a little curious as to what valid is
(vs verified), but I'll leave it for now. I've fixed an issue with URL
base, it seemed to only allow URLs which didn't end with a / (mine
do).

(r'^verify-number/(:?(?P<page_key>\w*)/)?$', 'verification.index'),

This is my sample URL.

I've changed the done() method to pass form_classes just as it would
originally. I've also changed _validate_all_forms to support this
change in the new _handle_post_request (renamed from POST) method. GET
is renamed to _handle_get_request. get_URL_base is renamed to
get_url_base.

When the done() method is called, the session is also cleared unless
done(). Maybe this should only happen if say a ValidationError isn't
raised? Maybe a setting that makes this not happen? In all of my
situations it makes sense to erase it when it's complete, and not have
to call methods manual. Let's decide which is more common.

Hanne Moa

unread,
Dec 4, 2008, 6:23:34 AM12/4/08
to django-d...@googlegroups.com
On Thu, Dec 4, 2008 at 03:34, David Cramer <dcr...@gmail.com> wrote:
> When the done() method is called, the session is also cleared unless
> done(). Maybe this should only happen if say a ValidationError isn't
> raised? Maybe a setting that makes this not happen? In all of my
> situations it makes sense to erase it when it's complete, and not have
> to call methods manual. Let's decide which is more common.

How about making it easier to have a preview also? As per today,
FormWizard and FormPreview aren't really combinable. Should probably
be it's own ticket...


HM

David Cramer

unread,
Dec 6, 2008, 4:15:31 PM12/6/08
to Django developers
Would have to look at how FormPreview's work, but I agree.

So far what I've done with the wizard is remove all of the data from
the session as much as possible. It calculates cleaned_data at the
very end and only stores the POST values (still need to solve a
potential exploit). One thing I didn't like was storing the base_forms
in the session (in order to dynamically change the forms based on what
has happened. The way this is done it would severely bloat the Form
version, or make it not possible. Anyone have suggestions on a way to
handle this? We could calculate the list of forms based on the data
stored each time, but not really sure.

On Dec 4, 5:23 am, "Hanne Moa" <hanne....@gmail.com> wrote:

David Durham, Jr.

unread,
Dec 9, 2008, 12:00:49 AM12/9/08
to django-d...@googlegroups.com
On Wed, Dec 3, 2008 at 7:50 PM, David Cramer <dcr...@gmail.com> wrote:
>
> So I needed the ability to specify initial data for a wizard, and I
> also liked the idea of storing it in the session (vs the POST). I was
> pointed to the SessionWizard patch, and I believe this was a 1.1
> possibility as well, so I figured I'd bring up the numerous issues.

Thanks for taking the time to look at it.


> First and foremost, the latest patch doesn't even work. There's
> several naming conflicts, and mismatches in the docs. There's a few
> things I like about this, that I think would work well in the base
> wizard class (which needs to be created):

If you want you can message me offlist about this. I'd be interested
to work with you to resolve any issues. I wrote all the code and am
quite familiar with it, though I'm not a django or python guru. I
know that my most recent patch passes all the tests I wrote, but I
have not spent too much time with the code recently because the
project I was using this for changed direction.


> - The possibility to interact with the GET request to get form data (I
> want to sometimes skip a step if they come from a location which
> provides that information).

There's a hook that can be used to interact with GETs, it's called
_show_form. This is also called on validation failure. My thinking
was that POSTs will be where people need to make workflow decisions,
so most of the hooks are in the POST processing.


> And a few of the obvious issues:
>
> - The API is *completely* different.
> - Much of the code is specifically written for sessions (which is good
> and bad).

Yes it is. This is why I didn't see a clear route to a base class.
I actually started with a base wizard class, but removed it when there
was no common functionality.


> I've got it working locally myself, but I've already begun making
> tweaks (to make it work in our environment, and fix the bugs). I'm
> going to refactor a BaseWizard class, and try to keep all the original
> FormWizard methods, such as render_template, done, etc..

I think if you decide to use render_template for both hidden field
form wizards and session-based form wizards you're going to be doing
conditional "is this is a session wizard or a hidden fields wizard"
type of checking. But I haven't seen your work so can't say for sure.


> Most of the original base methods are still possible without tweaking
> the design, but one of the very obvious differences is the done
> method. This may be up for debate, but I believe the done method
> should be what it says, done. This means that the session data should
> automatically wipe once the done method has been called. There is also
> the issue that the forms are not passed via the done method, and this
> needs changed so that it is the same API.

Sounds good to me. Thanks.


-Dave

David Durham, Jr.

unread,
Dec 9, 2008, 12:17:09 AM12/9/08
to django-d...@googlegroups.com
On Wed, Dec 3, 2008 at 8:34 PM, David Cramer <dcr...@gmail.com> wrote:
>
> So far I've refactored a bunch of the methods to store less in the
> session, and generate more on demand (otherwise you could change the
> method and then session data represents inaccurate information). I've
> completedly redesigned the pages storage so it's just storing valid
> and verified in the session. I'm a little curious as to what valid is
> (vs verified), but I'll leave it for now.

I think you mean validated and visited. The difference is that if
someone submits data for a page/form, then it's been visited. If
they've submitted valid data, then it's both valid and visited. My
examples in the docs didn't really draw attention to this point, but
here's the usage:

<ul>
{% for page_key,page in pages.items %}
<li class="{% if page.valid %}valid{% endif %}
{% if page.current_page %}current{% endif %}">
{% if page.visited %}
<a href="{{ URL_base }}{{ page_key }}">{{ page.title }}</a>
{% else %}
{{ page.title }}
{% end if %}
</li>
{% endfor %}
</ul>

In my example, visited pages get a link, otherwise no link. Valid
pages and the current page have a particular stylesheet rule applied.
Just an example of the kind of thing you might want to do with page
data in a wizard.


> I've fixed an issue with URL
> base, it seemed to only allow URLs which didn't end with a / (mine
> do).
>
> (r'^verify-number/(:?(?P<page_key>\w*)/)?$', 'verification.index'),

Yeah, allowing both trailing / versions and non-trailing / is better.
It wasn't a show-stopper for me, but that's a nice change. Think
there's a get_URL_base hook that could be overridden for odd URLs. Of
course, less work repetitive code is better.


> I've changed the done() method to pass form_classes just as it would
> originally. I've also changed _validate_all_forms to support this
> change in the new _handle_post_request (renamed from POST) method. GET
> is renamed to _handle_get_request. get_URL_base is renamed to
> get_url_base.

I didn't really see much of a point to pass the forms to done, since
there is already a method to retrieve forms. I was using this:

form_data = self._get_all_cleaned_data(request.session)

But I suppose everyone will be doing that early on in done(), so it
makes sense to make it a parameter.


> When the done() method is called, the session is also cleared unless
> done(). Maybe this should only happen if say a ValidationError isn't
> raised? Maybe a setting that makes this not happen? In all of my
> situations it makes sense to erase it when it's complete, and not have
> to call methods manual. Let's decide which is more common.

I think clear it automatically. I thought about this, but just didn't
implement it.


-Dave

David Durham, Jr.

unread,
Dec 9, 2008, 12:25:38 AM12/9/08
to django-d...@googlegroups.com
On Sat, Dec 6, 2008 at 3:15 PM, David Cramer <dcr...@gmail.com> wrote:
>
> Would have to look at how FormPreview's work, but I agree.
>
> So far what I've done with the wizard is remove all of the data from
> the session as much as possible. It calculates cleaned_data at the
> very end and only stores the POST values (still need to solve a
> potential exploit).

If you look at my patch, you'll see this line:

page_data = self._get_cleaned_data(request, page_key)
....
if issubclass(form_class, forms.ModelForm):
form = form_class(instance=form_class.Meta.model(**page_data))

I tried using the POST data with initial, but it failed for ModelForm
.. the foreign key fields weren't handled properly. Maybe you've
solved that in a different way.


-Dave

Reply all
Reply to author
Forward
0 new messages