LG-11012: redo document capture bug fixes by amirbey · Pull Request #9256 · 18F/identity-idp

amirbey · 2023-09-24T23:19:32Z

🎫 Ticket

LG-11012

🛠 Summary of changes

reset redo_document_capture in idv_session upon successful doc auth in the hybrid flow -- same as in the standard flow
allow a user to perform hybrid document capture during a document capture redo
doc capture polling to return new doc auth result (not original result) during a document capture redo
confirm ocr_pending_confirmation after a user confirms/submits a succesful doc auth
add a timestamp to the DocumentCaptureSessionResult

📜 Testing Plan

Scenario1: Standard doc auth + hybrid redo doc auth

Complete IdV until hybrid handoff is reached
Select desktop and submit images (that trigger barcode attention) and continue
continue IdV until verify info page is reached
On verify info page, click the "upload new photos" link in the warning banner at top of page
Select hybrid/send link and submit images successfully
return to original window and complete IdV

Scenario2: hybrid doc auth + hybrid redo doc auth

Complete IdV until hybrid handoff is reached
Select hybrid/sendlink and fetch link at example.com/test/telephony
Open mobile link in incognito/private window and submit images (that trigger barcode attention) and continue
return to original window and continue IdV until verify info page is reached
On verify info page, click the "upload new photos" link in the warning banner at top of page
Select hybrid/send link and fetch link at example.com/test/telephony
Open mobile link in incognito/private window and submit images (that trigger barcode attention) and continue
return to original window and complete IdV

app/models/document_capture_session.rb

soniaconnolly

Overall looks great. Thanks for digging into all this! Several comments, but the only blocking one is investigating whether memoization of @session_result is causing problems.

soniaconnolly · 2023-09-28T15:30:12Z

app/controllers/idv/capture_doc_status_controller.rb

I would not have thought of resetting that here! And I would have assumed that it would be too early, but if tests are passing my assumption would clearly be wrong.

fair point. this is relatively consistent with resetting the redo flag in the standard flow. We also don't have use for this redo_document_capture after the link sent step in the hybrid flow

you were correct ... moved appropriately to link_sent#update ... thanks! 👍🏿

soniaconnolly · 2023-09-28T15:34:24Z

app/controllers/idv/capture_doc_status_controller.rb

I like this definition, very practical. "Have we done this already?" I think it will be resilient as we enable the back button, too.

Would it make sense to define it as DocumentCaptureSession#redo?(captured_at:) to avoid repeating the definition in DocumentCaptureController ?

yes i wanted to do that. however, this line is the blocker ... do you knowi is this load_async still necessary? 🤔

Don't know off-hand. @aduth might know if it's part of the doc auth async code. Happy to dig in with you if that would be helpful.

Yes, I think the load_doc_auth_async_result is related to the now-defunct (as of #8377) async document capture code. So it would probably be safe to remove all of that.

thanks @aduth ... after briefly testing out the removal of load_doc_auth_async_result .. i think it'd be best to remove it in a follow up PR

app/controllers/idv/hybrid_mobile/document_capture_controller.rb

app/models/document_capture_session.rb

spec/controllers/idv/capture_doc_status_controller_spec.rb

app/controllers/idv/document_capture_controller.rb

spec/controllers/idv/hybrid_mobile/document_capture_controller_spec.rb

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

soniaconnolly

Tested locally, and when I upload a yml file with a different name on the second DocumentCapture (used first standard, then hybrid), it doesn't update the name in VerifyInfo. It also still shows the warning at the top, even if I refresh the page.

Using standard and standard, VerifyInfo changes as expected.

soniaconnolly · 2023-10-03T15:40:17Z

app/controllers/idv/capture_doc_status_controller.rb

Don't know off-hand. @aduth might know if it's part of the doc auth async code. Happy to dig in with you if that would be helpful.

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

changelog: Bug Fixes, Identity Verification, fix redo of hybrid document capture and barcode read error acceptance

…ture

… in doc cap session

…sion

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

soniaconnolly

Thanks for digging in to all this and sorting it out! LGTM, with a couple more small comments.

I tested standard/standard, standard/hybrid, and hybrid/hybrid, and they all work as expected.

Noticed that if someone enters a Puerto Rico address on a redo, they don't get prompted to edit their address (since the SSN step is skipped the second time), so entered LG-11201 for that.

app/controllers/idv/document_capture_controller.rb

soniaconnolly · 2023-10-04T15:39:29Z

app/models/document_capture_session.rb

+  def new_session_result
+    DocumentCaptureSessionResult.new(
+      id: generate_result_id,
+      captured_at: Time.zone.now,
+    )
+  end


It looks like you didn't end up using this method.

soniaconnolly · 2023-10-04T15:41:32Z

app/services/document_capture_session_result.rb

  :attention_with_barcode,
  :failed_front_image_fingerprints,
  :failed_back_image_fingerprints,
+  :captured_at,


Is there any issue with existing document_capture_sessions in redis not having this attribute?

great catch ... added dig(:captured_at) to check if it exists before evaluating 👍🏿

spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

…ithub.com:18F/identity-idp into amirbey/sonia-hybrid-mobile-redo-document-capture

…mobile

…d captured_at not yet stored in redis

soniaconnolly reviewed Sep 25, 2023

View reviewed changes

app/models/document_capture_session.rb Outdated Show resolved Hide resolved

amirbey changed the title ~~Amirbey/sonia hybrid mobile redo document capture~~ LG-11012: redo document capture bug fixes Sep 26, 2023

amirbey force-pushed the amirbey/sonia-hybrid-mobile-redo-document-capture branch from 7786219 to 57c3528 Compare September 27, 2023 16:00

amirbey self-assigned this Sep 27, 2023

amirbey marked this pull request as ready for review September 27, 2023 17:00

amirbey force-pushed the amirbey/sonia-hybrid-mobile-redo-document-capture branch from 14bf3e3 to 1ee42e3 Compare September 28, 2023 14:12

amirbey requested review from a team and soniaconnolly September 28, 2023 15:01

soniaconnolly reviewed Sep 28, 2023

View reviewed changes

amirbey force-pushed the amirbey/sonia-hybrid-mobile-redo-document-capture branch 2 times, most recently from b5a1026 to 4f5452b Compare October 2, 2023 22:59

amirbey requested a review from soniaconnolly October 2, 2023 23:21

soniaconnolly reviewed Oct 3, 2023

View reviewed changes

soniaconnolly and others added 17 commits October 3, 2023 14:02

Failing spec for hybrid_flow barcode error (redo document capture)

14504d5

Add the rest of a hybrid_mobile redo document capture spec (untested)

a57035b

redo hybrid doc capture fixed

4cad8df

hybrid document capture controller determine reload of document

9fc9da6

fix tests

b7b48e6

LG-11012

a6ca7e6

changelog: Bug Fixes, Identity Verification, fix redo of hybrid document capture and barcode read error acceptance

resave document capture session

1e550eb

rename document capture session result created_at to captured_at

7cfd1bc

add captured_at attr to DocumentCaptureSessionResult in spec

540e87e

confirm_ocr refactor

7c31b32

add tests for confirming hybrid document capture needed

4545aea

unset redo_document_capture in idv_session during hybrid

e2740b6

unsets redo document capture in idv session with successful doc auth

e031646

ensure hybrid doc capture can occur after successful standard doc cap…

6ed1278

…ture

test redo document hybrid after doc auth desktop

8e0e912

guard no longer needed

d74080d

happy linting

2b2166c

amirbey and others added 13 commits October 3, 2023 14:02

move confirm_ocr to document capture session and call in standard flow

62dd008

create doc cap session for doc cap controller spec

7c25830

make sessio result private

f7da966

create new session result for every doc auth response

0fd5164

fix init assignment

55f6482

test that a new result is created when a doc auth is result is stored…

a1df755

… in doc cap session

happy linting

862cc5d

undo session_result changes

e3a9312

add test to verify failed images are retained in document capture ses…

42b9eda

…sion

remove unused variable assignment

ca3f12e

Update app/controllers/idv/hybrid_mobile/document_capture_controller.rb

3ec054a

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

happy linting

7266674

reset idv_sesson.redo_document capture in link_sent#update

4f0ba64

amirbey force-pushed the amirbey/sonia-hybrid-mobile-redo-document-capture branch from 4f5452b to 4f0ba64 Compare October 3, 2023 19:58

amirbey added 2 commits October 3, 2023 16:37

test that pii pon verify _info is changesd upon redo_document_capture

2293105

fix text check

bf1f93a

amirbey force-pushed the amirbey/sonia-hybrid-mobile-redo-document-capture branch from f8496f2 to bf1f93a Compare October 4, 2023 02:11

amirbey added 3 commits October 3, 2023 22:40

barcode attention if not redoing document capture

34385da

happy linting

9b0447e

add guard for document_capture_session requested at

637ab57

soniaconnolly approved these changes Oct 4, 2023

View reviewed changes

amirbey and others added 5 commits October 4, 2023 14:44

remove unused new_session_result method

246b239

Update app/controllers/idv/document_capture_controller.rb

37e0b83

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

Merge branch 'amirbey/sonia-hybrid-mobile-redo-document-capture' of g…

33356f3

…ithub.com:18F/identity-idp into amirbey/sonia-hybrid-mobile-redo-document-capture

add pii value checks to document capture orig on desktop and redo on …

4668cc1

…mobile

guard incase barcode attention doc auth occurred before the release a…

e3ee502

…d captured_at not yet stored in redis

amirbey merged commit 6e9cca8 into main Oct 4, 2023

amirbey deleted the amirbey/sonia-hybrid-mobile-redo-document-capture branch October 4, 2023 21:15

jmhooper mentioned this pull request Oct 5, 2023

Deploy RC 320 to prod #9321

Merged

amirbey mentioned this pull request Oct 6, 2023

LG-11012 follow up PR to remove load doc auth async result #9327

Merged

Conversation

amirbey commented Sep 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirbey Oct 2, 2023 • 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!

soniaconnolly Sep 28, 2023 • 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!

aduth Oct 3, 2023 • 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!

amirbey Oct 4, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

soniaconnolly left a comment • 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amirbey commented Sep 24, 2023 •

edited

Loading

amirbey Oct 2, 2023 •

edited

Loading

soniaconnolly Sep 28, 2023 •

edited

Loading

aduth Oct 3, 2023 •

edited

Loading

amirbey Oct 4, 2023 •

edited

Loading

soniaconnolly left a comment •

edited

Loading