LG-7393 Two-Screen Phone Verification (WIP) by theabrad · Pull Request #7330 · 18F/identity-idp

theabrad · 2022-11-10T21:41:43Z

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Changed the phone verification in the IdV flow from 3 screens to two. Placed the One Time Password delivery method on the phone verification page.

👀 Screenshots

Before:

After:

zachmargolis · 2022-11-10T21:45:49Z

spec/controllers/idv/phone_controller_spec.rb

+          idv_phone_form:
+            {
+              phone: improbable_phone_number,
+              otp_delivery_preference: :🎷,


app/views/idv/phone/new.html.erb

aduth · 2022-11-16T18:04:40Z

config/locales/forms/en.yml

      buttons:
        delete: Delete email address
-    example: 'Example:'
+    example: 'example:'


I believe this labels is used with hint labels throughout the application and this change may impact those instances. Just want to confirm we're aware / okay with that.

I believe this labels is used with hint labels throughout the application and this change may impact those instances. Just want to confirm we're aware / okay with that.

@theabrad @eric-gade @jmhooper Thoughts on this question?

I only see this currently being used as a hint in one place (here), unless I've missed more instances. Since it's in one place I think the following two options might work:

Add a hints section to this form yml file and put the lowercase example text there, keeping the change in question;

Revert the change, and have whatever view that renders the text be responsible for capitalizing the first letter as needed.

Personally I'd pick the first option so we don't much about with more code in the views

We have a couple other "example" hints in the application (e.g. phone input, memorable date), so the main reason for asking is about consistency, since I think we should be consistent with the casing throughout the application, and this should be reflected in relevant design system guidance as well as Figma flow designs.

Could y'all confirm the change with UX folks and create tickets if we want to change the case?

-- What Prior to this branch, all IDV step pages had the same "continue" button, meaning `click_idv_continue` would find the same localization on a button ("Continue" for english) and click that to proceed. The new consolidated phone verification step breaks that expectation. Instead of casing for this all over the test suite, we are introducing a replacement `idv_click_continue_for_step` in the helpers, which takes the symbol for the current step name as its argument and then clicks the correct "continue" button -- which for the phone step is now "Send security code" (and can be manually called from `click_idv_send_security_code`). This commit represents an initial implementation of this new helper and a refactor of one of the example files that needs to use it.

-- What This method was previously on the otp_selection_controller, which is on the way to being deprecated. Some existing code required that this method now be present on the phone controller.

-- What The PhoneForm was changed to work with the otp phone verification, but this interfered with other kinds of phone validation controls because the form now expects an :otp_delivery_preference key. This being nil will fail in other contexts. With this commit we add a (rough, perhaps should be refactored) solution

-- What Some of the helper methods still assumed an extra "select otp verification" preference step. This commit removes those assumptions from the helpers.

-- What This test -- unlike its counterpart on main -- will end up taking the user to an account locked page, and for the moment it's not clear why.

-- What There were a couple of more methods from the deprecated otp_selection_controller that needed to be moved to phone_controller. There are also still some assumptions about the otp_delivery_preference that are unclear

-- What This is the step that has been combined with the phone number step, and therefore it is no longer needed.

-- What There were specific assumptions in place now about the default delivery method needed on PhoneForm submissions.

mitchellhenke · 2022-11-22T17:05:43Z

app/controllers/idv/otp_delivery_method_controller.rb

@@ -1,121 +0,0 @@
-module Idv
-  class OtpDeliveryMethodController < ApplicationController


I think this controller (and any associated behavior) will need to remain for one release cycle for backwards compatibility.

It's possible during the deploy while v1 and v2 are both live, the v1 will redirect to this controller and the v2 could receive the request and fail since the route doesn't exist (the reverse is also possible).

Ah, yes. This is going to require further work, I think, since I gutted some of the old stuff prematurely.

solipet

One TODO on a commented out spec.

solipet · 2022-11-22T19:44:12Z

spec/features/idv/steps/phone_otp_verification_step_spec.rb

  end

-  it 'redirects back to the step with an error if Telephony raises an error on resend' do
+  xit 'redirects back to the step with an error if Telephony raises an error on resend' do


Need to resolve what is going on with this test.

This reverts commit 7b26b8f.

This reverts commit c252d79.

[skip changelog]

aduth · 2022-11-28T18:52:48Z

app/views/idv/phone/new.html.erb

+      <%= f.radio_button(
+            :otp_delivery_preference,
+            :sms,
+            checked: true, # We want SMS to be default checked


How would this work if there's a vendor outage for SMS, since it would be checked but also disabled. IIRC in other screens this will be something more like... ? (with a corresponding assignment for "Voice" as well to select by default in the scenario that SMS is disabled)

Suggested change

checked: true, # We want SMS to be default checked

checked: !VendorStatus.new.vendor_outage?(:sms), # We want SMS to be default checked

these events will soon be removed once the new two phone page gets implemented. the otp_delivery_method selection step does not occur in this regression test

…-two-screen-phone

added in adapter to the analytics call for idv_phone_confirmation_otp_sent

…-two-screen-phone

zachmargolis · 2022-12-02T17:18:31Z

app/controllers/idv/phone_controller.rb

+    # Migrated from otp_delivery_method_controller
+    def save_delivery_preference
+      original_session = idv_session.user_phone_confirmation_session
+      idv_session.user_phone_confirmation_session = Idv::PhoneConfirmation::ConfirmationSession.new(


whoop sorry the rename was a bit different in #7409

Suggested change

idv_session.user_phone_confirmation_session = Idv::PhoneConfirmation::ConfirmationSession.new(

idv_session.user_phone_confirmation_session = Idv::PhoneConfirmationSession.new(

theabrad added 4 commits November 2, 2022 15:36

change idv phone page to new text

7868ae3

add otp selection to verfiy phone page

03f112f

added opt_delivery_preference to phone controller

98de538

WIP fix tests in phone_controller_spec

8e836b1

zachmargolis reviewed Nov 10, 2022

View reviewed changes

theabrad and others added 2 commits November 10, 2022 17:31

fix lint

abfadba

Setting SMS option as checked by default and adding test

8650aee

aduth reviewed Nov 16, 2022

View reviewed changes

eric-gade added 22 commits November 16, 2022 13:47

Adding new "send code" click helper

c87b2c0

Fixing re-entry test

d4e4d0f

Updating failed phone job example with correct button

84d4d36

Fixing some of the example test assumptions

45922c9

Updating to correct path assumptions in phone step tests

2278b95

Adding missing method to phone controller

304f128

-- What This method was previously on the otp_selection_controller, which is on the way to being deprecated. Some existing code required that this method now be present on the phone controller.

Updating helpers with new flow

163e4a0

-- What Some of the helper methods still assumed an extra "select otp verification" preference step. This commit removes those assumptions from the helpers.

Fixing tests

e42b18f

Skipping failing test temporarily

12b8add

-- What This test -- unlike its counterpart on main -- will end up taking the user to an account locked page, and for the moment it's not clear why.

Fixing more tests, migrating methods

809a853

-- What There were a couple of more methods from the deprecated otp_selection_controller that needed to be moved to phone_controller. There are also still some assumptions about the otp_delivery_preference that are unclear

Removing deprecated delivery selection step test

bcb3023

Removing deprecated otp_selection_submitted from analytics

c252d79

Fixing deprecated calls to otp_delivery_selection_step helpers

d100216

Removing old otp_delivery_method files

7b26b8f

-- What This is the step that has been combined with the phone number step, and therefore it is no longer needed.

Fixing phone controller spec assumptions

4f0e016

-- What There were specific assumptions in place now about the default delivery method needed on PhoneForm submissions.

Removing old tests and fixing skipped tests

f9877be

Fixing lints

722dcc6

Removing unused localizations

7bc17d0

Updating babel-loader to stable version

50e6bf7

Normalizing yaml

5ca9ea9

theabrad added 3 commits November 21, 2022 15:59

change translation of security code to one-time-code

d712065

remove test that rate limits otp_delivery_method_page

53e67cf

remove unused translation

0871b1a

theabrad marked this pull request as ready for review November 22, 2022 14:17

theabrad requested a review from a team November 22, 2022 14:17

mitchellhenke reviewed Nov 22, 2022

View reviewed changes

solipet reviewed Nov 22, 2022

View reviewed changes

eric-gade and others added 4 commits November 22, 2022 15:36

Revert "Removing old otp_delivery_method files"

a3ddc69

This reverts commit 7b26b8f.

Revert "Removing deprecated otp_selection_submitted from analytics"

08e8377

This reverts commit c252d79.

Restoring missing localization keys for otp_delivery_method

dada66d

[skip changelog]

add back in missing translations

1f5799d

aduth reviewed Nov 28, 2022

View reviewed changes

theabrad added 9 commits November 29, 2022 11:33

check for vendor outage before sms

a670eb5

allow redirect to old otp_delivery_method_page

0fd46ac

remove phone delivery otp from analytics regression tests

0615ca2

these events will soon be removed once the new two phone page gets implemented. the otp_delivery_method selection step does not occur in this regression test

fix reporting of delivery method tracking to irs

d555a9c

fix inputs to phone form in phone_controller_spec

d774c31

Merge branch 'main' of github.com:18F/identity-idp into abrad-lg-7393…

f96d54a

…-two-screen-phone

fix analytics on phone_controller

b24247b

added in adapter to the analytics call for idv_phone_confirmation_otp_sent

phone otp resend test pass

2235071

fix test

a59aab8

theabrad requested a review from a team December 1, 2022 21:25

theabrad added 2 commits December 2, 2022 11:50

Merge branch 'main' of github.com:18F/identity-idp into abrad-lg-7393…

a299b5b

…-two-screen-phone

move phone confirmation session to Idv::PhoneConfirmation session

af74400

zachmargolis reviewed Dec 2, 2022

View reviewed changes

fixup

a47c97c

jmhooper approved these changes Dec 2, 2022

View reviewed changes

theabrad merged commit 6465aba into main Dec 2, 2022

theabrad deleted the abrad-lg-7393-two-screen-phone branch December 2, 2022 18:06

aduth mentioned this pull request Dec 5, 2022

LG-8297: Capitalize "Example" hint labels #7432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-7393 Two-Screen Phone Verification (WIP)#7330

LG-7393 Two-Screen Phone Verification (WIP)#7330
theabrad merged 52 commits intomainfrom
abrad-lg-7393-two-screen-phone

theabrad commented Nov 10, 2022 •

edited

Loading

Uh oh!

zachmargolis Nov 10, 2022

Uh oh!

Uh oh!

aduth Nov 16, 2022

Uh oh!

aduth Dec 5, 2022

Uh oh!

eric-gade Dec 5, 2022

Uh oh!

aduth Dec 5, 2022

Uh oh!

mitchellhenke Nov 22, 2022

Uh oh!

eric-gade Nov 22, 2022

Uh oh!

solipet left a comment

Uh oh!

solipet Nov 22, 2022

Uh oh!

aduth Nov 28, 2022

Uh oh!

zachmargolis Dec 2, 2022 •

edited

Loading

Uh oh!

theabrad Dec 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

		@@ -1,121 +0,0 @@
		module Idv
		class OtpDeliveryMethodController < ApplicationController

	checked: true, # We want SMS to be default checked
	checked: !VendorStatus.new.vendor_outage?(:sms), # We want SMS to be default checked

	idv_session.user_phone_confirmation_session = Idv::PhoneConfirmation::ConfirmationSession.new(
	idv_session.user_phone_confirmation_session = Idv::PhoneConfirmationSession.new(

Conversation

theabrad commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

👀 Screenshots

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!

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!

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

zachmargolis Dec 2, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

theabrad commented Nov 10, 2022 •

edited

Loading

zachmargolis Dec 2, 2022 •

edited

Loading