Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#11726 closed Bug (fixed)

FormWizard sanity check on step number performed before dynamic steps can be inserted

Reported by: Eric Friesen <ericfriesen@…> Owned by: esper256
Component: contrib.formtools Version: master
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:

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)

django-11726.diff (1.0 KB) - added by esper256 4 years ago.
Fix as described in the original comment
ticket11726.diff (2.8 KB) - added by michalm 3 years ago.
Alternative fix, patched against trunk, includes failing testcase

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by anonymous

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

comment:3 Changed 4 years ago by esper256

  • Owner changed from anonymous to esper256
  • Status changed from assigned to new

comment:4 Changed 4 years ago by esper256

  • Status changed from new to assigned

Changed 4 years ago by esper256

Fix as described in the original comment

comment:5 Changed 4 years ago by esper256

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Version changed from 1.1 to SVN

Changed 3 years ago by michalm

Alternative fix, patched against trunk, includes failing testcase

comment:6 Changed 3 years ago by michalm

  • Cc michal.modzelewski@… added
  • Easy pickings unset
  • milestone set to 1.4
  • Needs documentation unset
  • Needs tests unset
  • Severity set to Normal
  • Type set to 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.

comment:7 follow-up: Changed 3 years ago by anonymous

Michael, your solution looks good. I am new to Django contribution, do I need to do anything to this ticket?

comment:8 in reply to: ↑ 7 Changed 3 years ago by esper256

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 Changed 3 years ago by michalm

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:10 Changed 3 years ago by esper256

  • Triage Stage changed from Accepted to Ready for checkin

Thanks, looks good!

comment:11 Changed 3 years ago by lukeplant

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

In [16119]:

Fixed #11726 - FormWizard does sanity check on step number performed before dynamic steps can be inserted

Thanks to Eric Friesen for the report and michalm/esper256 for the patch.

comment:12 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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.