LG-9489 Hybrid mobile document capture controller by matthinz · Pull Request #8243 · 18F/identity-idp

matthinz · 2023-04-18T23:53:01Z

🎫 Ticket

🛠 Summary of changes

The current "hybrid" document capture flow (the thing where you text yourself a special link you can use to take pictures of your ID with your phone) relies on the flow state machine.

This PR adds the third of three new controllers to replace the flow state machine version of this codepath.

The hybrid flow was always a strange beast, since the user is not technically "logged in" while they are uploading their documents--they have the ability to upload documents related to a specific DocumentCaptureSession.

Most of this PR is copy/pasting document-capture related code and specs, and adapting it slightly to work in a hybrid flow context where none of the following are available:

user_session
flow_session
idv_session

Where code can be shared between the hybrid flow and other idv controllers, this sharing is done with concerns that depend on controllers implementing a document_capture_session_uuid method. In the hybrid flow, that uuid is read from session, whereas in IdV, it comes out of flow_session. There will be more cleanup / refactoring work to do after this lands and we migrate production traffic to this route.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

Enable the doc_auth_hybrid_mobile_controllers_enabled feature flag in your application.yml
Sign up and enter the identity verification flow
Enter a phone number to send yourself a link to upload your photos
Verify that the link refers to the path /verify/documents--that is the path for this new set of controllers
Open the link in a new browser and upload your images
When told to return to your computer, close the browser and verify you can proceed through the flow.

app/controllers/concerns/idv/hybrid_mobile/hybrid_mobile_concern.rb

Just need the session id for this.

Was resolving relative to /verify for pages in Idv module.

…mobile side of hybrid idv flow

(We only need the session uuid)

…tep view

Don't need it

Rather than a separate feature test for the capture complete step, incorporate its checks into the feature test for the full hybrid flow.

We don't have a flow_session to work with

…troller

(Not used)

app/views/idv/shared/_document_capture.html.erb

matthinz · 2023-04-20T22:11:07Z

app/helpers/application_helper.rb


  def page_with_trust?
-    return false if current_page?(controller: 'users/sessions', action: 'new')
+    return false if current_page?(controller: '/users/sessions', action: 'new')


When rendering layouts/base from idv/hybrid_mobile/document_capture_controller, rails would attempt to resolve users/session here as idv/users/sessions, which would error out. Prefixing this with a / fixed it (which, I did not realize directory-style controller references could do that), but there may be some tweaks to routing or something that could obviate the need for this.

app/services/idv/steps/document_capture_step.rb

Add a spec for the AcuantConcern

matthinz · 2023-04-20T23:00:35Z

app/controllers/concerns/idv/acuant_concern.rb

@@ -0,0 +1,39 @@
+module Idv
+  module AcuantConcern


This concern shows how I'm thinking code sharing can work between hybrid and non-hybrid flows--it depends on the controller itself to implement document_capture_session_uuid, and does not contain references to e.g. flow_session

soniaconnolly

Looks great overall! Let me know if you want to sync up on the changes I made to desktop DocumentCaptureController while deleting the feature flag.

app/controllers/idv/document_capture_controller.rb

app/controllers/idv/hybrid_mobile/document_capture_controller.rb

app/services/idv/steps/document_capture_step.rb

soniaconnolly · 2023-04-21T21:35:14Z

app/views/idv/hybrid_mobile/document_capture/show.html.erb

+      document_capture_session_uuid: document_capture_session_uuid,
+      flow_path: 'hybrid',
+      sp_name: decorated_session.sp_name,
+      failure_to_proof_url: idv_doc_auth_return_to_sp_url,


Another recent change in the desktop DocumentCapture code, make this take a failure_to_proof_url variable that gets set directly rather than taking a detour through a route to DocAuthController#return_to_sp.

~~Addressed in 9624023~~ This was actually addressed in 243e65c, but I'm not 100% on if failure_to_proof_url is actually used in the hybrid flow

It's used in the shared template. It's reachable when there's a barcode error or something like that, which I think could happen on mobile too.

app/views/idv/shared/_document_capture.html.erb

spec/controllers/idv/hybrid_mobile/capture_complete_controller_spec.rb

spec/controllers/idv/hybrid_mobile/document_capture_controller_spec.rb

spec/services/idv/steps/document_capture_step_spec.rb

soniaconnolly · 2023-04-21T22:51:17Z

At some point we'll need to move the STEP_INDICATOR_STEPS out of CaptureDocFlow. Maybe when we delete the feature flag and delete that whole flow.

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

soniaconnolly

Tested locally and it worked! LGTM.

matthinz · 2023-04-25T15:29:22Z

Okey doke, I'm going to land in the interest of moving this forward!

matthinz added the status - work in progress label Apr 18, 2023

matthinz force-pushed the matthinz/9489-doc-capture branch from d564944 to f4b66d7 Compare April 19, 2023 15:58

zachmargolis reviewed Apr 19, 2023

View reviewed changes

app/controllers/concerns/idv/hybrid_mobile/hybrid_mobile_concern.rb Outdated Show resolved Hide resolved

matthinz force-pushed the matthinz/9489-doc-capture branch 3 times, most recently from a27f9aa to db6d479 Compare April 19, 2023 23:09

matthinz added 19 commits April 20, 2023 09:55

:show action for DocumentCaptureController

0c2e187

First crack at #update action

1137e53

Further cleanup & refinement

76f233e

Don't pass flow_session into _document_capture.html.erb

2b74bcc

Just need the session id for this.

Resolve SessionsController relative to app root

507cf7b

Was resolving relative to /verify for pages in Idv module.

Refactor some shared stuff out of capture + complete controllers

b82bb7a

Add beginnings of a feature spec

f15016e

Update capture complete step so tests are green

110616f

Bring in :override_csp_to_allow_acuant

c824de8

Use have_logged_event in DocumentCaptureController spec

3076365

Sync up more document capture controller tests

10127c3

Test that document_capture_session_uuid is passed to views

6aeb1b9

Pass document_capture_session_uuid through show view

e9730e8

Adapt hybrid flow feature spec for mobile hybrid version

92ae055

changelog: Internal, Flow state machine removal, Add controllers for …

33eb5e5

…mobile side of hybrid idv flow

Fix a couple more cases of passing flow_session to document capture view

694d91a

(We only need the session uuid)

Make sure we actually pass document_capture_session_uuid to capture s…

80cd541

…tep view

Refactor some helper methods to use defined? rather than ||=

51f29c9

Remove step count stuff

6a51160

Don't need it

matthinz force-pushed the matthinz/9489-doc-capture branch from 8139750 to 6a51160 Compare April 20, 2023 17:12

matthinz added 4 commits April 20, 2023 12:04

Update ApplicationHelper spec

54c7c04

Incorporate capture complete feature test into main one

7881f0d

Rather than a separate feature test for the capture complete step, incorporate its checks into the feature test for the full hybrid flow.

Refactor acuant a/b testing and csp into concern

8ec1e01

Remove references to flow_session from hybrid mobile controller

4ba143c

We don't have a flow_session to work with

matthinz added 3 commits April 20, 2023 14:34

Fix reference to document_capture_session_uuid in idv doc capture con…

59169da

…troller

Remove idv_session from HybridMobileConcern

42447fd

(Not used)

Minimize changes to document_capture_step on this branch

eb5e1f5

matthinz commented Apr 20, 2023

View reviewed changes

app/views/idv/shared/_document_capture.html.erb Show resolved Hide resolved

Remove automatic formatting changes to routes.rb

b4f8011

matthinz commented Apr 20, 2023

View reviewed changes

app/services/idv/steps/document_capture_step.rb Show resolved Hide resolved

Refactor acuant tests out of controller spec

d0ec7b1

Add a spec for the AcuantConcern

matthinz commented Apr 20, 2023

View reviewed changes

matthinz removed the status - work in progress label Apr 20, 2023

matthinz marked this pull request as ready for review April 20, 2023 23:01

matthinz requested a review from a team April 20, 2023 23:01

soniaconnolly reviewed Apr 21, 2023

View reviewed changes

soniaconnolly requested a review from jmhooper April 21, 2023 23:19

matthinz and others added 6 commits April 24, 2023 09:46

Don't remove old acuant tests yet

a60f81f

Update spec/controllers/idv/hybrid_mobile/document_capture_controller…

98386c2

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

Minor tweak to capture complete controller spec

06784f3

Update analytics processing to better match non-hybrid version

9624023

Merge branch 'main' into matthinz/9489-doc-capture

b6c4b75

Add failure_to_proof_url to view variables

243e65c

soniaconnolly approved these changes Apr 24, 2023

View reviewed changes

matthinz merged commit 4dc94c9 into main Apr 25, 2023

matthinz deleted the matthinz/9489-doc-capture branch April 25, 2023 15:29

soniaconnolly mentioned this pull request Apr 25, 2023

Remove hybrid mobile 404 action if feature flag disabled #8282

Merged

jmhooper mentioned this pull request Apr 26, 2023

Deploy RC 275 to prod #8284

Merged

jmax-gsa mentioned this pull request Apr 26, 2023

Jmax/lg 9107 remove fsm document capture hybrid flow step #8292

Merged

Conversation

matthinz commented Apr 18, 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!

Uh oh!

matthinz Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthinz Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soniaconnolly Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

matthinz Apr 24, 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!

soniaconnolly Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soniaconnolly commented Apr 21, 2023

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

matthinz commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthinz commented Apr 18, 2023 •

edited

Loading

matthinz Apr 24, 2023 •

edited

Loading