LG-11355: IPP Opt-in Choices by allthesignals · Pull Request #9452 · 18F/identity-idp

allthesignals · 2023-10-25T18:42:38Z

🎫 Ticket

🛠 Summary of changes

Draft PR to add the IPP opt-in page.

If in_person_proofing_opt_in_enabled is on, direct from the agreement page to the proofing method selection page (/how-to-verify)

📜 Testing Plan

N/A

👀 Screenshots

IRL:
en

es

fr

Design spec:

soniaconnolly · 2023-10-25T21:47:15Z

app/controllers/idv/agreement_controller.rb

+        if IdentityConfig.store.in_person_proofing_opt_in_option
+          redirect_to idv_how_to_verify_url


It makes a lot of sense to branch off this new page from the Agreement page! Lmk if you want to pair on any of this - setting up an A/B test, wrangling changes to the flow, etc.

app/controllers/idv/how_to_verify_controller.rb

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

…in page after agreement and before remote flow

sheldon-b

Looking good! Left a few comments I expect are already on your to-do list

app/controllers/idv/how_to_verify_controller.rb

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

app/forms/idv/how_to_verify_form.rb

app/controllers/idv/how_to_verify_controller.rb

app/services/analytics_events.rb

app/controllers/idv/how_to_verify_controller.rb

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

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

config/application.yml.default

n1zyy

I'm still getting used to the team's norms so I'm not sure if I should leave an approval just yet, but this looks great to me!

I spotted two possible things that are incredibly minor; I mentioned them in case you end up pushing another revision and agree with the comments, but please feel free to ignore them as well.

app/forms/idv/how_to_verify_form.rb

n1zyy · 2023-11-02T21:20:33Z

config/locales/doc_auth/es.yml

      text_message: Enviamos un mensaje a su teléfono
      upload_from_computer: Continuar en esta computadora
      upload_from_phone: Utilice su teléfono para tomar las fotos
+      verify_at_post_office: Verifique su identidad en una Oficina de Correos


Oh gosh I feel bad only finding ridiculously trivial things (I guess that's because this looks great?), but it looks like @gina-yamada's PR moves "Oficina de Correos" to lowercase. Was that part of an official change we want to duplicate here?

(Also, I'm not actually trying to review copy changes in other languages at all; my eyes just happened to notice this one for some reason.)

Welll hmm. @carmenrosalop noticed that I had the English version lowercased and that it needed to be capitalized. I believe these translations were already provided with the term capitalized. I'll defer to Carmen on this!

Good catch! Yes, Carmen and I just discussed this in my PR because I also had the C capitalized. If you do a global search for Post Office in Spanish, they are all lower case.

@allthesignals @gina-yamada Hey! Two days ago we were debating whether we should keep the Spanish and French version of Post Office in upper case, and we looked more into it and on the USPS website, the translation to Spanish shows it with lowercase. So we're going to follow that approach for French and Spanish of keeping the translation of Post Office lower case.

Okay, updated, but can @gina-yamada give it a once over for me?

Good eye, @n1zyy!

Yes! Reviewing it now. I will comment when I am finished.

Co-authored-by: Matt Wagner <mattwagner@navapbc.com>

n1zyy · 2023-11-03T00:58:39Z

D’oh! Unless Carmen objects, I am A-OK with leaving this as you have it. Wanted to flag it in case it had an obvious resolution, but I didn’t mean to hold things up over it.

…

On Thu, Nov 2, 2023 at 18:04 Matt Gardner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In config/locales/doc_auth/es.yml <#9452 (comment)>: > @@ -214,7 +215,9 @@ es: text_message: Enviamos un mensaje a su teléfono upload_from_computer: Continuar en esta computadora upload_from_phone: Utilice su teléfono para tomar las fotos + verify_at_post_office: Verifique su identidad en una Oficina de Correos Welll hmm. @carmenrosalop <https://github.com/carmenrosalop> noticed that I had the English version lowercased and that it needed to be capitalized. I believe these translations were already provided with the term capitalized. I'll defer to Carmen on this! — Reply to this email directly, view it on GitHub <#9452 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACM2J63FO265JW375AN5R3YCQKFXAVCNFSM6AAAAAA6P27RBKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJRGMYTGMRZGI> . You are receiving this because you commented.Message ID: ***@***.***>

spec/requests/idv/how_to_verify_spec.rb

spec/forms/idv/how_to_verify_form.rb

gina-yamada · 2023-11-06T14:57:24Z

@allthesignals The page looks great! Here are some notes from my testing...

When `in_person_proofing_opt_in_enabled` = true

⛔ Summary of changes (in the PR description above) should be changed from in_person_proofing_opt_in_option to in_person_proofing_opt_in_enabled.
⛔ Sentence "You have the option to verify your identity online, or in person at a participating Post Office" should have a period. (The mocks have a period.) Looks like you got the period in Spanish and French- so just English needs to be updated.
⛔ You are missing the Cancel link at the bottom of the page
❓Should the view have the step indicator at the top? The mocks do not show it but wanted to call it out as it is on pages both before and after. I understand the steps might change depending on the flow to so maybe intentional not to have it on this particular page.

❓ Consider displaying a 404 for the view if the flag is off. If we turn on flag- and someone bookmarks or tries to visit that URL again- might be nice.
✅ Help Center links are working as expected
✅ Design & content matches mocks (except for missing period mentioned above)
✅ Hitting Continue with nothing selected
✅ Navigation is working as expected: verify/agreement directs to /verify/how_to_verify [pick on-line] to verify/hybrid_handoff
✅ Navigation is working as expected: verify/agreement directs to /verify/how_to_verify [pick post office] to /verify/hybrid_handoff
✅ Observed idv_doc_auth_how_to_verify_submitted & idv_doc_auth_how_to_verify_submitted events in logs
✅ Tests coverage when flag is off and on

When `in_person_proofing_opt_in_enabled` = false

✅ Navigation is working as expected: /verify/agreement directs to /verify/hybrid_handoff
✅ Typing /verify/how_to_verify in the url does direct me to the page and I stay where I am at in the flow
✅ Clicking Cancel and then No, continue goes back to verify/hybrid_handoff as I'd expect
✅ Working as I'd expect when flag is off

Just a few minor comments, nothing blocking IMO. If we decide more work on cancel or step nav- maybe just get this in and open a new tickets. If you make changes and/or tickets to handle others- let me know and I will approve.

gina-yamada · 2023-11-06T15:07:27Z

app/controllers/idv/how_to_verify_controller.rb

+
+    before_action :confirm_step_allowed
+
+    check_or_render_not_found -> { self.class.enabled? }


I am not seeing a not found page when I type in the url. We displayed the 404 view when we worked on the non-FSM views. I think it is a good idea. I am not sure this is working.

Hmmmm. Good eye. Thanks.

allthesignals · 2023-11-06T15:14:12Z

⛔ You are missing the Cancel link at the bottom of the page

I think @sheldon-b will add this in a follow-up PR? This feature depends on the particulars of routing so I skipped it.

gina-yamada · 2023-11-06T15:26:08Z

spec/features/idv/doc_auth/how_to_verify_spec.rb

+      end
+    end
+
+    it 'displays expected content and requires a choice' do


What do you think about adding another context block and moving this it block inside?
context 'opt-in ipp is turned on' do

sheldon-b

This looks good! With and without the flag -- routing behaves as I expect. So when the feature flag is turned off it skips the new page. With the feature flag turned on it shows the new page and choosing either option proceeds to remote proofing (which is what we want for this ticket -- conditional routing will come next)

Responsiveness looks right using dev tools

One small request to update the routing to match the previous behavior when the user chooses remote proofing

spec/forms/idv/how_to_verify_form_spec.rb

app/controllers/idv/how_to_verify_controller.rb

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

app/controllers/idv/how_to_verify_controller.rb

aduth · 2023-11-07T21:30:40Z

spec/features/idv/doc_auth/how_to_verify_spec.rb

I'm seeing some flakey test failures from this file on an unrelated pull request. Can you take a look @allthesignals ?

https://gitlab.login.gov/lg/identity-idp/-/jobs/850239

allthesignals force-pushed the wmg/11355-new-page branch from d4df874 to ca9d0ca Compare October 25, 2023 18:44

Add basic routing and view. Draft redirection logic.

e0f8a46

allthesignals force-pushed the wmg/11355-new-page branch from ca9d0ca to e0f8a46 Compare October 25, 2023 18:47

soniaconnolly reviewed Oct 25, 2023

View reviewed changes

zachmargolis reviewed Oct 25, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

Add some radio buttons

4389f62

allthesignals commented Oct 26, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

allthesignals and others added 6 commits October 26, 2023 11:50

Update app/controllers/idv/how_to_verify_controller.rb

33f77a1

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Style it up

2676e95

Lint it up

b1ca9da

changelog: User-Facing improvements, In-Person Proofing, Add IPP opt-…

8fb122f

…in page after agreement and before remote flow

404 the page if disabled

05b4886

Use a const for options

f056a21

allthesignals force-pushed the wmg/11355-new-page branch from 0fcce87 to f056a21 Compare October 26, 2023 22:13

sheldon-b reviewed Oct 27, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Show resolved Hide resolved

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

app/controllers/idv/how_to_verify_controller.rb Show resolved Hide resolved

allthesignals and others added 5 commits October 27, 2023 10:32

Update app/controllers/idv/how_to_verify_controller.rb

27fe29b

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

Add entries for translations

28a21ca

Merge branch 'main' into wmg/11355-new-page

683a3d9

Translate and analytics

7cdf432

Fix missing tags

111682c

sheldon-b reviewed Oct 27, 2023

View reviewed changes

app/forms/idv/how_to_verify_form.rb Show resolved Hide resolved

soniaconnolly reviewed Oct 27, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

app/services/analytics_events.rb Outdated Show resolved Hide resolved

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Oct 27, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

app/views/idv/how_to_verify/show.html.erb Outdated Show resolved Hide resolved

allthesignals and others added 7 commits October 30, 2023 11:38

Update app/controllers/idv/how_to_verify_controller.rb

ba7a0ae

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

New analytics format

80f80c9

Merge branch 'main' into wmg/11355-new-page

71c4929

Prefer strings

78d1d58

Use constants this way

e21a262

Move into a class

61824c3

Scope styles to tablet

25ba0a9

allthesignals force-pushed the wmg/11355-new-page branch from 4fefd66 to b24156e Compare November 2, 2023 16:31

allthesignals added 2 commits November 2, 2023 14:29

Use the radio_button helpers to save some manual wiring

df8b772

Include hidden with same name

8e86004

sheldon-b reviewed Nov 2, 2023

View reviewed changes

config/application.yml.default Outdated Show resolved Hide resolved

allthesignals and others added 2 commits November 2, 2023 16:33

Change feature flag name

3e9ae6d

Require form selection

bbe0370

n1zyy reviewed Nov 2, 2023

View reviewed changes

Update app/forms/idv/how_to_verify_form.rb

38dcff3

Co-authored-by: Matt Wagner <mattwagner@navapbc.com>

aduth reviewed Nov 3, 2023

View reviewed changes

spec/requests/idv/how_to_verify_spec.rb Outdated Show resolved Hide resolved

spec/forms/idv/how_to_verify_form.rb Show resolved Hide resolved

allthesignals added 5 commits November 3, 2023 09:52

Remove generated cruft and append _spec to the end of the test file

41c3240

Remove unused verification options and auto freeze string literals

2c84102

Lowercase post office translation

eeb1c34

Merge branch 'main' into wmg/11355-new-page

8cb59f2

set title instead of calling as help

f0f43e5

gina-yamada reviewed Nov 6, 2023

View reviewed changes

sheldon-b approved these changes Nov 6, 2023

View reviewed changes

spec/forms/idv/how_to_verify_form_spec.rb Show resolved Hide resolved

app/controllers/idv/how_to_verify_controller.rb Outdated Show resolved Hide resolved

Update app/controllers/idv/how_to_verify_controller.rb

b138bfd

Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov>

allthesignals force-pushed the wmg/11355-new-page branch from 8649eee to b138bfd Compare November 6, 2023 18:35

allthesignals merged commit bf6b8b3 into main Nov 6, 2023

allthesignals deleted the wmg/11355-new-page branch November 6, 2023 18:49

gina-yamada reviewed Nov 6, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Show resolved Hide resolved

gina-yamada reviewed Nov 6, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Show resolved Hide resolved

soniaconnolly reviewed Nov 6, 2023

View reviewed changes

app/controllers/idv/how_to_verify_controller.rb Show resolved Hide resolved

matthinz mentioned this pull request Nov 6, 2023

Deploy RC 330 to Prod #9554

Merged

aduth reviewed Nov 7, 2023

View reviewed changes

aduth mentioned this pull request Sep 10, 2024

Remove unused radio button styles #11221

Merged

		if IdentityConfig.store.in_person_proofing_opt_in_option
		redirect_to idv_how_to_verify_url


		before_action :confirm_step_allowed

		check_or_render_not_found -> { self.class.enabled? }

Conversation

allthesignals commented Oct 25, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sheldon-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

n1zyy left a comment

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!

n1zyy commented Nov 3, 2023 via email

Uh oh!

Uh oh!

Uh oh!

gina-yamada commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

When in_person_proofing_opt_in_enabled = true

When in_person_proofing_opt_in_enabled = false

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allthesignals commented Nov 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheldon-b left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

allthesignals commented Oct 25, 2023 •

edited

Loading

gina-yamada commented Nov 6, 2023 •

edited

Loading

When `in_person_proofing_opt_in_enabled` = true

When `in_person_proofing_opt_in_enabled` = false