Skip to content

LG-13726 update verify info controller to redirect when pii is missing#11065

Merged
jennyverdeyen merged 1 commit intomainfrom
jverdeyen/LG-13726-fix-nil-merge-error
Aug 26, 2024
Merged

LG-13726 update verify info controller to redirect when pii is missing#11065
jennyverdeyen merged 1 commit intomainfrom
jverdeyen/LG-13726-fix-nil-merge-error

Conversation

@jennyverdeyen
Copy link
Contributor

@jennyverdeyen jennyverdeyen commented Aug 9, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13726

🛠 Summary of changes

An error recently surfaced in NewRelic: undefined method 'merge' for nil occurring in the verify info controller. This error would occur because a possibly nil "pii_from_user "object was being called "merge" upon. This updates the code to check for missing PII before rendering the verify info controller, as well as handle that nil object gracefully and prevent such an the error from breaking the app.

📜 Testing Plan

  • Reproduce the error:
    • On the main branch (without this fix) complete the IPP flow to the point of the 'verify/in_person/verify_info' page (after you've entered SSN).
    • Hit the 'Cancel' link near the bottom of the page
    • Choose "Exit Login.gov"
    • Hit the back button in your browser to return to Login.gov
    • Choose "Keep going"
    • Observe the application breaks with undefined method 'merge' for nil error
Screen.Recording.2024-08-23.at.8.46.52.PM.mov
  • Perform the same actions to confirm the fix on this branch:

    • Checkout this branch (jverdeyen/LG-13726-fix-nil-merge-error) and complete the IPP flow to the point of the 'verify/in_person/verify_info' page (after you've entered SSN).
    • Hit the 'Cancel' link near the bottom of the page
    • Choose "Exit Login.gov"
    • Hit the back button in your browser to return to Login.gov
    • Choose "Keep going"
    • Observe that it redirects to the /verify/welcome page without error
    Screen.Recording.2024-08-23.at.8.41.28.PM.mov
  • Confirm the tests are passing (spec/controllers/idv/in_person/verify_info_controller_spec.rb) ✅

@jennyverdeyen jennyverdeyen requested review from a team and WilliamBirdsall August 9, 2024 20:56
Copy link
Contributor

@mitchellhenke mitchellhenke Aug 9, 2024

Choose a reason for hiding this comment

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

How does IPP end up in a state where pii is nil at this point? It should already exist?

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 was wondering the same! We observed errors in NewRelic that traced back to pii being nil here. I couldn't reproduce it manually. Not sure if this indicates something bigger worth investigating? Perhaps handling the error here isn't sufficient? Open to discussion!

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, discussion was continued internally here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revisited this, and after investigating the logs I found that a user could cancel their in person proofing flow at the verify info page, exit login.gov, then hit the back button to re-enter login.gov, and hit "keep going" to return to the verify info page, where they would no longer have PII in their session since it was cleared on exit.

So, I've added a before_action to redirect if there is no IPP session data present. This follows the behavior on the remote flow side, where the verify info controller also checks that that the step is valid and otherwise redirects.

Copy link
Contributor

@WilliamBirdsall WilliamBirdsall left a comment

Choose a reason for hiding this comment

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

LGTM and tests pass :D

@jennyverdeyen jennyverdeyen force-pushed the jverdeyen/LG-13726-fix-nil-merge-error branch from cac2eb7 to ee94c47 Compare August 24, 2024 00:22
@jennyverdeyen jennyverdeyen changed the title LG-13726 update verify info controller to gracefully handle when pii is nil LG-13726 update verify info controller to redirect when pii is missing Aug 24, 2024
@jennyverdeyen jennyverdeyen force-pushed the jverdeyen/LG-13726-fix-nil-merge-error branch 2 times, most recently from 43d6cc8 to 4e1a5fc Compare August 25, 2024 15:37
Copy link
Contributor

@KeithNava KeithNava left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks for really "digging" into this! 😆

changelog: Bug Fixes, In-person proofing, addresses error that occurs when pii is nil in verify info controller
@jennyverdeyen jennyverdeyen force-pushed the jverdeyen/LG-13726-fix-nil-merge-error branch from 4e1a5fc to 4007852 Compare August 26, 2024 16:56
@WilliamBirdsall
Copy link
Contributor

Still LGTM!

@jennyverdeyen jennyverdeyen merged commit 9d75dd3 into main Aug 26, 2024
@jennyverdeyen jennyverdeyen deleted the jverdeyen/LG-13726-fix-nil-merge-error branch August 26, 2024 20:19
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