diff --git a/src/openforms/forms/tasks.py b/src/openforms/forms/tasks.py index bcb8684b18..5171ee2ce4 100644 --- a/src/openforms/forms/tasks.py +++ b/src/openforms/forms/tasks.py @@ -3,7 +3,6 @@ from functools import partial from django.db import DatabaseError, transaction -from django.db.utils import IntegrityError from django.utils import timezone from celery import chain @@ -18,20 +17,9 @@ def on_variables_bulk_update_event(form_id: int) -> None: """ - Celery chain of tasks to execute on a bulk update of variables event. + Celery tasks to execute on a "bulk update of variables" event. """ - 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( - repopulate_reusable_definition_variables_task, - recouple_submission_variables_task, - ) - actions_chain.delay() + repopulate_reusable_definition_variables_to_form_variables.delay(form_id=form_id) def on_formstep_save_event(form_id: int, countdown: int) -> None: @@ -44,65 +32,14 @@ def on_formstep_save_event(form_id: int, countdown: int) -> None: 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 - - When the FormVariable bulk create/update endpoint is called, all existing FormVariable related to the form are - deleted and new are created. If there are existing submissions for this form, the SubmissionValueVariables don't - have a related FormVariable anymore. This task tries to recouple them and does the same for - other Forms in case of reusable FormDefinitions - """ - from openforms.submissions.models import SubmissionValueVariable - - from .models import FormDefinition # due to circular import - - def recouple(form): - submission_variables_to_recouple = SubmissionValueVariable.objects.filter( - form_variable__isnull=True, submission__form=form - ) - - form_variables = { - variable.key: variable for variable in form.formvariable_set.all() - } - - submission_variables_to_update = [] - for submission_variable in submission_variables_to_recouple: - if form_variable := form_variables.get(submission_variable.key): - submission_variable.form_variable = form_variable - submission_variables_to_update.append(submission_variable) - - try: - SubmissionValueVariable.objects.bulk_update( - submission_variables_to_update, fields=["form_variable"] - ) - except IntegrityError: - # Issue #1970: If the form is saved again from the form editor while this task was running, the form variables - # retrieved don't exist anymore. Another task will be scheduled from the endpoint, so nothing more to do here. - logger.info("Form variables were updated while this task was runnning.") - - recouple(Form.objects.get(id=form_id)) - - fds = FormDefinition.objects.filter(formstep__form=form_id, is_reusable=True) - other_forms = Form.objects.filter(formstep__form_definition__in=fds).exclude( - id=form_id - ) - for form in other_forms: - recouple(form) - - @app.task(ignore_result=True) @transaction.atomic() def repopulate_reusable_definition_variables_to_form_variables(form_id: int) -> None: diff --git a/src/openforms/forms/tests/variables/test_tasks.py b/src/openforms/forms/tests/variables/test_tasks.py index 0d19484d3e..112f9fe60e 100644 --- a/src/openforms/forms/tests/variables/test_tasks.py +++ b/src/openforms/forms/tests/variables/test_tasks.py @@ -1,28 +1,14 @@ -import threading -import time -from unittest.mock import patch +from django.test import TestCase, override_settings, tag -from django.db import close_old_connections -from django.db.models import Q -from django.test import TestCase, TransactionTestCase, override_settings, tag - -from openforms.forms.tasks import ( - recouple_submission_variables_to_form_variables, - repopulate_reusable_definition_variables_to_form_variables, -) from openforms.forms.tests.factories import ( FormDefinitionFactory, FormFactory, FormStepFactory, - FormVariableFactory, ) -from openforms.submissions.models import SubmissionValueVariable from openforms.submissions.tests.factories import ( SubmissionFactory, SubmissionStepFactory, - SubmissionValueVariableFactory, ) -from openforms.variables.constants import FormVariableSources from ...models import FormVariable from ...tasks import ( @@ -32,191 +18,6 @@ ) -class FormVariableTasksTest(TestCase): - def test_recouple_submission_variables(self): - form = FormFactory.create( - generate_minimal_setup=True, - formstep__form_definition__configuration={ - "components": [ - {"type": "textfield", "key": "test1"}, - {"type": "textfield", "key": "test2"}, - ] - }, - ) - form_step = form.formstep_set.first() - submission1 = SubmissionFactory.create(form=form) - submission2 = SubmissionFactory.create(form=form) - - SubmissionStepFactory.create( - submission=submission1, - form_step=form_step, - data={"test1": "some data 1", "test2": "some data 1"}, - ) - SubmissionStepFactory.create( - submission=submission2, - form_step=form_step, - data={"test1": "some other data 1", "test2": "some other data 1"}, - ) - - form.formvariable_set.all().delete() - - self.assertEqual( - 4, - SubmissionValueVariable.objects.filter( - submission__form=form, form_variable__isnull=True - ).count(), - ) - - FormVariableFactory.create(key="test1", form=form) - FormVariableFactory.create(key="test2", form=form) - - recouple_submission_variables_to_form_variables(form.id) - - self.assertEqual( - 0, - SubmissionValueVariable.objects.filter( - submission__form=form, form_variable__isnull=True - ).count(), - ) - - def test_recouple_reusable_definition_variables(self): - reusable_fd = FormDefinitionFactory.create( - is_reusable=True, - configuration={"components": [{"type": "textfield", "key": "test1"}]}, - ) - form1 = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - form2 = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - - reusable_fd.configuration = { - "components": [{"type": "textfield", "key": "test1Updated"}] - } - reusable_fd.save() - - repopulate_reusable_definition_variables_to_form_variables(form1.id) - - new_variable = form2.formvariable_set.get() - - self.assertEqual(new_variable.key, "test1Updated") - - def test_recouple_reusable_definition_variables_do_not_delete_user_defined(self): - reusable_fd = FormDefinitionFactory.create( - is_reusable=True, - configuration={"components": [{"type": "textfield", "key": "test1"}]}, - ) - form = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - user_defined_var = FormVariableFactory.create( - form=form, key="variable4", user_defined=True - ) - - repopulate_reusable_definition_variables_to_form_variables(form.id) - - form_user_defined_var = form.formvariable_set.get( - source=FormVariableSources.user_defined - ) - - self.assertEqual(form_user_defined_var, user_defined_var) - - -class ThreadWithExceptionHandling(threading.Thread): - def run(self): - self.exc = None - try: - super().run() - except Exception as e: - self.exc = e - - def join(self, *args, **kwargs): - super().join(*args, **kwargs) - if self.exc: - raise self.exc - - -class FormVariableRaceConditionTasksTest(TransactionTestCase): - @tag("gh-1970") - def test_recouple_submission_variables_race_condition(self): - """ - Race condition: - - The task recouple_submission_variables_to_form_variables starts, it retrieves - the form variables on a form and tries to relate them to existing submission - variables. - - While it's processing the submission vars, the form is saved again, triggering - an update of form variables. - - The task gives an integrity error when saving the submission variables because - they are being related to non-existing form variables. - """ - form = FormFactory.create( - generate_minimal_setup=True, - formstep__form_definition__configuration={ - "components": [ - {"type": "textfield", "key": "test1"}, - {"type": "textfield", "key": "test2"}, - ] - }, - ) - form_step = form.formstep_set.first() - submission = SubmissionFactory.create(form=form) - - SubmissionStepFactory.create( - submission=submission, - form_step=form_step, - data={"test1": "some data 1", "test2": "some data 1"}, - ) - - form.formvariable_set.all().delete() - - FormVariableFactory.create(key="test1", form=form) - FormVariableFactory.create(key="test2", form=form) - - def recouple_variables_task(): - real_bulk_update = SubmissionValueVariable.objects.bulk_update - - def wrapped_bulk_update(*args, **kwargs): - time.sleep(0.6) - # give some time for the delete to complete before we - # make a DB query - return real_bulk_update(*args, **kwargs) - - with patch( - "openforms.submissions.models.submission_value_variable" - ".SubmissionValueVariableManager.bulk_update", - wraps=wrapped_bulk_update, - ) as mock_bulk_update: - try: - recouple_submission_variables_to_form_variables(form.id) - finally: - close_old_connections() - mock_bulk_update.assert_called_once() - - def race_condition(): - # ensure the delete runs at some point _after_ the - # recouple_variables_thread has started - time.sleep(0.5) - try: - form.formvariable_set.all().delete() - finally: - close_old_connections() - - recouple_variables_thread = ThreadWithExceptionHandling( - target=recouple_variables_task - ) - race_condition_thread = threading.Thread(target=race_condition) - - race_condition_thread.start() - recouple_variables_thread.start() - - race_condition_thread.join() - recouple_variables_thread.join() - - @tag("gh-4824") class CreateFormVariablesForComponentsTests(TestCase): def test_create_form_variables_for_components(self): @@ -259,88 +60,6 @@ def test_create_form_variables_for_components(self): self.assertEqual(variables.count(), 1) -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) -class RecoupleSubmissionVariablesToFormVariables(TestCase): - def test_recouple(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"}, - ) - - # Task should ignore keys for which there is no FormVariable - SubmissionValueVariableFactory.create( - submission=submission1, key="non-existent", form_variable=None - ) - SubmissionValueVariable.objects.filter( - Q(submission__form=form) | Q(submission__form=other_form), - ).update(form_variable=None) - - first_name_var1 = FormVariable.objects.get(form=form, key="firstName") - first_name_var2 = FormVariable.objects.get(form=other_form, key="firstName") - - recouple_submission_variables_to_form_variables(form.id) - - with self.subTest( - "Existing submission values are recoupled with new variables" - ): - submission_vars1 = SubmissionValueVariable.objects.filter( - submission__form=form - ) - - self.assertEqual(submission_vars1.count(), 2) - - first_name_submission_var1, _ = submission_vars1.order_by("key") - - 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_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) - - @tag("gh-4824") @override_settings(CELERY_TASK_ALWAYS_EAGER=True) class OnFormStepSaveEventTests(TestCase): @@ -412,32 +131,6 @@ def test_on_formstep_save_event_fixes_broken_form_variables_state(self): 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_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) - @override_settings(CELERY_TASK_ALWAYS_EAGER=True) class OnVariablesBulkUpdateEventTests(TestCase): @@ -502,29 +195,3 @@ def test_on_variables_bulk_update_event(self): 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_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) diff --git a/src/openforms/submissions/models/submission_value_variable.py b/src/openforms/submissions/models/submission_value_variable.py index 2e3722e55b..1126668d00 100644 --- a/src/openforms/submissions/models/submission_value_variable.py +++ b/src/openforms/submissions/models/submission_value_variable.py @@ -308,6 +308,11 @@ class SubmissionValueVariable(models.Model): help_text=_("The submission to which this variable value is related"), on_delete=models.CASCADE, ) + # XXX DeprecationWarning - this foreign key field actually doesn't serve any purpose + # except for some details in the to_python method. Instead, the + # submission.load_submission_value_variables_state method takes care of populating the + # state and resolving the matching form variables based on the variable key. This field + # is scheduled for removal to avoid confusion. form_variable = models.ForeignKey( to=FormVariable, verbose_name=_("form variable"), @@ -315,8 +320,6 @@ class SubmissionValueVariable(models.Model): on_delete=models.SET_NULL, # In case form definitions are edited after a user has filled in a form. null=True, ) - # Added for the case where a FormVariable has been deleted, but there is still a SubmissionValueValue. - # Having a key will help debug where the SubmissionValueValue came from key = models.TextField( verbose_name=_("key"), help_text=_("Key of the variable"), @@ -362,10 +365,6 @@ class Meta: unique_together = [["submission", "key"], ["submission", "form_variable"]] def __str__(self): - if self.form_variable: - return _("Submission value variable {name}").format( - name=self.form_variable.name - ) return _("Submission value variable {key}").format(key=self.key) def to_python(self) -> Any: @@ -382,6 +381,9 @@ def to_python(self) -> Any: return None # can't see any type information... + # FIXME: this may break when we remove the form_variable FK field. Instead, we'll + # need to look up the form variable from the related form based on the self.key + # value and resolve this in-memory. if not self.form_variable_id: return self.value