Skip to content

LG-7943: Avoid showing in-person option for field validation errors#7254

Merged
aduth merged 2 commits intomainfrom
aduth-lg-7943-ipp-cta-validation-error
Oct 31, 2022
Merged

LG-7943: Avoid showing in-person option for field validation errors#7254
aduth merged 2 commits intomainfrom
aduth-lg-7943-ipp-cta-validation-error

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 31, 2022

🎫 Ticket

LG-7943

🛠 Summary of changes

Updates display of document capture troubleshooting option to avoid rendering in-person call-to-action when field validation errors exist.

📜 Testing Plan

  • yarn test passes
  • Attempting to submit with field validation errors (e.g. empty fields, images with blur or glare) does not present in-person option

👀 Screenshots

Before: screenshot before
After: screenshot after

changelog: Bug Fixes, In-person proofing, Show in-person option at intended troubleshooting scenarios
@aduth aduth requested review from a team and tomas-nava October 31, 2022 13:17
Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

LGTM!

))}
{isLastStep ? <FormStepsButton.Submit /> : <FormStepsButton.Continue />}
<DocumentCaptureTroubleshootingOptions hasErrors={!!errors.length} />
<DocumentCaptureTroubleshootingOptions />
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if hasErrors should be renamed to something more specific to IPP hasDocCaptureErrors or something

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 wonder if hasErrors should be renamed to something more specific to IPP hasDocCaptureErrors or something

I was on the fence about making some further revisions to the props, because I also think there's some redundancy between showInPersonOption and hasErrors, at least in how they're both currently used exclusively in determining whether to show in-person troubleshooting options.

/**
* Whether to include option to verify in person.
*/
showInPersonOption?: boolean;
/**
* If there are any errors (toggles whether or not to show in person proofing option)
*/
hasErrors?: boolean;

For example, we could collapse them to a single prop, and maybe name it in such a way that allows for future off-ramp alternative proofing options to use (e.g. showAlternativeProofingOptions), especially since we're also still considering inPersonURL to control whether the feature is available for the current service provider.

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 wonder if hasErrors should be renamed to something more specific to IPP hasDocCaptureErrors or something

I was on the fence about making some further revisions to the props [...]

I gave this a shot in dfad071.

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it! Way more semantic. Ty.

Comment on lines +517 to +519
expect(queryByText('idv.troubleshooting.options.verify_in_person')).not.to.exist();
await userEvent.click(getByText('forms.buttons.submit.default'));
expect(queryByText('idv.troubleshooting.options.verify_in_person')).not.to.exist();
Copy link
Contributor

@allthesignals allthesignals Oct 31, 2022

Choose a reason for hiding this comment

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

Test expanded to capture bug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test expanded to capture bug

I'd contemplated not including any spec coverage here, since the way it had been implemented was as as additive feature of the documents step, and ideally I'd be removing whichever spec had asserted its presence, but one did not exist. But with all the different conditions involved in rendering troubleshooting options, I figured it wouldn't hurt to check that it's not visible when we're not expecting it to be.

@aduth aduth merged commit ffe9eb0 into main Oct 31, 2022
@aduth aduth deleted the aduth-lg-7943-ipp-cta-validation-error branch October 31, 2022 19:23
@aduth aduth mentioned this pull request Nov 3, 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.

2 participants