LG-9784 New Hybrid Handoff Controller fka Upload Step by eric-gade · Pull Request #8407 · 18F/identity-idp

eric-gade · 2023-05-16T20:10:17Z

🎫 Ticket

🛠 Summary of changes

Adding new HybridHandoffController with show method, view, and specs. This controller is behind a feature flag and is set to false by default.

Do not merge until #8394 has been merged and deployed.

config/routes.rb

soniaconnolly

LGTM!

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov> changelog: Internal, FSM Removal, Add new controller for hybrid handoff (feature-flagged)

Adding initial update method(s) and single passing test using it

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

Updated the flow hashes for steps and final_url to conditionally add the Upload step based on the presence/absence of the config flag

Analytics always need a bucket value for the Acuant AB Test, and will provide :default if you pass in nil.

Conditional FSM settings don't work well with CI

-- Why The original upload step spec did not test for the presence of the acuant analytics at all. We added this check to the _new_ controller test, but it's unclear that we want to even log acuant ab testing information at this step. We are also restoring the removed code in the acuant concern which checked for the presence of a document capture session uuid before returning any value

soniaconnolly

LGTM! Some comments, and a query about a feature test enclosing context.

Findings from testing locally:

Basic functionality works great. It goes to the page from Agreement with the flag set.
Throttling works on sending link.
On the hybrid_handoff page, visiting link_sent early redirects to document_capture instead of back to hybrid_handoff. I think this is worth investigating, but not a blocker to merge. The back button works!
Visiting hybrid_handoff from the hybrid_mobile flow 500s because of a before action using current_user. I'm going to put in a PR on main for that.
Can click back to get to hybrid_handoff after submitting docs, but clicking upload photos goes straight to SSN step because of guards in document_capture. We should either put a guard on hybrid_handoff if there's already pii in session, or remove the guards in document_capture. Again, I don't think this should block merging.

soniaconnolly · 2023-05-25T16:23:23Z

app/controllers/idv/hybrid_handoff_controller.rb

+      idv_session.phone_for_mobile_flow = params[:doc_auth][:phone]
+      flow_session[:phone_for_mobile_flow] = idv_session.phone_for_mobile_flow


Noting here that when we're ready to delete the feature flag, we'll stop adding flow_session[:phone_for_mobile_flow] and start reading it from idv_session in LinkSentController.

soniaconnolly · 2023-05-25T16:24:25Z

app/controllers/idv/hybrid_handoff_controller.rb

+        errors: { message: telephony_result.error&.friendly_message },
+        extra: {
+          telephony_response: telephony_result.to_h,
+          destination: :link_sent,


Wondering if we still need destination in analytics or if we can delete that everywhere.

soniaconnolly · 2023-05-25T16:25:27Z

app/controllers/idv/hybrid_handoff_controller.rb

+    before_action :confirm_agreement_step_complete
+
+    def show
+      flow_session[:flow_path] = 'standard'


Setting flow_path here so it will show up in analytics, although it doesn't really make sense if they haven't chosen a flow_path yet.

I believe this is done to get it into the analytics so we can make a GET request to the document capture controller. An alternative approach may be to set this param in the document capture controller or link sent controller methods. That way the last one the user visits will persist for the rest of the flow.

soniaconnolly · 2023-05-25T16:29:17Z

app/controllers/idv/link_sent_controller.rb

      if flow_session[:flow_path] == 'standard'
        redirect_to idv_document_capture_url


We're checking for flow_path == standard here because link_sent is currently the final url for DocAuthFlow. We'll need to think through this again when we change the final url again.

soniaconnolly · 2023-05-25T16:39:21Z

spec/features/idv/doc_auth/hybrid_handoff_spec.rb

+      and_return(fake_attempts_tracker)
+  end
+
+  context 'on a desktop device', js: true do


Do we need js: true? I believe this enclosing context is left over from when there was a mobile -> desktop flow, so it could be deleted to leave the specs at the top level. Or renamed to say the feature flag is set, and add a 404 test at the bottom for when the feature flag is not set. Optional, since that's in the controller tests already.

Unfortunately we need the JS here because anything with phone numbers gets cleaned up client side, so the numbers won't match otherwise

jmhooper · 2023-05-25T17:04:01Z

app/controllers/idv/hybrid_handoff_controller.rb

+        **analytics_arguments.merge(form_response(destination: :link_sent).to_h),
+      )
+
+      telephony_form_response


Returning the form response object is a relic of the flow state machine. I don't think we need it anymore. I think we should move the analytics call up with the irs_attempts_api_tracker call and then let the failure_reason conditional be the last thing we do here.

Unfortunately analytics_arguments needs to read from the flow_session to get the correct :flow_path, and that is only set on line 62 in hybrid cases. If analytics is moved before that line it will report the incorrect flow path. But agree on returning the response

gotcha. regardless I don't think we need this return

jmhooper · 2023-05-25T17:05:25Z

app/views/idv/hybrid_handoff/show.html.erb

+    <%= simple_form_for(
+          :doc_auth,
+          url: url_for(type: :desktop),
+          method: 'PUT',


We discussed making this a link. Did we decide not to take that approach in this changeset?

I decided against it for now because there were too many unknowns. For example, if we change how we are using the flow session to update/read the flow_path, we lose the ability to set it to standard explicitly in the form handler for manual upload (see bypass_send_link_steps). In the same vein, we would need to decide whether we really need an analytics call when users click that option, and, if so, whether that call would now happen on the frontend

Since we're setting flow_path to standard on :show, that might make it easier to make this a link. Good point about analytics though.

eric-gade added the status - do not merge label May 16, 2023

eric-gade requested a review from a team May 16, 2023 20:10

matthinz reviewed May 16, 2023

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

soniaconnolly approved these changes May 17, 2023

View reviewed changes

eric-gade force-pushed the eric-lg-9784 branch from b0f3221 to 68a110c Compare May 23, 2023 16:36

eric-gade and others added 21 commits May 25, 2023 10:56

Initial commit with feature flag

43a2713

Adding controller, view, and route

859760a

Casing redirect for upload step

4cb30bd

Adding view locals to hybrid handoff template

b573e07

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov> changelog: Internal, FSM Removal, Add new controller for hybrid handoff (feature-flagged)

Adding controller spec

1571295

Adding step indicator to show view

06b93ed

Adding initial feature spec

b04936e

lints

0318d19

Remove unneeded :service_provider from controller specs

82a834a

Initial commit for LG-9785 work

c36c41c

Adding initial update method(s) and single passing test using it

delint

f961bc3

Post rebase changes

d7804a8

Improve transfer from hybrid handoff to subsequent steps

37b2e18

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

Adding controller update test and fixing analytics

093137a

Telephony error feature tests passing

ca308f7

Throttling and back button feature tests passing

c91795c

Updated the flow hashes for steps and final_url to conditionally add the Upload step based on the presence/absence of the config flag

Remaining feature specs are now passing

290d844

Acuant AB Test Bucket always needs to respond with a value

0772fcf

Analytics always need a bucket value for the Acuant AB Test, and will provide :default if you pass in nil.

Fixed analytics and controller test flow session

8eb5185

Restoring older FSM settings

d8c13ac

Conditional FSM settings don't work well with CI

Fixing back / link sent cancel action with config flag

54c13aa

eric-gade force-pushed the eric-lg-9784 branch from 25062ad to 54c13aa Compare May 25, 2023 14:56

soniaconnolly removed the status - do not merge label May 25, 2023

soniaconnolly approved these changes May 25, 2023

View reviewed changes

soniaconnolly requested a review from jmhooper May 25, 2023 16:58

jmhooper reviewed May 25, 2023

View reviewed changes

jmhooper approved these changes May 25, 2023

View reviewed changes

Removing old FSM cruft

94c6cc6

eric-gade merged commit a6728ff into main May 25, 2023

eric-gade deleted the eric-lg-9784 branch May 25, 2023 20:07

mitchellhenke mentioned this pull request May 30, 2023

Deploy RC 284 to Production #8503

Merged

		idv_session.phone_for_mobile_flow = params[:doc_auth][:phone]
		flow_session[:phone_for_mobile_flow] = idv_session.phone_for_mobile_flow

		if flow_session[:flow_path] == 'standard'
		redirect_to idv_document_capture_url

Conversation

eric-gade commented May 16, 2023

🎫 Ticket

🛠 Summary of changes

Uh oh!

Uh oh!

soniaconnolly left a comment

Choose a reason for hiding this comment

Uh oh!

soniaconnolly 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!

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