#11726 closed Bug (fixed)
FormWizard sanity check on step number performed before dynamic steps can be inserted
Reported by: | Owned by: | esper256 | |
---|---|---|---|
Component: | contrib.formtools | Version: | dev |
Severity: | Normal | Keywords: | FormWizard |
Cc: | michal.modzelewski@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If you are making a FormWizard to have dynamically created steps, where the number of steps depends on the form input of the previous steps, you run into a problem with the sanity check inside FormWizard's __call__
method.
# Sanity check. if current_step >= self.num_steps(): raise Http404('Step %s does not exist' % current_step) # For each previous step, verify the hash and process. # TODO: Move "hash_%d" to a method to make it configurable. for i in range(current_step): form = self.get_form(i, request.POST) if request.POST.get("hash_%d" % i, '') != self.security_hash(request, form): return self.render_hash_failure(request, i) self.process_step(request, form, i)
Your cleaned_data to determine if you need to add more steps isn't available until process_step is called on the earlier steps. However it will never get that far because the current step will be over the number of steps the FormWizard was initially constructed with (in the urls.py for example).
Currently I have worked around this in my code by constructing FormWizard with enough dummy steps that are removed and replaced with real ones just so the formcount is always greater than the dynamic count will ever be in the very beginning.
My proposed solution would be to do a sanity check each iteration of the forloop ensuring that there's at least enough forms to process the next one.
Attachments (2)
Change History (14)
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:4 by , 14 years ago
Status: | new → assigned |
---|
by , 14 years ago
Attachment: | django-11726.diff added |
---|
comment:5 by , 14 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
Version: | 1.1 → SVN |
by , 14 years ago
Attachment: | ticket11726.diff added |
---|
Alternative fix, patched against trunk, includes failing testcase
comment:6 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
milestone: | → 1.4 |
Needs documentation: | unset |
Needs tests: | unset |
Severity: | → Normal |
Type: | → Bug |
This ticket is ageing, but is still valid after the fixes to FormWizard in [15196].
I propose fixing this by moving the sanity check into the get_form method. I've added a patch with this fix and a regression test.
follow-up: 8 comment:7 by , 14 years ago
Michael, your solution looks good. I am new to Django contribution, do I need to do anything to this ticket?
comment:8 by , 14 years ago
Replying to anonymous:
Michael, your solution looks good. I am new to Django contribution, do I need to do anything to this ticket?
Oops, this was me, I just never remember to log in.
comment:9 by , 14 years ago
If you've reviewed the patch you can set Triage Stage to "Ready for checkin". After that we just wait until one of the core developers reviews the patch.
comment:11 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
In [16119]:
(The changeset message doesn't reference this ticket)
Fix as described in the original comment