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

Refactor form step validation as required field #4243

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 28, 2020

Why: Reduces responsibility of step where "is required" is the only validation currently in use, and allows validation of conditionally-registered fields.

Specifically, this would be needed for the use-case described at #4237 (comment), where in the review screen introduced in #4237, the "Selfie" field is required, but only if liveness checking is enabled for the service provider. The current step validation behavior has no direct awareness of whether liveness is enabled.

Alternatives considered:

  • It could be possible to pre-bind a validation function with an argument controlling whether selfie should be checked.
const reviewStep = { validate: validateReviewStep.bind(null, isLivenessEnabled);
// ...
function validate(isLivenessEnabled, values) {
  const valuesToCheck = isLivenessEnabled ? ['front', 'back', 'selfie'] : ['front', 'back'];
  // ...
}

Note that some of the changes here assume or otherwise depend on changes from #4237 (e.g. initialStep -> autoFocus).

**Why:** With interdepedent functions, or when using React hooks which are subject to Rules of Hooks, not always possible to define function prior to referenced usage.
**Why**: Reduces responsibility of step where current validation is only required, allows validation of conditionally-registered fields.
@aduth
Copy link
Member Author

aduth commented Sep 28, 2020

Note that this also removes logic in FormSteps which would feasibly have allowed a user to pick up from where they left off after a reload. We don't currently take advantage of this, and it's not clear that we would care to. There is a low-priority ticket LG-3335 which would aim to allow this, though it may be very incompatible with other work with async uploads and had been contemplated as being cancelled altogether.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

.eslintrc Outdated Show resolved Hide resolved
@aduth aduth merged commit 31f4977 into master Sep 29, 2020
@aduth aduth deleted the aduth-validate-at-register branch September 29, 2020 14:55
@aduth
Copy link
Member Author

aduth commented Sep 29, 2020

Possible regression from these changes: I've noticed in the mobile flow that immediately after selecting a "Front of your ID" image, the "Back of your ID" field is flagged as "This field is required". The error should only be shown if the user attempts to continue to the next step before providing a value.

Comment on lines 134 to +137
// Errors are assigned at the first attempt to submit. Once errors are assigned, track value
// changes to remove validation errors as they become resolved.
if (activeErrors) {
const nextActiveErrors = getValidationErrors(effectiveStep, values);
const nextActiveErrors = getValidationErrors();
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue described at #4243 (comment) is related to this, where in toNextStep we will always assign setActiveErrors(getValidationErrors()). In the mobile flow this happens when continuing from the first "Mobile Intro" step. There are no actual errors, but it still sets an empty array, which is considered truthy by if (activeErrors) {. The idea with this code was to try to "chip away at" any active errors.

Possible fixes:

  • Change if (activeErrors) { to if (activeErrors?.length)
  • Guard setActiveErrors in toNextStep to only set active errors if non-empty
  • Return undefined from getValidationErrors if result would be empty

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

Successfully merging this pull request may close these issues.

2 participants