From 8f8c8337f407b1900d5fd857abec3c070b74e8ac Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 2 Aug 2022 15:43:51 -0400 Subject: [PATCH 1/4] Collapse profile creation to single method **Why**: So that deactivation_reason can be assigned in initial creation of the profile, rather than as an immediate follow-on transaction. changelog: Internal, Optimization, Optimize identity verification profile database creation --- app/controllers/idv/review_controller.rb | 2 - app/forms/api/profile_creation_form.rb | 32 ++++++------- app/services/idv/profile_maker.rb | 4 +- app/services/idv/session.rb | 45 ++++++++++--------- .../idv/personal_key_controller_spec.rb | 16 ++----- spec/services/idv/session_spec.rb | 20 +++++---- 6 files changed, 58 insertions(+), 61 deletions(-) diff --git a/app/controllers/idv/review_controller.rb b/app/controllers/idv/review_controller.rb index 2f1c0e457cb..b98737b848d 100644 --- a/app/controllers/idv/review_controller.rb +++ b/app/controllers/idv/review_controller.rb @@ -87,8 +87,6 @@ def idv_address_complete? def init_profile idv_session.create_profile_from_applicant_with_password(password) - idv_session.cache_encrypted_pii(password) - idv_session.complete_session if idv_session.profile.active? event = create_user_event_with_disavowal(:account_verified) diff --git a/app/forms/api/profile_creation_form.rb b/app/forms/api/profile_creation_form.rb index 9e2e41b3527..27ea9459517 100644 --- a/app/forms/api/profile_creation_form.rb +++ b/app/forms/api/profile_creation_form.rb @@ -18,12 +18,7 @@ def initialize(password:, jwt:, user_session:, service_provider: nil) def submit @form_valid = valid? - - if form_valid? - create_profile - cache_encrypted_pii - complete_session - end + create_profile if form_valid? response = FormResponse.new( success: form_valid?, @@ -39,34 +34,39 @@ def submit def create_profile profile_maker = build_profile_maker - profile = profile_maker.save_profile + profile = profile_maker.save_profile(deactivation_reason: deactivation_reason) @profile = profile session[:pii] = profile_maker.pii_attributes session[:profile_id] = profile.id session[:personal_key] = profile.personal_key - end - def cache_encrypted_pii - cacher = Pii::Cacher.new(user, session) - cacher.save(password, profile) - end - - def complete_session + cache_encrypted_pii associate_in_person_enrollment_with_profile if user_bundle.gpo_address_verification? - profile.deactivate(:gpo_verification_pending) create_gpo_entry elsif phone_confirmed? if pending_in_person_enrollment? UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment(user, session[:pii]) - profile.deactivate(:in_person_verification_pending) else complete_profile end end end + def deactivation_reason + if user_bundle.gpo_address_verification? + :gpo_verification_pending + elsif phone_confirmed? && pending_in_person_enrollment? + :in_person_verification_pending + end + end + + def cache_encrypted_pii + cacher = Pii::Cacher.new(user, session) + cacher.save(password, profile) + end + def associate_in_person_enrollment_with_profile return unless pending_in_person_enrollment? && user.establishing_in_person_enrollment user.establishing_in_person_enrollment.update(profile: profile) diff --git a/app/services/idv/profile_maker.rb b/app/services/idv/profile_maker.rb index 70e780b58ce..1a07cac6ec1 100644 --- a/app/services/idv/profile_maker.rb +++ b/app/services/idv/profile_maker.rb @@ -8,8 +8,8 @@ def initialize(applicant:, user:, user_password:) self.user_password = user_password end - def save_profile - profile = Profile.new(user: user, active: false) + def save_profile(deactivation_reason: nil) + profile = Profile.new(user: user, active: false, deactivation_reason: deactivation_reason) profile.encrypt_pii(pii_attributes, user_password) profile.proofing_components = current_proofing_components profile.save! diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index c561d46a110..64f541e0022 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -49,10 +49,34 @@ def proofing_started? def create_profile_from_applicant_with_password(user_password) profile_maker = build_profile_maker(user_password) - profile = profile_maker.save_profile + profile = profile_maker.save_profile(deactivation_reason: deactivation_reason) self.pii = profile_maker.pii_attributes self.profile_id = profile.id self.personal_key = profile.personal_key + + cache_encrypted_pii(user_password) + associate_in_person_enrollment_with_profile + + if address_verification_mechanism == 'gpo' + create_gpo_entry + elsif phone_confirmed? + if pending_in_person_enrollment? + UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment( + current_user, + applicant, + ) + else + complete_profile + end + end + end + + def deactivation_reason + if address_verification_mechanism == 'gpo' + :gpo_verification_pending + elsif phone_confirmed? && pending_in_person_enrollment? + :in_person_verification_pending + end end def cache_encrypted_pii(password) @@ -81,25 +105,6 @@ def phone_confirmed? vendor_phone_confirmation == true && user_phone_confirmation == true end - def complete_session - associate_in_person_enrollment_with_profile - - if address_verification_mechanism == 'gpo' - profile.deactivate(:gpo_verification_pending) - create_gpo_entry - elsif phone_confirmed? - if pending_in_person_enrollment? - UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment( - current_user, - applicant, - ) - profile.deactivate(:in_person_verification_pending) - else - complete_profile - end - end - end - def associate_in_person_enrollment_with_profile return unless pending_in_person_enrollment? && current_user.establishing_in_person_enrollment current_user.establishing_in_person_enrollment.update(profile: profile) diff --git a/spec/controllers/idv/personal_key_controller_spec.rb b/spec/controllers/idv/personal_key_controller_spec.rb index 74b93e26943..211fa3ad67a 100644 --- a/spec/controllers/idv/personal_key_controller_spec.rb +++ b/spec/controllers/idv/personal_key_controller_spec.rb @@ -27,17 +27,7 @@ def stub_idv_session let(:password) { 'sekrit phrase' } let(:user) { create(:user, :signed_up, password: password) } - let(:applicant) do - { - first_name: 'Some', - last_name: 'One', - address1: '123 Any St', - address2: 'Ste 456', - city: 'Anywhere', - state: 'KS', - zipcode: '66666', - } - end + let(:applicant) { Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE } let(:profile) { subject.idv_session.profile } describe 'before_actions' do @@ -150,7 +140,7 @@ def index subject.idv_session.address_verification_mechanism = 'phone' subject.idv_session.vendor_phone_confirmation = true subject.idv_session.user_phone_confirmation = true - subject.idv_session.complete_session + subject.idv_session.create_profile_from_applicant_with_password(password) end it 'redirects to sign up completed for an sp' do @@ -177,7 +167,7 @@ def index context 'user selected gpo verification' do before do subject.idv_session.address_verification_mechanism = 'gpo' - subject.idv_session.complete_session + subject.idv_session.create_profile_from_applicant_with_password(password) end it 'redirects to come back later path' do diff --git a/spec/services/idv/session_spec.rb b/spec/services/idv/session_spec.rb index db0887dc64e..e0f9636d6bb 100644 --- a/spec/services/idv/session_spec.rb +++ b/spec/services/idv/session_spec.rb @@ -36,24 +36,29 @@ end end - describe '#complete_session' do + describe '#create_profile_from_applicant_with_password' do + before do + subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN + end + context 'with phone verifed by vendor' do before do subject.address_verification_mechanism = 'phone' subject.vendor_phone_confirmation = true + subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE allow(subject).to receive(:complete_profile) end it 'completes the profile if the user has completed OTP phone confirmation' do subject.user_phone_confirmation = true - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(subject).to have_received(:complete_profile) end it 'does not complete the profile if the user has not completed OTP phone confirmation' do subject.user_phone_confirmation = nil - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(subject).not_to have_received(:complete_profile) end @@ -70,11 +75,10 @@ subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE.merge( same_address_as_id: true, ).with_indifferent_access - subject.create_profile_from_applicant_with_password(user.password) end it 'sets profile to pending in person verification' do - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(subject).not_to have_received(:complete_profile) expect(subject.profile.deactivation_reason).to eq('in_person_verification_pending') @@ -85,7 +89,7 @@ to receive(:schedule_in_person_enrollment). with(user, subject.applicant.transform_keys(&:to_s)) - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(enrollment.reload.profile).to eq(user.profiles.last) end @@ -102,7 +106,7 @@ it 'sets profile to pending gpo verification' do subject.applicant = {} subject.create_profile_from_applicant_with_password(user.password) - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(subject).not_to have_received(:complete_profile) expect(subject.profile.deactivation_reason).to eq('gpo_verification_pending') @@ -117,7 +121,7 @@ it 'does not complete the user profile' do allow(subject).to receive(:complete_profile) - subject.complete_session + subject.create_profile_from_applicant_with_password(user.password) expect(subject).not_to have_received(:complete_profile) end end From 51be413e00c97579137676761b37fe63d53f9943 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 2 Aug 2022 16:48:49 -0400 Subject: [PATCH 2/4] Fix spec failure for GPO address verification 1. Applicant is now assigned in `before` block of the top-level method `describe` 2. Avoid calling create_profile_from_applicant_with_password twice --- spec/services/idv/session_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/services/idv/session_spec.rb b/spec/services/idv/session_spec.rb index e0f9636d6bb..fbbf9e0c637 100644 --- a/spec/services/idv/session_spec.rb +++ b/spec/services/idv/session_spec.rb @@ -104,8 +104,6 @@ end it 'sets profile to pending gpo verification' do - subject.applicant = {} - subject.create_profile_from_applicant_with_password(user.password) subject.create_profile_from_applicant_with_password(user.password) expect(subject).not_to have_received(:complete_profile) From 8ae135ecb89006bfb5d2cb279578f090f1ce04b8 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 5 Aug 2022 09:14:39 -0400 Subject: [PATCH 3/4] Add active as required kwarg for save_profile So activation is centralized to the ProfileMaker and callsites must make an explicit choice https://github.com/18F/identity-idp/pull/6681#issuecomment-1204368498 --- app/forms/api/profile_creation_form.rb | 26 +++++++++----------- app/services/idv/profile_maker.rb | 3 ++- app/services/idv/session.rb | 32 +++++++++++-------------- spec/services/idv/profile_maker_spec.rb | 23 +++++++++++++++++- spec/services/idv/session_spec.rb | 25 ++++++++++++------- 5 files changed, 66 insertions(+), 43 deletions(-) diff --git a/app/forms/api/profile_creation_form.rb b/app/forms/api/profile_creation_form.rb index 27ea9459517..9a378a4c485 100644 --- a/app/forms/api/profile_creation_form.rb +++ b/app/forms/api/profile_creation_form.rb @@ -34,7 +34,10 @@ def submit def create_profile profile_maker = build_profile_maker - profile = profile_maker.save_profile(deactivation_reason: deactivation_reason) + profile = profile_maker.save_profile( + active: deactivation_reason.nil?, + deactivation_reason: deactivation_reason, + ) @profile = profile session[:pii] = profile_maker.pii_attributes session[:profile_id] = profile.id @@ -43,21 +46,19 @@ def create_profile cache_encrypted_pii associate_in_person_enrollment_with_profile - if user_bundle.gpo_address_verification? + if profile.active + move_pii_to_user_session + elsif user_bundle.gpo_address_verification? create_gpo_entry - elsif phone_confirmed? - if pending_in_person_enrollment? - UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment(user, session[:pii]) - else - complete_profile - end + elsif pending_in_person_enrollment? + UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment(user, session[:pii]) end end def deactivation_reason - if user_bundle.gpo_address_verification? + if !phone_confirmed? || user_bundle.gpo_address_verification? :gpo_verification_pending - elsif phone_confirmed? && pending_in_person_enrollment? + elsif pending_in_person_enrollment? :in_person_verification_pending end end @@ -81,11 +82,6 @@ def phone_confirmed? user_bundle.vendor_phone_confirmation? && user_bundle.user_phone_confirmation? end - def complete_profile - profile.activate - move_pii_to_user_session - end - def move_pii_to_user_session return if session[:decrypted_pii].blank? user_session[:decrypted_pii] = session.delete(:decrypted_pii) diff --git a/app/services/idv/profile_maker.rb b/app/services/idv/profile_maker.rb index 1a07cac6ec1..2c9be2cc064 100644 --- a/app/services/idv/profile_maker.rb +++ b/app/services/idv/profile_maker.rb @@ -8,11 +8,12 @@ def initialize(applicant:, user:, user_password:) self.user_password = user_password end - def save_profile(deactivation_reason: nil) + def save_profile(active:, deactivation_reason: nil) profile = Profile.new(user: user, active: false, deactivation_reason: deactivation_reason) profile.encrypt_pii(pii_attributes, user_password) profile.proofing_components = current_proofing_components profile.save! + profile.activate if active profile end diff --git a/app/services/idv/session.rb b/app/services/idv/session.rb index 64f541e0022..8b619777850 100644 --- a/app/services/idv/session.rb +++ b/app/services/idv/session.rb @@ -49,7 +49,10 @@ def proofing_started? def create_profile_from_applicant_with_password(user_password) profile_maker = build_profile_maker(user_password) - profile = profile_maker.save_profile(deactivation_reason: deactivation_reason) + profile = profile_maker.save_profile( + active: deactivation_reason.nil?, + deactivation_reason: deactivation_reason, + ) self.pii = profile_maker.pii_attributes self.profile_id = profile.id self.personal_key = profile.personal_key @@ -57,24 +60,22 @@ def create_profile_from_applicant_with_password(user_password) cache_encrypted_pii(user_password) associate_in_person_enrollment_with_profile - if address_verification_mechanism == 'gpo' + if profile.active? + move_pii_to_user_session + elsif address_verification_mechanism == 'gpo' create_gpo_entry - elsif phone_confirmed? - if pending_in_person_enrollment? - UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment( - current_user, - applicant, - ) - else - complete_profile - end + elsif pending_in_person_enrollment? + UspsInPersonProofing::EnrollmentHelper.schedule_in_person_enrollment( + current_user, + applicant, + ) end end def deactivation_reason - if address_verification_mechanism == 'gpo' + if !phone_confirmed? || address_verification_mechanism == 'gpo' :gpo_verification_pending - elsif phone_confirmed? && pending_in_person_enrollment? + elsif pending_in_person_enrollment? :in_person_verification_pending end end @@ -110,11 +111,6 @@ def associate_in_person_enrollment_with_profile current_user.establishing_in_person_enrollment.update(profile: profile) end - def complete_profile - profile.activate - move_pii_to_user_session - end - def create_gpo_entry move_pii_to_user_session self.pii = Pii::Cacher.new(current_user, user_session).fetch if pii.is_a?(String) diff --git a/spec/services/idv/profile_maker_spec.rb b/spec/services/idv/profile_maker_spec.rb index f996ad8c2bc..eb9a6130418 100644 --- a/spec/services/idv/profile_maker_spec.rb +++ b/spec/services/idv/profile_maker_spec.rb @@ -16,7 +16,7 @@ it 'creates an inactive Profile with encrypted PII' do proofing_component = ProofingComponent.create(user_id: user.id, document_check: 'acuant') - profile = subject.save_profile + profile = subject.save_profile(active: false) pii = subject.pii_attributes expect(profile).to be_a Profile @@ -31,5 +31,26 @@ expect(pii.first_name).to eq 'Some' expect(profile.reproof_at).to be_nil end + + context 'with deactivation reason' do + it 'creates an inactive profile with deactivation reason' do + profile = subject.save_profile( + active: false, + deactivation_reason: :gpo_verification_pending, + ) + + expect(profile.active).to eq false + expect(profile.deactivation_reason).to eq 'gpo_verification_pending' + end + end + + context 'as active' do + it 'creates an active profile' do + profile = subject.save_profile(active: true, deactivation_reason: nil) + + expect(profile.active).to eq true + expect(profile.deactivation_reason).to be_nil + end + end end end diff --git a/spec/services/idv/session_spec.rb b/spec/services/idv/session_spec.rb index fbbf9e0c637..fb239931809 100644 --- a/spec/services/idv/session_spec.rb +++ b/spec/services/idv/session_spec.rb @@ -46,21 +46,25 @@ subject.address_verification_mechanism = 'phone' subject.vendor_phone_confirmation = true subject.applicant = Idp::Constants::MOCK_IDV_APPLICANT_WITH_PHONE - allow(subject).to receive(:complete_profile) + allow(subject).to receive(:move_pii_to_user_session) end it 'completes the profile if the user has completed OTP phone confirmation' do subject.user_phone_confirmation = true subject.create_profile_from_applicant_with_password(user.password) - expect(subject).to have_received(:complete_profile) + expect(subject).to have_received(:move_pii_to_user_session) + expect(subject.profile.active?).to eq(true) + expect(subject.profile.deactivation_reason).to be_nil end it 'does not complete the profile if the user has not completed OTP phone confirmation' do subject.user_phone_confirmation = nil subject.create_profile_from_applicant_with_password(user.password) - expect(subject).not_to have_received(:complete_profile) + expect(subject).not_to have_received(:move_pii_to_user_session) + expect(subject.profile.active?).to eq(false) + expect(subject.profile.deactivation_reason).to eq('gpo_verification_pending') end context 'with establishing in person enrollment' do @@ -80,7 +84,8 @@ it 'sets profile to pending in person verification' do subject.create_profile_from_applicant_with_password(user.password) - expect(subject).not_to have_received(:complete_profile) + expect(subject).not_to have_received(:move_pii_to_user_session) + expect(subject.profile.active?).to eq(false) expect(subject.profile.deactivation_reason).to eq('in_person_verification_pending') end @@ -100,13 +105,14 @@ before do subject.address_verification_mechanism = 'gpo' subject.vendor_phone_confirmation = false - allow(subject).to receive(:complete_profile) + allow(subject).to receive(:move_pii_to_user_session) end it 'sets profile to pending gpo verification' do subject.create_profile_from_applicant_with_password(user.password) - expect(subject).not_to have_received(:complete_profile) + expect(subject).to have_received(:move_pii_to_user_session) + expect(subject.profile.active?).to eq(false) expect(subject.profile.deactivation_reason).to eq('gpo_verification_pending') end end @@ -115,12 +121,15 @@ before do subject.address_verification_mechanism = 'phone' subject.vendor_phone_confirmation = false + allow(subject).to receive(:move_pii_to_user_session) end it 'does not complete the user profile' do - allow(subject).to receive(:complete_profile) subject.create_profile_from_applicant_with_password(user.password) - expect(subject).not_to have_received(:complete_profile) + + expect(subject).not_to have_received(:move_pii_to_user_session) + expect(subject.profile.active?).to eq(false) + expect(subject.profile.deactivation_reason).to eq('gpo_verification_pending') end end end From 308c4524791efe73434d9010942d67c466a93d0a Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 5 Aug 2022 13:57:17 -0400 Subject: [PATCH 4/4] Add required kwarg to PersonalKeyController stub --- spec/controllers/idv/personal_key_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/idv/personal_key_controller_spec.rb b/spec/controllers/idv/personal_key_controller_spec.rb index 211fa3ad67a..150f924c7b7 100644 --- a/spec/controllers/idv/personal_key_controller_spec.rb +++ b/spec/controllers/idv/personal_key_controller_spec.rb @@ -18,7 +18,7 @@ def stub_idv_session user: user, user_password: password, ) - profile = profile_maker.save_profile + profile = profile_maker.save_profile(active: false) idv_session.pii = profile_maker.pii_attributes idv_session.profile_id = profile.id idv_session.personal_key = profile.personal_key