FormWizard done method can't revisit forms

[kenth]

The FormWizard done method is used for processing information collected by the form steps. However, it is not currently possible to revisit the forms to correct a problem which occurs in the done step. To extend the documentation example, it is possible to collect and validate a credit card (or other payment method) in a form step, but have the card be 'declined' (eg exceeds limit) when processing in done. A reasonable processing flow in this case would be to display the error, and revisit the payment page for a different payment method. However, the current implementation of FormWizard unconditionally clears its storage after calling done.

I propose that the done method be allowed to raise a (newly defined) RevalidationError to cause the Wizard to revisit a form step instead of clearing storage.

[Russell Keith-Magee]

The intended contract for done() is that at point of entry, all the forms are valid. If there is validation to be performed, it should be performed in the clean methods for the previous forms.

To follow through your example -- if one of the things you need to do for a valid form is to check the credit card, why isn't this being done as part of the cleaning for the form that took the credit card details? By your own admission - the right response if the card is declined is to re-show the credit card page, so why should the already existing clean method handle this?

Closing needsinfo; feel free to reopen if you can explain why this *can't* (not just "I don't want to") be handled as part of the form's clean method.

[kenth]

Patch with tests

[kenth]

Thanks for reviewing the ticket. I found the need to perform "revalidation" in the done step in an e-commerce project incorporating validation using stripe.com which separates validation (performed in the form's clean method) and charging (which I perform in done). However, I think the problem is more general than this specific case.

I had rejected performing a 'charge' in the clean of a confirmation step, instead preferring the done method for two reasons:

1. The wizard validates each form at least twice (when the form is initially validated and again in render_done). To ensure an action is performed only once, the clean method needs to leave a token to not repeat the action. This can be fragile.

2. The wizard does not revalidate earlier steps when first validating a form. Thus, if a confirmation step's clean charges when first validated, and an earlier step's form fails revalidation in render_done (say stock on an item is no longer available, or a discount window has expired), the charge has already occurred and the done step will not be processed. The transaction is a mess at that point.

An alternate solution would be to have earlier forms check if the charge has occurred and always return valid() in that case. I'm not sure how that would interact with tampering detection and the like.

Accordingly actions which must not be duplicated are best performed in the done method. The done method guarantees all forms validate & the wizard state is deleted upon completion. The current wizard allows for forms which do not revalidate, but does not allow for a done method which may not complete successfully. The proposed exception handles this case.

An implementation I currently use defines the Exception in my wizard subclass & then overrides render_done to catch the exception and step back into the FormWizard as needed. Django being open-source and Python, the workaround is quite functional. However I thought the problem and solution general enough for inclusion.

I have attached a patch w/ tests. I can work up the docs if it would help.

Thanks. Kent

[kenth]

implementation w/o patching Django

[Russell Keith-Magee]

#19285 was a duplicate.

As I noted on that ticket, part of the design contract for the Form Wizard is that once validated, the form will be able to reliably save without error.

However, your use case is a bit more complex than the username case from #19285. However, I'm still not 100% convinced that this isn't a documentation/"fix it another way" issue. While your use case is certianly valid, I'm not sure hacking the wizard workflow is the appropriate fix.

Marking this as design decision needed, because it's a pretty big change to the way wizards work that needs some serious discussion.

[Jacob]

Marking closed again; see Russ's comments. Kenth: if you'd like to revisit this further, please take it to the django-dev mailing list, thanks!