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
14 changes: 4 additions & 10 deletions app/controllers/concerns/idv_step_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
module IdvStepConcern
extend ActiveSupport::Concern

include VerifyProfileConcern
include IdvSessionConcern
include RateLimitConcern
include FraudReviewConcern
Expand All @@ -14,9 +15,7 @@ module IdvStepConcern
before_action :confirm_personal_key_acknowledged_if_needed
before_action :confirm_idv_needed
before_action :confirm_letter_recently_enqueued
before_action :confirm_no_pending_gpo_profile
before_action :confirm_no_pending_in_person_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.

Hey @jmhooper, for future PRs that affect in_person_enrollments, could you please tag @identity-joy-engineers for a review? It would help us track potential changes to our flow.

If you’re concerned about turnaround time, you can also post the PR in the #login-team-joy-eng channel and tag the @login-joy-engineers group. Thanks!

before_action :handle_fraud
before_action :confirm_no_pending_profile
before_action :check_for_mail_only_outage
end

Expand All @@ -35,13 +34,8 @@ def confirm_letter_recently_enqueued
return redirect_to idv_letter_enqueued_url if letter_recently_enqueued?
end

def confirm_no_pending_gpo_profile
redirect_to idv_verify_by_mail_enter_code_url if letter_not_recently_enqueued?
end

def confirm_no_pending_in_person_enrollment
return if !IdentityConfig.store.in_person_proofing_enabled
redirect_to idv_in_person_ready_to_verify_url if current_user&.pending_in_person_enrollment
def confirm_no_pending_profile
redirect_to url_for_pending_profile_reason if user_has_pending_profile?
end

def check_for_mail_only_outage
Expand Down
9 changes: 5 additions & 4 deletions app/controllers/concerns/verify_profile_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,12 @@ module VerifyProfileConcern
def url_for_pending_profile_reason
return idv_verify_by_mail_enter_code_url if current_user.gpo_verification_pending_profile?
return idv_in_person_ready_to_verify_url if current_user.in_person_pending_profile?
# We don't want to hit idv_please_call_url in cases where the user
# has fraud review pending and not passed at the post office
return idv_welcome_url if user_failed_ipp_with_fraud_review_pending?
return idv_please_call_url if current_user.fraud_review_pending?
idv_not_verified_url if current_user.fraud_rejection?
end

def user_has_pending_profile?
pending_profile_policy.user_has_pending_profile?
pending_profile_policy.user_has_pending_profile? && !user_failed_ipp_with_fraud_review_pending?
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.

Should this !user_failed_ipp_with_fraud_review_pending be part of PendingProfilePolicy?

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 had some back and forth on this. It is a little tough to track exactly what it does. It appears that if a user failed IPP with fraud review pending then their pending_profile does not count. That implementation is, perhaps, questionable but I think that is a bigger change than this.

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.

It feels like if we know they failed IPP we should cancel the profile

end

def pending_profile_policy
Expand All @@ -25,6 +22,10 @@ def pending_profile_policy
)
end

# Returns true if the user has not passed IPP at the post office and is
# flagged for fraud review, or has been rejected for fraud.
# Ultimately this is to allow users who fail at the post office to create another enrollment
# bypassing the typical flow of showing the Please Call or Fraud Rejection screens.
def user_failed_ipp_with_fraud_review_pending?
IdentityConfig.store.in_person_proofing_enforce_tmx &&
current_user.ipp_enrollment_status_not_passed? &&
Expand Down
3 changes: 1 addition & 2 deletions app/controllers/idv/personal_key_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ class PersonalKeyController < ApplicationController
# standard before_actions and handle them in our own special way below.
skip_before_action :confirm_idv_needed
skip_before_action :confirm_personal_key_acknowledged_if_needed
skip_before_action :confirm_no_pending_in_person_enrollment
skip_before_action :handle_fraud
skip_before_action :confirm_no_pending_profile

def show
analytics.idv_personal_key_visited(
Expand Down
26 changes: 3 additions & 23 deletions spec/controllers/concerns/idv_step_concern_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@ def show
end

describe 'before_actions' do
it 'includes handle_fraud' do
expect(idv_step_controller_class).to have_actions(
:before,
:handle_fraud,
)
end

it 'includes check_for_mail_only_outage before_action' do
expect(idv_step_controller_class).to have_actions(
:before,
Expand Down Expand Up @@ -176,6 +169,7 @@ def show
describe '#confirm_letter_recently_enqueued' do
controller(idv_step_controller_class) do
before_action :confirm_letter_recently_enqueued
before_action :confirm_no_pending_profile
end

before(:each) do
Expand Down Expand Up @@ -209,9 +203,9 @@ def show
end
end

describe '#confirm_no_pending_in_person_enrollment' do
describe '#confirm_no_pending_profile' do
controller(idv_step_controller_class) do
before_action :confirm_no_pending_in_person_enrollment
before_action :confirm_no_pending_profile
end

before(:each) do
Expand Down Expand Up @@ -245,20 +239,6 @@ def show
expect(response).to redirect_to idv_in_person_ready_to_verify_url
end
end
end

describe '#confirm_no_pending_gpo_profile' do
controller(idv_step_controller_class) do
before_action :confirm_no_pending_gpo_profile
end

before(:each) do
sign_in(user)
allow(subject).to receive(:current_user).and_return(user)
routes.draw do
get 'show' => 'anonymous#show'
end
end

context 'without pending gpo profile' do
it 'does not redirect' do
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/idv/personal_key_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def assert_personal_key_generated_for_profiles(*profile_pii_pairs)
:confirm_idv_needed,
:confirm_personal_key_acknowledged_if_needed,
:confirm_no_pending_in_person_enrollment,
:handle_fraud,
:confirm_no_pending_profile,
)
end

Expand Down
39 changes: 39 additions & 0 deletions spec/features/idv/verify_by_mail_pending_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require 'rails_helper'

RSpec.feature 'a user that is pending verify by mail', allowed_extra_analytics: [:*] do
include IdvStepHelper

it 'requires them to enter code or cancel to enter the proofing flow' do
user = create(:user, :fully_registered)
profile = create(:profile, :with_pii, :verify_by_mail_pending, user: user)
create(:gpo_confirmation_code, profile: profile, created_at: 2.days.ago, updated_at: 2.days.ago)

start_idv_from_sp(biometric_comparison_required: false)
sign_in_live_with_2fa(user)

expect(current_path).to eq(idv_verify_by_mail_enter_code_path)

# Attempting to start IdV should require enter-code to be completed
visit idv_welcome_path
expect(current_path).to eq(idv_verify_by_mail_enter_code_path)

# Cancelling redirects to IdV flow start
click_on t('idv.gpo.address_accordion.cta_link')
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.

This is wholly irrelevant to your PR, but just looking at this translation string I would never guess it read "Clear your information and start over". I would expect a CTA to be enticing the user to start some action, not cancel and start over.

click_idv_continue

expect(current_path).to eq(idv_welcome_path)
end

it 'does not require them to enter their code if they are upgrading to biometric' do
user = create(:user, :fully_registered)
profile = create(:profile, :with_pii, :verify_by_mail_pending, user: user)
create(:gpo_confirmation_code, profile: profile, created_at: 2.days.ago, updated_at: 2.days.ago)

start_idv_from_sp(biometric_comparison_required: true)
sign_in_live_with_2fa(user)

# The user is redirected to proofing since their pending profile does not meet
# the biometric comparison requirement
expect(current_path).to eq(idv_welcome_path)
end
end