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

FIO-9072: required validation triggered on form load #5846

Closed
wants to merge 6 commits into from
Closed

FIO-9072: required validation triggered on form load #5846

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e6bff5
fix validations triggered on form load
ZenMasterJacob20011 Oct 5, 2024
ae658f9
added back nyc
ZenMasterJacob20011 Oct 5, 2024
7565f58
changed from explicit submission to submission
ZenMasterJacob20011 Oct 5, 2024
796fec1
remove from submission in editgrid
ZenMasterJacob20011 Oct 6, 2024
7f5a910
fix merge conflict
ZenMasterJacob20011 Oct 8, 2024
9041a67
remove unnecessary line of code
ZenMasterJacob20011 Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/Webform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,7 @@ export default class Webform extends NestedDataComponent {
*/
onSetSubmission(submission, flags = {}) {
this.submissionSet = true;
flags.submission = structuredClone(submission);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.triggerChange(flags);
this.emit('beforeSetSubmission', submission);
this.setValue(submission, flags);
Expand Down Expand Up @@ -952,7 +953,6 @@ export default class Webform extends NestedDataComponent {
if (!flags.sanitize) {
this.mergeData(this.data, submission.data);
}

submission.data = this.data;
this._submission = submission;
return changed;
Expand Down Expand Up @@ -1433,14 +1433,14 @@ export default class Webform extends NestedDataComponent {
}

this.checkData(value.data, flags);
const shouldValidate =
!flags.noValidate ||
let shouldValidate = flags.noValidate ? false :
_.isUndefined(flags.noValidate) ||
flags.fromIframe ||
(flags.fromSubmission && this.rootPristine && this.pristine && flags.changed);
(flags.fromSubmission && this.rootPristine && this.pristine);
const errors = shouldValidate
? this.validate(value.data, {
...flags,
noValidate: false,
? this.validate(value.data, {
...flags,
noValidate: false,
process: 'change'
})
: [];
Expand Down
12 changes: 6 additions & 6 deletions src/components/_classes/component/Component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2810,7 +2810,6 @@
return;
}
_.set(this._data, this.key, value);
return;
}

/**
Expand Down Expand Up @@ -2969,7 +2968,7 @@

/**
* Returns if the value (e.g. array) should be divided between several inputs
* @returns {boolean}

Check warning on line 2971 in src/components/_classes/component/Component.js

View workflow job for this annotation

GitHub Actions / setup 

Missing JSDoc @returns description
*/
isSingleInputValue() {
return false;
Expand Down Expand Up @@ -3388,14 +3387,15 @@
if (flags.silentCheck) {
return [];
}
let dirty = this.dirty || flags.dirty
if (this.options.alwaysDirty) {
flags.dirty = true;
dirty = true;
}
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_.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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dirty = this.pristine && this.component.protected ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. It looks like I accidentally added this when trying to resolve merge conflicts. I will remove it

}
this.setDirty(flags.dirty);
return this.setComponentValidity(errors, flags.dirty, flags.silentCheck, flags.fromSubmission);
this.setDirty(dirty);
return this.setComponentValidity(errors, dirty, flags.silentCheck, flags.fromSubmission);
}

/**
Expand Down
14 changes: 8 additions & 6 deletions src/components/_classes/nested/NestedComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,22 +927,24 @@ export default class NestedComponent extends Field {

setNestedValue(component, value, flags = {}) {
component._data = this.componentContext(component);
let componentFlags = {...flags};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (component.type === 'button') {
return false;
}
if (component.type === 'components') {
if (component.tree && component.hasValue(value)) {
return component.setValue(_.get(value, component.key), flags);
return component.setValue(_.get(value, component.key), componentFlags);
}
return component.setValue(value, flags);
return component.setValue(value, componentFlags);
}
else if (value && component.hasValue(value)) {
return component.setValue(_.get(value, component.key), flags);
return component.setValue(_.get(value, component.key), componentFlags);
}
else if ((!this.rootPristine || component.visible) && component.shouldAddDefaultValue) {
flags.noValidate = !flags.dirty;
flags.resetValue = true;
return component.setValue(component.defaultValue, flags);
componentFlags.noValidate = !componentFlags.dirty;
componentFlags.resetValue = true;
return component.setValue(component.defaultValue, componentFlags);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/components/editgrid/EditGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,6 +1162,7 @@ export default class EditGridComponent extends NestedArrayComponent {
editRow.state === EditRowState.New ||
editRow.state === EditRowState.Editing ||
editRow.alerts ||
this.dirty ||
dirty;
}

Expand Down
6 changes: 3 additions & 3 deletions test/unit/EditGrid.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,12 +715,12 @@ describe('EditGrid Component', () => {
assert(form.submitted, 'Form should be submitted');
const editRow = editGrid.editRows[0];
assert(editRow.alerts, 'Should add an error alert to the modal');
assert.equal(editRow.errors.length, 2, 'Should add errors to components inside draft row aftre it was submitted');
assert.equal(editRow.errors.length, 2, 'Should add errors to components inside draft row after it was submitted');
const textField = editRow.components[0].getComponent('textField');

const alert = editGrid.alert;
assert(alert, 'Should show an error alert when drafts are enabled and form is submitted');
assert(textField.element.className.includes('has-error'), 'Should add error class to component even when drafts enabled if the form was submitted');
assert(textField.element.className.includes('error'), 'Should add error class to component even when drafts enabled if the form was submitted');

// 4. Change the value of the text field
textField.setValue('new value', { modified: true });
Expand All @@ -736,7 +736,7 @@ describe('EditGrid Component', () => {

setTimeout(() => {
assert.equal(textField2.dataValue, 'test val');
assert(!textField2.element.className.includes('has-error'), 'Should remove an error class from component when it was fixed');
assert(!textField2.element.className.includes('error'), 'Should remove an error class from component when it was fixed');

editGrid.saveRow(0);

Expand Down
92 changes: 89 additions & 3 deletions test/unit/Webform.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2150,10 +2150,10 @@ describe('Webform tests', function() {
// });
// });

it('Should not fire validation on init.', (done) => {
it('Should not show validation on init.', () => {
formElement.innerHTML = '';
const form = new Webform(formElement,{ language: 'en' });
form.setForm(
return form.setForm(
{ title: 'noValidation flag',
components: [{
label: 'Number',
Expand All @@ -2175,7 +2175,8 @@ describe('Webform tests', function() {
input: true
}],
}).then(() => {
checkForErrors(form, {}, {}, 0, done);
assert.equal(form.errors.length, 2);
assert.equal(form.visibleErrors.length, 0);
});
});

Expand Down Expand Up @@ -2339,6 +2340,91 @@ describe('Webform tests', function() {
});
});

it('Should only show errors on components that have their data explicitly set', (done) => {
formElement.innerHTML = '';
const form = new Webform(formElement,{ language: 'en' });
form.setForm(
{
components: [
{
label: 'Number',
validate: {
required: true,
min: 5
},
key: 'number',
type: 'number',
input: true
}, {
label: 'Text Area',
validate: {
required: true,
minLength: 10
},
key: 'textArea',
type: 'textarea',
input: true
}],
}
).then(() => {
form.submission = {
data: {
number: 2,
}
}
setTimeout(()=>{
const textAreaComponent = form.getComponent('textArea');
const numberComponent = form.getComponent('number');
assert.equal(textAreaComponent.errors.length, 1);
assert.equal(textAreaComponent.visibleErrors.length, 0);
assert.equal(numberComponent.errors.length, 1);
assert.equal(numberComponent.visibleErrors.length, 1);
done();
},200);
});
});

it('Should not show errors of sibling components if the previous component errors and the submission in not explicitly set', (done) => {
formElement.innerHTML = '';
const form = new Webform(document.createElement('div'), {language: 'en'});
form.setForm({
components: [
{
type: 'textarea',
key: 'textArea',
label: 'textArea',
validate: {
required: true
}
},
{
type: 'textarea',
key: 'textArea1',
label: 'textArea1',
validate: {
required: true
}
}
]
}).then(() => {
form.setSubmission({
data: {
textArea: ''
}
}).then(() => {
setTimeout(()=>{
const textAreaComponent = form.getComponent('textArea');
const textAreaComponent1 = form.getComponent('textArea1');
assert.equal(textAreaComponent.errors.length, 1);
assert.equal(textAreaComponent.visibleErrors.length, 1);
assert.equal(textAreaComponent1.errors.length, 1);
assert.equal(textAreaComponent1.visibleErrors.length, 0);
done();
},200);
})
})
});

it('Should not show errors on setSubmission with noValidate:TRUE', (done) => {
formElement.innerHTML = '';
const form = new Webform(formElement,{ language: 'en' });
Expand Down
8 changes: 4 additions & 4 deletions test/unit/Wizard.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ describe('Wizard tests', () => {
};

clickWizardBtn('link[4]');

setTimeout(() => {
checkPage(4);
clickWizardBtn('submit');
Expand All @@ -664,7 +664,7 @@ describe('Wizard tests', () => {
})
.catch((err) => done(err));
});


it('Should execute advanced logic for wizard pages', function(done) {
const formElement = document.createElement('div');
Expand Down Expand Up @@ -1178,15 +1178,15 @@ describe('Wizard tests', () => {

assert.equal(wizard.visibleErrors.length, 3, 'Should have page validation error');
assert.equal(wizard.refs.errorRef.length, 3, 'Should keep alert with validation errors');
checkInvalidComp('textField');
checkInvalidComp('textField', true);
clickWizardBtn('errorRef[1]', true);

setTimeout(() => {
checkPage(1);

assert.equal(wizard.visibleErrors.length, 3, 'Should have page validation error');
assert.equal(wizard.refs.errorRef.length, 3, 'Should keep alert with validation errors');
checkInvalidComp('checkbox');
checkInvalidComp('checkbox', true);
wizard.getComponent('checkbox').setValue(true);

setTimeout(() => {
Expand Down
Loading