jazzband / django-formtools

A set of high-level abstractions for Django forms
https://django-formtools.readthedocs.io
BSD 3-Clause "New" or "Revised" License
784 stars 135 forks source link

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

Open MrkGrgsn opened 1 year ago

MrkGrgsn commented 1 year ago

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.

schinckel commented 1 year ago

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 commented 1 year ago

@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 commented 10 months ago

Please resolve git conflicts.

MrkGrgsn commented 10 months ago

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 commented 1 week ago

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 commented 1 week ago

@claudep As noted, there are existing use cases that were broken in https://github.com/jazzband/django-formtools/pull/209 and are still broken, despite https://github.com/jazzband/django-formtools/issues/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?