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
5 changes: 5 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ def has_in_person_enrollment?
pending_in_person_enrollment.present? || establishing_in_person_enrollment.present?
end

# @return [Boolean] Whether the user has an establishing in person enrollment.
def has_establishing_in_person_enrollment?
establishing_in_person_enrollment.present?
end

# Trust `pending_profile` rather than enrollment associations
def has_establishing_in_person_enrollment_safe?
!!pending_profile&.in_person_enrollment&.establishing?
Expand Down
20 changes: 13 additions & 7 deletions app/services/idv/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ def respond_to_missing?(method_sym, include_private)
end

def create_profile_from_applicant_with_password(user_password, is_enhanced_ipp)
if user_has_unscheduled_in_person_enrollment?
UspsInPersonProofing::EnrollmentHelper.schedule_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.

Unimportant nit: I think these two lines read kind of confusingly: "If the user already as an in-person enrollment, schedule an in-person enrollment for them." I'm not sure if there's an easy way to fix this with a rename, though?

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.

I also find this confusing. Maybe we could rename has_in_person_enrollment? to has_unscheduled_in_person_enrollment?

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 agree with that the naming could be better... my only issue is that the has_in_person_enrollment? checks for pending or establishing in-person enrollments. Unscheduled should only care about establishing in-person enrollments right?

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.

Would needs_in_person_verification? be a clearer method name? I got that idea from the in_person_verification_needed param for profile_maker’s save_profile method.

It could also be renamed has_begun_in_person_proofing?, but I worry that’s too vague. Technically, a user who has a failed in_person_enrollment has also begun in person proofing.

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.

Unscheduled should only care about establishing in-person enrollments right?

I have a n00b question. If unscheduled only refers to establishing in_person_enrollments, why schedule an enrollment when a user has a pending or establishing in_person_enrollment? Could this conditional be refactored to only check if the user has an establishing in_person_enrollment?

The enrollment helper’s schedule_in_person_enrollment returns if the user doesn’t have an establishing in_person_enrollment, which makes me think that this refactor wouldn’t actually change the method’s behavior.

Let me know if I’m missing something.

user: current_user,
pii: Pii::Attributes.new_from_hash(applicant),
is_enhanced_ipp: is_enhanced_ipp,
opt_in: opt_in_param,
)
end

profile_maker = build_profile_maker(user_password)
profile = profile_maker.save_profile(
fraud_pending_reason: threatmetrix_fraud_pending_reason,
Expand All @@ -85,13 +94,6 @@ def create_profile_from_applicant_with_password(user_password, is_enhanced_ipp)

if profile.gpo_verification_pending?
create_gpo_entry(profile_maker.pii_attributes, profile)
elsif profile.in_person_verification_pending?
UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment(
user: current_user,
pii: profile_maker.pii_attributes,
is_enhanced_ipp: is_enhanced_ipp,
opt_in: opt_in_param,
)
end
end

Expand Down Expand Up @@ -323,5 +325,9 @@ def threatmetrix_fraud_pending_reason
'threatmetrix_review'
end
end

def user_has_unscheduled_in_person_enrollment?
current_user.has_establishing_in_person_enrollment?
end
end
end
27 changes: 27 additions & 0 deletions spec/features/idv/in_person_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
include IdvStepHelper
include SpAuthHelper
include InPersonHelper
include UspsIppHelper

before do
allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true)
Expand Down Expand Up @@ -494,4 +495,30 @@
expect(page).to have_current_path(idv_in_person_ready_to_verify_path)
end
end

context 'when the USPS enrollment fails during enter password' do
before do
user = user_with_2fa
sign_in_and_2fa_user(user)
begin_in_person_proofing(user)
complete_prepare_step(user)
complete_location_step
# Causes the schedule USPS enrollment request to throw a bad request error
complete_state_id_step(user, first_name: 'usps client error')
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.

Idle musing: it's easy to miss what this is doing, and instead just skim this and see it as doing normal steps, in which case it's unclear why an error would be expected. We had experimented a bit with descriptively-named helper methods to make some of this clearer, though I don't know where that ended up. I wonder if it would make sense to put this in a method like complete_up_to_enter_password_step_with_usps_client_error and just call that in the before here? (It's totally possible this is a bad idea. Just mentioning it in case it resonates.)

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.

Yeah I feel like it is unclear as well... I think this is because I was working around the fact that we are using the mock proofer in the tests. Maybe I should change the tests to use the real proofer, and mock the http endpoint. That would make the test setup have an explicit step for mocking the OptInIPPApplicant endpoint to return a 400 response.

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.

Actually this is turning out to be a bigger fish than I want to fry right now... I ended up adding a comment above that line that states why the first_name is set to usps client error. I think it would be nice to improve these feature tests at a later date.

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.

I wonder if it would make sense to put this in a method like complete_up_to_enter_password_step_with_usps_client_error and just call that in the before here?

I get wanting to make the test easier to follow. In the spirit of write everything twice, I wanted to point out that making a helper method which isn't used much could be a premature optimization.

complete_ssn_step(user)
complete_verify_step(user)
fill_out_phone_form_ok(MfaContext.new(user).phone_configurations.first.phone)
click_idv_send_security_code
fill_in_code_with_last_phone_otp
click_submit_default
complete_enter_password_step(user)
end

it 'then an error displayed on the enter password page', allow_browser_log: true do
expect_in_person_step_indicator_current_step(t('step_indicator.flows.idv.re_enter_password'))
expect(page).to have_content(
'There was an internal error processing your request. Please try again.',
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.

Thanks for adding test coverage for this! 👏🏻

)
end
end
end
18 changes: 18 additions & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,24 @@
end
end

describe '#has_establishing_in_person_enrollment?' do
context 'when the user has an establishing in person enrollment' do
before do
create(:in_person_enrollment, :establishing, user: subject)
end

it 'returns true' do
expect(subject.has_establishing_in_person_enrollment?).to be(true)
end
end

context 'when the user does not have an establishing in person enrollment' do
it 'returns false' do
expect(subject.has_establishing_in_person_enrollment?).to be(false)
end
end
end

describe 'deleting identities' do
it 'does not delete identities when the user is destroyed preventing uuid reuse' do
user = create(:user, :fully_registered)
Expand Down
120 changes: 89 additions & 31 deletions spec/services/idv/session_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'rails_helper'

RSpec.describe Idv::Session, allowed_extra_analytics: [:*] do
RSpec.describe Idv::Session do
let(:user) { create(:user) }
let(:user_session) { {} }
let(:is_enhanced_ipp) { false }
Expand Down Expand Up @@ -129,7 +129,7 @@

describe '#create_profile_from_applicant_with_password' do
let(:opt_in_param) { nil }
let(:is_enhanced_ipp) { false }

before do
subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN
end
Expand Down Expand Up @@ -181,10 +181,11 @@
expect(pii_from_session.ssn).to eq(Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn])
end

context 'with establishing in person enrollment' do
context 'when the user has an establishing in person enrollment' do
let!(:enrollment) do
create(:in_person_enrollment, :establishing, user: user, profile: nil)
end
let(:profile) { subject.profile }

before do
ProofingComponent.create(user: user, document_check: Idp::Constants::Vendors::USPS)
Expand All @@ -193,41 +194,98 @@
subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE.with_indifferent_access
end

it 'sets profile to pending in person verification' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
profile = subject.profile
context 'when the USPS enrollment is successful' do
before do
allow(UspsInPersonProofing::EnrollmentHelper).to receive(:schedule_in_person_enrollment)
end

it 'creates an USPS enrollment' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(UspsInPersonProofing::EnrollmentHelper).to have_received(
:schedule_in_person_enrollment,
).with(
user: user,
pii: Pii::Attributes.new_from_hash(subject.applicant),
is_enhanced_ipp: is_enhanced_ipp,
opt_in: opt_in_param,
)
end

it 'creates a profile with in person verification pending' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(profile).to have_attributes(
{
activated_at: nil,
active: false,
in_person_verification_pending?: true,
fraud_review_pending?: false,
gpo_verification_pending_at: nil,
initiating_service_provider: nil,
verified_at: nil,
},
)
end

it 'saves the pii to the session' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(Pii::Cacher.new(user, user_session).fetch(profile.id)).to_not be_nil
end

it 'associates the in person enrollment with the created profile' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(enrollment.reload.profile_id).to eq(profile.id)
end
end

expect(profile.activated_at).to eq nil
expect(profile.active).to eq false
expect(profile.in_person_verification_pending?).to eq(true)
expect(profile.fraud_review_pending?).to eq(false)
expect(profile.gpo_verification_pending_at.present?).to eq false
expect(profile.initiating_service_provider).to eq nil
expect(profile.verified_at).to eq nil
context 'when the USPS enrollment throws an enroll exception' do
before do
allow(UspsInPersonProofing::EnrollmentHelper).to receive(
:schedule_in_person_enrollment,
).and_throw('Enrollment Failure')
end

it 'does not create a profile' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
rescue
expect(profile).to be_nil
end
end
end

pii_from_session = Pii::Cacher.new(user, user_session).fetch(profile.id)
expect(pii_from_session).to_not be_nil
expect(pii_from_session.ssn).to eq(Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE[:ssn])
context 'when the user does not have an establishing in person enrollment' do
let(:profile) { subject.profile }

before do
allow(UspsInPersonProofing::EnrollmentHelper).to receive(:schedule_in_person_enrollment)
ProofingComponent.create(user: user, document_check: Idp::Constants::Vendors::USPS)
allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true)
subject.user_phone_confirmation = true
subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE.with_indifferent_access
end

it 'creates a USPS enrollment' do
expect(UspsInPersonProofing::EnrollmentHelper).
to receive(:schedule_in_person_enrollment).
with(user: user, pii: Pii::Attributes.new_from_hash(subject.applicant),
is_enhanced_ipp: is_enhanced_ipp,
opt_in: opt_in_param)
it 'does not create an USPS enrollment' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(UspsInPersonProofing::EnrollmentHelper).to_not have_received(
:schedule_in_person_enrollment,
)
end

it 'creates a profile' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(profile).to have_attributes(
{
active: true,
in_person_verification_pending?: false,
fraud_review_pending?: false,
gpo_verification_pending_at: nil,
initiating_service_provider: nil,
},
)
end

profile = enrollment.reload.profile
expect(profile).to eq(user.profiles.last)
expect(profile.activated_at).to eq nil
expect(profile.active).to eq false
expect(profile.in_person_verification_pending?).to eq(true)
expect(profile.fraud_review_pending?).to eq(false)
expect(profile.gpo_verification_pending_at.present?).to eq false
expect(profile.initiating_service_provider).to eq nil
expect(profile.verified_at).to eq nil
it 'saves the pii to the session' do
subject.create_profile_from_applicant_with_password(user.password, is_enhanced_ipp)
expect(Pii::Cacher.new(user, user_session).fetch(profile.id)).to_not be_nil
end
end
end
Expand Down
8 changes: 4 additions & 4 deletions spec/support/features/in_person_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ module InPersonHelper
GOOD_IDENTITY_DOC_ZIPCODE =
(Idp::Constants::MOCK_IDV_APPLICANT_STATE_ID_ADDRESS[:identity_doc_zipcode]).freeze

def fill_out_state_id_form_ok(same_address_as_id: false)
fill_in t('in_person_proofing.form.state_id.first_name'), with: GOOD_FIRST_NAME
def fill_out_state_id_form_ok(same_address_as_id: false, first_name: GOOD_FIRST_NAME)
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.

👍🏻

fill_in t('in_person_proofing.form.state_id.first_name'), with: first_name
fill_in t('in_person_proofing.form.state_id.last_name'), with: GOOD_LAST_NAME
year, month, day = GOOD_DOB.split('-')
fill_in t('components.memorable_date.month'), with: month
Expand Down Expand Up @@ -120,10 +120,10 @@ def complete_prepare_step(_user = nil)
click_on t('forms.buttons.continue')
end

def complete_state_id_step(_user = nil, same_address_as_id: true)
def complete_state_id_step(_user = nil, same_address_as_id: true, first_name: GOOD_FIRST_NAME)
# Wait for page to load before attempting to fill out form
expect(page).to have_current_path(idv_in_person_step_path(step: :state_id), wait: 10)
fill_out_state_id_form_ok(same_address_as_id: same_address_as_id)
fill_out_state_id_form_ok(same_address_as_id: same_address_as_id, first_name:)
click_idv_continue
unless same_address_as_id
expect(page).to have_current_path(idv_in_person_address_path, wait: 10)
Expand Down