🐛 [#4824] Schedule task to create FormVariables after saving FormStep

this ensures that the Form cannot be left in a broken state with regards to FormVariables, because every component will always have a FormVariable

this is a bandaid fix, because ideally we would have a single endpoint and the FormStep wouldn't be updated if there are errors with variables

Backport-of: #4893

src/openforms/forms/api/serializers/form_step.py

@@ -1,3 +1,6 @@
+from functools import partial
+
+from django.db import transaction
 from django.db.models import Q
 
 from rest_framework import serializers
@@ -10,6 +13,7 @@
 )
 
 from ...models import FormDefinition, FormStep
+from ...tasks import on_formstep_save_event
 from ...validators import validate_no_duplicate_keys_across_steps
 from ..validators import FormStepIsApplicableIfFirstValidator
 from .button_text import ButtonTextSerializer
@@ -170,3 +174,19 @@ def validate_form_definition(self, current_form_definition):
         )
 
         return current_form_definition
+
+    @transaction.atomic()
+    def save(self, **kwargs):
+        """
+        Bandaid fix for #4824
+
+        Ensure that the FormVariables are in line with the state of the FormDefinitions
+        after saving
+        """
+        instance = super().save(**kwargs)
+
+        transaction.on_commit(
+            partial(on_formstep_save_event, instance.form.id, countdown=60)
+        )
+
+        return instance

src/openforms/forms/tasks.py

@@ -34,6 +34,28 @@ def on_variables_bulk_update_event(form_id: int) -> None:
     actions_chain.delay()
 
 
+def on_formstep_save_event(form_id: int, countdown: int) -> None:
+    """
+    Celery chain of tasks to execute after saving a FormStep.
+    """
+    create_form_variables_for_components_task = create_form_variables_for_components.si(
+        form_id
+    )
+    repopulate_reusable_definition_variables_task = (
+        repopulate_reusable_definition_variables_to_form_variables.si(form_id)
+    )
+    recouple_submission_variables_task = (
+        recouple_submission_variables_to_form_variables.si(form_id)
+    )
+
+    actions_chain = chain(
+        create_form_variables_for_components_task,
+        repopulate_reusable_definition_variables_task,
+        recouple_submission_variables_task,
+    )
+    actions_chain.apply_async(countdown=countdown)
+
+
 @app.task(ignore_result=True)
 def recouple_submission_variables_to_form_variables(form_id: int) -> None:
     """Recouple SubmissionValueVariable to FormVariable
@@ -96,6 +118,29 @@ def repopulate_reusable_definition_variables_to_form_variables(form_id: int) ->
         FormVariable.objects.create_for_form(form)
 
 
+@app.task(ignore_result=True)
+@transaction.atomic()
+def create_form_variables_for_components(form_id: int) -> None:
+    """Create FormVariables for each component in the Form
+
+    This task is scheduled after creating/updating a FormStep, to ensure that the saved
+    Form has the appropriate FormVariables, even if errors occur in the variables update
+    endpoint. This is done to avoid leaving the Form in a broken state.
+
+    See also: https://github.com/open-formulieren/open-forms/issues/4824#issuecomment-2514913073
+    """
+    from .models import Form, FormVariable  # due to circular import
+
+    form = Form.objects.get(id=form_id)
+
+    # delete the existing form variables, we will re-create them
+    FormVariable.objects.filter(
+        form=form_id, source=FormVariableSources.component
+    ).delete()
+
+    FormVariable.objects.create_for_form(form)
+
+
 @app.task()
 def activate_forms():
     """Activate all the forms that should be activated by the specific date and time."""

src/openforms/forms/tests/test_api_formsteps.py

@@ -856,6 +856,103 @@ def test_with_non_unique_fd_slugs(self):
         slugs = {form_step.slug for form_step in form_steps}
         self.assertEqual(len(slugs), 2)  # we expect two unique slugs
 
+    @patch(
+        "openforms.forms.api.serializers.form_step.on_formstep_save_event",
+        autospec=True,
+    )
+    @tag("gh-4824")
+    def test_create_form_step_schedules_task_chain_to_fix_form_variables(
+        self, mock_on_formstep_save_event
+    ):
+        """
+        Regression test for https://github.com/open-formulieren/open-forms/issues/4824
+        """
+        assign_change_form_permissions(self.user)
+        form = FormFactory.create()
+        url = reverse("api:form-steps-list", kwargs={"form_uuid_or_slug": form.uuid})
+        form_definition = FormDefinitionFactory.create(
+            configuration={
+                "display": "form",
+                "components": [
+                    {
+                        "key": "lastName",
+                        "type": "textfield",
+                        "label": "Last Name",
+                    },
+                ],
+            }
+        )
+        formdefinition_detail_url = reverse(
+            "api:formdefinition-detail",
+            kwargs={"uuid": form_definition.uuid},
+        )
+        data = {
+            "formDefinition": f"http://testserver{formdefinition_detail_url}",
+            "index": 0,
+        }
+        self.client.force_login(user=self.user)
+
+        with self.captureOnCommitCallbacks(execute=True):
+            response = self.client.post(url, data=data)
+
+        mock_on_formstep_save_event.assert_called_once_with(form.id, 60)
+
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+        self.assertEqual(
+            FormStep.objects.filter(form_definition=form_definition).count(),
+            1,
+        )
+
+    @patch(
+        "openforms.forms.api.serializers.form_step.on_formstep_save_event",
+        autospec=True,
+    )
+    @tag("gh-4824")
+    def test_update_form_step_schedules_task_chain_to_fix_form_variables(
+        self, mock_on_formstep_save_event
+    ):
+        """
+        Regression test for https://github.com/open-formulieren/open-forms/issues/4824
+        """
+        assign_change_form_permissions(self.user)
+        form_step = FormStepFactory.create()
+        form_definition = FormDefinitionFactory.create(
+            configuration={
+                "display": "form",
+                "components": [
+                    {
+                        "key": "lastName",
+                        "type": "textfield",
+                        "label": "Last Name",
+                    },
+                ],
+            }
+        )
+        url = reverse(
+            "api:form-steps-detail",
+            kwargs={"form_uuid_or_slug": form_step.form.uuid, "uuid": form_step.uuid},
+        )
+        formdefinition_detail_url = reverse(
+            "api:formdefinition-detail",
+            kwargs={"uuid": form_definition.uuid},
+        )
+        data = {
+            "formDefinition": f"http://testserver{formdefinition_detail_url}",
+            "index": 0,
+        }
+        self.client.force_login(user=self.user)
+
+        with self.captureOnCommitCallbacks(execute=True):
+            response = self.client.put(url, data=data)
+
+        mock_on_formstep_save_event.assert_called_once_with(form_step.form.id, 60)
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(
+            FormStep.objects.filter(form_definition=form_definition).count(),
+            1,
+        )
+
 
 class FormStepsAPITranslationTests(APITestCase):
     maxDiff = None

src/openforms/forms/tests/variables/test_tasks.py

@@ -3,7 +3,7 @@
 from unittest.mock import patch
 
 from django.db import close_old_connections
-from django.test import TestCase, TransactionTestCase, tag
+from django.test import TestCase, TransactionTestCase, override_settings, tag
 
 from openforms.forms.tasks import (
     recouple_submission_variables_to_form_variables,
@@ -12,6 +12,7 @@
 from openforms.forms.tests.factories import (
     FormDefinitionFactory,
     FormFactory,
+    FormStepFactory,
     FormVariableFactory,
 )
 from openforms.submissions.models import SubmissionValueVariable
@@ -21,6 +22,9 @@
 )
 from openforms.variables.constants import FormVariableSources
 
+from ...models import FormVariable
+from ...tasks import create_form_variables_for_components, on_formstep_save_event
+
 
 class FormVariableTasksTest(TestCase):
     def test_recouple_submission_variables(self):
@@ -205,3 +209,143 @@ def race_condition():
 
         race_condition_thread.join()
         recouple_variables_thread.join()
+
+
+@tag("gh-4824")
+class CreateFormVariablesForComponentsTests(TestCase):
+    def test_create_form_variables_for_components(self):
+        form = FormFactory.create()
+        form_definition = FormDefinitionFactory.create(
+            configuration={
+                "display": "form",
+                "components": [
+                    {
+                        "key": "lastName",
+                        "type": "textfield",
+                        "label": "Last Name",
+                    },
+                ],
+            }
+        )
+        FormStepFactory.create(form=form, form_definition=form_definition)
+
+        # Form is in a broken state, because no FormVariable exists for `lastName`
+        FormVariable.objects.filter(form=form).delete()
+
+        with self.subTest("create variables for components"):
+            create_form_variables_for_components(form.id)
+
+            variables = FormVariable.objects.filter(form=form)
+
+            self.assertEqual(variables.count(), 1)
+
+            [variable] = variables
+
+            self.assertEqual(variable.form_definition, form_definition)
+            self.assertEqual(variable.source, "component")
+            self.assertEqual(variable.key, "lastName")
+
+        with self.subTest("task is idempotent"):
+            create_form_variables_for_components(form.id)
+
+            variables = FormVariable.objects.filter(form=form)
+
+            self.assertEqual(variables.count(), 1)
+
+
+@tag("gh-4824")
+@override_settings(CELERY_TASK_ALWAYS_EAGER=True)
+class OnFormStepSaveEventTests(TestCase):
+    def test_on_formstep_save_event_fixes_broken_form_variables_state(self):
+        form = FormFactory.create()
+        other_form = FormFactory.create()
+        form_definition = FormDefinitionFactory.create(
+            configuration={
+                "display": "form",
+                "components": [
+                    {
+                        "key": "firstName",
+                        "type": "textfield",
+                        "label": "First Name",
+                    },
+                    {
+                        "key": "lastName",
+                        "type": "textfield",
+                        "label": "Last Name",
+                    },
+                ],
+            },
+            is_reusable=True,
+        )
+        form_step1 = FormStepFactory.create(form=form, form_definition=form_definition)
+        form_step2 = FormStepFactory.create(
+            form=other_form, form_definition=form_definition
+        )
+
+        submission1 = SubmissionFactory.create(form=form)
+        submission2 = SubmissionFactory.create(form=other_form)
+
+        SubmissionStepFactory.create(
+            submission=submission1,
+            form_step=form_step1,
+            data={"firstName": "John"},
+        )
+        SubmissionStepFactory.create(
+            submission=submission2,
+            form_step=form_step2,
+            data={"firstName": "John"},
+        )
+
+        # Form is in a broken state, because no FormVariable exists for `lastName`
+        # Simulating the scenario where `lastName` was added but no variable was created
+        # because there are errors in the variables tab
+        FormVariable.objects.filter(form=form, key="lastName").delete()
+        FormVariable.objects.filter(form=other_form, key="lastName").delete()
+
+        on_formstep_save_event(form.id, 60)
+
+        with self.subTest("Form has appropriate FormVariables"):
+            self.assertEqual(FormVariable.objects.filter(form=form).count(), 2)
+            first_name_var1, last_name_var1 = FormVariable.objects.filter(
+                form=form
+            ).order_by("pk")
+            self.assertEqual(first_name_var1.key, "firstName")
+            self.assertEqual(last_name_var1.key, "lastName")
+
+        with self.subTest(
+            "other Form that reuses this FormDefinition has appropriate FormVariables"
+        ):
+            self.assertEqual(FormVariable.objects.filter(form=other_form).count(), 2)
+
+            first_name_var2, last_name_var2 = FormVariable.objects.filter(
+                form=other_form
+            ).order_by("pk")
+
+            self.assertEqual(first_name_var2.key, "firstName")
+            self.assertEqual(last_name_var2.key, "lastName")
+
+        with self.subTest(
+            "Existing submission values are recoupled with new variables"
+        ):
+            submission_vars1 = SubmissionValueVariable.objects.filter(
+                submission__form=form
+            )
+
+            self.assertEqual(submission_vars1.count(), 1)
+
+            [first_name_submission_var1] = submission_vars1
+
+            self.assertEqual(first_name_submission_var1.form_variable, first_name_var1)
+
+        # with self.subTest(
+        #     "Existing submission values for other form are recoupled with new variables"
+        # ):
+        #     submission_vars2 = SubmissionValueVariable.objects.filter(
+        #         submission__form=other_form
+        #     )
+
+        #     self.assertEqual(submission_vars1.count(), 1)
+
+        #     [first_name_submission_var2] = submission_vars2
+
+        #     self.assertEqual(first_name_submission_var2.form_variable, first_name_var2)