Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions app/forms/gpo_verify_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ def submit
pending_profile&.deactivate(:in_person_verification_pending)
elsif fraud_review_checker.fraud_check_failed? && threatmetrix_enabled?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: nest the if logic here so it's clear that fraud_review_checker.fraud_check_failed? is true for both current elsif clauses:

elsif fraud_review_checker.fraud_check_failed?
  if threatmetrix_enabled?
    bump_fraud_review_pending_timestamps
  end
  pending_profile&.activate_after_fraud_review_unnecessary
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't generally love nested conditionals. This conditional is already starting to look like a pyramid of doom.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏾

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
Expand Down
11 changes: 11 additions & 0 deletions app/models/profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
9 changes: 2 additions & 7 deletions spec/forms/gpo_verify_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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' }
Expand Down
55 changes: 48 additions & 7 deletions spec/models/profile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,51 @@
end
end

describe '#activate_after_fraud_review_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
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

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
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
Expand Down Expand Up @@ -993,7 +1038,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
Expand All @@ -1013,7 +1058,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
Expand All @@ -1028,11 +1073,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
Expand Down