LG-15166: Store doc auth vendor in Idv::Session#11742
Conversation
There was a problem hiding this comment.
Can you add similar checks to idv/document_capture_controller_spec.rb and idv/link_sent_controller_spec.rb?
There was a problem hiding this comment.
Hmmm, I didn't include this check in the other specs since I thought the behavior was sufficiently covered in spec/policies/idv/flow_policy_spec.rb. Thoughts?
There was a problem hiding this comment.
I think the flow policy spec updates do a good job of verifying that other steps don't mess with this value inappropriately. But I think what we want to test here is "after the user does the thing that we expect will set doc_auth_vendor, it is actually set".
extract_pii_from_doc is in the DocumentCaptureConcern, so you could add a test for that method, but I think that it makes sense to add a small assertion to each of the three paths (doc capture, link sent, and verify info for IPP) that the doc auth vendor is set as expected.
There was a problem hiding this comment.
Got it, should be fairly straightforward to add those checks.
There was a problem hiding this comment.
I definitely forgot about the hybrid flow, but I think this will work, since LinkSentController includes DocumentCaptureConcern and calls extract_pii_from_doc. However, I think we should have spec coverage for that.
There was a problem hiding this comment.
Good point. Working on that now.
app/services/idv/session.rb
Outdated
app/services/idv/session.rb
Outdated
There was a problem hiding this comment.
HELLO FELLOW ALPHABETIZER
There was a problem hiding this comment.
I think there are some edge cases that this will get mixed up on:
- User starts IPP, backs out, and goes down the remote doc auth path
- User starts IPP, abandons, then starts a new verification, selecting remote this time
In this case, we know the user has not chosen IPP for doc auth, so I don't think we should write it here. We likely want to set this to 'usps' somewhere else later on--perhaps on submission of the "Verify your info" screen?
There was a problem hiding this comment.
That's a good point and I hadn't thought about that.
There was a problem hiding this comment.
Instead of doing this, converted to updating in Idv::InPerson::StateIdController#update. (Figuring this out was a bit of pain since it's a "hidden" step.)
Adds a new key, `doc_auth_vendor`, to Idv::Session. This is part 1 to eliminate issues with 50/50 state handling. [skip changelog]
**Why** * Currently, we compute the vendor for remote document verification using DocAuthRouter, which requires downstream business logic to depend on context that may or may not be available. In order to eliminate calls to DocAuthRouter in places like Idv::ProofingComponents, we first need to start storing the vendor directly in Idv::Session. **How** * Add a new key `doc_auth_vendor` to Idv::Session. * Set the new key after successful document capture. If the user opted for in person proofing, it will be `usps`, otherwise we use the existing `Idv::DocumentCaptureConcern#doc_auth_vendor` method. * Clear `doc_auth_vendor` if the following steps are undone: link_sent, document_capture, and socure_document_capture. changelog: Internal, IdV flow, Simplify creation of Idv::ProofingComponents
lmgeorge
force-pushed
the
lmgeorge/LG-15166
branch
from
8e42632 to
dc210ce
Compare
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
I don't think I have all of the historical context here that other reviewers do, so I wouldn't mind making sure they are onboard first, but having reviewed this it looks good to me. One small question where I think I'm just missing context.
| issue_year: state_id_issued&.year, | ||
| doc_auth_success: successful_result?, | ||
| vendor: 'Socure', | ||
| vendor: 'Socure', # TODO: Replace with Idp::Constants::Vendors::SOCURE |
There was a problem hiding this comment.
Can we not do this today due to the case difference?
There was a problem hiding this comment.
Yeah. If there are any queries that match on the case exactly then they would break. I'll write a ticket to follow up on this.
|
|
||
| context 'socure is the default vendor but facial match is required' do | ||
| let(:idv_vendor) { Idp::Constants::Vendors::SOCURE } | ||
| let(:vot) { 'Pb' } |
There was a problem hiding this comment.
Off-topic, but I read this as the chemical substance for Lead 100% of the time, so good riddance to it.
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-15166
🛠 Summary of changes
Why
using DocAuthRouter, which requires downstream business logic to
depend on context that may or may not be available. In order to
eliminate calls to DocAuthRouter in places like Idv::ProofingComponents,
we first need to start storing the vendor directly in Idv::Session.
How
doc_auth_vendorto Idv::Session.for in person proofing, it will be
usps(managed directly inIdv::InPerson::StateIdController), otherwise we use theexisting
Idv::DocAuthVendorConcern#doc_auth_vendormethod.Idv::Session#doc_auth_vendorif the following steps are undone: link_sent,document_capture, and socure_document_capture, and ipp_state_id.
Additional notes
The
doc_auth_vendorkey is only being written to and not read in this PR. (LG-15168: Update ProofingComponents to only require Idv::Session will add code that reads this key from the session.