Skip to content

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

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

Merged
aduth merged 2 commits into from
Oct 31, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9e645a
LG-7943: Avoid showing in-person option for field validation errors
aduth Oct 31, 2022
dfad071
Collapse hasErrors, showInPersonOption to showAlternativeProofingOptions
aduth Oct 31, 2022
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
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/components/barcode-attention-warning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function BarcodeAttentionWarning({ onDismiss, pii }: BarcodeAttentionWarningProp
<DocumentCaptureTroubleshootingOptions
location="post_submission_warning"
showDocumentTips={false}
hasErrors
showAlternativeProofingOptions
/>
}
>
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/components/capture-advice.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function CaptureAdvice({ onTryAgain, isAssessedAsGlare, isAssessedAsBlurry }) {
<DocumentCaptureTroubleshootingOptions
heading={t('idv.troubleshooting.headings.still_having_trouble')}
location="capture_tips"
hasErrors
showAlternativeProofingOptions
/>
}
>
Expand Down
94 changes: 44 additions & 50 deletions ...pt/packages/document-capture/components/document-capture-troubleshooting-options.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,75 +125,69 @@ describe('DocumentCaptureTroubleshootingOptions', () => {
});

context('in person proofing links', () => {
context('no errors and no inPersonURL', () => {
context('without inPersonURL', () => {
it('has no IPP information', () => {
const { queryByText } = render(<DocumentCaptureTroubleshootingOptions />);

expect(queryByText('components.troubleshooting_options.new_feature')).to.not.exist();
});
});

context('hasErrors but no inPersonURL', () => {
it('has no IPP information', () => {
const { queryByText } = render(<DocumentCaptureTroubleshootingOptions hasErrors />);
context('with showAlternativeProofingOptions', () => {
it('has no IPP information', () => {
const { queryByText } = render(
<DocumentCaptureTroubleshootingOptions showAlternativeProofingOptions />,
);

expect(queryByText('components.troubleshooting_options.new_feature')).to.not.exist();
expect(queryByText('components.troubleshooting_options.new_feature')).to.not.exist();
});
});
});

context('hasErrors and inPersonURL', () => {
context('with inPersonURL', () => {
const wrapper: ComponentType = ({ children }) => (
<FlowContext.Provider value={{ inPersonURL } as FlowContextValue}>
{children}
</FlowContext.Provider>
);

it('has link to IPP flow', () => {
const { getByText, getByRole } = render(
<DocumentCaptureTroubleshootingOptions hasErrors />,
{ wrapper },
);

expect(getByText('components.troubleshooting_options.new_feature')).to.exist();

const link = getByRole('link', { name: 'idv.troubleshooting.options.verify_in_person' });

expect(link).to.exist();
});

it('logs an event when clicking the troubleshooting option', async () => {
const trackEvent = sinon.stub();
const { getByRole } = render(
<AnalyticsContextProvider trackEvent={trackEvent}>
<DocumentCaptureTroubleshootingOptions hasErrors />
</AnalyticsContextProvider>,
{ wrapper },
);

const link = getByRole('link', { name: 'idv.troubleshooting.options.verify_in_person' });
await userEvent.click(link);
it('has no IPP information', () => {
const { queryByText } = render(<DocumentCaptureTroubleshootingOptions />);

expect(trackEvent).to.have.been.calledWith(
'IdV: verify in person troubleshooting option clicked',
);
expect(queryByText('components.troubleshooting_options.new_feature')).to.not.exist();
});
});

context('hasErrors and inPersonURL but showInPersonOption is false', () => {
const wrapper: ComponentType = ({ children }) => (
<FlowContext.Provider value={{ inPersonURL } as FlowContextValue}>
{children}
</FlowContext.Provider>
);

it('does not have link to IPP flow', () => {
const { queryAllByText, queryAllByRole } = render(
<DocumentCaptureTroubleshootingOptions hasErrors showInPersonOption={false} />,
{ wrapper },
);

expect(queryAllByText('components.troubleshooting_options.new_feature').length).to.equal(0);
expect(queryAllByRole('button').length).to.equal(0);
context('with showAlternativeProofingOptions', () => {
it('has link to IPP flow', () => {
const { getByText, getByRole } = render(
<DocumentCaptureTroubleshootingOptions showAlternativeProofingOptions />,
{
wrapper,
},
);

expect(getByText('components.troubleshooting_options.new_feature')).to.exist();

const link = getByRole('link', { name: 'idv.troubleshooting.options.verify_in_person' });

expect(link).to.exist();
});

it('logs an event when clicking the troubleshooting option', async () => {
const trackEvent = sinon.stub();
const { getByRole } = render(
<AnalyticsContextProvider trackEvent={trackEvent}>
<DocumentCaptureTroubleshootingOptions showAlternativeProofingOptions />
</AnalyticsContextProvider>,
{ wrapper },
);

const link = getByRole('link', { name: 'idv.troubleshooting.options.verify_in_person' });
await userEvent.click(link);

expect(trackEvent).to.have.been.calledWith(
'IdV: verify in person troubleshooting option clicked',
);
});
});
});
});
Expand Down
14 changes: 4 additions & 10 deletions ...ascript/packages/document-capture/components/document-capture-troubleshooting-options.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,16 @@ interface DocumentCaptureTroubleshootingOptionsProps {
showDocumentTips?: boolean;

/**
* Whether to include option to verify in person.
* Whether to display alternative options for verifying.
*/
showInPersonOption?: boolean;

/**
* If there are any errors (toggles whether or not to show in person proofing option)
*/
hasErrors?: boolean;
showAlternativeProofingOptions?: boolean;
}

function DocumentCaptureTroubleshootingOptions({
heading,
location = 'document_capture_troubleshooting_options',
showDocumentTips = true,
showInPersonOption = true,
hasErrors,
showAlternativeProofingOptions,
}: DocumentCaptureTroubleshootingOptionsProps) {
const { t } = useI18n();
const { inPersonURL } = useContext(FlowContext);
Expand All @@ -49,7 +43,7 @@ function DocumentCaptureTroubleshootingOptions({

return (
<>
{hasErrors && inPersonURL && showInPersonOption && (
{showAlternativeProofingOptions && inPersonURL && (
<TroubleshootingOptions
isNewFeatures
heading={formatHTML(t('idv.troubleshooting.headings.are_you_near'), {
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/packages/document-capture/components/documents-step.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function DocumentsStep({
/>
))}
{isLastStep ? <FormStepsButton.Submit /> : <FormStepsButton.Continue />}
<DocumentCaptureTroubleshootingOptions hasErrors={!!errors.length} />
<DocumentCaptureTroubleshootingOptions />
Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
Contributor Author

@aduth aduth Oct 31, 2022

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.

identity-idp/app/javascript/packages/document-capture/components/document-capture-troubleshooting-options.tsx

Lines 26 to 34 in c2a3697

/**
* 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
Copy Markdown
Contributor Author

@aduth aduth Oct 31, 2022

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
Copy Markdown
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.

Love it! Way more semantic. Ty.

<Cancel />
</CaptureTroubleshooting>
);
Expand Down
3 changes: 1 addition & 2 deletions app/javascript/packages/document-capture/components/review-issues-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ function ReviewIssuesStep({
troubleshootingOptions={
<DocumentCaptureTroubleshootingOptions
location="post_submission_warning"
hasErrors={!!errors?.length}
showInPersonOption={!isFailedResult}
showAlternativeProofingOptions={!isFailedResult}
/>
}
>
Expand Down
6 changes: 5 additions & 1 deletion spec/javascripts/packages/document-capture/components/document-capture-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ describe('document-capture/components/document-capture', () => {
context('in person steps', () => {
it('renders the step indicator', async () => {
const endpoint = '/upload';
const { getByLabelText, getByText, findByText } = render(
const { getByLabelText, getByText, queryByText, findByText } = render(
<UploadContextProvider upload={httpUpload} endpoint={endpoint}>
<ServiceProviderContextProvider value={{ isLivenessRequired: false }}>
<FlowContext.Provider
Expand All @@ -514,6 +514,10 @@ describe('document-capture/components/document-capture', () => {
json: () => ({ success: false, remaining_attempts: 1, errors: [{}] }),
});

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();
Comment on lines +517 to +519
Copy link
Copy Markdown
Contributor

@allthesignals allthesignals Oct 31, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

@aduth aduth 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

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.


const frontImage = getByLabelText('doc_auth.headings.document_capture_front');
const backImage = getByLabelText('doc_auth.headings.document_capture_back');
await userEvent.upload(frontImage, validUpload);
Expand Down