-
Notifications
You must be signed in to change notification settings - Fork 166
LG-15168 | Proofing components now only takes idv_session #11771
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
Changes from all commits
34f820c
64006e2
269f3eb
f486fdf
dfaa0e4
d4a259b
2636f8c
d22972b
ac46a70
28cdc2e
e2d3126
59bb941
d4dd239
0fd5a07
f9c2f0a
9c9816e
a140ffb
277b069
261d971
664cf93
04b395f
870be44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,25 +2,12 @@ | |
|
|
||
| module Idv | ||
| class ProofingComponents | ||
| def initialize(idv_session:, session:, user:, user_session:) | ||
| def initialize(idv_session:) | ||
| @idv_session = idv_session | ||
| @session = session | ||
| @user = user | ||
| @user_session = user_session | ||
| end | ||
|
|
||
| def document_check | ||
| if user.establishing_in_person_enrollment || user.pending_in_person_enrollment | ||
| Idp::Constants::Vendors::USPS | ||
| elsif idv_session.remote_document_capture_complete? | ||
| DocAuthRouter.doc_auth_vendor( | ||
| request: nil, | ||
| service_provider: idv_session.service_provider, | ||
| session:, | ||
| user_session:, | ||
| user:, | ||
| ) | ||
| end | ||
| idv_session.doc_auth_vendor | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: There is probably some 50/50 state here that needs handling in the interim. I had to do something similar when I added
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I'm seeing it, but let's make sure I'm not just overlooking something. The constructor change should only be problematic if there's background job usage where what's available changes, and I didn't see any of that. I think all the controller-level action will be consistent since we're already storing this in the session in prod. But my comment here is just intended to outline my thinking. What else should I be looking at?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're totally right. I was thinking in terms of jobs not the controller context. This is fine |
||
| end | ||
|
|
||
| def document_type | ||
|
|
@@ -73,6 +60,6 @@ def to_h | |
|
|
||
| private | ||
|
|
||
| attr_reader :idv_session, :session, :user, :user_session | ||
| attr_reader :idv_session | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -212,7 +212,7 @@ | |
|
|
||
| context 'with in establishing in-person enrollment' do | ||
| let(:user) { build(:user, :with_establishing_in_person_enrollment) } | ||
| let(:enrollment) { user.establishing_in_person_enrollment } | ||
| let!(:enrollment) { user.establishing_in_person_enrollment } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This had me tearing my hair out because the enrollment stopped existing in tests here which seemed orthogonal to my changes. This really should have been |
||
|
|
||
| before do | ||
| allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -584,7 +584,7 @@ | |
| flow_path: 'standard', opted_in_to_in_person_proofing: false | ||
| }, | ||
| 'IdV: in person proofing state_id visited' => { | ||
| step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing', proofing_components: { document_check: 'usps' } | ||
| step: 'state_id', flow_path: 'standard', analytics_id: 'In Person Proofing' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to call out this behavior change.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. follow-up: We should reach out to any teams that are depending on this field being present through the whole flow or just generally make a few announcements at eng huddle. I have some scripts that I can share which check for usage of a particular field in CW queries and dashboard. |
||
| }, | ||
| 'IdV: in person proofing state_id submitted' => { | ||
| success: true, flow_path: 'standard', step: 'state_id', analytics_id: 'In Person Proofing', birth_year: '1938', document_zip_code: '12345', proofing_components: { document_check: 'usps' } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,12 +27,7 @@ | |
| end | ||
| end | ||
| let(:proofing_components) do | ||
| Idv::ProofingComponents.new( | ||
| idv_session:, | ||
| session:, | ||
| user:, | ||
| user_session:, | ||
| ) | ||
| Idv::ProofingComponents.new(idv_session:) | ||
| end | ||
|
|
||
| subject(:agent) { Idv::Agent.new(applicant) } | ||
|
|
@@ -147,7 +142,6 @@ | |
| expect(ResolutionProofingJob).to receive(:perform_later).with( | ||
| hash_including( | ||
| proofing_components: { | ||
| document_check: 'mock', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to sync up with someone tomorrow on this. This was previously being inferred in the ProofingComponents class. Should we figure out how to do that again, or should we not make this assumption? I slightly leaned towards the latter.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthinz I would value your input on this. This was previously being inferred in a Rube Goldberg fashion from DocAuthRouter. It seems like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is just checking that the proofing components are being passed to the ResolutionProofingJob. So I would say removing this one line is fine |
||
| document_type: 'state_id', | ||
| }, | ||
| ), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,9 +21,6 @@ | |
|
|
||
| subject do | ||
| described_class.new( | ||
| session:, | ||
| user:, | ||
| user_session:, | ||
| idv_session:, | ||
| ) | ||
| end | ||
|
|
@@ -40,12 +37,13 @@ | |
| idv_session.threatmetrix_review_status = 'pass' | ||
| idv_session.source_check_vendor = 'aamva' | ||
| idv_session.resolution_vendor = 'lexis_nexis' | ||
| idv_session.doc_auth_vendor = 'feedabee' | ||
| end | ||
|
|
||
| it 'returns expected result' do | ||
| expect(subject.to_h).to eql( | ||
| { | ||
| document_check: 'test_vendor', | ||
| document_check: 'feedabee', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I somehow made gitleaks very angry when refactoring and it decided this was a secret. 'feedabee' is one of its approved test values. I just went with it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good ol'
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (But much more happy!) |
||
| document_type: 'state_id', | ||
| source_check: 'aamva', | ||
| resolution_check: 'lexis_nexis', | ||
|
|
@@ -58,46 +56,10 @@ | |
| end | ||
|
|
||
| describe '#document_check' do | ||
| it 'returns nil by default' do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: these tests still seem valuable, no?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's debatable. The method now just returns whatever The IPP ones no longer make sense here, since we're looking at proofing components in isolation and it doesn't know anything about IPP. Equivalent coverage exists in the IPP code. 'returns nil by default' is valid-ish, but I figured I'd remove the whole thing because really it would just be testing that Ruby returns nil for uninitialized attributes. |
||
| expect(subject.document_check).to be_nil | ||
| end | ||
|
|
||
| context 'in-person proofing' do | ||
| context 'establishing' do | ||
| let!(:enrollment) { create(:in_person_enrollment, :establishing, user:) } | ||
|
|
||
| it 'returns USPS' do | ||
| expect(subject.document_check).to eql(Idp::Constants::Vendors::USPS) | ||
| end | ||
| end | ||
|
|
||
| context 'pending' do | ||
| let!(:enrollment) { create(:in_person_enrollment, :pending, user:) } | ||
|
|
||
| it 'returns USPS' do | ||
| expect(subject.document_check).to eql(Idp::Constants::Vendors::USPS) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context 'doc auth' do | ||
| before do | ||
| allow(IdentityConfig.store).to receive(:doc_auth_vendor_default).and_return('test_vendor') | ||
| end | ||
|
|
||
| context 'before doc auth complete' do | ||
| it 'returns nil' do | ||
| expect(subject.document_check).to be_nil | ||
| end | ||
| end | ||
|
|
||
| context 'after doc auth completed successfully' do | ||
| let(:pii_from_doc) { Idp::Constants::MOCK_IDV_APPLICANT } | ||
| before { idv_session.doc_auth_vendor = 'feedabee' } | ||
|
|
||
| it 'returns doc auth vendor' do | ||
| expect(subject.document_check).to eql('test_vendor') | ||
| end | ||
| end | ||
| it 'returns doc_auth_vendor' do | ||
| expect(subject.document_check).to eql('feedabee') | ||
| end | ||
| end | ||
|
|
||
|
|
||
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 need to log this after setting
doc_auth_vendornow.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.
This may or may not be my fault. (Unfortunately, it was the least janky way of doing it.)