Skip to content

Commit

Permalink
Merge pull request #5043 from open-formulieren/issue/5035-duplicated-…
Browse files Browse the repository at this point in the history
…json-summary-nodes

Fix accidental mutation side effect breaking the Objects API v1 registration
  • Loading branch information
sergei-maertens authored Jan 28, 2025
2 parents 306022a + 9e90a4c commit 65097fa
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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
);
Expand All @@ -36,12 +37,16 @@ const VariablesEditor = ({variables, onAdd, onDelete, onChange, onFieldChange})
<Tabs>
<TabList>
<Tab hasErrors={componentVariables.some(variable => variableHasErrors(variable))}>
{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)}
{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)} (
{componentVariables.length})
</Tab>
<Tab hasErrors={userDefinedVariables.some(variable => variableHasErrors(variable))}>
{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.userDefined)}
{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.userDefined)} (
{userDefinedVariables.length})
</Tab>
<Tab>
{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.static)} ({staticVariables.length})
</Tab>
<Tab>{intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.static)}</Tab>
<Tab>
<FormattedMessage
defaultMessage="Registration"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ export const EmptyUserDefinedVariableWithObjectsAPIRegistration = {
play: async ({canvasElement}) => {
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);
},
Expand Down Expand Up @@ -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');
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down
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
Original file line number Diff line number Diff line change
@@ -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


Expand All @@ -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:
Expand All @@ -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:
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
3 changes: 1 addition & 2 deletions src/openforms/submissions/rendering/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
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 65097fa

Please sign in to comment.