Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TreeDropdownField fails to validate after clearing value in react forms #1576

Open
blueo opened this issue Sep 6, 2023 · 6 comments
Open

Comments

@blueo
Copy link
Contributor

blueo commented Sep 6, 2023

When using a Treedropdownfield in a react form (eg the Linkfield Module SiteTreeLink) the field will pass a 'required' validation if the field is cleared - even though this should reset the field to an 'empty' state.

Steps to reproduce:

  1. open a FormBuilder Form with a required TreeDropdownField - eg the Link Field module silverstripe/linkfield
  2. Try submitting the form without any changes - observe the field will be correctly marked as 'required'
  3. Add a value to the TreeDropdownField
  4. Clear the TreeDropdownField using the 'x' symbol
  5. submit the form - observe that even though the field is 'empty' it will now pass validation

Investigation
it seems that when clearing the field it gets set to a value of 0 https://github.com/silverstripe/silverstripe-admin/blob/2/client/src/components/TreeDropdownField/TreeDropdownField.js#L425 which at some point gets converted to a string.

This means that when Validator.js checks the value of the field it sees '0' and this passes the 'required' validation rule as it is a non-empty string:
image
by contrast, when the field loads - the value is ""
image

Seems we should find another way to clear the field to an empty string instead of 0.
If I update SINGLE_EMPTY_VALUE to equal "" then the validation works as expected

this was tested on v 4.13.1 of the admin module - unsure if this is still a problem in v5 at this stage.

Acceptance criteria

  • A common empty value is defined. (probably an empty string)
  • The default value when instantiating the field is the common empty value.
  • When you clear a pre-existing value, the field is reset the the common empty value.

PR

@GuySartorelli
Copy link
Member

this was tested on v 4.13.1 of the admin module - unsure if this is still a problem in v5 at this stage

Just to clarify, you have a link to code in the 2 branch, which is specifically CMS 5 code. Can you please update your link if it's incorrect, or else update the description to indicate that both CMS 4 and 5 are affected, if both are affected?

@GuySartorelli
Copy link
Member

I don't want to test this with linkfield as that's not a supported module and I'm not familiar with its code.

open a FormBuilder Form with a required TreeDropdownField

Can you please provide me with reproduction instructions for setting this up without using linkfield? Could be in asset-admin or possible elemental inline-editable block if that uses the formbuilder.

@GuySartorelli GuySartorelli changed the title TreeDropdownField fails to validate after clearing value TreeDropdownField fails to validate after clearing value in react forms Sep 6, 2023
@sabina-talipova sabina-talipova self-assigned this Sep 7, 2023
@sabina-talipova
Copy link
Contributor

I confirm, this is the issue in CMS4 and CMS5 as well.
Replacing SINGLE_EMPTY_VALUE = 0 to SINGLE_EMPTY_VALUE = '' fix this issue and TreeDropdownField fails validation if it's required and empty.

@blueo
Copy link
Contributor Author

blueo commented Sep 7, 2023

ah great thanks @sabina-talipova I was having some trouble getting it going in another place!

@mfendeksilverstripe
Copy link
Contributor

Relates to silverstripe/silverstripe-elemental#329

@maxime-rainville
Copy link
Contributor

We spent half an hour arguing about what the proper empty value should be and coludn't reach a sensible decision. We'll create a SPIKE to review the option and wider implications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants