Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 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
42 changes: 32 additions & 10 deletions app/jobs/get_usps_proofing_results_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ def check_enrollment(enrollment)

status_check_attempted_at = Time.zone.now
enrollment_outcomes[:enrollments_checked] += 1
response = nil

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

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.

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.

Expand Down Expand Up @@ -305,6 +304,16 @@ def handle_expired_status_update(enrollment, response, response_message)
end
end

def handle_fraud_review_pending(enrollment)
Comment thread
n1zyy marked this conversation as resolved.
return unless IdentityConfig.store.in_person_proofing_enforce_tmx

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.

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.

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.

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.)

enrollment.profile&.deactivate_for_fraud_review

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.

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

@svalexander svalexander Jan 18, 2024

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.

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

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.

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.

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.

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?

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.

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?

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.

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.


analytics(user: enrollment.user).
idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review(
**enrollment_analytics_attributes(enrollment, complete: true),
)
Comment thread
n1zyy marked this conversation as resolved.
end

def handle_unexpected_response(enrollment, response_message, reason:, cancel: true)
enrollment.cancelled! if cancel

Expand Down Expand Up @@ -363,21 +372,24 @@ def handle_successful_status_update(enrollment, response)
reason: 'Successful status update',
job_name: self.class.name,
)
enrollment.profile.activate_after_passing_in_person

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.

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.

enrollment.update(
status: :passed,
proofed_at: proofed_at,
status_check_completed_at: Time.zone.now,
)

# send SMS and email
send_enrollment_status_sms_notification(enrollment: enrollment)
send_verified_email(enrollment.user, enrollment)
analytics(user: enrollment.user).idv_in_person_usps_proofing_results_job_email_initiated(
**email_analytics_attributes(enrollment),
email_type: 'Success',
job_name: self.class.name,
)
unless fraud_result_pending?(enrollment)

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.

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.

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.

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.

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.

enrollment.profile.activate_after_passing_in_person

# send SMS and email
send_enrollment_status_sms_notification(enrollment: enrollment)
Comment thread
n1zyy marked this conversation as resolved.
send_verified_email(enrollment.user, enrollment)
analytics(user: enrollment.user).idv_in_person_usps_proofing_results_job_email_initiated(
**email_analytics_attributes(enrollment),
email_type: 'Success',
job_name: self.class.name,
)
end
end

def handle_unsupported_secondary_id(enrollment, response)
Expand Down Expand Up @@ -405,12 +417,22 @@ def handle_unsupported_secondary_id(enrollment, response)
)
end

def fraud_result_pending?(enrollment)
enrollment.profile&.fraud_pending_reason.present?
end
Comment thread
n1zyy marked this conversation as resolved.

def process_enrollment_response(enrollment, response)
unless response.is_a?(Hash)
handle_response_is_not_a_hash(enrollment)
return
end

# We want to deactivate them regardless of status, but then allow the
# case statement below to pick up the correct flow.
if fraud_result_pending?(enrollment)
handle_fraud_review_pending(enrollment)
end

case response['status']
Comment thread
n1zyy marked this conversation as resolved.
when IPP_STATUS_PASSED
if passed_with_unsupported_secondary_id_type?(response)
Expand Down
13 changes: 13 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2246,6 +2246,19 @@ def idv_in_person_usps_proofing_results_job_unexpected_response(
)
end

# A user has been moved to fraud review after completing proofing at the USPS
# @param [String] enrollment_id
Comment thread
JackRyan1989 marked this conversation as resolved.
def idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review(
enrollment_id:,
**extra
)
track_event(
:idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review,
enrollment_id: enrollment_id,
**extra,
)
end

# Tracks if USPS in-person proofing enrollment request fails
# @param [String] context
# @param [String] reason
Expand Down
3 changes: 2 additions & 1 deletion app/services/idv/profile_maker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ def save_profile(

profile.save!
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

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.

@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!

profile.deactivate_for_fraud_review
end
profile
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ in_person_email_reminder_early_benchmark_in_days: 11
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

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.

Global default is false

in_person_proofing_opt_in_enabled: false
in_person_enrollment_validity_in_days: 30
in_person_enrollments_ready_job_email_body_pattern: '\A\s*(?<enrollment_code>\d{16})\s*\Z'
Expand Down Expand Up @@ -392,6 +393,7 @@ development:
hmac_fingerprinter_key_queue: '["11111111111111111111111111111111", "22222222222222222222222222222222"]'
identity_pki_local_dev: true
in_person_proofing_enabled: true
in_person_proofing_enforce_tmx: true

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.

But it's true in dev

in_person_send_proofing_notifications_enabled: true
logins_per_ip_limit: 5
logo_upload_enabled: true
Expand Down
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ def self.build_store(config_map)
config.add(:in_person_outage_expected_update_date, type: :string)
config.add(:in_person_outage_message_enabled, type: :boolean)
config.add(:in_person_proofing_enabled, type: :boolean)
config.add(:in_person_proofing_enforce_tmx, type: :boolean)
config.add(:in_person_proofing_opt_in_enabled, type: :boolean)
config.add(:in_person_public_address_search_enabled, type: :boolean)
config.add(:in_person_results_delay_in_hours, type: :integer)
Expand Down
85 changes: 85 additions & 0 deletions spec/jobs/get_usps_proofing_results_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,91 @@
error_type: Faraday::NilStatusError,
)
end

context 'when a user was flagged with fraud_pending_reason' do
before(:each) do
pending_enrollment.profile.update!(fraud_pending_reason: 'threatmetrix_review')
stub_request_passed_proofing_results
end

it 'updates the enrollment' do

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.

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.

freeze_time do
expect(pending_enrollment.status).to eq 'pending'
job.perform(Time.zone.now)
expect(pending_enrollment.reload.status).to eq 'passed'
end
end

context 'when the in_person_proofing_enforce_tmx flag is false' do
before do
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).
and_return(false)
end

it 'does not mark the user as fraud_review_pending_at' do
freeze_time do
job.perform(Time.zone.now)

pending_enrollment.reload
profile = pending_enrollment.profile
expect(profile).not_to be_fraud_review_pending
end
end

it 'does not log a user_sent_to_fraud_review analytics event' do
job.perform(Time.zone.now)

expect(job_analytics).not_to have_logged_event(
:idv_in_person_usps_proofing_results_job_user_sent_to_fraud_review,
)
end
end

context 'when the in_person_proofing_enforce_tmx flag is true' do
before do
allow(IdentityConfig.store).to receive(:in_person_proofing_enforce_tmx).
and_return(true)
end

it 'preserves the fraud_pending_reason attribute' do
job.perform(Time.zone.now)

expect(pending_enrollment.profile.fraud_pending_reason).to eq 'threatmetrix_review'
end

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,
),
)
end

it 'does not send a success email' do
user = pending_enrollment.user

freeze_time do
expect do
job.perform(Time.zone.now)
end.not_to have_enqueued_mail(UserMailer, :in_person_verified).with(
params: { user: user, email_address: user.email_addresses.first },
args: [{ enrollment: pending_enrollment }],
)
end
end

it 'marks the user as fraud_review_pending_at' do
job.perform(Time.zone.now)

profile = pending_enrollment.reload.profile
expect(profile.fraud_review_pending_at).not_to be_nil
expect(profile).not_to be_active
end

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.

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.

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.

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.

end
end
end

describe 'Proofed with secondary id' do
Expand Down
24 changes: 22 additions & 2 deletions spec/services/idv/profile_maker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,18 @@
end

context 'with fraud review needed' do
let(:gpo_verification_needed) { false }
let(:in_person_verification_needed) { false }
let(:profile) do
subject.save_profile(
fraud_pending_reason: 'threatmetrix_review',
gpo_verification_needed: false,
gpo_verification_needed: gpo_verification_needed,
deactivation_reason: nil,
in_person_verification_needed: false,
in_person_verification_needed: in_person_verification_needed,
selfie_check_performed: false,
)
end

it 'creates a pending profile for fraud review' do
expect(profile.activated_at).to be_nil
expect(profile.active).to eq(false)
Expand All @@ -84,9 +87,26 @@
expect(profile.initiating_service_provider).to eq(nil)
expect(profile.verified_at).to be_nil
end

it 'marks the profile as legacy_unsupervised' do
expect(profile.idv_level).to eql('legacy_unsupervised')
end

context 'when GPO verification is needed' do
let(:gpo_verification_needed) { true }

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

context 'when IPP is needed' do
let(:in_person_verification_needed) { true }

it 'is not fraud_review_pending?' do
expect(profile.fraud_review_pending?).to eq(false)
end
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.

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.

end

context 'with gpo_verification_needed' do
Expand Down