Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: restore original form list generation and support dynamic form lists Fixes #220 #240

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MrkGrgsn
Copy link

This is an attempt to fix #220 while restoring pre-#209 features and supporting dynamic form lists using custom get_form_list() implementations.

The solution has two parts:

  1. New method get_form_class() uses self.form_list as a key to determine whether the class is implemented using the original form list generation strategy (ie, form_list and condition_dict) or the more dynamic approach (ie, overriding get_form_list()). If form_list is populated, the strategy is inferred to be the original one.
  2. get_cleaned_data_for_step() now supports an optional form class argument so that get_form() doesn't need to call get_form_list() and trigger infinite recursion. This enables custom get_form_list() implementations to access submitted data from other steps.

Change 1 will be a breaking change for implementations that have a custom get_form_list() - the developers would need to stop passing any forms via as_view() or setting them in the class, and may need to modify get_form_list() to adapt for this. Another option would be an explicit argument to as_view() that selects the form list generation strategy but form_list seems an adequate and natural key for this purpose. Change 1 ensure that the original form list generation strategy is not prone to infinite recursion.

re: change 2, there are probably some different and possibly better ways of handling this, but I think this way is okay and backwards compatible. In the fully dynamic / custom get_form_list() approach, the form class for a given step is known only to get_form_list() so it must explicitly tell the other methods what form class to instantiate for each step.

I explored another option where the infinite recursion was broken by having get_form_list() return a partial cached form list. This allowed callables in condition_dict to access data from earlier steps. This would replace change 1 but something like change 2 would still be required for custom get_form_list() methods. I liked this option because it introduced caching but it was more complicated and it broke a test that implied that changing condition_dict during request handling was supported. Not sure what the use case for that would be.

Anyway, hit me with your feedback. Additional manual testing is needed. I have run the test suite but not tried it in any projects, so please try it out. It also needs docs updates.

Mark Gregson added 3 commits May 10, 2023 15:01
If self.form_list is not empty then assume the wizard has been implemented according to the original form list generation strategy (static) and take the form class from there. If self.form_list is empty, however, then get the form class from the dynamically generated list provided by get_form_list().

This allows for the original behaviour as well as form lists that are dynamic and determined solely by get_form_list().
Provides a way to retrieve cleaned form data without calling
get_form_list() and ending up in infinite recursion.
@schinckel
Copy link
Contributor

That doesn't work for my use-case: form_list is supplied on the class, but dynamically updated based on previous data. Specifically, it injects duplicates of form Y (with different data) n times based on the selection of form X.

@MrkGrgsn
Copy link
Author

@schinckel Thanks for having a look. I envisaged your use case being met with a custom get_form_list() of this kind:

def get_form_list(self):
    form_list = OrderedDict(
        # Forms previously in self.form_list
        [("step1", FormX)],
    )
    # Forms injected as required per submitted data.
    form_data = self.get_cleaned_data_for_step("step1", form_cls=FormX) or {}
    if form_data.get("foo", False):
        for i in range(2):
            form_list[f"step{i}"] = FormY
    return form_list

I have to admit I'm not certain where it falls down (in preparing initial data and form args?) but maybe I can change tack if you can clarify where the sticking point is.

@cclauss
Copy link
Contributor

cclauss commented Sep 26, 2023

Please resolve git conflicts.

@MrkGrgsn
Copy link
Author

Hi @cclauss, thanks for looking at this merge request. I'm happy to resolve the conflicts if the change is actually going somewhere but I've had only one response and that was to the effect that it doesn't meet one of the known use cases so I'm not clear that it has a future.

@1001nights
Copy link

Is this PR going anywhere? There seemed to be a fair bit of interest in allowing the use of get_cleaned_data_for_step() in condition callables. The fix for the recursion error did nothing in this regard, so pinning to 2.3 is still necessary.

@MrkGrgsn
Copy link
Author

@claudep As noted, there are existing use cases that were broken in #209 and are still broken, despite #220, requiring some projects, including my own, to pin to 2.3. This PR may not be the best solution but it explores some of the challenges of a fix; could you review please and provide some feedback so we can move toward a solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite Recursion possible in 2.4
4 participants