Skip to content

LG-13003: Make sure we are checking all relevant Threatmetrix flags where necessary#10391

Merged
JackRyan1989 merged 9 commits intomainfrom
jryan/lg-13003-add-necessary-checks-for-tmx-enabled
Apr 10, 2024
Merged

LG-13003: Make sure we are checking all relevant Threatmetrix flags where necessary#10391
JackRyan1989 merged 9 commits intomainfrom
jryan/lg-13003-add-necessary-checks-for-tmx-enabled

Conversation

@JackRyan1989
Copy link
Contributor

@JackRyan1989 JackRyan1989 commented Apr 9, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13003

🛠 Summary of changes

Added FeatureManagement.proofing_device_profiling_decisioning_enabled? in a couple of places to verify that TMX is indeed enabled. Also did the same in fraud_review_concern via the fraud_review_pending? method which ultimately depends on this flag for setting it's value.

📜 Testing Plan

Ultimately the behavior for TMX should not change. Make sure that you have in_person_proofing_enforce_tmx set to true.

Complete local testing for Threatmetrix for a passed enrollment:

  • Complete an enrollment with fraud status set to 'review' via the drop down on the SSN page
  • In the rails console:
  • Grab the enrollment: e = InPersonEnrollment.where(enrollment_code: <enrollment_code>).first
  • Mock a passed response: json = UspsInPersonProofing::Mock::Fixtures.request_passed_proofing_results_response
  • Parse it: res = JSON.parse(json)
  • Create a new job instance: job = GetUspsProofingResultsJob.new
  • Stop the job from failing: job.instance_variable_set('@enrollment_outcomes', { enrollments_passed: 0 })
  • Send it: job.send(:process_enrollment_response, e, res)
  • Update your cache: e.reload
  • Log back in and confirm you see the Please Call screen

Complete local testing for Threatmetrix for a failed enrollment:

  • Complete an enrollment with fraud status set to 'review' via the drop down on the SSN page
  • In the rails console:
  • Grab the enrollment: e = InPersonEnrollment.where(enrollment_code: <enrollment_code>).first
  • Mock a passed response: json = UspsInPersonProofing::Mock::Fixtures.request_failed_proofing_results_response
  • Parse it: res = JSON.parse(json)
  • Create a new job instance: job = GetUspsProofingResultsJob.new
  • Stop the job from failing: job.instance_variable_set('@enrollment_outcomes', { enrollments_failed: 0 })
  • Send it: job.send(:process_enrollment_response, e, res)
  • Update your cache: e.reload
  • Log back in and confirm you DO NOT see the Please Call screen.
  • You should be able to restart the IPP flow going from the account page.

@JackRyan1989 JackRyan1989 requested review from a team and KeithNava April 9, 2024 18:43
@JackRyan1989 JackRyan1989 self-assigned this Apr 9, 2024
Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

I gave this a quick look and it seems correct, but I want to give it some more review (after our next meeting) before formally approving.

end

def handle_fraud_rejection
return if in_person_prevent_fraud_redirection?
Copy link
Contributor

Choose a reason for hiding this comment

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

Talking through this some more just for posterity: the idea here is that in the case of fraud rejection, the question of whether to redirect them to the please-call page is irrelevant, because that's really for the pending state, not the rejection state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@n1zyy my thinking was incorrect at the time, see this slack thread. note that the changes I made are designed to handle the case where someone fails at the post office, and then is fraud rejected, which is super unlikely given that the user will not be shown the Please Call screen.

JackRyan1989 added 2 commits April 9, 2024 16:02
IdentityConfig.store.in_person_proofing_enforce_tmx &&
current_user.ipp_enrollment_status_not_passed?
current_user.ipp_enrollment_status_not_passed? &&
(fraud_review_pending? || fraud_rejection?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undoing previous change around fraud rejection. See this slack thread for justification

Checking for fraud_review_pending and fraud_rejection because this is called before both fraud redirection cases.

Copy link
Contributor

@gina-yamada gina-yamada Apr 10, 2024

Choose a reason for hiding this comment

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

Jack- can you explain again why the additional check ((fraud_review_pending? || fraud_rejection?)) is needed? I am thinking about the method that is calling this method and I am not sure about this. 🤔

Update: After getting on a call with Jack and others on Joy- we talked through this logic - ipp_enrollment_status_not_passed (When a user fails IPP- we let them try again. I mixed up the logic so this is okay. Here is the table that says okay https://docs.google.com/document/d/1Xvhz5xXQgQK7oYDlHYBkG-8-v6mY9yS9g_s-70Kzr1g/edit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a comment above this method to help clarify what's going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


def tmx_status
return nil unless IdentityConfig.store.in_person_proofing_enforce_tmx
return nil unless FeatureManagement.proofing_device_profiling_decisioning_enabled?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has an impact on analytics, hence the change to the analytics spec.

if in_person_verification_needed
if IdentityConfig.store.in_person_proofing_enforce_tmx
if IdentityConfig.store.in_person_proofing_enforce_tmx &&
FeatureManagement.proofing_device_profiling_decisioning_enabled?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthinz I think this addresses your concerns here

Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider moving this into a new method in FeatureManagement--I believe the intention there is that when we have these kind of relationships between config flags we can encapsulate them in method in there. For example, you could do FeatureManagement.ipp_proofing_device_profiling_decisioning_enabled? (side note: I just filed a ticket to come up with a better name than "proofing_device_profiling" but for now that is what we have)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthinz I think we have a bunch of these flags that could probably be moved in to that class. I think I'll open our own ticket for consolidating our features. Thanks!

@svalexander
Copy link
Contributor

svalexander commented Apr 10, 2024

i'm testing this out and so far for the 1st scenario I got the please call email and saw the screen without having to pass the enrollment in the rails console. So it appears having the tmx "review" status w/o updating the enrollment will show the screen.

Edit to add: We tested this again together and looked at this particular enrollment. When this enrollment was created it was right before the 1/2 hr mark (which is the cadence at which the usps job is run) and it was passed by the job w/o having to update in the console. So this was a fluke not related to the pr. Might be useful to note time when enrollment is created as this may happen again with future testing.

Copy link
Contributor

@svalexander svalexander left a comment

Choose a reason for hiding this comment

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

everything is working as expected, LGTM

Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

I tested this code "normally," and with the newly-considered flag disabled (causing us to ignore TMX), and in that case, the process is normal. I'm calling this good.

+1 to the idea we just discussed of adding a quick comment to the first section since it's dealing with kind of confusing logic, though.

@JackRyan1989 JackRyan1989 merged commit 2875f1c into main Apr 10, 2024
@JackRyan1989 JackRyan1989 deleted the jryan/lg-13003-add-necessary-checks-for-tmx-enabled branch April 10, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants