FIO-9072: required validation triggered on form load #5846
Conversation
|
The changes seem logical to me. But I feel like they're a little risky. I want someone else to review them. |
| if (flags.fromSubmission && this.hasValue(data)) { | ||
| flags.dirty = this.pristine && this.component.protected ? false : true; | ||
| if (flags.fromSubmission && !_.isUndefined(_.get(flags?.submission?.data, this.key)) && !(this.pristine && this.protected)) { | ||
| dirty = this.pristine && this.component.protected ? false : true; |
There was a problem hiding this comment.
It seems to me that this.protected is always undefined. I was not able to find a place where it is set to true/false.
Why do we need the check for 'protected' and 'pristine' in both line 3395 and line 3394?
There was a problem hiding this comment.
I am not sure. It looks like I accidentally added this when trying to resolve merge conflicts. I will remove it
| @@ -795,6 +795,7 @@ export default class Webform extends NestedDataComponent { | |||
| */ | |||
| onSetSubmission(submission, flags = {}) { | |||
| this.submissionSet = true; | |||
| flags.submission = structuredClone(submission); | |||
There was a problem hiding this comment.
Why is this necessary? I don't like the idea of creating a clone of the submission object here so it can be passed along to the setValue method. Is this absolutely necessary?
There was a problem hiding this comment.
This is so that the explicit submission can be used to determine whether or not to show validation. I could not use this.data or this.submission because these are filled with this.empty value. e.g. button components get false, textfield components get and empty string. I need to know the exact submission at the time it was inputted to determine if it was given an empty value or if it was not even a property in the submission json.
| } | ||
| if (flags.fromSubmission && this.hasValue(data)) { | ||
| flags.dirty = this.pristine && this.component.protected ? false : true; | ||
| if (flags.fromSubmission && !_.isUndefined(_.get(flags?.submission?.data, this.key)) && !(this.pristine && this.protected)) { |
There was a problem hiding this comment.
_.get(submission.data, this.key) not always the way to fetch the correct data for a component. Particularly the Checkbox components configured as Radio components would fail here. Why can we not use "this.hasValue"? It looks like you can pass data to it... maybe change this to this.hasValue(flags?.submission.data)?
There was a problem hiding this comment.
hasValue(data) {
return !.isUndefined(.get(data || this.data, this.key));
}
hasValue will fallback on this.data. I do not want it to fall back on this.data because I want the explicit submission to set if the dirty flag is true. this.data will be populated with this.empty values in the init function of setting up the form. I need absolutely sure that the property was set by whoever defined the submission object in setSubmission(...). This is why I added the submission to the flags object when a submission is set.
| @@ -927,22 +927,24 @@ export default class NestedComponent extends Field { | |||
|
|
|||
| setNestedValue(component, value, flags = {}) { | |||
| component._data = this.componentContext(component); | |||
| let componentFlags = {...flags}; | |||
There was a problem hiding this comment.
Is this necessary anymore now that we are no longer mutating the "dirty" property? I would prefer to not have to clone if we don't need to.
Link to Jira Ticket
https://formio.atlassian.net/browse/FIO-9072
Description
Webform.js
Component.js
NestedComponent.js
Editgrid.js
Why have you chosen this solution?
This solution is probably the closest to what the best solution would be. Unfortunately it is not a small change to make it so that the required validation is not triggered on form init. There is a lot of assumptions made around the dirty flag and it seems as though we were just getting lucky that this hasn't appeared before. A lot of the changes made here fix these assumptions.
Breaking Changes / Backwards Compatibility
N/A
Dependencies
N/A
How has this PR been tested?
Added automated tests and manually tested. Also changed some tests which I will discuss in dev support
Checklist: