LG-9072 Applicant verify your information page "update state ID" workflows by gina-yamada · Pull Request #8288 · 18F/identity-idp

gina-yamada · 2023-04-26T16:14:29Z

🎫 Ticket

LG-9072 Applicant verify your information page "update state ID" workflows

🛠 Summary of changes

I am making workflow updates only after the Verify your info page (only behind the in_person_capture_secondary_id_enabled feature flag) to capture different scenarios/flows a user might encounter related to state id address and their current residential address when a user selects Update on Verify your information for State-issued ID.

📜 Testing Plan

Step 1: Set env variable in_person_capture_secondary_id_enabled to true to test the following.
Step 1A: Test Scenario 1 Given a user who changes their indication from current address IS NOT LISTED to IS LISTED (“No” to “Yes”)

1. Route user to Update info on your state ID page
2. When user changes the radio options from "No" to "Yes" and clicks Update, route user back to the Verify Info page. (Make some updates to state ID inputs before clicking Update.)
3. Show the state ID address for the residential address
4. If any additional changes were made to the State ID page, they should be reflected on the Verify Info page

Step 1B: Test Scenario 2 Given a user who changes their indication from IS LISTED to IS NOT LISTED (“Yes” to “No”)

1. Route user to Update info on your state ID page
2. When user changes the radio options from "Yes" to "No" and clicks Update, route the user to a blank Enter your current residential address page. (Make some updates to state ID inputs before clicking Update too.)
3. When user clicks Continue, route user back to the Verify Info page (skip SSN page)
4. Show updates to residential address

Step 1C Test Scenario 4: Given a user updates state ID page and DOES NOT change the radio options (this should be current behavior)

1. Route user to Update info your state ID page
2. When a user clicks Update, route user back to Verify info page and show their updates in the State ID section

Step 2: Set env variable in_person_capture_secondary_id_enabled to false and ensure the existing flows have not been affected. (I compared my branch to main with in_person_capture_secondary_id_enabled to false. I did not notice any difference.) Please confirm/test this out as well.

👀 Screenshots

You will need to walk through the flow to test this out so no screenshots

…applicant-update-workflows

Variables were recently renamed

…ur information page- update state ID workflow

…applicant-update-workflows

svalexander · 2023-04-27T22:35:09Z

spec/services/idv/steps/in_person/address_step_spec.rb

+
+  describe '#updating_address effects on extra_view_variables.updating_address' do
+    let(:address1) { '123 Fourth St' }
+    let(:uuid) { '0000' }


we should delete this as it's not being used

svalexander · 2023-04-27T22:36:18Z

spec/services/idv/steps/in_person/address_step_spec.rb

  end
+
+  describe '#updating_address effects on extra_view_variables.updating_address' do
+    let(:address1) { '123 Fourth St' }


let's use the constant we have for address1 in this test: InPersonHelper::GOOD_ADDRESS1

svalexander · 2023-04-27T22:47:53Z

spec/services/idv/steps/in_person/state_id_step_spec.rb

+    let(:pii_from_user) { flow.flow_session[:pii_from_user] }
+    let(:params) { ActionController::Parameters.new({ state_id: submitted_values }) }
+    let(:capture_secondary_id_enabled) { true }
+    let(:dob) { '1972-02-23' }


for lines 247-260 lets use the constants in the in_person_helper file

svalexander · 2023-04-28T13:06:04Z

spec/services/idv/steps/in_person/state_id_step_spec.rb

+        and_return(enrollment)
+    end
+
+    # User picks "Yes, I live at the address on my state-issued ID"


can we break these comments up and put them in the test at the point were we're doing each behavior described?

eileen-nava

I left some thoughts. Please feel free to ask follow-up questions. Thanks! 🙏🏻

eileen-nava · 2023-04-28T15:24:43Z

app/services/idv/steps/in_person/state_id_step.rb

          end
+
+          if capture_secondary_id_enabled?
+            if (initial_state_of_same_address_as_id === 'true' &&


I'd suggest making this syntax more Rubyesque by

changing === to ==

removing the parentheses from the if statement on line 34

Right now, the syntax looks more like a JavaScript conditional statement.

I’m interested in ways to keep this conditional logic readable and maintainable. What would you think about a refactor that uses nesting and helper methods, like…..

if capture_secondary_id_enabled? if pii_from_user[:same_address_as_id] == 'true' copy_state_id_address_to_residential_address(pii_from_user) mark_step_complete(:address) end if initial_state_of_same_address_as_id === 'true' && pii_from_user[:same_address_as_id] === 'false' clear_residential_address(pii_from_user) mark_step_incomplete(:address) end end

And then the helper methods could be defined further down....

private def copy_state_id_address_to_residential_address pii_from_user[:address1] = flow_params[:identity_doc_address1] pii_from_user[:address2] = flow_params[:identity_doc_address2] pii_from_user[:city] = flow_params[:identity_doc_city] pii_from_user[:state] = flow_params[:identity_doc_address_state] pii_from_user[:zipcode] = flow_params[:identity_doc_zipcode] end def clear_residential_address pii_from_user.delete(:address1) pii_from_user.delete(:address2) pii_from_user.delete(:city) pii_from_user.delete(:state) pii_from_user.delete(:zipcode) end

It might also be cute to have a one-liner method for flow_session[:pii_from_user][:same_address_as_id] == 'true', like state_id_address_and_residential_address_initially_same?. Although, yikes, that is a long method name! 😓 You could also make a method for initial_state_of_same_address_as_id == 'true' && pii_from_user[:same_address_as_id] == 'false', like state_id_address_and_address_diverge?. These are all suggestions and the method names are very rough draft. Let me know what you think! :)

eileen-nava · 2023-04-28T15:56:55Z

spec/services/idv/steps/in_person/address_step_spec.rb

    end
  end
+
+  describe '#updating_address effects on extra_view_variables.updating_address' do


What would you think about combining these specs with the existing #extra_view_variables describe block in this file?

eileen-nava · 2023-04-28T16:01:10Z

spec/services/idv/steps/in_person/state_id_step_spec.rb

+    end
+
+    before(:each) do
+      allow(IdentityConfig.store).to receive(:in_person_capture_secondary_id_enabled).


You can delete line 281. Lines 247 and 283 are sufficient setup.

spec/services/idv/steps/in_person/state_id_step_spec.rb

eileen-nava · 2023-04-28T16:12:40Z

spec/services/idv/steps/in_person/state_id_step_spec.rb

    end
  end
+
+  describe '#call' do


What do you think about combining this with the other #call describe block in this spec? Or changing this line so that there aren't two describe blocks with the same name in one file?

…applicant-update-workflows

NavaTim · 2023-05-01T17:58:52Z

spec/services/idv/steps/in_person/state_id_step_spec.rb

I recommend additionally testing the following cases in this file.

:same_address_as_id field is not updated while DAV is enabled

:same_address_as_id is set from 'false' to 'true' while DAV is enabled

JackRyan1989 · 2023-05-01T18:23:10Z

spec/features/idv/in_person_spec.rb

+        complete_prepare_step(user)
+      end
+
+      it 'but does not update their previous selection of "Yes,


This is super tiny, but can you remove the 'but' from start of the test names? I keep reading it as "it but does not" and I get confused.

JackRyan1989 · 2023-05-01T18:23:31Z

spec/features/idv/in_person_spec.rb

+        )
+      end
+
+      it 'and updates their previous selection from "Yes" TO "No, I live at a different address"' do


Same with the 'ands'.

gina-yamada · 2023-05-01T18:29:40Z

@18F/identity-joy-designers I did not take screenshots because it is really about the redirects here. I am happy to jump on a call and walk through each of the scenarios so that you can see the flow

eileen-nava

This looks good to me. That being said, I think Jack and Tim had good points about testing.

JackRyan1989

LGTM!

…applicant-update-workflows

kellular

Gina walked me through the scenarios in her local env. LGTM, thank you!

gina-yamada added 2 commits April 26, 2023 09:21

LG-9072 Update state ID workflows

5e3c9a9

LG-9072 Add testing for workflow updates

b80cc7a

gina-yamada marked this pull request as draft April 26, 2023 16:14

gina-yamada added 12 commits April 26, 2023 12:42

LG-9072 Fix lint issues

c009405

LG-9072 Fix lint issues

6d21332

LG-9072 Fix link issues

e1f4539

LG-9072 Fix lint issues

303d424

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

21f79e2

…applicant-update-workflows

LG-9072 Update all instance of state_id_... I introduced.

f3933ff

Variables were recently renamed

changelog: Upcoming Features, In-person proofing, Applicant verify yo…

24379db

…ur information page- update state ID workflow

LG-9072 Updates from var name changes

9d0c7ae

LG-9072 Removed unnecessary change and testing

661087a

LG-9072 Update what I am eval to hit clearing addr

001a166

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

37302c1

…applicant-update-workflows

LG-9072 update text

dc3c7bf

gina-yamada marked this pull request as ready for review April 27, 2023 21:10

gina-yamada changed the title ~~DRAFT LG-9072 Applicant verify your information page "update state ID" workflows~~ LG-9072 Applicant verify your information page "update state ID" workflows Apr 27, 2023

gina-yamada requested review from a team, NavaTim and tomas-nava April 27, 2023 21:25

svalexander reviewed Apr 27, 2023

View reviewed changes

svalexander reviewed Apr 28, 2023

View reviewed changes

gina-yamada added 3 commits April 28, 2023 08:19

LG-9072 Use helper to set variable, del unused var

1800007

LG-9072 Use helper to set var, move comments

a9827da

LG-9072 update comments

6ff7329

gina-yamada requested a review from svalexander April 28, 2023 14:53

eileen-nava reviewed Apr 28, 2023

View reviewed changes

LG-9072 tests for flow and updates

f44a31b

gina-yamada added 5 commits May 1, 2023 09:29

LG-9072 Combine specs with existing describe block

b5a8342

LG-9072 Remove unnecessary setup code

3fc24a3

LG-9072 Combine call specs

692590d

LG-9072 Use helper methods, lint

f616a4f

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

ddf57dd

…applicant-update-workflows

gina-yamada requested a review from eileen-nava May 1, 2023 16:35

NavaTim approved these changes May 1, 2023

View reviewed changes

JackRyan1989 reviewed May 1, 2023

View reviewed changes

gina-yamada requested a review from a team May 1, 2023 18:23

kellular requested review from a team and kellular and removed request for a team and tomas-nava May 1, 2023 19:19

eileen-nava approved these changes May 1, 2023

View reviewed changes

LG-9072 Remove but & and from test descriptions

608dc45

JackRyan1989 approved these changes May 2, 2023

View reviewed changes

gina-yamada added 2 commits May 2, 2023 08:30

LG-9072 Add more tests, linter

5b66b8a

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

aca81eb

…applicant-update-workflows

kellular approved these changes May 2, 2023

View reviewed changes

gina-yamada merged commit 4886b29 into main May 2, 2023

gina-yamada deleted the gina/lg-9072-applicant-update-workflows branch May 2, 2023 15:21

mdiarra3 mentioned this pull request May 4, 2023

Deploy RC 277 to Prod #8338

Merged

Conversation

gina-yamada commented Apr 26, 2023 • edited by eileen-nava 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!

svalexander Apr 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eileen-nava 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!

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!

gina-yamada commented May 1, 2023

Uh oh!

eileen-nava left a comment

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

kellular left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gina-yamada commented Apr 26, 2023 •

edited by eileen-nava

Loading

svalexander Apr 27, 2023 •

edited

Loading