Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
RegisterFieldCallback,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kellular and @daviddsilvanava - This is replacing #10165 and thanks to other work from Timnit, by now shouldn't have the UX issues we talked about on that PR.

} from '@18f/identity-form-steps';
import AcuantCapture from './acuant-capture';
import SelfieCaptureContext from '../context/selfie-capture';

interface DocumentSideAcuantCaptureProps {
side: 'front' | 'back' | 'selfie';
Expand Down Expand Up @@ -43,6 +44,9 @@ function DocumentSideAcuantCapture({
}: DocumentSideAcuantCaptureProps) {
const error = errors.find(({ field }) => field === side)?.error;
const { changeStepCanComplete } = useContext(FormStepsContext);
const { isSelfieCaptureEnabled, isSelfieDesktopMode } = useContext(SelfieCaptureContext);
const isUploadAllowed = isSelfieDesktopMode || !isSelfieCaptureEnabled;

return (
<AcuantCapture
ref={registerField(side, { isRequired: true })}
Expand Down Expand Up @@ -74,6 +78,7 @@ function DocumentSideAcuantCapture({
errorMessage={error ? error.message : undefined}
name={side}
className={className}
allowUpload={isUploadAllowed}
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { createContext, useState } from 'react';
import { createContext, useContext, useState } from 'react';
import type { ReactNode } from 'react';
import useCounter from '../hooks/use-counter';
import SelfieCaptureContext from './selfie-capture';

interface CaptureAttemptMetadata {
isAssessedAsGlare: boolean;
Expand Down Expand Up @@ -125,6 +126,7 @@ function FailedCaptureAttemptsContextProvider({
useCounter();
const [failedSubmissionAttempts, incrementFailedSubmissionAttempts] = useCounter();
const [failedCameraPermissionAttempts, incrementFailedCameraPermissionAttempts] = useCounter();
const { isSelfieCaptureEnabled } = useContext(SelfieCaptureContext);

const [failedSubmissionImageFingerprints, setFailedSubmissionImageFingerprints] =
useState<UploadedImageFingerprints>(failedFingerprints);
Expand All @@ -143,10 +145,14 @@ function FailedCaptureAttemptsContextProvider({
incrementFailedCameraPermissionAttempts();
}

const forceNativeCamera =
const failedAttemptsGreaterThanOrEqualToMaxAttempts =
Comment thread
night-jellyfish marked this conversation as resolved.
Outdated
failedCaptureAttempts >= maxCaptureAttemptsBeforeNativeCamera ||
failedSubmissionAttempts >= maxSubmissionAttemptsBeforeNativeCamera;

const forceNativeCamera = isSelfieCaptureEnabled
? false
: failedAttemptsGreaterThanOrEqualToMaxAttempts;

return (
<FailedCaptureAttemptsContext.Provider
value={{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@ interface SelfieCaptureProps {
* Specify whether to show the selfie capture on the doc capture screen.
*/
isSelfieCaptureEnabled: boolean;
/**
* Specify whether to allow uploads for selfie when in test mode.
*/
isSelfieDesktopMode: boolean;
}

const SelfieCaptureContext = createContext<SelfieCaptureProps>({
isSelfieCaptureEnabled: false,
isSelfieDesktopMode: false,
});

SelfieCaptureContext.displayName = 'SelfieCaptureContext';
Expand Down
19 changes: 11 additions & 8 deletions app/javascript/packs/document-capture.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ interface AppRootData {
skipDocAuth: string;
howToVerifyURL: string;
uiExitQuestionSectionEnabled: string;
docAuthSelfieDesktopTestMode: string;
}

const appRoot = document.getElementById('document-capture-form')!;
Expand Down Expand Up @@ -106,6 +107,7 @@ const {
skipDocAuth,
howToVerifyUrl,
uiExitQuestionSectionEnabled = '',
docAuthSelfieDesktopTestMode,
} = appRoot.dataset as DOMStringMap & AppRootData;

let parsedUsStatesTerritories = [];
Expand Down Expand Up @@ -177,6 +179,15 @@ const App = composeComponents(
value: getServiceProvider(),
},
],
[
SelfieCaptureContext.Provider,
{
value: {
isSelfieCaptureEnabled: getSelfieCaptureEnabled(),
isSelfieDesktopMode: String(docAuthSelfieDesktopTestMode) === 'true',
},
},
],
[
FailedCaptureAttemptsContextProvider,
{
Expand All @@ -192,14 +203,6 @@ const App = composeComponents(
},
},
],
[
SelfieCaptureContext.Provider,
{
value: {
isSelfieCaptureEnabled: getSelfieCaptureEnabled(),
},
},
],
[
DocumentCapture,
{
Expand Down
1 change: 1 addition & 0 deletions app/views/idv/shared/_document_capture.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
in_person_outage_expected_update_date: IdentityConfig.store.in_person_outage_expected_update_date,
us_states_territories: us_states_territories,
doc_auth_selfie_capture: FeatureManagement.idv_allow_selfie_check? && doc_auth_selfie_capture,
doc_auth_selfie_desktop_test_mode: IdentityConfig.store.doc_auth_selfie_desktop_test_mode,
skip_doc_auth: skip_doc_auth,
how_to_verify_url: idv_how_to_verify_url,
ui_exit_question_section_enabled: IdentityConfig.store.doc_auth_exit_question_section_enabled,
Expand Down

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we rename this file to .tsx extension to make sure that all new code is TypeScript ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I did this in 4680c34, but it did start to cause larger issues.

In order to get elements like the context providers in this file to be recognized by the TS linter, I had to add the path with .tsx to tsconfig.json. Doing this caused other files to start erroring out when running yarn tsc. I fixed the one that was easy to fix (document-capture-abandon-spec.tsx). But when I tried to fix spec/javascript/support/document-capture.jsx, I found myself reaching into several additional files that then failed and needed adjustments themselves, until the whole change was getting to be quite large. I also found some of the issues I wasn't sure how to fix.

So, I decided to keep what was easy to change, and use // @ts-ignore on the rest.

@aduth aduth Mar 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wow, that's a great catch that we weren't type-checking .tsx files. Good call on fixing that.

While I feel pretty uneasy about @ts-ignore generally speaking, I think if it's causing a lot of short-term issues, we could follow-up on it separately. Can you either create a ticket or plan to follow-up with a separate pull request for that, to keep ourselves accountable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with Andrew, I think making a separate ticket (or doing a follow-up PR) for this work is a good idea. 👍🏻

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you both!!

I just created a ticket for this: LG-12711.

Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { DeviceContext, SelfieCaptureContext } from '@18f/identity-document-capture';
import DocumentSideAcuantCapture from '@18f/identity-document-capture/components/document-side-acuant-capture';
import { render } from '../../../support/document-capture';

describe('DocumentSideAcuantCapture', () => {
const DEFAULT_PROPS = {
errors: [],
registerField: () => undefined,
};

context('when selfie is _not_ enabled', () => {
it('_does_ display a photo upload button', () => {
const { queryAllByText } = render(
<SelfieCaptureContext.Provider value={{ isSelfieCaptureEnabled: false }}>
<DeviceContext.Provider value={{ isMobile: true }}>
Comment thread
night-jellyfish marked this conversation as resolved.
Outdated
<DocumentSideAcuantCapture {...DEFAULT_PROPS} side="front" />
<DocumentSideAcuantCapture {...DEFAULT_PROPS} side="back" />
</DeviceContext.Provider>
</SelfieCaptureContext.Provider>,
);

const takeOrUploadPictureText = queryAllByText(
'doc_auth.buttons.take_or_upload_picture_html',
);
expect(takeOrUploadPictureText).to.have.lengthOf(2);
});
});

context('when selfie _is_ enabled', () => {
it('does _not_ display a photo upload button', () => {
const { queryAllByText } = render(
<SelfieCaptureContext.Provider value={{ isSelfieCaptureEnabled: true }}>
<DeviceContext.Provider value={{ isMobile: true }}>
<DocumentSideAcuantCapture {...DEFAULT_PROPS} side="front" />
<DocumentSideAcuantCapture {...DEFAULT_PROPS} side="back" />
<DocumentSideAcuantCapture {...DEFAULT_PROPS} side="selfie" />
</DeviceContext.Provider>
</SelfieCaptureContext.Provider>,
);

const takePictureText = queryAllByText('doc_auth.buttons.take_picture');
expect(takePictureText).to.have.lengthOf(3);

const notExpectedText = 'doc_auth.buttons.take_or_upload_picture_html';
expect(queryAllByText(notExpectedText)).to.be.an('array').that.is.empty;
});
});
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { useContext } from 'react';
import { renderHook } from '@testing-library/react-hooks';
import userEvent from '@testing-library/user-event';
import { DeviceContext, AnalyticsContext } from '@18f/identity-document-capture';
import {
DeviceContext,
AnalyticsContext,
SelfieCaptureContext,
} from '@18f/identity-document-capture';
import { Provider as AcuantContextProvider } from '@18f/identity-document-capture/context/acuant';
import AcuantCapture from '@18f/identity-document-capture/components/acuant-capture';
import FailedCaptureAttemptsContext, {
Expand Down Expand Up @@ -164,6 +168,58 @@ describe('FailedCaptureAttemptsContext testing of forceNativeCamera logic', () =
expect(result.current.failedCaptureAttempts).to.equal(1);
expect(result.current.forceNativeCamera).to.equal(false);
});

describe('when selfie is enabled', () => {
it('forceNativeCamera is always false, no matter how many times any attempt fails', () => {
const trackEvent = sinon.spy();
const { result, rerender } = renderHook(() => useContext(FailedCaptureAttemptsContext), {
wrapper: ({ children }) => (
<SelfieCaptureContext.Provider value={{ isSelfieCaptureEnabled: true }}>
<Provider
maxCaptureAttemptsBeforeNativeCamera={2}
maxSubmissionAttemptsBeforeNativeCamera={2}
>
{children}
</Provider>
</SelfieCaptureContext.Provider>
),
});

result.current.onFailedCaptureAttempt({
isAssessedAsGlare: true,
isAssessedAsBlurry: false,
});
rerender(true);
expect(result.current.forceNativeCamera).to.equal(false);
result.current.onFailedCaptureAttempt({
isAssessedAsGlare: false,
isAssessedAsBlurry: true,
});
rerender(true);
expect(result.current.forceNativeCamera).to.equal(false);
result.current.onFailedCaptureAttempt({
isAssessedAsGlare: false,
isAssessedAsBlurry: true,
});
rerender({});
expect(result.current.failedCaptureAttempts).to.equal(3);
expect(result.current.forceNativeCamera).to.equal(false);

result.current.onFailedSubmissionAttempt();
rerender(true);
expect(result.current.forceNativeCamera).to.equal(false);
result.current.onFailedSubmissionAttempt();
rerender(true);
expect(result.current.forceNativeCamera).to.equal(false);
result.current.onFailedSubmissionAttempt();
rerender({});
expect(result.current.failedSubmissionAttempts).to.equal(3);
expect(result.current.forceNativeCamera).to.equal(false);
expect(trackEvent).to.not.have.been.calledWith(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻

'IdV: Native camera forced after failed attempts',
);
});
});
});

describe('maxCaptureAttemptsBeforeNativeCamera logging tests', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ describe('document-capture/context/feature-flag', () => {
it('has expected default properties', () => {
const { result } = renderHook(() => useContext(SelfieCaptureContext));

expect(result.current).to.have.keys(['isSelfieCaptureEnabled']);
expect(result.current).to.have.keys(['isSelfieCaptureEnabled', 'isSelfieDesktopMode']);
expect(result.current.isSelfieCaptureEnabled).to.be.a('boolean');
});
});
Comment thread
night-jellyfish marked this conversation as resolved.