Log ThreatMetrix status on IPP events by daphnegold · Pull Request #10101 · 18F/identity-idp

daphnegold · 2024-02-15T21:08:08Z

🎫 Ticket

Link to the relevant ticket:
LG-12000

🛠 Summary of changes

Added ThreatMetrix status logging to relevant analytics events:

"GetUspsProofingResultsJob: Enrollment status updated"
"USPS IPPaaS enrollment created"

Note: profile.fraud_pending_reason is an enum that contains statuses for review and reject, but not pass, which makes sense. The condition for pass is nil.

Do we like tmx_status for the log? I went with that, it was not specified in the ticket.

📜 Testing Plan

Manual testing of enrollment_helper.rb locally

Create a new account
Run through the entire account creation flow and proceed to verification
Fail verification to trigger the option for IPP
Select 'pass, review, or reject' and proceed to the last step with barcode for IPP
tail /log/events.log | jq
Search for tmx_status and review that result is appropriate

Run tests for enrollment_helper_spec.rb and get_usps_proofing_results_job_spec.rb to confirm that tmx_status log is present.

👀 Screenshots

Log tails

tmx pass

tmx review

tmx reject

…ge for reject

…n IPP events

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-02-15T23:31:00Z

I re-ran failing tests, but the test for AWS Pinpoint is still failing on an invalid country code. I think this came up previously in the past week or two and is almost uncertainly related to your PR.

The good news is that the "no PII in logs" checker didn't encounter the string "pii" in a random chunk of Base64 this time. 😆

I've got to run but will see if I can investigate the remaining error tomorrow.

gina-yamada · 2024-02-16T18:15:41Z

This extra field on both events are not behind the feature flag (in_person_proofing_enforce_tmx) but is for an epic.
I would consider either putting it behind the flag or confirm from product that logging this field is okay now.

n1zyy · 2024-02-20T22:48:47Z

This is now only failing due to the changelog checker. I'd just make the first category "Internal" and then keep the other two and resubmit.

I had put some ramblings in Slack about the feature flag issue Gina raised. Let me know if I just made it more confusing instead of being helpful.

gina-yamada · 2024-02-21T22:10:23Z

This extra field on both events are not behind the feature flag (in_person_proofing_enforce_tmx) but is for an epic. I would consider either putting it behind the flag or confirm from product that logging this field is okay now.

Hi @daphnegold
When you are back, I am happy to pair on this comment if you'd like. We can also look pipeline failures and talk about next steps and how it relates to the sprint board. Poke me on Slack else add some time on my calendar if some or all of that sounds helpful.

…trouble with enrollment_helper

…g as intended.

JackRyan1989

Looks good! Happy to approve if you think my comment isn't worth the squeeze.

JackRyan1989 · 2024-02-27T15:13:17Z

spec/services/usps_in_person_proofing/enrollment_helper_spec.rb

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.

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.

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

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.

svalexander · 2024-02-27T15:41:01Z

spec/jobs/get_usps_proofing_results_job_spec.rb

    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)


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

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

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

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.

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.

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?).

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

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

JackRyan1989

LGTM!

n1zyy · 2024-02-28T16:50:47Z

Daphne is out today, so I'm going to go ahead and merge this.

daphnegold added 5 commits February 12, 2024 08:45

Add tmx_status to analytics events

deea575

Change 'pass' => 'threatmetrix_pass' to match already-existing langua…

53f493c

…ge for reject

Fix tests for enrollment_helper_spec

76c3c77

Fix tests for spec/jobs/get_usps_proofing_results_job_spec.rb

7a95b9f

changelog: ThreatMetrix, analytics logging, log ThreatMetrix status o…

24edfd0

…n IPP events

daphnegold self-assigned this Feb 15, 2024

daphnegold requested review from a team and KeithNava February 15, 2024 21:18

JackRyan1989 reviewed Feb 15, 2024

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Outdated Show resolved Hide resolved

app/jobs/get_usps_proofing_results_job.rb Outdated Show resolved Hide resolved

Merge branch 'main' into daphnegold/lg-12000-log-threatmetrix-status

7cd4c08

daphnegold added 2 commits February 26, 2024 17:32

Pull out logic into small helper method to accommodate feature flag

21c6899

Merge branch 'main' into daphnegold/lg-12000-log-threatmetrix-status

b988652

daphnegold force-pushed the daphnegold/lg-12000-log-threatmetrix-status branch from 2b359d6 to b988652 Compare February 27, 2024 01:55

daphnegold added 3 commits February 26, 2024 17:58

Profile is undefined in some tests, add & safeguard for method

4fc312c

Trying to get the tests to pass with feature flag, still having some …

f685f9a

…trouble with enrollment_helper

was being silly, profile is nil in enrollment_helper. It's functionin…

5b1ebf1

…g as intended.

daphnegold requested review from JackRyan1989 and n1zyy February 27, 2024 02:18

changelog: Internal, analytics logging, log ThreatMetrix status on IPP

939740f

JackRyan1989 reviewed Feb 27, 2024

View reviewed changes

svalexander reviewed Feb 27, 2024

View reviewed changes

JackRyan1989 self-requested a review February 27, 2024 15:47

JackRyan1989 approved these changes Feb 27, 2024

View reviewed changes

n1zyy approved these changes Feb 28, 2024

View reviewed changes

n1zyy merged commit a295f12 into main Feb 28, 2024

n1zyy deleted the daphnegold/lg-12000-log-threatmetrix-status branch February 28, 2024 16:51

Conversation

daphnegold commented Feb 15, 2024 • edited by JackRyan1989 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Log tails

Uh oh!

Uh oh!

Uh oh!

n1zyy commented Feb 15, 2024

Uh oh!

gina-yamada commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1zyy commented Feb 20, 2024

Uh oh!

gina-yamada commented Feb 21, 2024

Uh oh!

JackRyan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daphnegold Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daphnegold Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

n1zyy commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daphnegold commented Feb 15, 2024 •

edited by JackRyan1989

Loading

gina-yamada commented Feb 16, 2024 •

edited

Loading

daphnegold Feb 27, 2024 •

edited

Loading

daphnegold Feb 27, 2024 •

edited

Loading

JackRyan1989 Feb 27, 2024 •

edited

Loading