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

Regression from #168: unable to call get_cleaned_data_for_step on conditionally step #266

Open mlorant opened 5 months ago

mlorant commented 5 months ago

I have updated my project requirements from django-formtools 2.3 to 2.5.1. However some of my tests using WizardView are now broken and I can sort of bisect the problem between the 2.3.0 and 2.4.0 version.

Suppose a form where you're asked to upload an Excel file and process some data in it, you may need to select a sheet if multiple are present in the file, but that step can be ignored if the file has a single sheet. The WizardView would look like this:

class MyWizardView(WizardView):
    form_list = [
        ('excel_file_selection', ...),
        ('excel_sheet_selection', ...),
        ('do_other_stuff', ...),
        ...
    ]

    condition_dict = {
        'excel_sheet_selection': is_sheet_selection_needed,
        ...
    }

    def get_sheet_index(self):
        if cleaned_data := self.get_cleaned_data_for_step('excel_sheet_selection'):
            return int(cleaned_data['sheet_number'])

        return 0

In formtools 2.3 version, get_cleaned_data_for_step is returning None when the excel_sheet_selection step is passed (e.g because is_sheet_selection_needed returned False because there's a single sheet in the file in our case)

In formtools 2.4 and after, I get the following exception:

  File "<mycode>/views.py", line 177, in get_sheet_index
    cleaned_data = self.get_cleaned_data_for_step('excel_sheet_selection')
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<venv>/formtools/wizard/views.py", line 494, in get_cleaned_data_for_step
    form_obj = self.get_form(
               ^^^^^^^^^^^^^^
  File "<venv>/formtools/wizard/views.py", line 409, in get_form
    form_class = self.get_form_list()[step]
                 ~~~~~~~~~~~~~~~~~~~~^^^^^^
KeyError: 'excel_sheet_selection'

I guess the change happened with the issue #168 that is using get_form_list most of the time except in the first line of get_cleaned_data_step:

    def get_cleaned_data_for_step(self, step):
        """
        Returns the cleaned data for a given `step`. Before returning the
        cleaned data, the stored values are revalidated through the form.
        If the data doesn't validate, None will be returned.
        """
        if step in self.form_list:  # <---- Based on form_list declaration while get_form will hit `self.get_form_list()[step]` later
            form_obj = self.get_form(

By looking at the doc, this behaviour was undefined (it doesn't say if it should return None or raise an exception), but I think it should be clarified, either through a more explicit/nicer exception ("excel_sheet_selection step not found in forms submitted") or by returning None as before.

I have an easy way to fix that (by checking myself if the form is in get_form_list) but that feels wonky since the lib was doing it for me before.