open-formulieren / open-forms

Smart and dynamic forms
https://open-forms.readthedocs.io
Other
37 stars 26 forks source link

Hidden components with "Multiple values" trigger validation #4659

Closed LaurensBurger closed 1 month ago

LaurensBurger commented 2 months ago

Product versie / Product version

latest

Customer reference

no particular but it get's reported

Omschrijf het probleem / Describe the bug

Hidden component with "Multiple values" set get their defaultValue changed from:

  "defaultValue": null,

to:

  "defaultValue": [
    null
  ],

This causes validation to trigger right after loading since their value is now [] instead of [ null ]

  "telefoonnummer": []

This can be resolved by changing the defaultValue to via the json of the components:

  "defaultValue": [],
joeribekker commented 2 months ago

Refinement: Task estimated here is changing the default value to an array ([]) without the null.

Robin can do this in an hour. So, let's timebox this to 2 hours. After that, the proper estimate is: (whatever Robin thinks)

robinmolen commented 2 months ago

I cannot reproduce this issue. I've followed these steps:

robinmolen commented 2 months ago

I've made a PR that ensures that textfields always have a valid value (so null will be turned into '').

When a textfield (with the default value null) gets the multiple values property, the default value will change to [''].

sergei-maertens commented 1 month ago

Looking at https://github.com/open-formulieren/formio-builder/pull/184 I fail to see how this relates to the reported bug. Combined with the comment saying it can't be reproduced, I suspect we're looking in the wrong place.

For any fix to be attempted, the issue must first be reproduced, then a failing regression test needs to be provided and finally a patch. The failing regression test now passing then proves the patch works, the regression test proves the problem is understood and will be prevented in the future.

sergei-maertens commented 1 month ago

This seems to be some sort of regression of #2213 - that one was limited to copying, but here it seems to apply even for new text fields dragged into the editor.

When fixing this, make sure to check all related issues (and issues related to #3128) for possible regressions.

sergei-maertens commented 1 month ago

I quickly checked email component, and it has the same issue - defaultValue is null.

sergei-maertens commented 1 month ago

Discussed in the office:

  1. Let's write an e2e regression test with playwright, for reference: https://github.com/open-formulieren/open-forms/blob/master/src/openforms/forms/tests/e2e_tests/test_form_designer.py
  2. the test should: drag and drop the textfield (and other affected types) component, click the save button (without additional changes), and save the form definition
  3. then query the created FD and assert that the component defaultValue is equal to the empty string (as opposed to None)
  4. for the patch, you can probably apply the same fixup like the cosign.js, see 1e8d6af3cc
  5. we'll need a data migration to update existing components - CTRL + F on ConvertComponentsOperation for examples