Skip to content

LG-12376 state id update - check ssn#10567

Merged
svalexander merged 4 commits intomainfrom
shannon/lg-12376-update-route-check-ssn
May 9, 2024
Merged

LG-12376 state id update - check ssn#10567
svalexander merged 4 commits intomainfrom
shannon/lg-12376-update-route-check-ssn

Conversation

@svalexander
Copy link
Contributor

@svalexander svalexander commented May 7, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12376

🛠 Summary of changes

This pr fixes a bug introduced into the 1st pr addressing the update route for the state id controller. We are now also checking that there is an ssn for the user as a means to determine if the user is updating their id information. Previously we did not check the ssn value and so if a user had entered their name on the page but validation failed the page would change to show the appropriate view for someone who is now updating the page.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Turn on flag for state id controller
  • Go through the ipp flow and arrive at the state id page
  • Enter inputs that will fail for name and address
  • Click continue
  • Verify that validation errors are present
  • Verify that page view is still of someone first arriving at state id page, NOT of someone updating the page

@svalexander svalexander requested review from a team and KeithNava May 7, 2024 15:56

def updating_state_id?
pii_from_user.has_key?(:first_name)
pii_from_user.has_key?(:first_name) && !user_session[:idv][:ssn].nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some test coverage for this change?

Also, is user_session[:idv] guaranteed to exist so that we don't run into an error undefined method []' for nil, or should we use dig` instead?

Suggested change
pii_from_user.has_key?(:first_name) && !user_session[:idv][:ssn].nil?
pii_from_user.has_key?(:first_name) && user_session.dig(:idv, :ssn).present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i made this update and also added test coverage

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Trying to understand the problem this is addressing, it sounds like we're setting values into the session even when the submission is considered invalid? To me, that sounds like the bigger issue, and we should only be assigning values if the submission is valid.

@svalexander
Copy link
Contributor Author

submission is valid

@aduth that's a good point. I think we were originally doing it like this because we needed the value for same_address_as_id for the submitted event. It does make sense though to move both of those things so that they're only done when the form submission is valid.
For the value that determines if we're updating the state id form I think we should still be checking that an ssn is present as they updating version of the page should only be visible once a user has submitted the ssn. This also ensures that if the user goes back to the state id page before submitting their ssn that they will still see the non-updating version of the page.

Copy link
Contributor

@JackRyan1989 JackRyan1989 left a comment

Choose a reason for hiding this comment

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

LGTM!

@svalexander svalexander merged commit 3452435 into main May 9, 2024
@svalexander svalexander deleted the shannon/lg-12376-update-route-check-ssn branch May 9, 2024 15:59
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.

3 participants