From bcb85346331d6acd8721c40255abaeddf52f3c93 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 19 Jul 2023 13:29:58 -0400 Subject: [PATCH 1/4] Clear `fraud_pending_reason` when activating profiles when TMx is disabled When ThreatMetrix decisioning is not enabled we do not put users through the fraud review process after they enter their GPO code. This requires clearing out the fraud review columns on their profile. Previously, this was done by explicitly overwriting the fraud review columns on their profile. We are trying to move away from this pattern and move these methods into the profile. While we were explicitly overwriting the columns we missed that we need to also overwrite `fraud_pending_reason` which was added later than the other columns. This commit adds a method to the profile for activating the profile when fraud review is not necessary. That method is used in the verify form under these conditions instead of explicitly modifying the profile. [skip changelog] --- app/forms/gpo_verify_form.rb | 6 ++---- app/models/profile.rb | 11 +++++++++++ spec/forms/gpo_verify_form_spec.rb | 7 +------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/forms/gpo_verify_form.rb b/app/forms/gpo_verify_form.rb index a9998f72c8d..891ee80c756 100644 --- a/app/forms/gpo_verify_form.rb +++ b/app/forms/gpo_verify_form.rb @@ -25,11 +25,9 @@ def submit pending_profile&.deactivate(:in_person_verification_pending) elsif fraud_review_checker.fraud_check_failed? && threatmetrix_enabled? bump_fraud_review_pending_timestamps + elsif fraud_review_checker.fraud_check_failed? + pending_profile&.activate_after_fraud_review_unnecessary else - pending_profile&.update!( - fraud_review_pending_at: nil, - fraud_rejection_at: nil, - ) activate_profile end else diff --git a/app/models/profile.rb b/app/models/profile.rb index 0598fdbf489..0dfb29e91fb 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -98,6 +98,17 @@ def activate_after_passing_review track_fraud_review_adjudication(decision: 'pass') if active? end + def activate_after_fraud_review_unnecessary + transaction do + update!( + fraud_review_pending_at: nil, + fraud_rejection_at: nil, + fraud_pending_reason: nil, + ) + activate + end + end + def activate_after_passing_in_person transaction do update!( diff --git a/spec/forms/gpo_verify_form_spec.rb b/spec/forms/gpo_verify_form_spec.rb index ae8618710f1..0a68e1d7cb1 100644 --- a/spec/forms/gpo_verify_form_spec.rb +++ b/spec/forms/gpo_verify_form_spec.rb @@ -151,12 +151,7 @@ context 'ThreatMetrix rejection' do let(:pending_profile) do - create( - :profile, - user: user, - gpo_verification_pending_at: 1.day.ago, - fraud_review_pending_at: 1.day.ago, - ) + create(:profile, :verify_by_mail_pending, :fraud_review_pending, user: user) end let(:threatmetrix_review_status) { 'reject' } From 917d2d7616101abced62733b971530f0301215dd Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 19 Jul 2023 14:40:16 -0400 Subject: [PATCH 2/4] add a test for the new profile method --- spec/models/profile_spec.rb | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 3450ccedcb7..d9257fdb240 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -891,6 +891,47 @@ end end + describe '#activate_after_fraud_review_unnecessary' do + it 'activates a profile if raud review is unnecessary' do + profile = create(:profile, :fraud_review_pending, user: user) + + expect(profile.activated_at).to be_nil # to change + expect(profile.active).to eq(false) # to change + expect(profile.deactivation_reason).to be_nil + expect(profile.fraud_review_pending?).to eq(true) # to change + expect(profile.gpo_verification_pending_at).to be_nil + expect(profile.initiating_service_provider).to be_nil + expect(profile.verified_at).to be_nil # to change + + profile.activate_after_fraud_review_unnecessary + + expect(profile.activated_at).to be_present # changed + expect(profile.active).to eq(true) # changed + expect(profile.deactivation_reason).to be_nil + expect(profile.fraud_review_pending?).to eq(false) # changed + expect(profile.gpo_verification_pending_at).to be_nil + expect(profile.initiating_service_provider).to be_nil + expect(profile.verified_at).to be_present # changed + + expect(profile).to be_active + expect(profile.fraud_review_pending_at).to be_nil + expect(profile.fraud_pending_reason).to be_nil + end + + it 'does not activate a profile if transaction raises an error' do + profile = create(:profile, :fraud_review_pending, user: user) + + allow(profile).to receive(:update!).and_raise(RuntimeError) + + suppress(RuntimeError) do + profile.activate_after_fraud_review_unnecessary + end + + expect(profile.fraud_review_pending_at).to_not eq nil + expect(profile).to_not be_active + end + end + # TODO: does deactivating make sense for a non-active profile? Should we prevent it? # TODO: related: should we test against an active profile here? describe '#deactivate_for_gpo_verification' do From d399c45adddf632b24e24c01e714e3832d3dc275 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Wed, 19 Jul 2023 15:00:13 -0400 Subject: [PATCH 3/4] cleanup some factory traits --- spec/forms/gpo_verify_form_spec.rb | 2 +- spec/models/profile_spec.rb | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/spec/forms/gpo_verify_form_spec.rb b/spec/forms/gpo_verify_form_spec.rb index 0a68e1d7cb1..4e9b69843e7 100644 --- a/spec/forms/gpo_verify_form_spec.rb +++ b/spec/forms/gpo_verify_form_spec.rb @@ -13,8 +13,8 @@ let(:pending_profile) do create( :profile, + :verify_by_mail_pending, user: user, - gpo_verification_pending_at: 1.day.ago, proofing_components: proofing_components, ) end diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index d9257fdb240..53d2fac4621 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -1034,7 +1034,7 @@ context 'it notifies the user' do let(:profile) do - profile = user.profiles.create(fraud_review_pending_at: 1.day.ago) + profile = create(:profile, :fraud_review_pending, user: user) profile.reject_for_fraud(notify_user: true) profile end @@ -1054,7 +1054,7 @@ context 'it does not notify the user' do let(:profile) do - profile = user.profiles.create(fraud_review_pending_at: 1.day.ago) + profile = create(:profile, :fraud_review_pending, user: user) profile.reject_for_fraud(notify_user: false) profile end @@ -1069,11 +1069,7 @@ context 'when the SP is the IRS' do let(:sp) { create(:service_provider, :irs) } let(:profile) do - user.profiles.create( - active: false, - fraud_review_pending_at: 1.day.ago, - initiating_service_provider: sp, - ) + create(:profile, :fraud_review_pending, active: false, initiating_service_provider: sp) end context 'and notify_user is true' do From 17f4fd61137ef27e65959bbb6083d7a8ad2f46a1 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Thu, 20 Jul 2023 10:33:13 -0400 Subject: [PATCH 4/4] code review --- spec/models/profile_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 53d2fac4621..cd023aefc8d 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -892,7 +892,7 @@ end describe '#activate_after_fraud_review_unnecessary' do - it 'activates a profile if raud review is unnecessary' do + it 'activates a profile if fraud review is unnecessary' do profile = create(:profile, :fraud_review_pending, user: user) expect(profile.activated_at).to be_nil # to change @@ -903,6 +903,10 @@ expect(profile.initiating_service_provider).to be_nil expect(profile.verified_at).to be_nil # to change + expect(profile).to_not be_active + expect(profile.fraud_review_pending_at).to_not be_nil + expect(profile.fraud_pending_reason).to_not be_nil + profile.activate_after_fraud_review_unnecessary expect(profile.activated_at).to be_present # changed