Skip to content

LG-6160: Add personal key validation behavior#6222

Merged
aduth merged 24 commits intomainfrom
aduth-custom-validate-form-step
Apr 21, 2022
Merged

LG-6160: Add personal key validation behavior#6222
aduth merged 24 commits intomainfrom
aduth-custom-validate-form-step

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 19, 2022

Why: As a user, I expect feedback if I have incorrectly entered the personal key, so that I can correct my mistakes and continue with the proofing flow.

Testing Instructions:

  1. Set idv_api_enabled: "true" in local config/application.yml
  2. Go to: http://localhost:3000/verify/v2/personal_key_confirm
  3. Click "Submit"
  4. Observe error "You've entered an incorrect personal key", and that focus is returned to the field
  5. Enter some text
  6. Observe that the error is removed after input, consistent with other IdP inputs
  7. Enter a correctly-formatted, but incorrect personal key (e.g. "0000-0000-0000-0001")
  8. Click "Submit"
  9. Observe error "You've entered an incorrect personal key", and that focus is returned to the field
  10. Enter the correct personal key "0000-0000-0000-0000"
  11. Click "Submit"
  12. Observe there is no error message

Screenshot:

image

Draft Progress:

  • Improve test coverage

if (element.validationMessage) {
error = new Error(element.validationMessage);
element.reportValidity();
} else if (isRequired && !values[key]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future refactoring could see about eliminating this custom idea of isRequired and just use element.validityState.valueMissing in combination with <input required>.

@aduth aduth force-pushed the aduth-custom-validate-form-step branch from b1a1536 to 1c164db Compare April 20, 2022 15:09
@aduth aduth marked this pull request as ready for review April 20, 2022 15:09
@aduth aduth requested review from nprimak, peggles2 and solipet April 20, 2022 15:09

return (
<Button type="submit" isBig isWide className={classes}>
<Button onClick={toNextStep} isBig isWide className={classes}>
Copy link
Contributor Author

@aduth aduth Apr 20, 2022

Choose a reason for hiding this comment

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

This change is necessary because the personal key confirmation modal content is rendered outside the <form> element, due to the createPortal usage in <FullScreen />. While React does support event bubbling through portals, the submit event is never fired by the browser, since the button is technically not within a form.

Example: https://codepen.io/aduth/pen/KKZEPER

Copy link
Contributor Author

@aduth aduth Apr 21, 2022

Choose a reason for hiding this comment

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

This change is necessary because the personal key confirmation modal content is rendered outside the <form> element, due to the createPortal usage in <FullScreen />. While React does support event bubbling through portals, the submit event is never fired by the browser, since the button is technically not within a form.

Example: https://codepen.io/aduth/pen/KKZEPER

An issue with this approach is that we also want to support implicit submission (i.e. pressing Enter from a field to submit), which relies on a submit button being present within the same form.

If the form has no submit button, then the implicit submission mechanism must do nothing if the form has more than one field [...] whose type attribute is in one of the following states: Text, Search, URL, Telephone, Email, Password, Date, Month, Week, Time, Local Date and Time, Number

I found an alternative solution to the portal form submission challenge in bea0810 by using an empty <form> tag , which tricks the browser into emitting the submit event. As mentioned in the previous comment, React will bubble this event across the portal boundary to be handled by FormSteps.

@aduth aduth marked this pull request as draft April 20, 2022 16:13
@aduth aduth marked this pull request as ready for review April 20, 2022 16:17
Comment on lines 16 to 17
const { toNextStep } = useContext(FormStepsContext);
const { registerField, value, toPreviousStep } = stepProps;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's not really a rhyme or reason to why some values are in context and others are in props. I think it would make sense to pass all the same values, both as props, and as context.

const context = { toNextStep() {} /*, ... */ };
return (
  <FormStepsContext.Provider value={context}>
    <FormStep {...context} />
  </FormStepsContext.Provider>
);

Copy link
Contributor

@nprimak nprimak left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 269 to 277
Copy link
Contributor Author

@aduth aduth Apr 20, 2022

Choose a reason for hiding this comment

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

Kinda an edge case, but I spotted an issue with this revised logic. When a user uploads invalid documents on the document capture step and we need them to re-upload, we expect that they would not be able to submit until they replace both front and back. With the revised implementation here, they would have been allowed to submit after replacing any of the values.

I added a failing regression test case in d02093076 and am working to revise this a bit, hopefully returning more to the previous logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if it's the same bug... but when I was testing doc auth in staging yesterday, I accidentally set the front image for both front and back, so the real fix was to only replace back image, but I had to replace both images in order for the form to let me go

Copy link
Contributor Author

@aduth aduth Apr 20, 2022

Choose a reason for hiding this comment

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

Unsure if it's the same bug... but when I was testing doc auth in staging yesterday, I accidentally set the front image for both front and back, so the real fix was to only replace back image, but I had to replace both images in order for the form to let me go

I think in the majority of cases it's expected that the user will need to replace both, at least based on my recollection of how the back-end error logic is rarely able to pinpoint issues with a specific side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure if it's the same bug... but when I was testing doc auth in staging yesterday, I accidentally set the front image for both front and back, so the real fix was to only replace back image, but I had to replace both images in order for the form to let me go

Also, were you using real images which would normally pass, since staging would be running the actual vendor code?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup staging, real vendors, real images that would have passed if I put the right back image in the right field

Copy link
Contributor Author

@aduth aduth Apr 21, 2022

Choose a reason for hiding this comment

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

I checked the logs and it looks like the back-end had reported an error on both sides of the image in that attempt.

properties.event_properties.errors.back.0 | Please add a new image
properties.event_properties.errors.front.0 | Please add a new image
properties.event_properties.errors.general.0 | We couldn’t verify your ID. Try taking new pictures.

A closer look at failed authentication tests shows that it failed "Document Classification", which isn't associated with a particular side.

'Document Classification': { type: ID, msg_key: Errors::ID_NOT_RECOGNIZED },

aduth added 16 commits April 21, 2022 09:11
**Why**: As a user, I expect feedback if I have incorrectly entered the personal key, so that I can correct my mistakes and continue with the proofing flow.

changelog: Upcoming Features, Identity Verification, Add personal key step screen
reportValidity is meant to be combination of (a) check validity and (b) report to user
**Why**: For compatibility with portal'd content. While React does support event bubbling through portal boundaries, the DOM itself is a bit more picky, and will not fire the submit event if the submit button is not within the same DOM hierarchy as its form.

see: https://codepen.io/aduth/pen/KKZEPER
@aduth aduth force-pushed the aduth-custom-validate-form-step branch from 534e5e9 to 4110e4e Compare April 21, 2022 13:14
@aduth aduth merged commit f5cab7a into main Apr 21, 2022
@aduth aduth deleted the aduth-custom-validate-form-step branch April 21, 2022 13:47
@jmdembe jmdembe mentioned this pull request Apr 26, 2022
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.

3 participants