From cad4e9817fca9c05a211834761d91ab098e6f4b3 Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Wed, 26 Feb 2025 15:41:21 +0100 Subject: [PATCH 1/4] :test_tube: [#5104] Added tests for radio component default value The test covers the entire bug reproduce flow. Starting by creating a form with a radio component and saving it. This should (also with the bug still present) set the defaultValue to empty string in the database. When editing the form and opening the radio component in the json view, the defaultValue will show as `null`. Because playwright cannot target the json view correctly (certain dom elements aren't targetable), we just save the component (setting the defaultValue to `null`) and save the form. With the bug present, the database should now have the defaultValue as `null`. This bug is a combination of problems that result in the defaultValue being wrongly set by formio. The problem comes from our radio component, which defines a defaultValue in the schema. This schema is not set on the formio component, unless we specify it with `get defaultSchema`. When calculating the defaultValue, formio starts with the defaultValue on the schema, which isn't defined in the regular formio radio component schema. If the defaultValue defined on the component (via the admin) is truety, this will be used as 'real' the defaultValue. Because the defaultValue of the component was an empty string, which is falsey, and the defaultValue wasn't set by the component schema, the defaultValue was set to null. If you would edit the component and save it, then the defaultValue `null` would be saved in the database. --- .../tests/e2e_tests/test_form_designer.py | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/src/openforms/forms/tests/e2e_tests/test_form_designer.py b/src/openforms/forms/tests/e2e_tests/test_form_designer.py index 338f7b699e..a93ff4cb10 100644 --- a/src/openforms/forms/tests/e2e_tests/test_form_designer.py +++ b/src/openforms/forms/tests/e2e_tests/test_form_designer.py @@ -511,6 +511,98 @@ def assertFormValues(): await assertFormValues() + @tag("dh-5104") + async def test_radio_component_default_value_empty_string(self): + @sync_to_async + def setUpTestData(): + form = FormFactory.create() + FormStepFactory.create( + form=form, form_definition__configuration={"components": []} + ) + return form + + await create_superuser() + form = await setUpTestData() + admin_url = str( + furl(self.live_server_url) + / reverse("admin:forms_form_change", args=(form.pk,)) + ) + + async with browser_page() as page: + await self._admin_login(page) + await page.goto(str(admin_url)) + + with phase("Populate and save form"): + await page.get_by_role("tab", name="Steps and fields").click() + + await drag_and_drop_component(page, "Radio", "group-panel-custom") + await page.get_by_test_id("input-label").fill("Custom Radio") + await page.get_by_test_id("input-values[0].label").fill("Label") + await page.get_by_test_id("input-values[0].value").fill("Value") + await close_modal(page, "Save") + + # Save form + await page.get_by_role( + "button", name="Save and continue editing" + ).click() + await page.wait_for_url(admin_url) + + with phase("Validate default value after create"): + + @sync_to_async + def assertFormValues(): + configuration = ( + form.formstep_set.get().form_definition.configuration + ) + radio_component = configuration["components"][0] + self.assertEqual( + radio_component["defaultValue"], + "", + ) + + await assertFormValues() + + # When creating a radio component, the defaultValue is set correctly in the + # database. + # + # This bug appears in the json view, where the `defaultValue` will show as + # `null` after `Save and continue editing`. + # The json view is a conditionally rendered html list of divs + # (only elements that are directly shown are targetable in the dom) + # + # So to test if this bug happens, we just save it again, and then check the + # database. + with phase("Edit the form"): + await page.get_by_role("tab", name="Steps and fields").click() + + # The defaultValue in the bug is set to `null`. + # If we open the component, and immediately save it, the defaultValue + # will change from `""` (in the db) to `null` (set by bug) + await open_component_options_modal(page, "Custom Radio", exact=True) + await close_modal(page, "Save", exact=True) + + # Save form + await page.get_by_role( + "button", name="Save and continue editing" + ).click() + await page.wait_for_url(admin_url) + + with phase("Validate default value after editing"): + + @sync_to_async + def assertFormValues(): + configuration = ( + form.formstep_set.get().form_definition.configuration + ) + radio_component = configuration["components"][0] + + self.assertEqual( + radio_component["defaultValue"], + "", + ) + + await assertFormValues() + @tag("gh-2805") async def test_enable_translations_and_create_new_step(self): await create_superuser() From c477dd3645287eeff44872b5502966f45f702ede Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Wed, 26 Feb 2025 16:54:08 +0100 Subject: [PATCH 2/4] :bug: [#5104] Setting textfield and radio defaultSchema and defaultValue properly The previous solution for a similar bug for the textfield component was a bandage fix (#4659). The real problem is that formio doesn't normally define defaultValue's. This is something that we have to define and enforce ourselfs. By setting our custom schema (which is the formio schema + some custom schema definition) we can ensure that a proper defaultValue is defined. This also makes the components more predictable. If for some reason you set the defaultValue to `null`, then this will be respected. With the previous solution, a empty string was forced as the minimum defaultValue (if the defaultValue was falsey, it would be replaced by an empty string) --- src/openforms/js/components/form/email.js | 9 ++++----- src/openforms/js/components/form/iban.js | 7 +------ src/openforms/js/components/form/licenseplate.js | 7 +------ src/openforms/js/components/form/phoneNumber.js | 7 +------ src/openforms/js/components/form/radio.js | 4 ++++ src/openforms/js/components/form/textarea.js | 10 +++------- src/openforms/js/components/form/textfield.js | 10 ++++------ src/openforms/js/components/form/time.js | 7 ------- 8 files changed, 18 insertions(+), 43 deletions(-) diff --git a/src/openforms/js/components/form/email.js b/src/openforms/js/components/form/email.js index cdbb062e7f..90a22ecdd4 100644 --- a/src/openforms/js/components/form/email.js +++ b/src/openforms/js/components/form/email.js @@ -9,6 +9,7 @@ class EmailField extends FormioEmail { static schema(...extend) { const schema = FormioEmail.schema( { + defaultValue: '', validateOn: 'blur', }, ...extend @@ -31,12 +32,10 @@ class EmailField extends FormioEmail { super(...args); patchValidateDefaults(this); + } - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } + get defaultSchema() { + return EmailField.schema(); } } diff --git a/src/openforms/js/components/form/iban.js b/src/openforms/js/components/form/iban.js index 43b4f09a32..ac5c5a6aa2 100644 --- a/src/openforms/js/components/form/iban.js +++ b/src/openforms/js/components/form/iban.js @@ -8,6 +8,7 @@ class IbanField extends TextField { static schema(...extend) { return TextField.schema( { + defaultValue: '', type: 'iban', label: 'IBAN', key: 'iban', @@ -31,12 +32,6 @@ class IbanField extends TextField { super(...args); patchValidateDefaults(this); - - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } } get defaultSchema() { diff --git a/src/openforms/js/components/form/licenseplate.js b/src/openforms/js/components/form/licenseplate.js index ebf038d560..ad4cfb894e 100644 --- a/src/openforms/js/components/form/licenseplate.js +++ b/src/openforms/js/components/form/licenseplate.js @@ -9,6 +9,7 @@ class LicensePlate extends TextField { static schema(...extend) { const schema = TextField.schema( { + defaultValue: '', type: 'licenseplate', label: 'License plate', key: 'licenseplate', @@ -39,12 +40,6 @@ class LicensePlate extends TextField { super(...args); patchValidateDefaults(this); - - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } } get defaultSchema() { diff --git a/src/openforms/js/components/form/phoneNumber.js b/src/openforms/js/components/form/phoneNumber.js index a0a8266945..f60739ba4f 100644 --- a/src/openforms/js/components/form/phoneNumber.js +++ b/src/openforms/js/components/form/phoneNumber.js @@ -9,6 +9,7 @@ class PhoneNumberField extends PhoneNumber { static schema(...extend) { const schema = PhoneNumber.schema( { + defaultValue: '', inputMask: null, }, ...extend @@ -30,12 +31,6 @@ class PhoneNumberField extends PhoneNumber { super(...args); patchValidateDefaults(this); - - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } } get defaultSchema() { diff --git a/src/openforms/js/components/form/radio.js b/src/openforms/js/components/form/radio.js index e0a44edef7..3c1ede38e3 100644 --- a/src/openforms/js/components/form/radio.js +++ b/src/openforms/js/components/form/radio.js @@ -38,6 +38,10 @@ class RadioField extends RadioFormio { return super.setSelectedClasses(); } + + get defaultSchema() { + return RadioField.schema(); + } } export default RadioField; diff --git a/src/openforms/js/components/form/textarea.js b/src/openforms/js/components/form/textarea.js index 9fda017466..fb38e053fa 100644 --- a/src/openforms/js/components/form/textarea.js +++ b/src/openforms/js/components/form/textarea.js @@ -9,7 +9,9 @@ const FormioTextarea = Formio.Components.components.textarea; class TextArea extends FormioTextarea { static schema(...extend) { - return localiseSchema(FormioTextarea.schema({validate: {maxLength: 10000}}, ...extend)); + return localiseSchema( + FormioTextarea.schema({defaultValue: '', validate: {maxLength: 10000}}, ...extend) + ); } static get builderInfo() { @@ -23,12 +25,6 @@ class TextArea extends FormioTextarea { super(...args); patchValidateDefaults(this); - - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } } get defaultSchema() { diff --git a/src/openforms/js/components/form/textfield.js b/src/openforms/js/components/form/textfield.js index 4945d818d6..d70bee80be 100644 --- a/src/openforms/js/components/form/textfield.js +++ b/src/openforms/js/components/form/textfield.js @@ -30,7 +30,7 @@ export const patchValidateDefaults = instance => { class TextField extends FormioTextField { static schema(...extend) { - return localiseSchema(FormioTextField.schema(...extend)); + return localiseSchema(FormioTextField.schema({defaultValue: ''}, ...extend)); } static get builderInfo() { @@ -44,12 +44,10 @@ class TextField extends FormioTextField { super(...args); patchValidateDefaults(this); + } - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } + get defaultSchema() { + return TextField.schema(); } } diff --git a/src/openforms/js/components/form/time.js b/src/openforms/js/components/form/time.js index 34cfff341c..09c5dfb981 100644 --- a/src/openforms/js/components/form/time.js +++ b/src/openforms/js/components/form/time.js @@ -1,7 +1,6 @@ import {Formio} from 'formiojs'; import {localiseSchema} from './i18n'; -import {patchValidateDefaults} from './textfield'; const Time = Formio.Components.components.time; @@ -13,12 +12,6 @@ const Time = Formio.Components.components.time; class TimeField extends Time { constructor(...args) { super(...args); - - // somewhere the default emptyValue/defaultValue does not seem to be used and it forces - // component.defaultValue to be null, which causes issues with multiples #4659 - if (this.component.defaultValue === null) { - this.component.defaultValue = ''; - } } static schema(...extend) { From 01657e2546f96681315845f2ea7e00b36c3597bb Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Wed, 5 Mar 2025 16:39:20 +0100 Subject: [PATCH 3/4] :card_file_box: [#5104] Add migration for fixing radio component defaultValue's --- src/openforms/formio/migration_converters.py | 5 ++- .../formio/tests/test_migration_converters.py | 38 +++++++++++++++++++ .../0101_fix_radio_empty_default_value.py | 14 +++++++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/openforms/forms/migrations/0101_fix_radio_empty_default_value.py diff --git a/src/openforms/formio/migration_converters.py b/src/openforms/formio/migration_converters.py index 07f1f753b5..4645697ac1 100644 --- a/src/openforms/formio/migration_converters.py +++ b/src/openforms/formio/migration_converters.py @@ -385,7 +385,10 @@ def fix_empty_default_value(component: Component) -> bool: "currency": { "fix_empty_validate_lengths": fix_empty_validate_lengths, }, - "radio": {"set_openforms_datasrc": set_openforms_datasrc}, + "radio": { + "set_openforms_datasrc": set_openforms_datasrc, + "fix_empty_default_value": fix_empty_default_value, + }, # Special components "iban": { "fix_empty_validate_lengths": fix_empty_validate_lengths, diff --git a/src/openforms/formio/tests/test_migration_converters.py b/src/openforms/formio/tests/test_migration_converters.py index b5b4551a38..8c59e3ec51 100644 --- a/src/openforms/formio/tests/test_migration_converters.py +++ b/src/openforms/formio/tests/test_migration_converters.py @@ -716,3 +716,41 @@ def test_missing_interactions(self): component["interactions"], {"marker": True, "polygon": False, "polyline": False}, ) + + +class RadioTests(SimpleTestCase): + def test_no_default_value_doesnt_change(self): + component: Component = { + "type": "radio", + "key": "radio", + "label": "Radio field", + } + + changed = fix_empty_default_value(component) + + self.assertFalse(changed) + + def test_none_as_default_value_does_change(self): + component: Component = { + "type": "radio", + "key": "radio", + "label": "Radio field", + "defaultValue": None, + } + + changed = fix_empty_default_value(component) + + self.assertTrue(changed) + self.assertEqual(component["defaultValue"], "") + + def test_empty_string_as_default_value_doesnt_change(self): + component: Component = { + "type": "radio", + "key": "radio", + "label": "Radio field", + "defaultValue": "", + } + + changed = fix_empty_default_value(component) + + self.assertFalse(changed) diff --git a/src/openforms/forms/migrations/0101_fix_radio_empty_default_value.py b/src/openforms/forms/migrations/0101_fix_radio_empty_default_value.py new file mode 100644 index 0000000000..53c43d0311 --- /dev/null +++ b/src/openforms/forms/migrations/0101_fix_radio_empty_default_value.py @@ -0,0 +1,14 @@ +# Generated by Django 4.2.18 on 2025-03-05 15:25 + +from django.db import migrations + +from openforms.forms.migration_operations import ConvertComponentsOperation + + +class Migration(migrations.Migration): + + dependencies = [ + ("forms", "0100_add_interaction_config_to_map_component"), + ] + + operations = [ConvertComponentsOperation("radio", "fix_empty_default_value")] From 57a51ea359f5cdba770d393b5d69471786d4b166 Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Thu, 6 Mar 2025 09:49:14 +0100 Subject: [PATCH 4/4] :children_crossing: [#5104] Add fix script for fixing radio components defaultValue This can be used by admins to automatically fix the radio components with faulty defaultValue's --- bin/fix_radio_component_default_values.py | 59 +++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100755 bin/fix_radio_component_default_values.py diff --git a/bin/fix_radio_component_default_values.py b/bin/fix_radio_component_default_values.py new file mode 100755 index 0000000000..cea58104ed --- /dev/null +++ b/bin/fix_radio_component_default_values.py @@ -0,0 +1,59 @@ +#!/usr/bin/env python +# +# Fix the `defaultValue` of radio components. +# +# Due to a bug in the form editor, when a radio component had a defaultValue of "" +# (empty string) it would automatically be set to `null`. This in turn could cause json +# logic to stop working properly, due to the defaultValue being changed. +# +# This script automatically fixes all defaultValues of radio components that where set +# to `null`. +# Related to ticket: https://github.com/open-formulieren/open-forms/issues/5104 +from __future__ import annotations + +import sys +from pathlib import Path + +import django + +SRC_DIR = Path(__file__).parent.parent / "src" +sys.path.insert(0, str(SRC_DIR.resolve())) + + +def fix_radio_component_default_values(): + from openforms.forms.models import FormDefinition + + form_definitions_to_update = [] + fds = FormDefinition.objects.iterator() + for form_definition in fds: + # Flag to update Form Definition. Is set when child component is changed + should_persist_fd = False + + for component in form_definition.iter_components(): + # The fix is only needed for radio components whose defaultValue is `None` + if component["type"] != "radio" or component["defaultValue"] is not None: + continue + + component["defaultValue"] = "" + # Set flag to update Form Definition + should_persist_fd = True + + if should_persist_fd: + form_definitions_to_update.append(form_definition) + + FormDefinition.objects.bulk_update( + form_definitions_to_update, fields=["configuration"] + ) + + +def main(**kwargs): + from openforms.setup import setup_env + + setup_env() + django.setup() + + return fix_radio_component_default_values(**kwargs) + + +if __name__ == "__main__": + main()