LG-12307: don't allow upload when selfie capture is enabled by night-jellyfish · Pull Request #10232 · 18F/identity-idp

night-jellyfish · 2024-03-12T01:18:38Z

🎫 Ticket

🛠 Summary of changes

When doc auth with selfie is required, the FE only allows adding ID and selfie images using the Acuant SDK.

(unless doc_auth_selfie_desktop_test_mode: true, then we still want to allow uploads)

📜 Testing Plan

This one is a bit involved when it comes to testing. This is because between the differences between mobile and desktop, and the two flags:

  doc_auth_selfie_capture_enabled
  doc_auth_selfie_desktop_test_mode

... there are 8 different states it can be in. To make it a little easier, I made an Excel spreadsheet with the 8 states and what their expected result are.

Choose a line in the spreadsheet of possible outcomes
Make the needed adjustments to the two flags in your application.yml
Start / Restart the server (if this is your second+ time through)
Get to the doc auth capture screen (if you follow these directions you should only need one account and can mostly cut the server on this page and just reload)
Test the outcome according to the "Expected outcome" column for the line of the spreadsheet you're on
Mark how it went under the "When testing PR" column
ctrl + c out of the server (don't run make setup)
start again

In addition, note that you can fail the SDK capture by trying to take a photo of your laptop trackpad. If it's too well lit, SDK capture may still pass, but you can put a hand over your phone to cause a shadow that will make it fail.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:

After:

The "IdV: Native camera forced after failed attempts" event was firing even though the previous commit disallows file upload and use of the native camera. This commit prevents this event from firing.

[See this Slack thread for details](https://gsa-tts.slack.com/archives/C05HSH9RQ57/p1709940634853369?thread_ts=1709857278.599209&cid=C05HSH9RQ57).

isSelfieDesktopMode is needed in the next commit to allow upload when isSelfieDesktopMode is true.

Without looking at this value, we won't be able to upload yml files if desktop test mode and selfie are both enabled.

spec/javascript/packages/document-capture/context/selfie-capture-spec.jsx

app/javascript/packages/document-capture/context/failed-capture-attempts.tsx

night-jellyfish · 2024-03-12T01:40:28Z

app/javascript/packages/document-capture/components/document-side-acuant-capture.tsx

@@ -7,6 +7,7 @@ import type {
  RegisterFieldCallback,


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

app/javascript/packages/document-capture/context/failed-capture-attempts.tsx

aduth · 2024-03-12T12:24:41Z

spec/javascript/packages/document-capture/components/document-side-acuant-capture-spec.jsx

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

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.

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?

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

Thank you both!!

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

dawei-nava · 2024-03-12T14:48:47Z

@night-jellyfish , looks like for row3/row5, after failed 3 times and taking id image using native camera, the image is not uploaded to the box most of the time.

Update: tried on main branch, similar issue, so ....

… enabled, only allow images through SDK

spec/javascript/packages/document-capture/components/document-side-acuant-capture-spec.jsx

eileen-nava

Approved. 👍🏻 Two things:

I only locally tested the mobile scenarios where doc_auth_selfie_capture_enabled: true and doc_auth_selfie_desktop_test_mode: false or doc_auth_selfie_capture_enabled: true and doc_auth_selfie_desktop_test_mode: true.
I agree with writing a ticket or making a follow-up PR to remove // @ts-ignore from spec/javascript/support/document-capture.jsx.

eileen-nava · 2024-03-13T21:01:41Z

app/javascript/packages/document-capture/context/failed-capture-attempts.tsx

  }

-  const forceNativeCamera =
+  const hasExhaustedAttempts =


Nice variable naming here. 👍🏻

Thank you! Credit goes to @aduth for thinking of this name 😁

eileen-nava · 2024-03-13T21:04:37Z

spec/javascript/packages/document-capture/components/document-side-acuant-capture-spec.tsx

+      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;


I thought this was nice too! Really readable syntax and we have it elsewhere in our code. However as I was adding tests in d81b08c, I found that that.is.empty returned true even when the length of the array was more than 0. In other words...it doesn't seem to do what I'd expect, or what the docs imply. So I updated it to to.have.lengthOf(0). Less pleasant to read, but more accurate results.

that.is.empty returned true even when the length of the array was more than 0

WHAT. 😮 That is unexpected. I would be curious to hear more during a 1:1.

I'd wonder if it's the lack of parentheses? I'm not sure I'd expect Chai's default optional parentheses to not work, but we do use the dirty-chai plugin with the expectation to prefer explicit parantheses.

Oh my gosh you're right! I just tested and if I do .that.is.empty() it fails where I'd expect it to. If I do .that.is.empty it passes.

That is so confusing since the document from Chai I linked displays it without parens.

Yeah, that's a TIL, and very confusing. I would have thought dirty-chai just makes the parentheses work, not that it requires them 😕

eileen-nava · 2024-03-13T21:06:19Z

spec/javascript/packages/document-capture/context/failed-capture-attempts-spec.jsx

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


eileen-nava · 2024-03-13T21:10:04Z

spec/javascript/packages/document-capture/components/document-side-acuant-capture-spec.jsx

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

…ie-use-new-react-context

Brittany Greaner added 5 commits March 11, 2024 13:50

Prevent upload if selfie capture is enabled

288cd84

Prevent native camera event if selfie enabled

3b37ac9

The "IdV: Native camera forced after failed attempts" event was firing even though the previous commit disallows file upload and use of the native camera. This commit prevents this event from firing.

Mv SelfieCaptureContext above FailedCaptureAttempts

94a1aaf

[See this Slack thread for details](https://gsa-tts.slack.com/archives/C05HSH9RQ57/p1709940634853369?thread_ts=1709857278.599209&cid=C05HSH9RQ57).

Add isSelfieDesktopMode to selfieCaptureContext

53d47b9

isSelfieDesktopMode is needed in the next commit to allow upload when isSelfieDesktopMode is true.

Allow upload when desktopMode enabled

aeecf5f

Without looking at this value, we won't be able to upload yml files if desktop test mode and selfie are both enabled.

night-jellyfish mentioned this pull request Mar 12, 2024

LG-12307: if selfie req, no upload permitted #10165

Closed

16 tasks

night-jellyfish requested review from a team, charleyf and kellular and removed request for a team March 12, 2024 01:36

night-jellyfish commented Mar 12, 2024

View reviewed changes

spec/javascript/packages/document-capture/context/selfie-capture-spec.jsx Show resolved Hide resolved

night-jellyfish commented Mar 12, 2024

View reviewed changes

app/javascript/packages/document-capture/context/failed-capture-attempts.tsx Outdated Show resolved Hide resolved

night-jellyfish commented Mar 12, 2024

View reviewed changes

aduth reviewed Mar 12, 2024

View reviewed changes

changelog: User-Facing Improvements, Doc Auth, When selfie capture is…

54f48c7

… enabled, only allow images through SDK

dawei-nava reviewed Mar 12, 2024

View reviewed changes

spec/javascript/packages/document-capture/components/document-side-acuant-capture-spec.jsx Outdated Show resolved Hide resolved

Brittany Greaner added 4 commits March 12, 2024 13:32

Update name of var that is too long

2eca1f8

Fix order of contexts to reflect reality

35ccbe5

Fix lint error after name shortening

51aa7f7

Change document-side-acuant-capture-spec.jsx --> tsx file

4680c34

eileen-nava approved these changes Mar 13, 2024

View reviewed changes

Brittany Greaner added 6 commits March 13, 2024 14:32

Update name of var

f542847

Add new line for clearer test expectations

5d94d3e

Add all test cases

d81b08c

Yarn lint fixes

6b5ff50

Merge branch 'main' into brittany/lg-12307-only-capture-sdk-when-self…

52cb866

…ie-use-new-react-context

Merge branch 'main' into brittany/lg-12307-only-capture-sdk-when-self…

992ce68

…ie-use-new-react-context

night-jellyfish merged commit e9ee7b3 into main Mar 14, 2024

night-jellyfish deleted the brittany/lg-12307-only-capture-sdk-when-selfie-use-new-react-context branch March 14, 2024 18:02

matthinz mentioned this pull request Mar 19, 2024

Deploy RC 365 to Production #10268

Merged

Conversation

night-jellyfish commented Mar 12, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dawei-nava commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth Mar 13, 2024 •

edited

Loading

dawei-nava commented Mar 12, 2024 •

edited

Loading