From 00db4047588922f75c775496f52d1f044367eafd Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 27 Jan 2025 14:56:48 +0100 Subject: [PATCH 1/5] :bug: [#5035] Fix components in repeating groups creating FormVariable records Components nested in repeating groups are not real variables. The frontend code showed these in the variables table AND this also caused them to be PUT to the backend, resulting in crashes when processing file uploads because there are no saved submission variables available for them. Unfortunately, this doesn't appear to fix the problem with too much keys being included in the submission data during registration, so it must have a different root cause. --- .../form_design/variables/VariablesEditor.js | 15 ++++++++++----- .../variables/VariablesEditor.stories.js | 14 +++++++------- .../admin/form_design/variables/utils.js | 13 +++++++++++++ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/openforms/js/components/admin/form_design/variables/VariablesEditor.js b/src/openforms/js/components/admin/form_design/variables/VariablesEditor.js index 7833a21aa3..c7dd592f70 100644 --- a/src/openforms/js/components/admin/form_design/variables/VariablesEditor.js +++ b/src/openforms/js/components/admin/form_design/variables/VariablesEditor.js @@ -1,7 +1,8 @@ -import React from 'react'; +import React, {useContext} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {TabList, TabPanel, Tabs} from 'react-tabs'; +import {FormContext} from 'components/admin/form_design/Context'; import Tab from 'components/admin/form_design/Tab'; import Fieldset from 'components/admin/forms/Fieldset'; @@ -14,10 +15,10 @@ import {variableHasErrors} from './utils'; const VariablesEditor = ({variables, onAdd, onDelete, onChange, onFieldChange}) => { const intl = useIntl(); + const {staticVariables} = useContext(FormContext); const userDefinedVariables = variables.filter( variable => variable.source === VARIABLE_SOURCES.userDefined ); - const componentVariables = variables.filter( variable => variable.source === VARIABLE_SOURCES.component ); @@ -36,12 +37,16 @@ const VariablesEditor = ({variables, onAdd, onDelete, onChange, onFieldChange}) variableHasErrors(variable))}> - {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)} + {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)} ( + {componentVariables.length}) variableHasErrors(variable))}> - {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.userDefined)} + {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.userDefined)} ( + {userDefinedVariables.length}) + + + {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.static)} ({staticVariables.length}) - {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.static)} { const canvas = within(canvasElement); - const userDefinedVarsTab = canvas.getByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = canvas.getByRole('tab', {name: /Gebruikersvariabelen/}); await userEvent.click(userDefinedVarsTab); expect(canvas.queryAllByText('record.geometry')).toHaveLength(0); }, @@ -696,7 +696,7 @@ export const WithJSONDumpRegistrationBackend = { }); await step('now checkbox checked', async () => { - const staticVariables = canvas.getByRole('tab', {name: 'Vaste variabelen'}); + const staticVariables = canvas.getByRole('tab', {name: /Vaste variabelen/}); await userEvent.click(staticVariables); const editIcon = canvas.getByTitle('Registratie-instellingen bewerken'); @@ -712,7 +712,7 @@ export const ConfigurePrefill = { play: async ({canvasElement}) => { const canvas = within(canvasElement); - const userDefinedVarsTab = await canvas.findByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = await canvas.findByRole('tab', {name: /Gebruikersvariabelen/}); expect(userDefinedVarsTab).toBeVisible(); await userEvent.click(userDefinedVarsTab); @@ -753,7 +753,7 @@ export const ConfigurePrefillObjectsAPI = { const canvas = within(canvasElement); await step('Open configuration modal', async () => { - const userDefinedVarsTab = await canvas.findByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = await canvas.findByRole('tab', {name: /Gebruikersvariabelen/}); expect(userDefinedVarsTab).toBeVisible(); await userEvent.click(userDefinedVarsTab); @@ -859,7 +859,7 @@ export const ConfigurePrefillObjectsAPIWithCopyButton = { const canvas = within(canvasElement); await step('Open configuration modal', async () => { - const userDefinedVarsTab = await canvas.findByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = await canvas.findByRole('tab', {name: /Gebruikersvariabelen/}); expect(userDefinedVarsTab).toBeVisible(); await userEvent.click(userDefinedVarsTab); @@ -964,7 +964,7 @@ export const WithValidationErrors = { play: async ({canvasElement}) => { const canvas = within(canvasElement); - const userDefinedVarsTab = await canvas.findByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = await canvas.findByRole('tab', {name: /Gebruikersvariabelen/}); expect(userDefinedVarsTab).toBeVisible(); await userEvent.click(userDefinedVarsTab); @@ -1023,7 +1023,7 @@ export const ConfigurePrefillObjectsAPIWithValidationErrors = { const canvas = within(canvasElement); await step('Open configuration modal', async () => { - const userDefinedVarsTab = await canvas.findByRole('tab', {name: 'Gebruikersvariabelen'}); + const userDefinedVarsTab = await canvas.findByRole('tab', {name: /Gebruikersvariabelen/}); expect(userDefinedVarsTab).toBeVisible(); await userEvent.click(userDefinedVarsTab); diff --git a/src/openforms/js/components/admin/form_design/variables/utils.js b/src/openforms/js/components/admin/form_design/variables/utils.js index 393e75cf49..c4ec01238a 100644 --- a/src/openforms/js/components/admin/form_design/variables/utils.js +++ b/src/openforms/js/components/admin/form_design/variables/utils.js @@ -84,13 +84,26 @@ const shouldNotUpdateVariables = (newComponent, oldComponent, mutationType, step return isComponentWithVariable || isEditGridChild; }; +/** + * Transform the Formio configuration into FormVariable instances. + * @param {String} formDefinition API resource of the form definition that the configuration belongs to. + * @param {OBject} configuration The Formio form configuration. + */ const getFormVariables = (formDefinition, configuration) => { const newFormVariables = []; FormioUtils.eachComponent(configuration.components, component => { if (component.type === 'softRequiredErrors') return; + + // sEE #5035 - the client side upload components variables are created on load, and + // then they get pushed to the server on save and are persisted too, which causes + // upload issues. This may even have been the root cause of this issue where + // "phantom" variables show up in the step data. + if (isInEditGrid(component, configuration)) return; + newFormVariables.push(makeNewVariableFromComponent(component, formDefinition)); }); + return newFormVariables; }; From 68831c9f1ae7b154956f8691caac93552d2de481 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 27 Jan 2025 15:06:39 +0100 Subject: [PATCH 2/5] :technologist: Fix registration management command --- .../commands/register_submission.py | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/openforms/registrations/management/commands/register_submission.py b/src/openforms/registrations/management/commands/register_submission.py index 514f094d35..4ed0918539 100644 --- a/src/openforms/registrations/management/commands/register_submission.py +++ b/src/openforms/registrations/management/commands/register_submission.py @@ -1,5 +1,8 @@ from django.core.management import BaseCommand +from openforms.submissions.constants import PostSubmissionEvents, RegistrationStatuses +from openforms.submissions.models import Submission + from ...tasks import pre_registration, register_submission @@ -10,11 +13,22 @@ def add_arguments(self, parser): parser.add_argument( "submission_id", type=int, help="ID of submission to register" ) + parser.add_argument("--force", action="store_true") def handle(self, **options): + if options["force"]: + submission = Submission.objects.get(pk=options["submission_id"]) + submission.pre_registration_completed = False + submission.registration_status = RegistrationStatuses.pending + submission.registration_attempts = 0 + submission.registration_result = {} + submission.save() + self.stdout.write("Pre-registering submission...") try: - pre_registration(options["submission_id"]) + pre_registration( + options["submission_id"], PostSubmissionEvents.on_completion + ) except Exception as exc: self.stderr.write(f"Pre-registration failed with error: {exc}") else: @@ -24,7 +38,9 @@ def handle(self, **options): self.stdout.write("Registering submission...") try: - register_submission(options["submission_id"]) + register_submission( + options["submission_id"], PostSubmissionEvents.on_completion + ) except Exception as exc: self.stderr.write(f"Registration failed with error: {exc}") else: From 73d451fbbbdac36fcd9e0e2d6d0436430f4926c0 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 27 Jan 2025 15:50:02 +0100 Subject: [PATCH 3/5] :art: DRY accessing the submission steps This is available on the model too, so we can build on top of that. --- src/openforms/submissions/rendering/renderer.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/openforms/submissions/rendering/renderer.py b/src/openforms/submissions/rendering/renderer.py index d65f1df9ab..d6bf4bfedd 100644 --- a/src/openforms/submissions/rendering/renderer.py +++ b/src/openforms/submissions/rendering/renderer.py @@ -52,8 +52,7 @@ def steps(self): """ Return the submission steps in the correct order. """ - execution_state = self.submission.load_execution_state() - return execution_state.submission_steps + return self.submission.steps @property def has_children(self) -> bool: From 97d0cc47edc4fba5a02d74b974ef5ee36f7fc3cf Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 27 Jan 2025 16:00:27 +0100 Subject: [PATCH 4/5] :test_tube: [#5035] Add test for unintended mutation side effect Damn you, side effects. --- .../submissions/tests/test_models.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/openforms/submissions/tests/test_models.py b/src/openforms/submissions/tests/test_models.py index 282059960f..c6c5550844 100644 --- a/src/openforms/submissions/tests/test_models.py +++ b/src/openforms/submissions/tests/test_models.py @@ -456,3 +456,54 @@ def test_names_do_not_break_pdf_saving_to_disk(self): report.generate_submission_report_pdf() self.assertTrue(report.content.storage.exists(report.content.name)) + + @tag("gh-5035") + def test_total_configuration_wrapper_does_not_mutate_first_step(self): + form = FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "key": "textfield1", + "type": "textfield", + "label": "textfield", + } + ] + }, + ) + FormStepFactory.create( + form=form, + order=1, + form_definition__configuration={ + "components": [ + { + "key": "textfield2", + "type": "textfield", + "label": "Text field 2", + } + ] + }, + ) + submission = SubmissionFactory.create(form=form) + + configuration_wrapper = submission.total_configuration_wrapper + + with self.subTest("all keys present"): + self.assertIn("textfield1", configuration_wrapper) + self.assertIn("textfield2", configuration_wrapper) + + step1, step2 = submission.steps + + with self.subTest("step 1 keys"): + step1_keys = [ + c["key"] + for c in step1.form_step.form_definition.configuration["components"] + ] + self.assertEqual(step1_keys, ["textfield1"]) + + with self.subTest("step 2 keys"): + step2_keys = [ + c["key"] + for c in step2.form_step.form_definition.configuration["components"] + ] + self.assertEqual(step2_keys, ["textfield2"]) From 9e90a4ce856d52d88bf91fd38043580a223f5a4c Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Mon, 27 Jan 2025 16:04:51 +0100 Subject: [PATCH 5/5] :bug: [#5035] Avoid the total configuration wrapper mutating the first step configuration The total configuration wrapper merges the configuration wrapper of each step into a single object for optimized access to values/ components. It takes the first step and merges the remaining steps into it. However, this had the unintended side-effect of mutating the config of the first step, manifesting in the objects API v1 registration with the json_summary tag which contained extra, unexpected keys in the submission data of the first step. Fixed by making a deep copy first to end up with a different instance that can be safely mutated. --- src/openforms/submissions/models/submission.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openforms/submissions/models/submission.py b/src/openforms/submissions/models/submission.py index 8592e308c2..1fa6c42d4d 100644 --- a/src/openforms/submissions/models/submission.py +++ b/src/openforms/submissions/models/submission.py @@ -2,6 +2,7 @@ import logging import uuid +from copy import deepcopy from dataclasses import dataclass from typing import TYPE_CHECKING, Any, Mapping @@ -429,9 +430,8 @@ def total_configuration_wrapper(self) -> FormioConfigurationWrapper: if len(form_steps) == 0: return FormioConfigurationWrapper(configuration={}) - wrapper = FormioConfigurationWrapper( - form_steps[0].form_definition.configuration - ) + begin_configuration = deepcopy(form_steps[0].form_definition.configuration) + wrapper = FormioConfigurationWrapper(begin_configuration) for form_step in form_steps[1:]: wrapper += form_step.form_definition.configuration_wrapper self._total_configuration_wrapper = wrapper