Skip to content

LG-16257 Add email to socure ID plus requests#12222

Merged
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-16257
Jun 10, 2025
Merged

LG-16257 Add email to socure ID plus requests#12222
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-16257

Conversation

@shanechesnutt-ft
Copy link
Contributor

@shanechesnutt-ft shanechesnutt-ft commented May 30, 2025

🎫 Ticket

Link to the relevant ticket:
LG-16257

🛠 Summary of changes

Add user email address to Socure ID+ and DocumentVerification+ Requests

📜 Testing Plan

  • Setup app config to use the sandbox socure environment
  • Create an new account
  • Complete remote flow reaching the verify phone step
  • Login into socure dashboard
  • Ensure DocumentVerification Request displays the user email
  • Ensure the ID+ request displays the user email
  • Ensure no user email is logged during the process

@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/LG-16257 branch 2 times, most recently from 5d0eb99 to f4da68e Compare June 3, 2025 14:44
@shanechesnutt-ft shanechesnutt-ft marked this pull request as ready for review June 4, 2025 13:37
Copy link
Contributor

@amirbey amirbey Jun 4, 2025

Choose a reason for hiding this comment

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

doesn't seem like we should be removing this nor the comment above .. it's used in this method ... 🤔

Copy link
Contributor Author

@shanechesnutt-ft shanechesnutt-ft Jun 5, 2025

Choose a reason for hiding this comment

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

The reason why user_email exists in the proof method is because the threatmetrix_plugin.call method needs a user_email. I removed this from the proof method because I have added the user_email to the constructor and as a class variable. I did this so that I didn't have to pass it through a call chain in order to add it the Proofing::Socure::IdPlus::Config. I see that user_uuid is similar, but was not removed from the proof method before. I am wondering if we should have since it is also in the constructor/class variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did notice a potential issue in this file while reviewing - user_email_for_proofing() at line 145 could crash if last_sign_in_email_address returns nil (since it doesn't use safe navigation like your new code in SocureDocvResultsJob does). I know it's pre-existing but flagging it here for a potential future fix.

Copy link
Contributor

@Mawar2 Mawar2 left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

Implementation meets all the acceptance criteria and follows existing patterns in the codebase. Ready to merge.

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏿

[skip changelog]

Co-authored-by: William Birdsall <william.birdsall@gsa.gov>
@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/LG-16257 branch 2 times, most recently from 97ab57e to e64c88b Compare June 9, 2025 18:24
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

LGTM!

@shanechesnutt-ft shanechesnutt-ft merged commit 7eb858b into main Jun 10, 2025
1 check passed
@shanechesnutt-ft shanechesnutt-ft deleted the sc/LG-16257 branch June 10, 2025 17:24
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.

4 participants