Skip to content

Commit

Permalink
🐛 [#5035] Fix "phantom" properties in legacy objects API
Browse files Browse the repository at this point in the history
Fix 1:

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.

Fix 2:

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.

Backport-of: #5043
  • Loading branch information
sergei-maertens committed Jan 28, 2025
1 parent c7e5d89 commit 3e6fd8d
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 3 deletions.
13 changes: 13 additions & 0 deletions src/openforms/js/components/admin/form_design/variables/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
6 changes: 3 additions & 3 deletions src/openforms/submissions/models/submission.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import logging
import uuid
from copy import deepcopy
from dataclasses import dataclass
from typing import TYPE_CHECKING, Any, Mapping

Expand Down Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions src/openforms/submissions/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"])

0 comments on commit 3e6fd8d

Please sign in to comment.