Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14498 closed (fixed)

Forms passed to FormWizard.process_step are not guaranteed to have clean validate data

Reported by: Staś Małolepszy Owned by: Luke Plant
Component: contrib.formtools Version: 1.2
Severity: Keywords: regression
Cc: niels.busch@…, stas@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The docs for FormWizard.process_step at source:trunk/django/contrib/formtools/wizard.py (line 263) read:

Hook for modifying the FormWizard's internal state, given a fully validated Form object. The Form is guaranteed to have clean, valid data.

There is however a case in FormWizard._ _call_ _ when this will not be true.

In source:trunk/django/contrib/formtools/wizard.py (line 95), the forms for each previous step are instantiated but they are not validated, which makes them not have the cleaned_data property.

for i in range(current_step):
    form = self.get_form(i, request.POST)
    if not self._check_security_hash(request.POST.get("hash_%d" % i, ''), request, form):
        return self.render_hash_failure(request, i)
    self.process_step(request, form, i)

A solution would be to call form.is_valid in the above loop, or to edit the docs to account for this particular case.

Attachments (1)

14498_regression_fix.patch (5.7 KB) - added by Staś Małolepszy 6 years ago.
A patch fixing the regression discovered in comment:5

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Luke Plant

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Luke Plant
Patch needs improvement: unset
Status: newassigned

comment:2 Changed 6 years ago by Luke Plant

Triage Stage: UnreviewedAccepted

comment:3 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

(In [14290]) Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

comment:4 Changed 6 years ago by Luke Plant

(In [14291]) [1.2.X] Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

Backport of [14290] from trunk.

comment:5 Changed 6 years ago by Niels Sandholt Busch

Cc: niels.busch@… added
Resolution: fixed
Status: closedreopened

I believe this changeset broke the Formwizard in other ways. It is not possible to alter the form_list in process_step anymore, because the current_step form is instantiated before previous_steps processing has been done. If I change the initial step2 form to a different form when processing step1 the current_step form is clearly not the right one anymore.

Notice I am creating the formwizard in a view, so I don't have previous state cached.

comment:6 in reply to:  5 Changed 6 years ago by Staś Małolepszy

Cc: stas@… added

I can confirm the same issue that niels described. I have a 3-step wizard where the step1 form is set up depending on the user's choices in step0. When I go from step0 to step1 the wizard works as expected, because form = self.get_form(next_step) is executed after the processing loop from source:django/trunk/django/contrib/formtools/wizard.py#14574#L121.

However, when I go from step1 to step2, step1 gets instantiated with form = self.get_form(current_step, request.POST) in source:django/trunk/django/contrib/formtools/wizard.py#14574#L96, before the processing loop has a chance to modify the form list. As a result, step1 form is still the one I defined in my view when instantiating my wizard.

This is broken both in 1.2.X and on trunk.

Changed 6 years ago by Staś Małolepszy

Attachment: 14498_regression_fix.patch added

A patch fixing the regression discovered in comment:5

comment:7 Changed 6 years ago by Łukasz Rekucki

Has patch: set
Keywords: regression added
milestone: 1.3
Version: SVN1.2

Updated flags to current state.

comment:8 Changed 6 years ago by Luke Plant

milestone: 1.3
Resolution: fixed
Status: reopenedclosed

As a technicality, this is now a different bug. Since the original bug remains fixed, it is more helpful to open a new ticket - otherwise the metadata from the original bug is lost, and the change logs referring to bugs becoming confusing. I've opened the new ticket now - #15075

comment:9 Changed 6 years ago by Staś Małolepszy

Thanks, lukeplant, for opening #15075. I wasn't sure if I should do it or not myself. I'll know better for the next time!

Note: See TracTickets for help on using tickets.
Back to Top