Skip to content

Log ThreatMetrix status on IPP events #10101

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.

Sign up for GitHub

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

Merged
n1zyy merged 12 commits into from
Feb 28, 2024
Merged

Log ThreatMetrix status on IPP events #10101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
deea575
Add tmx_status to analytics events
daphnegold Feb 12, 2024
53f493c
Change 'pass' => 'threatmetrix_pass' to match already-existing langua…
daphnegold Feb 12, 2024
76c3c77
Fix tests for enrollment_helper_spec
daphnegold Feb 14, 2024
7a95b9f
Fix tests for spec/jobs/get_usps_proofing_results_job_spec.rb
daphnegold Feb 14, 2024
24edfd0
changelog: ThreatMetrix, analytics logging, log ThreatMetrix status o…
daphnegold Feb 15, 2024
7cd4c08
Merge branch 'main' into daphnegold/lg-12000-log-threatmetrix-status
daphnegold Feb 20, 2024
21c6899
Pull out logic into small helper method to accommodate feature flag
daphnegold Feb 27, 2024
b988652
Merge branch 'main' into daphnegold/lg-12000-log-threatmetrix-status
daphnegold Feb 27, 2024
4fc312c
Profile is undefined in some tests, add & safeguard for method
daphnegold Feb 27, 2024
f685f9a
Trying to get the tests to pass with feature flag, still having some …
daphnegold Feb 27, 2024
5b1ebf1
was being silly, profile is nil in enrollment_helper. It's functionin…
daphnegold Feb 27, 2024
939740f
changelog: Internal, analytics logging, log ThreatMetrix status on IPP
daphnegold Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/jobs/get_usps_proofing_results_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ def handle_unsupported_id_type(enrollment, response)
primary_id_type: response['primaryIdType'],
reason: 'Unsupported ID type',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)
enrollment.update(
status: :failed,
Expand Down Expand Up @@ -263,6 +264,7 @@ def handle_expired_status_update(enrollment, response, response_message)
passed: false,
reason: 'Enrollment has expired',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)
enrollment.update(
status: :expired,
Expand Down Expand Up @@ -334,6 +336,7 @@ def handle_failed_status(enrollment, response)
passed: false,
reason: 'Failed status',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)

enrollment.update(
Expand Down Expand Up @@ -370,6 +373,7 @@ def handle_successful_status_update(enrollment, response)
passed: true,
reason: 'Successful status update',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)
enrollment.update(
status: :passed,
Expand Down Expand Up @@ -400,6 +404,7 @@ def handle_passed_with_fraud_review_pending(enrollment, response)
passed: true,
reason: 'Passed with fraud pending',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)
enrollment.update(
status: :passed,
Expand All @@ -425,6 +430,7 @@ def handle_unsupported_secondary_id(enrollment, response)
passed: false,
reason: 'Provided secondary proof of address',
job_name: self.class.name,
tmx_status: enrollment.profile&.tmx_status,
)
enrollment.update(
status: :failed,
Expand Down
6 changes: 6 additions & 0 deletions app/models/profile.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ def activate(reason_deactivated: nil)
end
# rubocop:enable Rails/SkipsModelValidations

def tmx_status
return nil unless IdentityConfig.store.in_person_proofing_enforce_tmx

fraud_pending_reason || :threatmetrix_pass
end

def reason_not_to_activate
if pending_reasons.any?
"Attempting to activate profile with pending reasons: #{pending_reasons.join(',')}"
Expand Down
1 change: 1 addition & 0 deletions app/services/usps_in_person_proofing/enrollment_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def schedule_in_person_enrollment(user, pii, opt_in = nil)
second_address_line_present: pii[:address2].present?,
service_provider: enrollment.service_provider&.issuer,
opted_in_to_in_person_proofing: opt_in,
tmx_status: enrollment.profile&.tmx_status,
)

send_ready_to_verify_email(user, enrollment)
Expand Down
4 changes: 4 additions & 0 deletions spec/jobs/get_usps_proofing_results_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
transaction_end_date_time: anything,
transaction_start_date_time: anything,
job_name: 'GetUspsProofingResultsJob',
tmx_status: :threatmetrix_pass,
)
end
end
Expand Down Expand Up @@ -194,6 +195,7 @@
'%m/%d/%Y %H%M%S',
).in_time_zone('UTC')
end
let(:in_person_proofing_enforce_tmx) { true }

before do
allow(IdentityConfig.store).
Expand All @@ -207,6 +209,8 @@
allow(job).to receive(:analytics).and_return(job_analytics)
allow(IdentityConfig.store).to receive(:get_usps_proofing_results_job_reprocess_delay_minutes).
and_return(reprocess_delay_minutes)
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).
and_return(in_person_proofing_enforce_tmx)
Copy link
Copy Markdown
Contributor

@svalexander svalexander Feb 27, 2024

Choose a reason for hiding this comment

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

i think we should add a test that shows tmx_status getting logged in a analytics event

Copy link
Copy Markdown
Contributor

@svalexander svalexander Feb 27, 2024

Choose a reason for hiding this comment

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

oh oops, I see lower down there's a convo about this

Copy link
Copy Markdown
Contributor

@svalexander svalexander Feb 27, 2024

Choose a reason for hiding this comment

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

oh wait scratch that, on re-read I think that's specifically about the enrollment_helper file. In that case my comment still stands.

Copy link
Copy Markdown
Contributor Author

@daphnegold daphnegold Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So I think what actually might be my preferred solution here, and doesn't conflate concerns, is to write some tests in profile_spec.rb that test the helper. I should have done that when pulling the logic out into a separate helper method, probably, but I overlooked it because it was getting inadvertently tested everywhere it's implemented. But if we trust that the helper method in profile works appropriately, and we trust that the model for profile works appropriately, and we trust that the the analytics helper logs things appropriately, how important is it to have separate testing for each class consuming a method for a log event to test the output specifically?

On the other hand, maybe the answer is it is important, and I should do both. Including adding logic for profile to enrollment_helper_spec. I just think if we continue to add "logs x characteristic specifically" the test files will get a lot heavier. Should we test every combination? But we already have that precedent somewhat.

Copy link
Copy Markdown
Contributor Author

@daphnegold daphnegold Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw the analytics event does show up in the tests. They fail unless I accommodate the new log output. The output shows tmx_status: :threatmetrix_passed in the get_usps_proofing_results_job_spec and as tmx_status: nil in enrollment_helper_spec because a profile is not defined.

Copy link
Copy Markdown
Contributor

@svalexander svalexander Feb 27, 2024

Choose a reason for hiding this comment

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

i think that makes sense. Our convention has been to add the additional property to more of the analytics tests but I do agree with you that is does add a lot to the file (perhaps unnecessarily?).

Copy link
Copy Markdown
Contributor

@svalexander svalexander Feb 27, 2024

Choose a reason for hiding this comment

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

also might be an interesting topic for eng huddle - lightening our test files by reducing some redundancy

Copy link
Copy Markdown
Contributor

@JackRyan1989 JackRyan1989 Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm of the opinion that if things are being tested elsewhere, then no need to duplicate the tests. I think it is good to be explicit, but if you can point to a spec that is already covering the particular thing you want to test, then maybe that's enough? But IDK, I think this warrants a broader discussion (but I mean that discussion to take place outside of the scope of this ticket). So I second @svalexander's point above

stub_const(
'GetUspsProofingResultsJob::REQUEST_DELAY_IN_SECONDS',
request_delay_ms / GetUspsProofingResultsJob::MILLISECONDS_PER_SECOND,
Expand Down
5 changes: 5 additions & 0 deletions spec/services/usps_in_person_proofing/enrollment_helper_spec.rb
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@JackRyan1989 JackRyan1989 Feb 27, 2024

Choose a reason for hiding this comment

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

This looks good, but I would like to see a single test where the tmx_status value is tested to be something other than nil. I think this a nice to have for completeness and not necessarily blocking since you do this kind of test in the get_usps_proofing_results_job_spec.

Copy link
Copy Markdown
Contributor Author

@daphnegold daphnegold Feb 27, 2024

Choose a reason for hiding this comment

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

I discussed this with @n1zyy before when I asked for guidance on new tests I was considering writing and how far down the rabbit hole I should go. profile is undefined in this file. My feeling is that the issues with how profile is handled in different places, including if it can be nil, might be a larger issue and work outside the scope of this PR (if I remember correctly). I'm a bit fuzzy as the conversation happened a bit ago. Defer to whatever you both think.

Copy link
Copy Markdown
Contributor

@JackRyan1989 JackRyan1989 Feb 27, 2024

Choose a reason for hiding this comment

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

That's a good point. I think it's fine to leave as is then.

Copy link
Copy Markdown
Contributor

@n1zyy n1zyy Feb 27, 2024

Choose a reason for hiding this comment

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

My feeling is that the issues with how profile is handled in different places, including if it can be nil, might be a larger issue and work outside the scope of this PR (if I remember correctly). I'm a bit fuzzy as the conversation happened a bit ago. Defer to whatever you both think.

I think your read here, and above on analytics, is a good one. At some point we should make profile available here, but I agree that adding it in here would blow up the scope.

Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@
opted_in_to_in_person_proofing: nil,
second_address_line_present: false,
service_provider: nil,
tmx_status: nil,
)
end
end
Expand All @@ -216,6 +217,7 @@
opted_in_to_in_person_proofing: nil,
second_address_line_present: false,
service_provider: issuer,
tmx_status: nil,
)
end
end
Expand All @@ -242,6 +244,7 @@
opted_in_to_in_person_proofing: nil,
second_address_line_present: false,
service_provider: nil,
tmx_status: nil,
)
end

Expand All @@ -261,6 +264,7 @@
opted_in_to_in_person_proofing: nil,
second_address_line_present: true,
service_provider: nil,
tmx_status: nil,
)
end
end
Expand All @@ -279,6 +283,7 @@
opted_in_to_in_person_proofing: true,
second_address_line_present: false,
service_provider: nil,
tmx_status: nil,
)
end
end
Expand Down