-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-14973 Add PII check to Socure flow #11747
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we decided to defer displaying PII errors in LG-15307, here when PII fails the below is displayed to the user.
the failing PII scenario can recreated using http://localhost:3000/verify/socure/document_capture_update?docv_token=1111-1111 which passes back an expired date
i think adding a test would be helpful to make sure the user is seeing what we're intending ... thanks!
d9d6b9e
to
8568002
Compare
if !successful_result? | ||
{ socure: { reason_codes: get_data(DATA_PATHS[:reason_codes]) } } | ||
elsif !pii_valid? | ||
{ pii_validation: 'failed' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could call this like, local_attribute_validation
or something and not have to worry about having pii
in the key name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda prefer the clarity, since we have a ticket coming up shortly to actually display more specific errors about pii, and would likely want to use pii*
keys anyway; might as well cope with it now.
8568002
to
8c68295
Compare
Check PII using DocPiiForm. changelog: Upcoming Features,Socure,Add Idv::DocPiiForm check to Socure flow.
8c68295
to
422ed47
Compare
Merging following conversation with @amirbey. Further work from that conversation to be done in LG-15309. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
🎫 Ticket
LG-14973
🛠 Summary of changes
Add the PII checks in the
DocPiiForm
to the Socure flow.📜 Testing Plan
Verify that the tests in in
spec/services/doc_auth/socure/responses/docv_result_response_spec.rb
adequately check the functionality and pass.