Skip to content

LG-10886 start moving ssn to idv session#9129

Merged
soniaconnolly merged 13 commits intomainfrom
sonia-lg-10886-start-moving-ssn-to-idv-session
Sep 7, 2023
Merged

LG-10886 start moving ssn to idv session#9129
soniaconnolly merged 13 commits intomainfrom
sonia-lg-10886-start-moving-ssn-to-idv-session

Conversation

@soniaconnolly
Copy link
Contributor

@soniaconnolly soniaconnolly commented Aug 31, 2023

🎫 Ticket

LG-10886

🛠 Summary of changes

This implements the first half of moving ssn to idv_session. Part 2 will remove references to ssn as an attribute of flow_session[:pii_from_doc] and flow_session[:pii_from_user].

When a user moves through IdV, their SSN is currently stored as an attribute of :pii_from_doc or :pii_from_user. As part of moving :pii_from_doc out of flow_session, this gives ssn its own entry in idv_session.

Reasons:

  • SSN is not actually read from a document; the user types it in directly, so it does not really belong in :pii_from_doc
  • It is awkward to handle :ssn during the transition when :pii_from_doc is in the flow_session and also idv_session
  • SSN is entered in a separate page for In Person flow and is currently stored separately in :pii_from_user. If ssn has its own attribute, both flows could save it to idv_session, and it becomes more possible to merge these two screens.

📜 Testing Plan

  • Create account, start IdV
  • Edit SSN in VerifyInfo step
  • Expect SSN to be saved correctly in verified profile
  • Create new account, start IdV
  • Upload error yaml to enter In Person flow
  • Edit SSN in VerifyInfo step
  • Expect SSN to be saved correctly when barcode is generated.

@soniaconnolly soniaconnolly requested review from a team, amirbey, jmhooper and svalexander August 31, 2023 22:26
@matthinz
Copy link
Contributor

matthinz commented Sep 1, 2023

I am 👍 on the general idea, but I have thoughts on some of the specifics.

I think there is value in the term pii_from_user, since it incorporates the source of the data into the name of the variable.

Most (all?) of the attributes currently on IdvSession don't contain user input. I wonder if we could enable something like:

idv_session.pii_from_user[:ssn]

or even

idv_session.pii_from_user.ssn

I could envision folks getting confused about the source of idv_session.ssn, like maybe it got pulled in from the drivers license.

(Though I guess when you edit your address currently we just write that data back into pii_from_doc, so there goes the consistency.)

@svalexander
Copy link
Contributor

SSN is not actually read from a document; the user types it in directly, so it does not really belong in :pii_from_doc
This is such a good point, I've never thought about it before you mentioned it in this pr!


def show
@ssn_form = Idv::SsnFormatForm.new(current_user, flow_session)
@ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn, flow_session)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This form only consumes the flow_session to get the ssn (from pii_from_doc), so we can remove the flow_session argument here.


def show
@ssn_form = Idv::SsnFormatForm.new(current_user, flow_session)
@ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn, flow_session)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass in only the ssn instead of flow_session, and either pass in idv_session.ssn or flow_session[:ssn].

@error_message = nil

@ssn_form = Idv::SsnFormatForm.new(current_user, flow_session)
@ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn, flow_session)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

end

def initialize(user, flow_session = {})
def initialize(user, incoming_ssn, flow_session = {})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take only ssn as an argument.

@soniaconnolly
Copy link
Contributor Author

soniaconnolly commented Sep 6, 2023

idv_session.pii_from_user.ssn

I could envision folks getting confused about the source of idv_session.ssn, like maybe it got pulled in from the drivers license.
(Though I guess when you edit your address currently we just write that data back into pii_from_doc, so there goes the consistency.)

We want to use idv_session.pii_from_user to replace flow_session[:pii_from_user] in the in person flow. I'd rather have ssn as its own attribute to get rid of the extra checking for whether the parent is there when it is accessed, but I'm open to being convinced. @jmhooper and I were discussing having the address as a separate attribute for exactly the reason you said, although that might get complicated with the address in pii_from_doc as well and needing to check for both.

@soniaconnolly soniaconnolly marked this pull request as ready for review September 6, 2023 23:54
}.merge(ab_test_args)
end

let(:idv_session) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This let won't affect idv_session

Comment on lines -163 to -166
expect(subject.idv_session.resolution_successful).to be_blank
expect(subject.idv_session.profile_confirmation).to be_blank
expect(subject.idv_session.vendor_phone_confirmation).to be_blank
expect(subject.idv_session.user_phone_confirmation).to be_blank
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These attributes are deleted from the spec because they have to be blank because they are set later by the phone step or review step, and ssn step can't be accessed when they are set.

@gina-yamada
Copy link
Contributor

Tested both IPP Flow & Remote Verification for Initial entry, edit on verify, entering bad ssn to confirm attempts remaining, move to phone and all the way to get a barcode- working as expected.

return unless defined?(flow_session) && defined?(idv_session) && user_session
pii_from_doc_ssn = idv_session&.ssn || flow_session[:pii_from_doc]&.[](:ssn)
return pii_from_doc_ssn if pii_from_doc_ssn
flow_session[:pii_from_user]&.[](:ssn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I thought we needed to make an update to line 70 to include an or statement but whether Remote (pii_from_doc) or IPP (pii_from_user) - if idv_session.ssn was present, line 68 would return it so it would not make it past. Nothing to change - just writing my thoughts.

@gina-yamada gina-yamada self-requested a review September 7, 2023 21:54
Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I poked around the repo more after testing. It looks like you got all of the updates I would have expected when I search. Thanks for adding in those tests as well.

@soniaconnolly soniaconnolly merged commit 885af33 into main Sep 7, 2023
@soniaconnolly soniaconnolly deleted the sonia-lg-10886-start-moving-ssn-to-idv-session branch September 7, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants