Closed jsma closed 1 year ago
@schinckel Do you have an opinion on this approach?
Merging #228 (f45851b) into master (97441f1) will decrease coverage by
1.18%
. The diff coverage is93.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%) |
:arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
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.
OK, thanks, let this PR aside for now. Thanks anyway @jsma for proposing it.
(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?
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.
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 methodprocess_condition_dict
which directly modifiesform_list
in place, in a single step executed prior to dispatching toget
orpost
. This eliminates infinite recursion since theform_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
.