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

Fixes #32667 - add job wizard form validation #622

Merged
merged 1 commit into from Oct 19, 2021
Merged

Fixes #32667 - add job wizard form validation #622

merged 1 commit into from Oct 19, 2021

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Jul 20, 2021 •
edited

Depends on: #556
Doesn't include schedule validation yet

@MariaAga MariaAga force-pushed the job-wizard-fields-validation branch from 426cb76 to 8d9fe27 Compare July 28, 2021 16:17
@MariaAga MariaAga changed the title Fixed #32667 - add job wizard form validation Fixes #32667 - add job wizard form validation Jul 28, 2021
@MariaAga MariaAga marked this pull request as ready for review September 1, 2021 12:53
@MariaAga
Copy link
Member Author

MariaAga commented Sep 1, 2021

@LaViro can you take a look please?

Ron-Lavi
Ron-Lavi reviewed Sep 13, 2021
View reviewed changes
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga !

webpack/JobWizard/steps/CategoryAndTemplate/index.js
setJobTemplate(
Number(filterJobTemplates(response?.data?.results)[0]?.id) || null
),
);
},
Copy link
Member

Choose a reason for hiding this comment

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

just curious, did it change something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, I think it was just "more correct" for it not to return the value

webpack/JobWizard/steps/form/FormHelpers.js Outdated
@@ -18,3 +18,7 @@ export const helpLabel = (text, id) => {
</Popover>
);
};

export const isPostiveNumber = number => /^\d+$/.test(number);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const isPostiveNumber = number => /^\d+$/.test(number);
export const isPositiveNumber = number => /^\d+$/.test(number);

also, can't we use parseInt(textWithNumber) > 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, also changed to the suggestion

webpack/JobWizard/validation.js Show resolved Hide resolved
webpack/JobWizard/validation.js
Comment on lines +33 to +37
inputValidation(templateInputs, templateValues, () =>
setValid(currValid => ({ ...currValid, hostsAndInputs: false }))
);

inputValidation(advancedTemplateInputs, advancedValues.templateValues, () =>
Copy link
Member

Choose a reason for hiding this comment

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

I see that we run twice the inputValidation,
can we combine advancedValues & templateValues and then run inputValidation once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why should we combine them? They just run forEach on the input so there is no duplicate code running. also they set different values on invalid

webpack/JobWizard/JobWizard.js Show resolved Hide resolved
webpack/JobWizard/steps/Schedule/StartEndDates.js Show resolved Hide resolved
@MariaAga MariaAga force-pushed the job-wizard-fields-validation branch 2 times, most recently from d789959 to a35dc20 Compare September 13, 2021 15:37
Ron-Lavi
Ron-Lavi reviewed Sep 14, 2021
View reviewed changes
webpack/JobWizard/steps/form/FormHelpers.js Outdated Show resolved Hide resolved
webpack/JobWizard/steps/Schedule/StartEndDates.js
else setValidEnd(false);
}, [starts, ends, isNeverEnds]);
useEffect(() => {
setValid(validEnd);
Copy link
Member

Choose a reason for hiding this comment

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

if eventually we update setValid with validEnd, do we really need to manage a state for validEnd? or can we just use setValid ?

webpack/JobWizard/JobWizard.js
@@ -116,23 +126,32 @@ export const JobWizard = () => {
jobTemplateID={jobTemplateID}
/>
),
canJumpTo: isTemplate,
canJumpTo: isTemplate && valid.hostsAndInputs,
enableNext: isTemplate && valid.hostsAndInputs && valid.advanced,
Copy link
Member

Choose a reason for hiding this comment

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

If the user got here, it probably means that valid.hostsAndInputs is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

But a user can go a step back and enter invalid values

webpack/JobWizard/JobWizard.js
canJumpTo:
isTemplate && valid.hostsAndInputs && valid.advanced && valid.schedule,
enableNext:
isTemplate && valid.hostsAndInputs && valid.advanced && valid.schedule,
Copy link
Member

Choose a reason for hiding this comment

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

same here, I think that for clicking next / run we can check just for the validity of the step itself

webpack/JobWizard/validation.js
Comment on lines +50 to +51
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [advancedValues, templateValues]);
Copy link
Member

Choose a reason for hiding this comment

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

should it be:

Suggested change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [advancedValues, templateValues]);
}, [advancedValues, templateValues, advancedTemplateInputs, templateInputs]);

Copy link
Member Author

Choose a reason for hiding this comment

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

On initial load this causes the useEffect to be called 23 times instead of 3.
There is no need to re check validation if the values didn't change as we are validating the values.

Copy link
Member

Choose a reason for hiding this comment

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

since the data is being based on advancedTemplateInputs & templateInputs if they change I guess re-validation should happen as well. does it happen because they are objects? maybe we need to useMemo for them? I think @amirfefer faced a similar issue with the API hook in foreman.

Copy link
Member Author

Choose a reason for hiding this comment

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

They only change when the a new template is fetched, and when that happens advancedValues is changed every time when setting defaults.

Ron-Lavi
Ron-Lavi approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@Ron-Lavi Ron-Lavi left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga, everything seems great, can you just rebase?

@MariaAga
Copy link
Member Author

Thanks @LaViro, rebased!

@Ron-Lavi
Copy link
Member

@adamruzicka can you take a look?

adamruzicka
adamruzicka approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Looks fine

@adamruzicka adamruzicka merged commit b57f364 into theforeman:master Oct 19, 2021
@adamruzicka
Copy link
Contributor

Thank you @MariaAga & @LaViro !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamruzicka adamruzicka adamruzicka approved these changes

@Ron-Lavi Ron-Lavi Ron-Lavi approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
4 participants
@MariaAga @Ron-Lavi @adamruzicka @theforeman-bot