Please Call Screen for IPP Threatmetrix review by JackRyan1989 · Pull Request #10033 · 18F/identity-idp

JackRyan1989 · 2024-02-05T19:12:06Z

🎫 Ticket

Link to the relevant ticket:
LG-11995

🛠 Summary of changes

Added new IPP specific fraud conditions to the fraud checker and concern
Referenced those new check in the review page
Added new boolean var to the please call page template to hide the progress bar for IPP
Updated specs

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

Test with a failed enrollment:

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Images

…t a pending ipp enrollment exists for please call

…se call page when flagged for fraud review

n1zyy

I need to circle back for a more thorough review after some meetings, but taking a quick look this looks like good stuff. My comments are optional suggestions.

app/controllers/idv/please_call_controller.rb

app/views/idv/please_call/show.html.erb

…5-threatmetrix-please-call-ipp-screen Merging in main.

carmenrosalop · 2024-02-06T17:41:17Z

Shared design feedback on this slack thread https://gsa-tts.slack.com/archives/C03FA4VBN76/p1707240954111969?thread_ts=1707160403.648729&cid=C03FA4VBN76

svalexander · 2024-02-06T20:41:13Z

app/controllers/idv/please_call_controller.rb

      end
    end
+
+    def in_person_proofing?


this is not a big thing but i'm wondering about naming here. This is checking both that the user went through ipp and that the status is passed. Is there maybe some naming that could capture both? not blocking at all though

I had the same thought at first.

Where I landed is that in the particular context of the PleaseCallController, "Is this for IPP?" basically means what is implemented in this method.

That said, since this is assigned to the @in_person variable in the show method, we could still name this something more precise. But I don't feel strongly on this one. (And I don't have an obvious name to suggest.)

But, see my next comment.

svalexander · 2024-02-06T21:03:12Z

spec/controllers/idv/in_person/ready_to_verify_controller_spec.rb

+        end
+
+        context 'in_person_proofing_enforce_tmx enabled, pending fraud review,
+          enrollment not passed' do


I think we might actually not want to show the screen in this case but maybe @daviddsilvanava or @eryn-sobing can confirm. The reason I think this is that we currently send the "please call" email to users who get a passed status but not to the users who get a failed status. Ticket where we implemented that behavior.

Aww shoot, good catch @svalexander. I'm going back to the ticket and finding it's way too vaguely worded. (I wrote it. This is my fault.) This is a tricky gotcha.

I think I would prefer to take discussion of this offline to Slack, but I agree with tagging in David and Eryn and chatting about this tomorrow. It's a question of whether we're consistent with the email we send (I think we should be), or consistent with the behavior in other parts of the app (which I also think we should be, but the two are contradictory here).

So @svalexander I realized I messed up the description text for this spec. It's actually working as intended. The 'establishing' enrollment does not redirect to please call, but does render the ready to verify text which is indeed what we want to happen.

oh ok so i interpreted "not passed" as "failed" but it's actually "establishing". Can we update the context to enrollment status equals establishing or something like that to make the spec super clear?

maybe we should also add a spec where enrollment has a status of failed?

actually maybe that last statement is not needed? looking at the spec in the please_call_controller_spec on line 78 I think that would satisfy my previous suggestion?

Line 78 handles threatmetrix rejection whereas the setting I think you're referring to is for the enrollment. The idea behind saying 'not passed' is that a passed enrollment is the only status we're looking for. We don't do anything special with any of the other statuses, so I could put anything else in there and the result would be pretty much the same.

Also, this is for fraud review pending. I could do another test for rejection but as you say that is kinda handled in the please_call_controller_spec.

ok that makes sense

n1zyy

Blargh. I think this is really good code (with one suggestion left for a possible tweak), but I think this needs to wait for us to figure out https://github.com/18F/identity-idp/pull/10033/files#r1480537532 which is a requirements issue. I feel bad I didn't see this ahead of time.

n1zyy · 2024-02-07T03:42:04Z

app/controllers/idv/please_call_controller.rb

      end
    end
+
+    def in_person_proofing?


I had the same thought at first.

Where I landed is that in the particular context of the PleaseCallController, "Is this for IPP?" basically means what is implemented in this method.

That said, since this is assigned to the @in_person variable in the show method, we could still name this something more precise. But I don't feel strongly on this one. (And I don't have an obvious name to suggest.)

But, see my next comment.

app/controllers/idv/please_call_controller.rb

n1zyy · 2024-02-07T03:58:52Z

app/services/fraud_review_checker.rb

+
+  def ipp_fraud_review_pending?
+    fraud_review_pending? &&
+      user&.fraud_review_pending_profile&.in_person_enrollment&.status == 'passed'


Oh, actually... The similarity to the in_person_proofing? method above maybe feels like a bonus reason to just put this on User and whistle innocently while avoiding the thorny question of exactly where the line between user.rb and profile.rb should be.

Ahhhh. Ok. Yeah I think we can make a good case for checking for enrollment status on the user.

n1zyy · 2024-02-07T04:08:59Z

spec/controllers/idv/in_person/ready_to_verify_controller_spec.rb

+        end
+
+        context 'in_person_proofing_enforce_tmx enabled, pending fraud review,
+          enrollment not passed' do


Aww shoot, good catch @svalexander. I'm going back to the ticket and finding it's way too vaguely worded. (I wrote it. This is my fault.) This is a tricky gotcha.

I think I would prefer to take discussion of this offline to Slack, but I agree with tagging in David and Eryn and chatting about this tomorrow. It's a question of whether we're consistent with the email we send (I think we should be), or consistent with the behavior in other parts of the app (which I also think we should be, but the two are contradictory here).

svalexander · 2024-02-07T23:07:15Z

app/controllers/idv/please_call_controller.rb

+      IdentityConfig.store.in_person_proofing_enabled
+    end
+
+    # we only want to handle enrollments that have passed


this is a great comment to add for future reference, thank you!

svalexander · 2024-02-07T23:07:34Z

app/controllers/idv/please_call_controller.rb

      analytics.idv_please_call_visited
      pending_at = current_user.fraud_review_pending_profile.fraud_review_pending_at
      @call_by_date = pending_at + FRAUD_REVIEW_CONTACT_WITHIN_DAYS
+      @in_person = ipp_enabled_and_enrollment_passed?


this is super clear now, thanks for updating this!

spec/services/fraud_review_check_spec.rb

svalexander

Approving as none of my current suggestions are blocking.

…0 50 state and adding spec

svalexander · 2024-02-08T16:26:51Z

spec/services/fraud_review_check_spec.rb

+      let!(:profile) { create(:profile, fraud_review_pending_at: 1.day.ago, user: user) }
+      let!(:enrollment) { create(:in_person_enrollment, :passed, user: user, profile: profile) }
+
+      it 'returns true' do


thanks for this update!

…le_pending_fraud_review on users with ipp enrollments

…l case respects feature flag

gina-yamada · 2024-02-09T22:16:57Z

app/controllers/idv/please_call_controller.rb

    end
+
+    def in_person_proofing_enabled?
+      IdentityConfig.store.in_person_proofing_enabled


If a SP has not opted in, the user should not have made it to this point but should we also be checking if in_person_proofing is enabled on the SP? Maybe too much but just pondering it.

This page is rendered asynchronously and always after the user has completed the identity verification flow. If they have not opted in but have completed IdV and been flagged for fraud they should absolutely see this page.

gina-yamada · 2024-02-09T22:44:14Z

spec/views/idv/please_call/show.html.erb_spec.rb

+        text: t('step_indicator.flows.idv.secure_account'),
+      )
+    end
+  end


I was hoping to see this test as it is being conditionally rendered. Nice ⭐

gina-yamada · 2024-02-12T16:36:15Z

gina-yamada

Failing tests will need to be resolved but I was able to successfully see Please Call screens in all three languages with no progress bar. I did leave a comment last week about an additional check but don't think it is a road blocker.

…ly when not in ipp

…5-threatmetrix-please-call-ipp-screen Merging in main to get pinpoint checks to pass.

gina-yamada · 2024-03-04T20:06:16Z

@JackRyan1989 I was looking at specs for the rollout plan today. I noticed the mocks say the user has 30 days to call. The call by date on the screen is 14 days. See please_call_controller FRAUD_REVIEW_CONTACT_WITHING_DAYS

When I look back at my screenshots above, they are 3 weeks old and would be second week in Feb. If it was 30 days- the date on the screen should be in March but it reads February- proving the display reads only 14 days. Maybe not a big deal but wondering if product needs to approve this change or if we need to make a new ticket to handle IPP differently. Sorry, I missed checking the date entirely.

JackRyan1989 · 2024-03-04T20:24:09Z

Excellent catch @gina-yamada. I can escalate this to product and check. I'll open another pull request to make the change when the time comes.

gina-yamada · 2024-03-05T15:58:13Z

@JackRyan1989 I was looking at specs for the rollout plan today. I noticed the mocks say the user has 30 days to call. The call by date on the screen is 14 days. See please_call_controller FRAUD_REVIEW_CONTACT_WITHING_DAYS

When I look back at my screenshots above, they are 3 weeks old and would be second week in Feb. If it was 30 days- the date on the screen should be in March but it reads February- proving the display reads only 14 days. Maybe not a big deal but wondering if product needs to approve this change or if we need to make a new ticket to handle IPP differently. Sorry, I missed checking the date entirely.

@JackRyan1989 discovered the 14 days date displayed is accurate and expected behavior. We do not need to change the date or take any further action. See this Slack thread.

JackRyan1989 added 14 commits January 30, 2024 17:02

update fraud review concern

07b2cbf

add contitional for use in displaying the progress bar

d76d05c

add fraud review concern as before action

98e0979

update view to use conditional var

61b6bd5

updating specs

ec818bf

trying to get please call view to conditionally hide step indicator

ca553cc

update concerns and checker

d1a0930

Confirm that the enrollment is passed in fraud review and confrim tha…

ab909b3

…t a pending ipp enrollment exists for please call

get spec to passing

554e3c9

remove unneeded session info from spec

3f491be

edits to please call controller

a1feb3b

revising check for fraud review checker and spec

005c19c

changelog: User-Facing Improvements, In Person Proofing, Display plea…

788adfc

…se call page when flagged for fraud review

changelog: User-Facing Improvements, In Person Proofing, Display plea…

cc17f3f

…se call page when flagged for fraud review

JackRyan1989 self-assigned this Feb 5, 2024

JackRyan1989 requested review from a team, WheresHJ and gina-yamada and removed request for a team February 5, 2024 19:12

n1zyy reviewed Feb 5, 2024

View reviewed changes

app/controllers/idv/please_call_controller.rb Outdated Show resolved Hide resolved

app/views/idv/please_call/show.html.erb Show resolved Hide resolved

JackRyan1989 added 3 commits February 6, 2024 10:18

lintfix

a1d8f15

ready to verify spec passing

641d3aa

Merge branch 'main' of github.com:18F/identity-idp into jryan/lg-1199…

28f6932

…5-threatmetrix-please-call-ipp-screen Merging in main.

carmenrosalop requested review from carmenrosalop and removed request for WheresHJ February 6, 2024 17:39

svalexander reviewed Feb 6, 2024

View reviewed changes

n1zyy reviewed Feb 7, 2024

View reviewed changes

svalexander reviewed Feb 7, 2024

View reviewed changes

spec/services/fraud_review_check_spec.rb Outdated Show resolved Hide resolved

svalexander approved these changes Feb 7, 2024

View reviewed changes

JackRyan1989 requested a review from n1zyy February 8, 2024 14:25

JackRyan1989 added 3 commits February 8, 2024 09:39

addressing Shannon's review

b514176

enforcing tmx flag for call to new user method to prevent issues in 5…

80f60bc

…0 50 state and adding spec

lintfix

317480d

JackRyan1989 requested a review from svalexander February 8, 2024 16:10

svalexander reviewed Feb 8, 2024

View reviewed changes

JackRyan1989 added 2 commits February 9, 2024 13:53

call in_person_handle_pending_fraud_review initially and prevetn hand…

d8183a0

…le_pending_fraud_review on users with ipp enrollments

enforce redirection only for passing enrollments and refactor so usua…

977bdef

…l case respects feature flag

gina-yamada reviewed Feb 9, 2024

View reviewed changes

gina-yamada self-requested a review February 12, 2024 16:37

gina-yamada approved these changes Feb 12, 2024

View reviewed changes

JackRyan1989 added 5 commits February 12, 2024 12:41

refactor to be more explicit and to handle enrollment statuses proper…

05f36b7

…ly when not in ipp

lintfix

97d5b12

use the right before action

3ca117c

Merge branch 'main' of github.com:18F/identity-idp into jryan/lg-1199…

d11924c

…5-threatmetrix-please-call-ipp-screen Merging in main to get pinpoint checks to pass.

fix before action and page rendering expectations

713f5be

JackRyan1989 merged commit 4600159 into main Feb 15, 2024

JackRyan1989 deleted the jryan/lg-11995-threatmetrix-please-call-ipp-screen branch February 15, 2024 14:46

solipet mentioned this pull request Feb 15, 2024

Deploy RC 356 to Production #10093

Merged

This was referenced Feb 20, 2024

Jryan/lg 12172 tmx rejection screen #10116

Closed

LG-12172 implement tmx rejection screen and refactor please call code #10119

Merged

Conversation

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

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Test with a failed enrollment:

👀 Screenshots

Uh oh!

n1zyy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carmenrosalop commented Feb 6, 2024

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!

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

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!

Uh oh!

svalexander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gina-yamada Feb 9, 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!

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

Uh oh!

Uh oh!

gina-yamada left a comment • 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!

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

Uh oh!

Uh oh!

JackRyan1989 commented Mar 4, 2024

Uh oh!

gina-yamada commented Mar 5, 2024

Uh oh!

JackRyan1989 commented Feb 5, 2024 •

edited

Loading

gina-yamada Feb 9, 2024 •

edited

Loading

gina-yamada commented Feb 12, 2024 •

edited

Loading

gina-yamada left a comment •

edited

Loading

gina-yamada commented Mar 4, 2024 •

edited

Loading