From f8e8888db79c5fdc5c96772641f87221fb365746 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 9 Dec 2024 13:34:05 +0100 Subject: [PATCH] :bug: [#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 --- .../forms/api/serializers/form_step.py | 20 +++ src/openforms/forms/tasks.py | 45 ++++++ .../forms/tests/test_api_formsteps.py | 97 ++++++++++++ .../forms/tests/variables/test_tasks.py | 146 +++++++++++++++++- 4 files changed, 307 insertions(+), 1 deletion(-) diff --git a/src/openforms/forms/api/serializers/form_step.py b/src/openforms/forms/api/serializers/form_step.py index 8fbf625c49..71bb819069 100644 --- a/src/openforms/forms/api/serializers/form_step.py +++ b/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 diff --git a/src/openforms/forms/tasks.py b/src/openforms/forms/tasks.py index 1590b4bd86..3db9565dd1 100644 --- a/src/openforms/forms/tasks.py +++ b/src/openforms/forms/tasks.py @@ -36,6 +36,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 detect_formiojs_configuration_snake_case( form_definition_id: int, raise_exception=False @@ -136,6 +158,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.""" diff --git a/src/openforms/forms/tests/test_api_formsteps.py b/src/openforms/forms/tests/test_api_formsteps.py index 7161d430d1..b339ff47ce 100644 --- a/src/openforms/forms/tests/test_api_formsteps.py +++ b/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 diff --git a/src/openforms/forms/tests/variables/test_tasks.py b/src/openforms/forms/tests/variables/test_tasks.py index e8cce9edd1..896499e9a5 100644 --- a/src/openforms/forms/tests/variables/test_tasks.py +++ b/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)