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.

Sign up for GitHub

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

Jump to bottom

Iterate validation bypass #4046

Closed
andlbrei opened this issue Oct 2, 2024 · 13 comments · Fixed by #4087
Closed

Iterate validation bypass #4046

andlbrei opened this issue Oct 2, 2024 · 13 comments · Fixed by #4087
Labels
bug Something isn't working released

Comments

@andlbrei
Copy link
Contributor

andlbrei commented Oct 2, 2024

🐛 Bug Report

When trying to add an invalid object to an iterate array, the object seems to be added while invalid. It seems to go from PushContainer to EditContainer.
Then, after typing something, the ViewContainer becomes active even though the object is invalid.

Screen.Recording.2024-10-02.at.15.24.32.mov

To Reproduce

Steps to reproduce the behavior:
https://codesandbox.io/p/sandbox/iterate-validation-dwlkl3

Expected behavior

I expect validation error messages to be displayed when trying to add an invalid object to an iterate array.
I expect the object not to be added when invalid after typing in one of the fields.

Eufemia Version

10.51.1
@andlbrei andlbrei added the bug Something isn't working label Oct 2, 2024
@tujoworker
Copy link
Member

The fields inside MyNewItemForm do not have required. Is that on purpose?

@tujoworker
Copy link
Member

The Iterate.PushContainer does actually not support a schema as of today.

@tujoworker
Copy link
Member

tujoworker commented Oct 6, 2024 •
edited
Loading

btw. the CSB uses a different version, not the latest. But that does not effect showing the issue.

Now, in v10.51.1 the default data from the PushContainer was null, which is falsy, and therefore the EditContainer opens:

Screenshot 2024-10-06 at 21 39 36

In the next version, we needed to change it from null to an empty object {} (with this PR). So there it will go to a ViewContainer.

Screenshot 2024-10-06 at 21 43 41

@andlbrei
Copy link
Contributor Author

andlbrei commented Oct 7, 2024

Yea, I noticed that it acted differently when I tried it on the defaultValue PR branch build from last week.

So the issue here is that PushContainer is not using schema for validation, but EditContainer does?
The rest will be solved in the next release?

@tujoworker
Copy link
Member

Correct. But we can add support for schema to the PushContainer (Form.Isolation does support it), but then you would need to align your schema to the additional path /pushContainerItems/0 (previously "/newItems"). Is that of interest?

@andlbrei
Copy link
Contributor Author

andlbrei commented Oct 7, 2024

Hmm, ok.
That feels like internal logic that the consumer should not know about, but it's understandable with how the PushContainer is implemented with that kind of temporary path.
A bit unfortunate, but I guess it's not a big deal that the user goes from push-mode to edit-mode and then receives validation error messages.

@tujoworker tujoworker mentioned this issue Oct 8, 2024
Merged
@tujoworker
Copy link
Member

With this PR #4087, the EditContainer stays open during editing when it got force opened.

It also opens the EditContainer with an empty object. Does that make sense?

@tujoworker
Copy link
Member

but it's understandable with how the PushContainer is implemented with that kind of temporary path.

Agree, its not ideal for a schema. Do you have any thoughts about how we could solve this?

Related to this subject, this PR #4076 did introduce a new prop isolatedData on the PushContainer, so its possible to put data for fields to consume with an ordinary path.

Here is a working example (simplified) on how this can be utilized.

@andlbrei
Copy link
Contributor Author

andlbrei commented Oct 9, 2024

With this PR #4087, the EditContainer stays open during editing when it got force opened.

It also opens the EditContainer with an empty object. Does that make sense?

I think that makes sense, yes. I'll try it out.

Related to this subject, this PR #4076 did introduce a new prop >isolatedData on the PushContainer, so its possible to put data for fields to consume with an ordinary path.

That looks interesting. I will try and change our implementation to use it, and let you know.

@andlbrei
Copy link
Contributor Author

andlbrei commented Oct 9, 2024

I tried both, and it seems to work fine 👍
Regarding #4087 it might be nice to get validation messages when it adds the invalid object, but I don't think it's a big deal. I hope most users actually add something to those fields before adding... 😅

I have some thoughts on #4076 .
The way I did it before was obviously a hack, and kind of messy.
The new way of doing it is definitely better, but might not be very intuitive.
I think what is a bit complicated is the fact that we are using a path in the Selection component to handle the selection of persons, which implies that we have a field in the form that will end up storing some data in the form.
It doesn't happen of course, since we are in the Iterate context here, which was why the hack worked.
But it's a bit confusing, or at least not intuitive.
I have no idea what to do about it, but if you happen to have an idea on some kind of "integrated form value that is not actually stored in the form data" concept, then it might be interesting to rename/rework the way this logic works so we don't use the path keyword for something that is not supposed to be stored.
This might relate to #4072 and buttons that needs to be pressed and stored, or even confirmation checkboxes ("I confirm that I have understood bla bla") where Eufemia Forms might need to know that the user has acted on these required actions, but not store the values in the form.

@tujoworker
Copy link
Member

I think, its fine to put control/management paths into the data. We have several ways to filter them out, if needed. Can that work for you?

@andlbrei
Copy link
Contributor Author

andlbrei commented Oct 9, 2024

I'd be interested to see. I can always filter them out before sending it to the backend, so it's not a big problem either way.
I'm thinking more about how intuitive it is to use the correct pieces of functionality to solve a problem. But I guess that some things are just complicated, and we need to look at examples to see how to do it regardless with the "tools" that are available.

tujoworker pushed a commit that referenced this issue Oct 11, 2024
@semantic-release-bot
chore(release): 10.52.0 [skip ci]
7789277 
## [10.52.0](v10.51.2...v10.52.0) (2024-10-11)

### 📝 Documentation

* **NationalIdentityNumber:** add docs about validation ([#4077](#4077)) ([51bfd80](51bfd80))
* **OrganizationNumber:** adds docs about validation ([#4078](#4078)) ([c785c51](c785c51))

### 🐛 Bug Fixes

* **Autocomplete:** Show whole suggestion list after item selection ([#4060](#4060)) ([0acf061](0acf061))
* **Field.NationalIdentityNumber:** validate on all digits(not only 11 digits) ([#4079](#4079)) ([7c34fc9](7c34fc9))
* **Field.OrganizationNumber:** should validate on all digits(not only when 9) ([#4071](#4071)) ([08a4b51](08a4b51))
* **Forms:** add support for `Form.SubmitConfirmation` inside Wizard ([#4088](#4088)) ([e1167a4](e1167a4)), closes [#4086](#4086)
* **Forms:** don't call internal `exportValidators` when they not are exported as an array ([#4113](#4113)) ([cd54ed0](cd54ed0)), closes [#4106](#4106)
* **Forms:** ensure `emptyValue` is set in the data context when defined ([#4111](#4111)) ([dcc5694](dcc5694)), closes [#4070](#4070)
* **Forms:** ensure `onBlurValidator` gets called when `validateInitially` is true ([#4069](#4069)) ([59cf6c5](59cf6c5)), closes [#4068](#4068) [#4066](#4066)
* **Forms:** ensure Field.SelectCountry has a fallback locale (nb-NO) ([#4114](#4114)) ([568229a](568229a))
* **Forms:** ensure Form.clearData works in React.StrictMode ([#4053](#4053)) ([da0c93a](da0c93a)), closes [#4048](#4048)
* **Forms:** Fix use of unpolyfilled structuredClone in useData hook ([#4108](#4108)) ([1f59f10](1f59f10))
* **Forms:** keep `Iterate.EditContainer` open when falsy value or empty object was given as the iterate value ([#4087](#4087)) ([e932059](e932059)), closes [#4046](#4046)
* **Forms:** show error on every value change when using exported validators ([#4068](#4068)) ([cab6d01](cab6d01)), closes [#4067](#4067)
* **Icon:** icon property typing should accept FormStatus icons ([#4091](#4091)) ([f49eb34](f49eb34))
* **NationalIdentityNumber:** use `onBlurValidator` instead of `validator` ([#3982](#3982)) ([0a93755](0a93755))
* **Typography:** match medium heading font size in Sbanken theme ([#4039](#4039)) ([ce50529](ce50529))

### ✨ Features

* **Forms:** add `bubbleValidation` to Form.Isolation and Iterate.PushContainer to prevent the form from being submitted when there are fields with errors ([#4103](#4103)) ([880f870](880f870)), closes [#4072](#4072)
* **Forms:** add `createMinimumAgeValidator` to Field.NationalIdentityNumber make a customizable `adultValidator` ([#4057](#4057)) ([6c20ba2](6c20ba2))
* **Forms:** add `Form.useSnapshot` hook to handle snapshots of data ([#4102](#4102)) ([d451793](d451793))
* **Forms:** add `isolatedData` prop to `Iterate.PushContainer` ([#4076](#4076)) ([ede5f8e](ede5f8e))
* **Forms:** add `isValid` to Form.Visibility for showing content based on the validation of a field ([#4038](#4038)) ([7536752](7536752))
* **Forms:** add `Iterate.ItemNo` component ([#4095](#4095)) ([c736c9e](c736c9e))
* **Forms:** add `transformLabel` to all Value.* component ([#4056](#4056)) ([d63e472](d63e472))
* **Forms:** add support for `defaultValue` (and `value`) for fields used in `Iterate.Array` ([#3987](#3987)) ([afbdddf](afbdddf)), closes [#3882](#3882)
* **Forms:** add support for `id` for when using dynamic steps with `activeWhen` ([#4093](#4093)) ([248da92](248da92))
* **Forms:** remove internal pattern from `Field.NationalIdentityNumber` in favor of the internal validator ([#4098](#4098)) ([fb35722](fb35722))
* **Forms:** remove internal pattern from `Field.OrganizationNumber` in favor of the internal validator ([#4092](#4092)) ([e829f8b](e829f8b)), closes [#4073](#4073)
* **Skeleton:** stop animation after 30 seconds ([#3479](#3479)) ([f67b3bb](f67b3bb))
* **Value.PhoneNumber and NumberFormat:** displays country code using prefix `+` instead of `00` ([#4051](#4051)) ([fb363d0](fb363d0))
@tujoworker
Copy link
Member

🎉 This issue has been resolved in version 10.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working released
Milestone
No milestone
2 participants
@tujoworker @andlbrei