LG-11997 | Conditionally marks users as fraudulent by n1zyy · Pull Request #9928 · 18F/identity-idp

n1zyy · 2024-01-16T22:11:33Z

🎫 Ticket

Link to the relevant ticket:
LG-11997

🛠 Summary of changes

If a user has a fraud_pending_reason from TMX, mark them as fraud_review_pending_at when we get a response back from the USPS.

📜 Testing Plan

It may make sense to slow-roll this until Keith's PR for LG-12016 is ready, because his story will come first for a user.

If a user has a fraud_pending_reason, mark them as fraud_review_pending_at when we get a response. changelog: Internal, IPP, Mark users as fraud_review_pending_at if appropriate

n1zyy · 2024-01-16T22:12:32Z

app/jobs/get_usps_proofing_results_job.rb

-    response = nil

    response = proofer.request_proofing_results(
      enrollment.unique_id, enrollment.enrollment_code


The assignment on line 102 was useless since we unconditionally assign it here. My editor happened to highlight it while I was working my way through here.

n1zyy · 2024-01-16T22:13:48Z

app/jobs/get_usps_proofing_results_job.rb


+  # MW: What else do we want to log here?
+  def handle_fraud_review_pending(enrollment)
+    enrollment.profile&.deactivate_for_fraud_review


.profile& based on LG-12020. It's rare and weird for profile to be missing, but it can happen.

sidenote, do we want to update other places where we're accessing properties from profile? (in a future ticket)

I'm conflicted on this. It's a good idea, but the case in LG-12020 is currently the only one that has ever broken. I'm not sure if I want to create a ticket that will just rot in the backlog.

Hmm, so in following this code a bit more, it seems like if the profile doesn't exist we wouldn't fail fraud_review_pending?, and then the application would crash at line 382, when we try to call activate_after_passing_in_person on the profile. So it seems to me that we'd want to do the safe navigation operator in lines 382 & 421 (which you already have), and you could probably remove the operator in the handle_fraud_review_pending.

But I feel like if the profile is missing we should bounce outta here sooner, right?

Oh, nice catch!

I think I added this one when I had LG-12020 still on my mind, and then failed to consider it elsewhere. Maybe I should add a quick test case for this too?

I deleted my last comment, which turns out to be suggesting a ton of work for little benefit. I updated the instances you referred to, and removed it here. It is indeed impossible to reach this line without a profile.

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-01-16T22:15:57Z

app/jobs/get_usps_proofing_results_job.rb

      reason: 'Successful status update',
      job_name: self.class.name,
    )
-    enrollment.profile.activate_after_passing_in_person


This, and 373-380, are now conditional on the user not being fraud_pending?, so this got moved down. We still want the enrollment update below, though.

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-01-16T22:18:42Z

app/jobs/get_usps_proofing_results_job.rb

    # send SMS and email
    send_enrollment_status_sms_notification(enrollment: enrollment)
+    # MW: I _think_ this is desired behavior?
    send_failed_email(enrollment.user, enrollment)


The idea is that if you get a failure proofing, we'll just send you that, and the fact that we also had you flagged as fraudulent is kind of irrelevant.

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-01-16T22:24:19Z

app/jobs/get_usps_proofing_results_job.rb

      return
    end

+    # We want to deactivate them regardless of status:


I.e., regardless of response['status']. We'll always take this step if they're so flagged, and then we just needed to tweak the success case to not fully activate them.

spec/jobs/get_usps_proofing_results_job_spec.rb

n1zyy · 2024-01-16T22:31:23Z

✅ FIXME: This should respect the feature flag being added in LG-12016.

app/jobs/get_usps_proofing_results_job.rb

gina-yamada · 2024-01-17T21:37:36Z

spec/jobs/get_usps_proofing_results_job_spec.rb

+              profile = pending_enrollment.profile
+              expect(profile).to be_fraud_review_pending
+              expect(profile).not_to be_active
+            end


The AC may have changed. You and I talked about exiting behavior vs what was in the issue. I would have expected fraud_review_pending to be null and fraud_review_pending_at to have a timestamp on in. I may have missed the ask here and/or the requirements/AC have changed upon diving into the code and discovering existing processes. If they have changed- we can update the AC in the ticket.

Thanks for flagging this. It did change; there is now a link in the Jira ticket to the Slack thread where it came up. It turns out that the existing pattern is to leave fraud_pending_reason in place.

I tried to be clever with expect(profile).to be_fraud_review_pending?, which exercises this method:

def fraud_review_pending? fraud_review_pending_at.present? end

But in hindsight I think it might be clearer if I just did expect(profile.fraud_review_pending_at).to eq (Time.zone.now) (or similar), rather than relying on a magic matcher + non-obvious helper method in Profile.

spec/jobs/get_usps_proofing_results_job_spec.rb

Alphabetize method (it was, but then I renamed it)

(My editor sold me out when I moved this!)

n1zyy · 2024-01-23T23:06:58Z

app/services/analytics_events.rb

+    **extra
+  )
+    track_event(
+      :idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review,


TIL that we are moving to this model for new events, vs. a string, and it must match the method name. This was discussed as a previous eng huddle, but apparently one where I wasn't present.

n1zyy · 2024-01-23T23:08:37Z

app/services/idv/profile_maker.rb

      profile.deactivate_for_gpo_verification if gpo_verification_needed
-      if fraud_pending_reason.present? && !gpo_verification_needed
+
+      if fraud_pending_reason.present? && !gpo_verification_needed && !in_person_verification_needed


@svalexander This is the fix you helped catch yesterday! 🥳 I also updated the ProfileMakerSpec to cover this. I think it would have taken way longer for me to find this if you hadn't caught it; thanks!

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-01-24T22:46:30Z

spec/services/idv/profile_maker_spec.rb

+        it 'is not fraud_review_pending?' do
+          expect(profile.fraud_review_pending?).to eq(false)
+        end
+      end


These test my change to the ProfileMaker. Without the change, but with this test, the GPO verification case passes, but the IPP one did not.

spec/jobs/get_usps_proofing_results_job_spec.rb

n1zyy · 2024-01-24T22:50:59Z

app/jobs/get_usps_proofing_results_job.rb


+  # MW: What else do we want to log here?
+  def handle_fraud_review_pending(enrollment)
+    enrollment.profile&.deactivate_for_fraud_review


I'm conflicted on this. It's a good idea, but the case in LG-12020 is currently the only one that has ever broken. I'm not sure if I want to create a ticket that will just rot in the backlog.

app/jobs/get_usps_proofing_results_job.rb

n1zyy · 2024-01-24T22:54:46Z

config/application.yml.default

 in_person_email_reminder_final_benchmark_in_days: 1
 in_person_email_reminder_late_benchmark_in_days: 4
 in_person_proofing_enabled: false
+in_person_proofing_enforce_tmx: false


Global default is false

n1zyy · 2024-01-24T22:54:54Z

config/application.yml.default

  hmac_fingerprinter_key_queue: '["11111111111111111111111111111111", "22222222222222222222222222222222"]'
  identity_pki_local_dev: true
  in_person_proofing_enabled: true
+  in_person_proofing_enforce_tmx: true


But it's true in dev

n1zyy · 2024-01-24T22:55:46Z

spec/jobs/get_usps_proofing_results_job_spec.rb

+            stub_request_passed_proofing_results
+          end
+
+          it 'updates the enrollment' do


This one example happens whether the flag is true or false, because it happens before we look at fraud statuses. It feels a little lonely out here though.

n1zyy · 2024-01-26T17:47:48Z

I added a commit which makes FakeAnalytics add user_id if a User object is passed into the constructor, like the real track_event it replaces.

With that, this test passes:

            it 'logs a user_sent_to_fraud_review analytics event' do
              job.perform(Time.zone.now)

              expect(job_analytics).to have_logged_event(
                :idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review,
                hash_including(
                  enrollment_code: pending_enrollment.enrollment_code,
                  user_id: user.uuid,
                ),
              )
            end

However, the change would require updating a ton of existing tests which make assertions about what is logged and aren't expecting user_id to be there. I think I'm going to remove the user_id line from this PR and get it through without it, and then separately add it back in a follow-on PR fixing all the other tests it would break.

This reverts commit 952b935. (This is good stuff IMHO, but it belongs in its own PR.)

JackRyan1989

Looks real good! Just sending over some comments but willing to approve upon further discussion.

JackRyan1989 · 2024-01-26T14:45:32Z

app/jobs/get_usps_proofing_results_job.rb


+  # MW: What else do we want to log here?
+  def handle_fraud_review_pending(enrollment)
+    enrollment.profile&.deactivate_for_fraud_review


Hmm, so in following this code a bit more, it seems like if the profile doesn't exist we wouldn't fail fraud_review_pending?, and then the application would crash at line 382, when we try to call activate_after_passing_in_person on the profile. So it seems to me that we'd want to do the safe navigation operator in lines 382 & 421 (which you already have), and you could probably remove the operator in the handle_fraud_review_pending.

But I feel like if the profile is missing we should bounce outta here sooner, right?

app/services/analytics_events.rb

JackRyan1989 · 2024-01-26T18:08:41Z

app/jobs/get_usps_proofing_results_job.rb

  end

+  def handle_fraud_review_pending(enrollment)
+    return unless IdentityConfig.store.in_person_proofing_enforce_tmx


So this is totally a super nit, but I'm wondering if it makes sense to move this config check into it's own method like we do for ipp_enabled on line 65 and then use it on line 432 to determine if we should call handle_fraud_review_pending? I kinda like that pattern of escalating and making more explicit the prerequisites for a method to be called. But also, this is non-blocking I think.

That's exactly the pattern @svalexander took in her PR for LG-12091. I like it!

(Some part of my brain really wants to have the method itself enforce this, but I don't think it's necessary.)

JackRyan1989 · 2024-01-26T18:10:53Z

app/jobs/get_usps_proofing_results_job.rb

-      email_type: 'Success',
-      job_name: self.class.name,
-    )
+    unless fraud_result_pending?(enrollment)


Do we need to check the config setting here too? Based on your specs I think no, but I'm not totally clear on when fraud_pending_reason is available on the enrollment profile.

Oh! I think we should.

We're not the first ones to use fraud_pending_reason, so the feature flag should toggle whether or not we act on it here. Seems like I need to clean up the test as well.

Better yet, your suggestion in https://github.com/18F/identity-idp/pull/9928/files#r1467975629 gets us this for free.

n1zyy

@JackRyan1989 Thanks for your thorough review here—you caught a few different things I missed! (And thanks also for pairing earlier on the logging issue I had.)

n1zyy · 2024-01-26T19:53:29Z

app/jobs/get_usps_proofing_results_job.rb


+  # MW: What else do we want to log here?
+  def handle_fraud_review_pending(enrollment)
+    enrollment.profile&.deactivate_for_fraud_review


Oh, nice catch!

I think I added this one when I had LG-12020 still on my mind, and then failed to consider it elsewhere. Maybe I should add a quick test case for this too?

n1zyy · 2024-01-26T19:55:40Z

app/jobs/get_usps_proofing_results_job.rb

  end

+  def handle_fraud_review_pending(enrollment)
+    return unless IdentityConfig.store.in_person_proofing_enforce_tmx


That's exactly the pattern @svalexander took in her PR for LG-12091. I like it!

(Some part of my brain really wants to have the method itself enforce this, but I don't think it's necessary.)

n1zyy · 2024-01-26T20:06:13Z

app/jobs/get_usps_proofing_results_job.rb

-      email_type: 'Success',
-      job_name: self.class.name,
-    )
+    unless fraud_result_pending?(enrollment)


Oh! I think we should.

We're not the first ones to use fraud_pending_reason, so the feature flag should toggle whether or not we act on it here. Seems like I need to clean up the test as well.

app/services/analytics_events.rb

JackRyan1989

Lgtm!

…r fraud-pending in-person users When the identity verification report was built the fraud-review functionality did not apply to in-person pending. This was changed in #9928, but the report was not updated to filter fraud-pending users out of the successfully verified count for in-person events. This commit modifies the cloudwatch query to filter those users who are not verified from being counted as successfully verified. [skip changelog]

… to filter fraud-pending in-person users (#10798) When the identity verification report was built the fraud-review functionality did not apply to in-person pending. This was changed in #9928, but the report was not updated to filter fraud-pending users out of the successfully verified count for in-person events. This commit modifies the cloudwatch query to filter those users who are not verified from being counted as successfully verified. [skip changelog]

… to filter fraud-pending in-person users (18F#10798) When the identity verification report was built the fraud-review functionality did not apply to in-person pending. This was changed in 18F#9928, but the report was not updated to filter fraud-pending users out of the successfully verified count for in-person events. This commit modifies the cloudwatch query to filter those users who are not verified from being counted as successfully verified. [skip changelog]

n1zyy added 2 commits January 16, 2024 17:04

LG-11997 | Mark users as fraudulent on USPS return

60b47fc

If a user has a fraud_pending_reason, mark them as fraud_review_pending_at when we get a response. changelog: Internal, IPP, Mark users as fraud_review_pending_at if appropriate

Merge branch 'main' into mattw/LG-11997

ac041b0

n1zyy commented Jan 16, 2024

View reviewed changes

lintfix

d1020d9

JackRyan1989 reviewed Jan 17, 2024

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Show resolved Hide resolved

gina-yamada reviewed Jan 17, 2024

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Show resolved Hide resolved

gina-yamada reviewed Jan 17, 2024

View reviewed changes

svalexander reviewed Jan 18, 2024

View reviewed changes

spec/jobs/get_usps_proofing_results_job_spec.rb Outdated Show resolved Hide resolved

n1zyy added 11 commits January 22, 2024 11:04

Merge branch 'main' into mattw/LG-11997

52b39fc

Fix issue with premature fraud_review_pending_at

515d6b6

Fix analytics event to use new pattern

ba9c58b

Merge branch 'main' into mattw/LG-11997

2300254

Updates test and example name

81bfd78

Merge branch 'main' into mattw/LG-11997

929f845

lintfix

ca8e665

Alphabetize method (it was, but then I renamed it)

Merge branch 'main' into mattw/LG-11997

ae0ef1f

lintfix

29b06a3

(My editor sold me out when I moved this!)

Adds feature flag and updates test

039bd7c

More cleanup

46ce9d6

n1zyy commented Jan 24, 2024

View reviewed changes

n1zyy added 3 commits January 24, 2024 21:06

Merge branch 'main' into mattw/LG-11997

fa914e6

Merge branch 'main' into mattw/LG-11997

7eb3800

Actually test analytics in the spec

952b935

n1zyy added 2 commits January 26, 2024 12:51

Revert "Actually test analytics in the spec"

30ba485

This reverts commit 952b935. (This is good stuff IMHO, but it belongs in its own PR.)

Merge branch 'main' into mattw/LG-11997

75e97fc

JackRyan1989 reviewed Jan 26, 2024

View reviewed changes

n1zyy commented Jan 26, 2024

View reviewed changes

PR feedback

a8007a9

JackRyan1989 approved these changes Jan 29, 2024

View reviewed changes

n1zyy added 2 commits January 29, 2024 12:20

lintfix

0afc7cd

Merge branch 'main' into mattw/LG-11997

89b2d6f

n1zyy merged commit e4ff006 into main Jan 29, 2024

n1zyy deleted the mattw/LG-11997 branch January 29, 2024 17:51

aduth mentioned this pull request Feb 1, 2024

Deploy RC 352 to Production #10018

Merged

jmhooper mentioned this pull request Jun 11, 2024

LG-13593 Update the cloudwatch query for identity verification report to filter fraud-pending in-person users #10798

Merged

Conversation

n1zyy commented Jan 16, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svalexander Jan 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

n1zyy Jan 16, 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!

Uh oh!

n1zyy commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

n1zyy commented Jan 26, 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!

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!

n1zyy left a comment

svalexander Jan 18, 2024 •

edited

Loading

n1zyy Jan 16, 2024 •

edited

Loading

n1zyy commented Jan 16, 2024 •

edited

Loading