LG-8901: New Hybrid Handoff (upload step) with feature flag by eric-gade · Pull Request #7849 · 18F/identity-idp

eric-gade · 2023-02-17T16:14:41Z

🎫 Ticket

🛠 Summary of changes

This PR is the first in a multi-step process of completing the parent story LG-8230. At issue is combining the current upload and send_link FSM steps. The Figma for the new designs can be found here.

These particular changes do the following:

Create a new feature flag / config variable called doc_auth_combined_hybrid_handoff_enabled, which is by default set to false.
Updates the upload_step to handle functionality that is currently handled by the send_link step (in particular, the sending of the hybrid flow SMS link), but only in cases where the feature flag is enabled
Creation of split views for the upload step -- one for the "combined" version (with feature flag enabled) and one that keeps the status quo
Updating of relevant tests with respect to the feature flag
Updating templates, locales, and styles to comply with the designs for the new combined hybrid handoff view (ie upload step)

📜 Testing Plan

The description for ticket LG-8230 contains a plan for how these PRs/deployments should work. For this particular step, we should do the following:

Deploy these changes with the feature flag disabled (which is the default), which should maintain the status quo.
Test the alternation of the feature flag in a dev environment, particularly trying to simulate a 50/50 deploy state.
If there are no errors/bugs caught in the dev testing or in the status quo in prod, update the feature flag to true in prod and monitor.
Continue with the remaining sub-tasks of LG-8230, which include the removal of the old code and references to the feature flag.

👀 Screenshots

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

Before (upload step with feature flag disabled, ie status quo):

After (upload step with feature flag enabled):

-- What Adding initial fields and new icons

-- What Old code was testing for case where user is at the hybrid handoff screen (upload step), but is on mobile. We have not allowed that to happen for a long time, so the code is dead. changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

-- What Due to the 50/50 issue, and the fact that this ticket requires combining two FSM steps into one, we need to feature flag all capabilities relating to the new combined hybrid handoff step while retaining the existing functionality. With that in mind, this commit does the following: 1. Creates a new feature flag called doc_auth_combined_hybrid_handoff_enabled; 2. Splits the normal upload step view to use one or another of two partials based on the status of that feature flag (the disabled feature flag version of the partial is exactly the same as the existing view); 3. Adds some feature tests for the new combined version with the flag enabled only in the tests 4. Updates the upload_step.rb file to include checking for the feature flag, while preserving the existing and expected behavior in the default case of the flag being false

changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

-- What The change was causing other steps -- especially the phone step -- to fail.

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

-- What This commit represents a complete merging of the send_link_step feature tests into the new combined hybrid handoff / upload step (when feature flag is enabled).

-- What This prepares us for the removal of the send_link step, while preserving the existing testing for the step controller in its new form under the upload_step

app/views/idv/doc_auth/_combined_upload.html.erb

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

matthinz · 2023-02-17T20:36:14Z

I might have found an issue in the 50/50 case:

Turn feature flag on
Enter your phone number on the new combined page (/verify/doc_auth/upload), but don't submit the form
Stop the server, turn feature flag off, restart the server
Submit the new combined page
End up on the old "Take a photo with a phone" screen with the phone number field blank; have to re-enter my phone number

eric-gade · 2023-02-17T21:14:04Z

@matthinz Good catch. I just tried that locally and you are correct. Gonna think about this a bit

@matthinz

-- What As @matthinz pointed out, the current code does not account well for the following 50/50 states: 1. User is shown the new combined upload view, but submits the phone number to an instance with the feature flag off; 2. User is shown the original upload view, but clicks the phone button submitting to an instance where the feature flag is now on We handle this case by adding a request param _only_ to the form rendered by the new combined view. That view is only shown to users who have the feature flag enabled. The upload step will now check for that param -- called :combined -- and can determine regardless of the current state of the feature flag, that the user actually has submitted a phone number to the upload step itself, and that the send link step should be skipped.

eric-gade · 2023-02-17T23:27:55Z

@matthinz I have just pushed a commit (here) that I think fixes this and other potential 50/50 issues.

The basic idea is this: if the user was ever shown the new view, we will now know because we only tell the form to pass a specific param in the form specified in that view. So regardless of the feature flag state at the time the submission comes in, the upload step will know how to handle the situation.

This assumes that we have deployed with the feature flag off, and that all users are using the new upload step.

That said, I'm not entirely confident in this fix, but it seems to work.

matthinz · 2023-02-17T23:59:55Z

@eric-gade I think it is fair to assume that the code will go out initially with the feature flag off, then we'll turn it on. Your solution works for me!

matthinz

This looks really good! Are you planning on doing the "We sent a message to your phone" screen in this PR? Looks like there are a couple changes in the Figma there.

app/views/idv/doc_auth/_combined_upload.html.erb

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

eric-gade · 2023-02-21T15:33:54Z

@matthinz There is another ticket for dealing with the "link sent" screen redesign (LG-8231) and @kbighorse is already on the case.

aduth

LGTM 👍

aduth · 2023-02-22T15:52:44Z

app/views/idv/doc_auth/_combined_upload.html.erb

+  <%= t('doc_auth.info.combined_upload') %>
+</p>
+
+<div class="grid-row grid-gap grid-gap-1">


From the design reference, it looks like the gap should be 1rem, which would be 2 units in the design system.

Suggested change

<div class="grid-row grid-gap grid-gap-1">

<div class="grid-row grid-gap grid-gap-2">

app/views/idv/doc_auth/_combined_upload.html.erb

aduth · 2023-02-22T15:58:38Z

app/services/idv/steps/upload_step.rb

+
+      def formatted_destination_phone
+        raw_phone = permit(:phone)[:phone]
+        PhoneFormatter.format(raw_phone, country_code: 'US')


Recognizing there's a lot of legacy here being copied verbatim, I still want to flag that it's a bit problematic how we assume phone numbers are U.S., especially since PhoneInputComponent will provide us with a international_code parameter we can plug in here. I suspect this is a hold-over logic from previous implementation where the phone input was a plain input field.

aduth · 2023-02-22T16:00:23Z

app/views/idv/doc_auth/_combined_upload.html.erb

+      <%= t('doc_auth.info.tag') %>
+    </div>
+    <h2 class="margin-y-105">
+      <%= t('doc_auth.headings.combined_upload_from_phone') %>


In the future, maybe we circle back to unqualify these as "combined", since once the feature is enabled and the feature flag removed, I don't think we need to consider this as anything other than the default text for the screen.

Yeah that was the plan. Anything with "combined" will be renamed, including querystring params.

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

aduth · 2023-02-22T18:55:43Z

app/views/idv/doc_auth/_combined_upload.html.erb

+  <%= t('doc_auth.info.combined_upload') %>
+</p>
+
+<div class="grid-row grid-gap grid-gap-1">


This grid-gap-2 update was missed

eric-gade · 2023-02-22T19:09:25Z

@aduth follow up here: #7873

@matthinz

* LG-8796: Add recaptcha disclosure form at bottom of phone setup screen for new and existing accounts (#7827) * changelog: Upcoming Features, Authentication, Add recaptcha disclosure form at bottom of phone setup screen * styling fixes and dont hardcode login.gov * hide and show other movile service * add captcha submit button to add new phone * add to hide grecaptcha-badge * use recaptcha disclosure shared * remove empty space * lint recaptcha badge * Skip devices database query when identifier is null (#7856) changelog: Internal, Performance, Skip devices database query when identifier is null * Removed analytics check from password visibility toggle spec. (#7860) The test was flaky and the flag is unused, per Andrew Duthie. [skip changelog] * Removed use of Faker email creation in user factory primary email (#7861) * Removed use of Faker email creation in user factory primary email We suspect that using Faker email, rather than explicitly setting sequential emails, was occasionally creating collisions and leading to flaky test failures. [skip changelog] * Add 2023 SAML certs (#7862) (symlink 2022 ones to have non-blank files there) changelog: Internal, Certificates, 2023 SAML Certificate rotation * LG-8215 Update Event: IDV Reproof using Address verify by Mail (#7859) changelog: Internal, Attempts API, Update in tracking event * LG-8604 - Optimize s3 by streaming data w/ feature flag (#7843) * WIP - optimize s3 by streaming data changelog: Internal, Attempts API, Scalability Optimization * shifted a line - rubocop apparently wasn't picky about this * added feature flag. updated spec to reflect nested conditionals * cleaned up some comments. * broke out s3 response code into its own method. * cleaned up code based on feedback * updated default buffer size and made some clarifying changes based on feedback changelog: Internal, Attempts Api, Optimization: S3 Streaming * cleaned up more test descriptions. changelog: Internal, Attempts Api, Feature: S3 Streaming * Add logging ahead of removing Analytics logic (#7864) * Add Rails.logger.info logging near proposed call sites changelog: Internal, Logging, Add logging ahead of streamlining analytics events * Remove unused Authentication Presenter data (#7868) * Remove unused Authentication Presenter data changelog: Internal, Maintenance, Remove unused methods and variables * remove unused pivcac email data * remove more unused methods * Disable notify for good_job (#7870) changelog: Internal, Background Jobs, Update GoodJob and disable NOTIFY * LG-8423 log cancel analytics from barcode page (#7838) * analytics logged when user cancels from barcode pg * update event and add spec for for event in sessions controller * event and spec added for cancellations controller * changelog: Internal, Analytics Events, add cancel from barcode page analytics event * fix lint issue w/ spec * change step to barcode to make unique * use cancellation go back w/ extra attributes when user comes from barcode pg * remove new event and update session controller * update spec * change enrollment key * update cancel spec * check for enrollment and merge extra attributes and update tests * use pending enrollment and remove sp as it is redundant * update spec to create pending enrollment * LG-8901: New Hybrid Handoff (upload step) with feature flag (#7849) * Adding initial fields and new icons * Deleting useless hybrid handoff mobile tests * Pulling in send_link step functionality to upload_step * Adding feature flag for new combined hybrid handoff step * Adding additional checking for 50/50 conditions -- What As @matthinz pointed out, the current code does not account well for the following 50/50 states: 1. User is shown the new combined upload view, but submits the phone number to an instance with the feature flag off; 2. User is shown the original upload view, but clicks the phone button submitting to an instance where the feature flag is now on We handle this case by adding a request param _only_ to the form rendered by the new combined view. That view is only shown to users who have the feature flag enabled. The upload step will now check for that param -- called :combined -- and can determine regardless of the current state of the feature flag, that the user actually has submitted a phone number to the upload step itself, and that the send link step should be skipped. Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen * Unpack a dynamic send to make code easier to trace (#7869) * Remove now-unused constant changelog: Internal, Refactor, Small update to remove dynamic send in 2FA mixin * LG-8875 / LG-8877: Try to avoid race condition in document-capture-welcome (#7842) * "Convert" document-capture-welcome to typescript (It was already valid TS) * Rework camera check to block form submission Don't let the form submit until the promise we use to check for the presence / nonpresence of a camera has resolved. * Disable submit buttons while we wait * TEMP: Allow injecting delay into hasCamera() Set HAS_CAMERA_DELAY_MS in localStorage to configure. * Update app/javascript/packs/document-capture-welcome.ts Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Use SpinnerButtonComponent for welcome/agreement * Simplify cameraCheckPromise usage Eliminate additional local variable and just race the promises * Wrap agreement spinner button in <div> Adding margin-top-4 to button does not get applied to the spinner dots * changelog: User-Facing Improvements, Identity verification, help prevent mobile users from ending up in hybrid handoff flow * Log event from frontend for mobile/camera checks Ideally, we want to see if this code actually fixes anything. (Relates to LG-8877) Co-authored-by: Eric Gade <eric.gade@gsa.gov> * Refactor perf measuring slightly Pull out to a function that just augments the promise with perf data * Add spin_on_click to SpinnerButtonComponent Allow setting spin-on-click as needed. * Toggle spinner on form submission Just in case there is a form validation issue, don't start spinning until we're sure we can actually make the submission. * await trackEvent that could occur during form submission * Add idv_mobile_device_and_camera_check to AnalyticsEvents Document the event and make it so it's not prefixed with "Frontend:" * Revert "TEMP: Allow injecting delay into hasCamera()" This reverts commit 96cbc3e. * Don't use performance.measure() Complicated API, don't need it for what we're doing. --------- Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov> * LG-7836 Automatically refresh USPS auth token when necessary (#7808) * implement retry behavior with repetitive code * remove begin/rescue blocks and get failing tests * add retry options to faraday. tests still fail. * refactor implementation & spec * changelog:Bug Fixes, In-person proofing, refresh USPS auth token when request fails * attempt broader retry behavior. tests still fail. * attempt to refresh token before pinging USPS api. more tests fail * got first test to pass, need to get other expired token tests passing * get all expired token tests to pass * DRY up proofer test setup * add buffer to expires_at conditional * fix broken tests by adding redis cache setup * fix broken enrollment helper tests by adding caching setup * respond to feedback * remove no-op * Fixing missed grid gap sizing (follow up to LG-8901) (#7873) * Fixing missed grid gap sizing h/t @aduth changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen * LG-9025 Go to new SsnController from hybrid flow when feature flag is set (#7875) From the DocumentCaptureStep, we only want to redirect to the new SsnController if we are in the DocAuthFlow. This step is reused from the Hybrid Flow, and we don't want to go to the SsnController on mobile. In hybrid flow, we don't return to the DocumentCaptureStep, so redirect to SsnController from LinkSentStep when the feature flag is true as well. Duplicate the hybrid flow test with doc_auth_ssn_controller_enabled flag set to true and make sure it passes. Note that on my machine even on main without the feature flag I have to set a binding.pry and delay the mobile browser part of the test to get it to pass. changelog: Internal, refactor, fix hybrid flow with new SsnController (feature flagged) Co-authored-by: eric-gade <eric.gade@gsa.gov> --------- Co-authored-by: Malick Diarra <malick.diarra@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Osman Latif <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: Rwolfe-Nava <87499456+Rwolfe-Nava@users.noreply.github.com> Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <eric.gade@gsa.gov> Co-authored-by: eileen-nava <80347702+eileen-nava@users.noreply.github.com> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

eric-gade added 17 commits February 9, 2023 12:45

Initial commit of new combined hybrid handoff step

a4cd9d2

-- What Adding initial fields and new icons

Pulling in send_link step functionality to upload_step

56a7007

changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

Existing tests, feature flagged, are now passing

84525ac

changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

Undo bad analytics change

17e18d8

-- What The change was causing other steps -- especially the phone step -- to fail.

Adding test to ensure phone number is prefilled

f1a92ac

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> changelog: User-Facing Improvements, Hybrid Handoff Screen, Combining upload and send link steps into one screen

Test that combined hybrid proceeds to link sent with valid info

973e2a3

Adding feature flagged version of form_submit to new upload step

18fbc0f

Adding throttle feature test for combined upload step spec

027bccb

Completing addition of send_link feature specs to comb. upload

eb41f8c

-- What This commit represents a complete merging of the send_link_step feature tests into the new combined hybrid handoff / upload step (when feature flag is enabled).

Adding a plain step test for upload that copies send_link

912681e

-- What This prepares us for the removal of the send_link step, while preserving the existing testing for the step controller in its new form under the upload_step

Updating locales and styling for new combined hybrid upload step

b6b7992

Adding click helpers for new upload hybrid view and updating tests

a08b253

Normalizing yaml changes

512907f

Lints

fbfa4d3

Optimizing assets (new images)

f1bbb48

eric-gade requested a review from a team February 17, 2023 16:14

eric-gade added 2 commits February 17, 2023 11:16

Removing TODO

b67f3e5

Adding missing translations

43ff1d5

aduth reviewed Feb 17, 2023

View reviewed changes

eric-gade and others added 6 commits February 17, 2023 13:49

Update app/views/idv/doc_auth/_combined_upload.html.erb

c79df80

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Update app/views/idv/doc_auth/_combined_upload.html.erb

9d2782e

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Updating image size and erb indentation

22ee1f2

Updating grid for tablet and adding simple margin to icon images

fc84259

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Switching buttons back to wide, for idv consistency

441cb93

Lints!

0f83be7

matthinz reviewed Feb 18, 2023

View reviewed changes

aduth reviewed Feb 21, 2023

View reviewed changes

app/views/idv/doc_auth/_combined_upload.html.erb Outdated Show resolved Hide resolved

Update app/views/idv/doc_auth/_combined_upload.html.erb

f1c48c7

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

eric-gade force-pushed the eric-lg-8230 branch from 7f36267 to 41d49b3 Compare February 21, 2023 15:36

Updating grid styling for icon display and gaps

b09792f

eric-gade force-pushed the eric-lg-8230 branch from 41d49b3 to b09792f Compare February 21, 2023 15:39

eric-gade added 2 commits February 21, 2023 10:39

lint

01783a6

Updating phone input to be required in combined case

30a02c2

aduth approved these changes Feb 22, 2023

View reviewed changes

matthinz approved these changes Feb 22, 2023

View reviewed changes

Update app/views/idv/doc_auth/_combined_upload.html.erb

9bee92c

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

eric-gade merged commit 826f9ad into main Feb 22, 2023

eric-gade deleted the eric-lg-8230 branch February 22, 2023 18:27

aduth reviewed Feb 22, 2023

View reviewed changes

solipet mentioned this pull request Feb 23, 2023

Deploy RC 256 to Prod #7878

Merged

	<div class="grid-row grid-gap grid-gap-1">
	<div class="grid-row grid-gap grid-gap-2">

Conversation

eric-gade commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthinz commented Feb 17, 2023

Uh oh!

eric-gade commented Feb 17, 2023

Uh oh!

eric-gade commented Feb 17, 2023

Uh oh!

matthinz commented Feb 17, 2023

Uh oh!

matthinz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eric-gade commented Feb 21, 2023

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduth Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

aduth Feb 22, 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!

eric-gade Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

aduth Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

eric-gade commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eric-gade commented Feb 17, 2023 •

edited

Loading

aduth Feb 22, 2023 •

edited

Loading