Fix infinite recursion error #220

[jsma]

This removes the undocumented get_form_list since the process of repeatedly and dynamically generating an OrderedDict of steps/forms pairs leads to infinite recursion. This is replaced with a new method process_condition_dict which directly modifies form_list in place, in a single step executed prior to dispatching to get or post. This eliminates infinite recursion since the form_list is calculated exactly once prior to any other attempts to access and process forms.

Removed test_form_condition_unstable since this is an odd test (why would a condition_dict return value suddenly change in the middle of a request/response cycle?) that was attempting to deal with invalid steps (invalid steps are better handled through actual validation, see #224).

For users who need to dynamically add forms, this can be handled by overriding process_condition_dict.

[claudep]

@schinckel Do you have an opinion on this approach?

[codecov]

Codecov Report

Merging #228 (f45851b) into master (97441f1) will decrease coverage by 1.18%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   93.88%   92.69%   -1.19%     
==========================================
  Files          10       10              
  Lines         523      520       -3     
  Branches       66       66              
==========================================
- Hits          491      482       -9     
- Misses         19       21       +2     
- Partials       13       17       +4     

Impacted Files	Coverage Δ
formtools/wizard/views.py	92.20% <93.33%> (-2.10%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[schinckel]

I’ll oppose this change, since there is no way to add additional forms using it: that’s my primary reason for the initial change; I have a for that may need to be repeated N times based on a previous form’s cleaned data.

[claudep]

OK, thanks, let this PR aside for now. Thanks anyway @jsma for proposing it.

[jsma]

(Just saw this was closed while writing this reply)

FYI, I'm still testing this branch in a project with several different wizards with varying customizations. I'm currently getting odd test failures for one wizard where my test suite fails when run as a suite but passes when a specific test method is run. All of my other wizards are working just fine. I'll try to get to the bottom of this shortly to determine whether it's simply an issue with how my tests are written or if it's a problem of when/where this PR prunes form_list based on condition_dict.

The bug I'm trying to fix was due to a feature enhancement that broke documented uses cases (checking cleaned data in a condition). From my perspective, new features should not break existing, documented functionality. Unfortunately the 2.4 release did not do this. Software is hard and I'm not here to criticize anyone. The reality is that 2.4 is a release that introduces infinite recursion errors and needs to be fixed one way or another ASAP. Reverting the change is one quick option while we think this through a bit more, but this PR was an attempt to restore existing functionality while still making it possible to inject additional forms.

What about this PR prevents you from adding forms? I tried to leave this option open, but perhaps I failed. That is why I was soliciting feedback specifically from folks who are trying to add forms that are not initially present in form_list.

In my project, the subclass customizations either use condition_dict in a straightforward manner or swap out the specific form on a given step using get_form (e.g. if the user is logged in, show a simplified form since some details are known from their user record vs a user who is not logged in), so I admit my imagination is limited here. If you have a simplified code example of your use case, that would be helpful to think through since I can only guess how you're doing things at the moment.

As far as I can tell, condition_dict is specifically about pruning previous forms in form_list, not the current or a future step/form. I can see how the existing implementation might be a problem since once the current step is validated, it immediately checks if it's the last step and renders the done method, so perhaps we simply need to add a hook in between validation and evaluating whether we're on the last step?

django-formtools/formtools/wizard/views.py

Lines 294 to 306 in 3eb4a43

	if form.is_valid():
	# if the form is valid, store the cleaned data and files.
	self.storage.set_step_data(self.steps.current, self.process_step(form))
	self.storage.set_step_files(self.steps.current, self.process_step_files(form))
	# check if the current step is the last step
	if self.steps.current == self.steps.last:
	# no more steps, render done view
	return self.render_done(form, **kwargs)
	else:
	# proceed to the next step
	return self.render_next_step(form)
	return self.render(form)

[jsma]

FYI, I totally goofed in this initial draft, it modified the form_list class attribute so that lead to issues where one form instance would delete a form using condition_dict which meant the next view instance started life without the form at all without any chance to evaluate the condition_dict. I've started work on a modified approach, but again, really need input from folks who are requesting this new feature (to dynamically add forms) to understand real world examples.