From 995250d7c1fa8e7dbceb61dd108617f98a3a153e Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Mon, 11 Sep 2023 13:23:01 -0400 Subject: [PATCH 01/11] Remove the `aws_kms_multi_region_read_enabled` flag (#9187) The `aws_kms_multi_region_read_enabled` was in place to toggle the changes introduced in #9047. Those changes have been deployed, enabled, and have been running without issue for some time. This commit removes the tooling to toggle the feature since we will be using it going forward. [skip changelog] --- .../encryption/regional_ciphertext_pair.rb | 6 +- config/application.yml.default | 1 - lib/identity_config.rb | 1 - .../users/sessions_controller_spec.rb | 3 + spec/features/legacy_passwords_spec.rb | 4 + .../profile_migration_job_spec.rb | 4 - .../user_migration_job_spec.rb | 4 - spec/models/profile_spec.rb | 79 ++++---------- spec/models/user_spec.rb | 88 ++++++--------- .../encryptors/pii_encryptor_spec.rb | 70 +++--------- .../profile_migrator_spec.rb | 4 - .../user_migrator_spec.rb | 4 - .../encryption/password_verifier_spec.rb | 102 ++++-------------- 13 files changed, 96 insertions(+), 274 deletions(-) diff --git a/app/services/encryption/regional_ciphertext_pair.rb b/app/services/encryption/regional_ciphertext_pair.rb index 73646f14e79..1c5ab32b013 100644 --- a/app/services/encryption/regional_ciphertext_pair.rb +++ b/app/services/encryption/regional_ciphertext_pair.rb @@ -6,10 +6,6 @@ def to_ary end def multi_or_single_region_ciphertext - if IdentityConfig.store.aws_kms_multi_region_read_enabled - multi_region_ciphertext.presence || single_region_ciphertext - else - single_region_ciphertext - end + multi_region_ciphertext.presence || single_region_ciphertext end end diff --git a/config/application.yml.default b/config/application.yml.default index d95b9ab29a1..63d9d0941d4 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -51,7 +51,6 @@ aws_kms_key_id: alias/login-dot-gov-test-keymaker aws_kms_client_contextless_pool_size: 5 aws_kms_client_multi_pool_size: 5 aws_kms_multi_region_key_id: alias/login-dot-gov-keymaker-multi-region -aws_kms_multi_region_read_enabled: false aws_kms_session_key_id: alias/login-dot-gov-test-keymaker aws_logo_bucket: '' aws_region: 'us-west-2' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 79645c6d376..ab4bfce0cc9 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -138,7 +138,6 @@ def self.build_store(config_map) config.add(:aws_kms_client_multi_pool_size, type: :integer) config.add(:aws_kms_key_id, type: :string) config.add(:aws_kms_multi_region_key_id, type: :string) - config.add(:aws_kms_multi_region_read_enabled, type: :boolean) config.add(:aws_kms_session_key_id, type: :string) config.add(:aws_logo_bucket, type: :string) config.add(:aws_region, type: :string) diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 13fed2ef90d..21dc0bff8fe 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -238,6 +238,9 @@ profile = create(:profile, :active, :verified, user: user, pii: { ssn: '1234' }) profile.update!( encrypted_pii: { encrypted_data: Base64.strict_encode64('nonsense') }.to_json, + encrypted_pii_multi_region: { + encrypted_data: Base64.strict_encode64('nonsense'), + }.to_json, ) stub_analytics diff --git a/spec/features/legacy_passwords_spec.rb b/spec/features/legacy_passwords_spec.rb index 7396123944e..44751d51310 100644 --- a/spec/features/legacy_passwords_spec.rb +++ b/spec/features/legacy_passwords_spec.rb @@ -5,6 +5,7 @@ user = create(:user, :fully_registered) user.update!( encrypted_password_digest: Encryption::UakPasswordVerifier.digest('legacy password'), + encrypted_password_digest_multi_region: nil, ) expect( @@ -26,6 +27,7 @@ user = create(:user, :fully_registered) user.update!( encrypted_password_digest: Encryption::UakPasswordVerifier.digest('legacy password'), + encrypted_password_digest_multi_region: nil, ) signin(user.email, 'a different password') @@ -40,6 +42,7 @@ user = create(:user, :fully_registered) user.update!( encrypted_recovery_code_digest: Encryption::UakPasswordVerifier.digest('1111 2222 3333 4444'), + encrypted_recovery_code_digest_multi_region: nil, ) expect( @@ -59,6 +62,7 @@ user = create(:user, :fully_registered) user.update!( encrypted_recovery_code_digest: Encryption::UakPasswordVerifier.digest('1111 2222 3333 4444'), + encrypted_recovery_code_digest_multi_region: nil, ) sign_in_user(user) diff --git a/spec/jobs/multi_region_kms_migration/profile_migration_job_spec.rb b/spec/jobs/multi_region_kms_migration/profile_migration_job_spec.rb index 04bf863ea33..aa7880b5c0b 100644 --- a/spec/jobs/multi_region_kms_migration/profile_migration_job_spec.rb +++ b/spec/jobs/multi_region_kms_migration/profile_migration_job_spec.rb @@ -1,10 +1,6 @@ require 'rails_helper' RSpec.describe MultiRegionKmsMigration::ProfileMigrationJob do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - let!(:profiles) { create_list(:profile, 4, :with_pii) } let!(:single_region_ciphertext_profiles) do single_region_profiles = profiles[2..3] diff --git a/spec/jobs/multi_region_kms_migration/user_migration_job_spec.rb b/spec/jobs/multi_region_kms_migration/user_migration_job_spec.rb index cfc917cbbae..90e831c86c5 100644 --- a/spec/jobs/multi_region_kms_migration/user_migration_job_spec.rb +++ b/spec/jobs/multi_region_kms_migration/user_migration_job_spec.rb @@ -1,10 +1,6 @@ require 'rails_helper' RSpec.describe MultiRegionKmsMigration::UserMigrationJob do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - let!(:multi_region_password_digest_user) { create(:user, password: 'salty pickles') } let!(:single_region_password_digest_user) do user = create(:user, password: 'salty_pickles') diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 1b386a80696..8adea5833d9 100644 --- a/spec/models/profile_spec.rb +++ b/spec/models/profile_spec.rb @@ -195,91 +195,58 @@ end describe '#decrypt_pii' do - it 'decrypts PII' do - expect(profile.encrypted_pii).to be_nil - + it 'decrypts the PII for users with a multi region ciphertext' do profile.encrypt_pii(pii, user.password) + expect(profile.encrypted_pii_multi_region).to_not be_nil + decrypted_pii = profile.decrypt_pii(user.password) expect(decrypted_pii).to eq pii end - it 'fails if the encryption context from the uuid is incorrect' do + it 'decrypts the PII for users with only a single region ciphertext' do profile.encrypt_pii(pii, user.password) + profile.update!(encrypted_pii_multi_region: nil) - allow(profile.user).to receive(:uuid).and_return('a-different-uuid') + decrypted_pii = profile.decrypt_pii(user.password) - expect { profile.decrypt_pii(user.password) }.to raise_error(Encryption::EncryptionError) + expect(decrypted_pii).to eq pii end - context 'with aws_kms_multi_region_read_enabled enabled' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'decrypts the PII for users with a multi region ciphertext' do - profile.encrypt_pii(pii, user.password) - - expect(profile.encrypted_pii_multi_region).to_not be_nil - - decrypted_pii = profile.decrypt_pii(user.password) - - expect(decrypted_pii).to eq pii - end - - it 'decrypts the PII for users with only a single region ciphertext' do - profile.encrypt_pii(pii, user.password) - profile.update!(encrypted_pii_multi_region: nil) + it 'fails if the encryption context from the uuid is incorrect' do + profile.encrypt_pii(pii, user.password) - decrypted_pii = profile.decrypt_pii(user.password) + allow(profile.user).to receive(:uuid).and_return('a-different-uuid') - expect(decrypted_pii).to eq pii - end + expect { profile.decrypt_pii(user.password) }.to raise_error(Encryption::EncryptionError) end end describe '#recover_pii' do - it 'decrypts the encrypted_pii_recovery using a personal key' do - expect(profile.encrypted_pii_recovery).to be_nil - + it 'decrypts recovery PII with personal key for users with a multi region ciphertext' do profile.encrypt_pii(pii, user.password) personal_key = profile.personal_key normalized_personal_key = PersonalKeyGenerator.new(user).normalize(personal_key) - expect(profile.recover_pii(normalized_personal_key)).to eq pii - end - - context 'with aws_kms_multi_region_read_enabled enabled' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'decrypts the PII for users with a multi region ciphertext' do - profile.encrypt_pii(pii, user.password) - personal_key = profile.personal_key - - normalized_personal_key = PersonalKeyGenerator.new(user).normalize(personal_key) + expect(profile.encrypted_pii_recovery_multi_region).to_not be_nil - expect(profile.encrypted_pii_recovery_multi_region).to_not be_nil + decrypted_pii = profile.recover_pii(normalized_personal_key) - decrypted_pii = profile.recover_pii(normalized_personal_key) - - expect(decrypted_pii).to eq pii - end + expect(decrypted_pii).to eq pii + end - it 'decrypts the PII for users with only a single region ciphertext' do - profile.encrypt_pii(pii, user.password) - profile.update!(encrypted_pii_recovery_multi_region: nil) - personal_key = profile.personal_key + it 'decrypts recovery PII with personal key for users with only a single region ciphertext' do + profile.encrypt_pii(pii, user.password) + profile.update!(encrypted_pii_recovery_multi_region: nil) + personal_key = profile.personal_key - normalized_personal_key = PersonalKeyGenerator.new(user).normalize(personal_key) + normalized_personal_key = PersonalKeyGenerator.new(user).normalize(personal_key) - decrypted_pii = profile.recover_pii(normalized_personal_key) + decrypted_pii = profile.recover_pii(normalized_personal_key) - expect(decrypted_pii).to eq pii - end + expect(decrypted_pii).to eq pii end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 88a125c61f2..0a211ae5066 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -432,43 +432,30 @@ end describe '#valid_password?' do - it 'returns true if the password matches the stored digest' do + it 'validates the password for a user with a multi-region digest' do user = build(:user, password: 'test password') + expect(user.encrypted_password_digest_multi_region).to_not be_nil + expect(user.valid_password?('test password')).to eq(true) expect(user.valid_password?('wrong password')).to eq(false) end - context 'aws_kms_multi_region_read_enabled is set to true' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'validates the password for a user with a multi-region digest' do - user = build(:user, password: 'test password') - - expect(user.encrypted_password_digest_multi_region).to_not be_nil - - expect(user.valid_password?('test password')).to eq(true) - expect(user.valid_password?('wrong password')).to eq(false) - end - - it 'validates the password for a user with a only a single-region digest' do - user = build(:user, password: 'test password') - user.encrypted_password_digest_multi_region = nil + it 'validates the password for a user with a only a single-region digest' do + user = build(:user, password: 'test password') + user.encrypted_password_digest_multi_region = nil - expect(user.valid_password?('test password')).to eq(true) - expect(user.valid_password?('wrong password')).to eq(false) - end + expect(user.valid_password?('test password')).to eq(true) + expect(user.valid_password?('wrong password')).to eq(false) + end - it 'validates the password for a user with a only a single-region UAK digest' do - user = build(:user) - user.encrypted_password_digest = Encryption::UakPasswordVerifier.digest('test password') - user.encrypted_password_digest_multi_region = nil + it 'validates the password for a user with a only a single-region UAK digest' do + user = build(:user) + user.encrypted_password_digest = Encryption::UakPasswordVerifier.digest('test password') + user.encrypted_password_digest_multi_region = nil - expect(user.valid_password?('test password')).to eq(true) - expect(user.valid_password?('wrong password')).to eq(false) - end + expect(user.valid_password?('test password')).to eq(true) + expect(user.valid_password?('wrong password')).to eq(false) end end @@ -493,44 +480,31 @@ end describe '#valid_personal_key?' do - it 'returns true if the personal key matches the stored digest' do + it 'validates the personal key for a user with a multi-region digest' do user = build(:user, personal_key: 'test personal key') + expect(user.encrypted_recovery_code_digest_multi_region).to_not be_nil + expect(user.valid_personal_key?('test personal key')).to eq(true) expect(user.valid_personal_key?('wrong personal key')).to eq(false) end - context 'aws_kms_multi_region_read_enabled is set to true' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'validates the personal key for a user with a multi-region digest' do - user = build(:user, personal_key: 'test personal key') - - expect(user.encrypted_recovery_code_digest_multi_region).to_not be_nil - - expect(user.valid_personal_key?('test personal key')).to eq(true) - expect(user.valid_personal_key?('wrong personal key')).to eq(false) - end - - it 'validates the personal key for a user with a only a single-region digest' do - user = build(:user, personal_key: 'test personal key') - user.encrypted_recovery_code_digest_multi_region = nil + it 'validates the personal key for a user with a only a single-region digest' do + user = build(:user, personal_key: 'test personal key') + user.encrypted_recovery_code_digest_multi_region = nil - expect(user.valid_personal_key?('test personal key')).to eq(true) - expect(user.valid_personal_key?('wrong personal key')).to eq(false) - end + expect(user.valid_personal_key?('test personal key')).to eq(true) + expect(user.valid_personal_key?('wrong personal key')).to eq(false) + end - it 'validates the personal key for a user with a only a single-region UAK digest' do - user = build(:user) - user.encrypted_recovery_code_digest = - Encryption::UakPasswordVerifier.digest('test personal key') - user.encrypted_recovery_code_digest_multi_region = nil + it 'validates the personal key for a user with a only a single-region UAK digest' do + user = build(:user) + user.encrypted_recovery_code_digest = + Encryption::UakPasswordVerifier.digest('test personal key') + user.encrypted_recovery_code_digest_multi_region = nil - expect(user.valid_personal_key?('test personal key')).to eq(true) - expect(user.valid_personal_key?('wrong personal key')).to eq(false) - end + expect(user.valid_personal_key?('test personal key')).to eq(true) + expect(user.valid_personal_key?('wrong personal key')).to eq(false) end end diff --git a/spec/services/encryption/encryptors/pii_encryptor_spec.rb b/spec/services/encryption/encryptors/pii_encryptor_spec.rb index c2f442b1465..164ad30f92a 100644 --- a/spec/services/encryption/encryptors/pii_encryptor_spec.rb +++ b/spec/services/encryption/encryptors/pii_encryptor_spec.rb @@ -110,7 +110,7 @@ kms_client = subject.send(:multi_region_kms_client) expect(kms_client).to receive(:decrypt). - with('kms_ciphertext', { 'context' => 'pii-encryption', 'user_uuid' => 'uuid-123-abc' }). + with('kms_ciphertext_mr', { 'context' => 'pii-encryption', 'user_uuid' => 'uuid-123-abc' }). and_return('aes_ciphertext') cipher = subject.send(:aes_cipher) @@ -120,11 +120,15 @@ ciphertext_pair = Encryption::RegionalCiphertextPair.new( single_region_ciphertext: { - encrypted_data: Base64.strict_encode64('kms_ciphertext'), + encrypted_data: Base64.strict_encode64('kms_ciphertext_sr'), + salt: salt, + cost: '800$8$1$', + }.to_json, + multi_region_ciphertext: { + encrypted_data: Base64.strict_encode64('kms_ciphertext_mr'), salt: salt, cost: '800$8$1$', }.to_json, - multi_region_ciphertext: nil, ) result = subject.decrypt(ciphertext_pair, user_uuid: 'uuid-123-abc') @@ -132,59 +136,17 @@ expect(result).to eq(plaintext) end - context 'aws_kms_multi_region_read_enabled is set to true' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'uses the multi-region ciphertext if it is available' do - test_ciphertext_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: subject.encrypt( - 'single-region-text', user_uuid: '123abc' - ).single_region_ciphertext, - multi_region_ciphertext: subject.encrypt( - 'multi-region-text', user_uuid: '123abc' - ).multi_region_ciphertext, - ) - - result = subject.decrypt(test_ciphertext_pair, user_uuid: '123abc') - - expect(result).to eq('multi-region-text') - end - - it 'uses the single region ciphertext if the multi-region ciphertext is nil' do - test_ciphertext_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: subject.encrypt( - 'single-region-text', user_uuid: '123abc' - ).single_region_ciphertext, - multi_region_ciphertext: nil, - ) - - result = subject.decrypt(test_ciphertext_pair, user_uuid: '123abc') - - expect(result).to eq('single-region-text') - end - end - - context 'aws_kms_multi_region_read_enabled is set to false' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(false) - end - - it 'uses the single-region ciphertext to decrypt' do - test_ciphertext_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: subject.encrypt( - 'single-region-text', user_uuid: '123abc' - ).single_region_ciphertext, - multi_region_ciphertext: subject.encrypt( - 'multi-region-text', user_uuid: '123abc' - ).multi_region_ciphertext, - ) + it 'uses the single region ciphertext if the multi-region ciphertext is nil' do + test_ciphertext_pair = Encryption::RegionalCiphertextPair.new( + single_region_ciphertext: subject.encrypt( + 'single-region-text', user_uuid: '123abc' + ).single_region_ciphertext, + multi_region_ciphertext: nil, + ) - result = subject.decrypt(test_ciphertext_pair, user_uuid: '123abc') + result = subject.decrypt(test_ciphertext_pair, user_uuid: '123abc') - expect(result).to eq('single-region-text') - end + expect(result).to eq('single-region-text') end end end diff --git a/spec/services/encryption/multi_region_kms_migration/profile_migrator_spec.rb b/spec/services/encryption/multi_region_kms_migration/profile_migrator_spec.rb index 068c702cbbf..bd1aca5932a 100644 --- a/spec/services/encryption/multi_region_kms_migration/profile_migrator_spec.rb +++ b/spec/services/encryption/multi_region_kms_migration/profile_migrator_spec.rb @@ -7,10 +7,6 @@ subject { described_class.new(profile) } - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - describe '#migrate!' do context 'for a user without multi-region ciphertexts' do before do diff --git a/spec/services/encryption/multi_region_kms_migration/user_migrator_spec.rb b/spec/services/encryption/multi_region_kms_migration/user_migrator_spec.rb index 9a6256aaf96..f7840e468a2 100644 --- a/spec/services/encryption/multi_region_kms_migration/user_migrator_spec.rb +++ b/spec/services/encryption/multi_region_kms_migration/user_migrator_spec.rb @@ -1,10 +1,6 @@ require 'rails_helper' RSpec.describe Encryption::MultiRegionKmsMigration::UserMigrator do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - let(:user) { create(:user, password: 'salty pickles', personal_key: '1234-ABCD') } subject { described_class.new(user) } diff --git a/spec/services/encryption/password_verifier_spec.rb b/spec/services/encryption/password_verifier_spec.rb index 14715ef8fed..86c8ed14c77 100644 --- a/spec/services/encryption/password_verifier_spec.rb +++ b/spec/services/encryption/password_verifier_spec.rb @@ -130,92 +130,26 @@ expect(bad_match_result).to eq(false) end - context 'aws_kms_multi_region_read_enabled is set to true' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(true) - end - - it 'uses the multi-region digest if it is available' do - test_digest_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: subject.create_digest_pair( - password: 'single-region-password', - user_uuid: user_uuid, - ).single_region_ciphertext, - multi_region_ciphertext: subject.create_digest_pair( - password: 'multi-region-password', - user_uuid: user_uuid, - ).multi_region_ciphertext, - ) - - single_region_result = subject.verify( - password: 'single-region-password', - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - multi_region_result = subject.verify( - password: 'multi-region-password', - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - - expect(single_region_result).to eq(false) - expect(multi_region_result).to eq(true) - end - - it 'uses the single region digest if the multi-region digest is nil' do - test_digest_pair = subject.create_digest_pair( - password: password, - user_uuid: user_uuid, - ) - test_digest_pair.multi_region_ciphertext = nil - - correct_password_result = subject.verify( - password: password, - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - incorrect_password_result = subject.verify( - password: 'this is a fake password lol', - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - - expect(correct_password_result).to eq(true) - expect(incorrect_password_result).to eq(false) - end - end + it 'uses the single region digest if the multi-region digest is nil' do + test_digest_pair = subject.create_digest_pair( + password: password, + user_uuid: user_uuid, + ) + test_digest_pair.multi_region_ciphertext = nil - context 'aws_kms_multi_region_read_enabled is set to false' do - before do - allow(IdentityConfig.store).to receive(:aws_kms_multi_region_read_enabled).and_return(false) - end + correct_password_result = subject.verify( + password: password, + digest_pair: test_digest_pair, + user_uuid: user_uuid, + ) + incorrect_password_result = subject.verify( + password: 'this is a fake password lol', + digest_pair: test_digest_pair, + user_uuid: user_uuid, + ) - it 'uses the single-region digest to verify' do - test_digest_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: subject.create_digest_pair( - password: 'single-region-password', - user_uuid: user_uuid, - ).single_region_ciphertext, - multi_region_ciphertext: subject.create_digest_pair( - password: 'multi-region-password', - user_uuid: user_uuid, - ).multi_region_ciphertext, - ) - - single_region_result = subject.verify( - password: 'single-region-password', - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - multi_region_result = subject.verify( - password: 'multi-region-password', - digest_pair: test_digest_pair, - user_uuid: user_uuid, - ) - - expect(single_region_result).to eq(true) - expect(multi_region_result).to eq(false) - end + expect(correct_password_result).to eq(true) + expect(incorrect_password_result).to eq(false) end end From c3d83a733c4029f7b794a01b74790d2d05980b50 Mon Sep 17 00:00:00 2001 From: eileen-nava <80347702+eileen-nava@users.noreply.github.com> Date: Mon, 11 Sep 2023 13:42:02 -0400 Subject: [PATCH 02/11] LG-10538: update help center links in app workflow (#9143) * remove troubleshooting component from "add photos of your id/check your images & try again" * update link copy for We could not verify your ID page * display help center link on review images page when doc is not a failedDocType * changelog:User-Facing Improvements, Help Center Updates, update help center links in app * make tests consistent with changes in help center links * add spacing and use getHelpCenterURL * fix spacing to occur before help center link * fix spacing after body paragraph on review issues pg * attempt to use getHelpCenterURL in upload.ts * refactor to display help center link from UnknownError * add tests for TipList and UnknownError * change help center link to open in new tab * update spec to reflect link change --- .../document-capture-review-issues.tsx | 7 +- .../components/document-capture-warning.tsx | 3 + .../components/documents-step.jsx | 3 +- .../components/review-issues-step.tsx | 2 + .../document-capture/components/tip-list.tsx | 5 +- .../components/unknown-error.tsx | 23 ++- config/locales/doc_auth/en.yml | 1 + config/locales/doc_auth/es.yml | 1 + config/locales/doc_auth/fr.yml | 1 + config/locales/idv/en.yml | 4 +- config/locales/idv/es.yml | 5 +- config/locales/idv/fr.yml | 4 +- .../idv/doc_auth/document_capture_spec.rb | 16 -- .../document-capture-review-issues-spec.jsx | 28 --- .../components/document-capture-spec.jsx | 3 +- .../components/documents-step-spec.jsx | 26 +-- .../components/review-issues-step-spec.jsx | 22 -- .../components/tip-list-spec.jsx | 14 +- .../components/unknown-error-spec.jsx | 195 +++++++++++------- 19 files changed, 174 insertions(+), 189 deletions(-) diff --git a/app/javascript/packages/document-capture/components/document-capture-review-issues.tsx b/app/javascript/packages/document-capture/components/document-capture-review-issues.tsx index 3615ce763a3..07be6b3d90a 100644 --- a/app/javascript/packages/document-capture/components/document-capture-review-issues.tsx +++ b/app/javascript/packages/document-capture/components/document-capture-review-issues.tsx @@ -10,7 +10,6 @@ import { useI18n } from '@18f/identity-react-i18n'; import UnknownError from './unknown-error'; import TipList from './tip-list'; import DocumentSideAcuantCapture from './document-side-acuant-capture'; -import DocumentCaptureTroubleshootingOptions from './document-capture-troubleshooting-options'; interface DocumentCaptureReviewIssuesProps { isFailedDocType: boolean; @@ -22,6 +21,7 @@ interface DocumentCaptureReviewIssuesProps { errors: FormStepError[]; onChange: (...args: any) => void; onError: OnErrorCallback; + hasDismissed: boolean; } type DocumentSide = 'front' | 'back'; @@ -40,6 +40,7 @@ function DocumentCaptureReviewIssues({ onChange = () => undefined, onError = () => undefined, value = {}, + hasDismissed, }: DocumentCaptureReviewIssuesProps) { const { t } = useI18n(); return ( @@ -50,9 +51,11 @@ function DocumentCaptureReviewIssues({ remainingAttempts={remainingAttempts} isFailedDocType={isFailedDocType} altFailedDocTypeMsg={isFailedDocType ? t('doc_auth.errors.doc.wrong_id_type') : null} + hasDismissed={hasDismissed} /> {!isFailedDocType && captureHints && ( ))} - - ); diff --git a/app/javascript/packages/document-capture/components/document-capture-warning.tsx b/app/javascript/packages/document-capture/components/document-capture-warning.tsx index 42ae52b3455..24c74a56edf 100644 --- a/app/javascript/packages/document-capture/components/document-capture-warning.tsx +++ b/app/javascript/packages/document-capture/components/document-capture-warning.tsx @@ -13,6 +13,7 @@ interface DocumentCaptureWarningProps { remainingAttempts: number; actionOnClick?: () => void; unknownFieldErrors: FormStepError<{ front: string; back: string }>[]; + hasDismissed: boolean; } const DISPLAY_ATTEMPTS = 3; @@ -23,6 +24,7 @@ function DocumentCaptureWarning({ remainingAttempts, actionOnClick, unknownFieldErrors = [], + hasDismissed, }: DocumentCaptureWarningProps) { const { t } = useI18n(); const { inPersonURL } = useContext(InPersonContext); @@ -59,6 +61,7 @@ function DocumentCaptureWarning({ unknownFieldErrors={unknownFieldErrors} remainingAttempts={remainingAttempts} isFailedDocType={isFailedDocType} + hasDismissed={hasDismissed} /> {!isFailedDocType && remainingAttempts <= DISPLAY_ATTEMPTS && ( diff --git a/app/javascript/packages/document-capture/components/documents-step.jsx b/app/javascript/packages/document-capture/components/documents-step.jsx index 4158186a2ff..bc88c754cf7 100644 --- a/app/javascript/packages/document-capture/components/documents-step.jsx +++ b/app/javascript/packages/document-capture/components/documents-step.jsx @@ -7,7 +7,6 @@ import HybridDocCaptureWarning from './hybrid-doc-capture-warning'; import DocumentSideAcuantCapture from './document-side-acuant-capture'; import DeviceContext from '../context/device'; import UploadContext from '../context/upload'; -import DocumentCaptureTroubleshootingOptions from './document-capture-troubleshooting-options'; import TipList from './tip-list'; /** @@ -51,6 +50,7 @@ function DocumentsStep({ {t('doc_auth.headings.document_capture')}

{t('doc_auth.info.document_capture_intro_acknowledgment')}

))} {isLastStep ? : } - ); diff --git a/app/javascript/packages/document-capture/components/review-issues-step.tsx b/app/javascript/packages/document-capture/components/review-issues-step.tsx index 649a7574a56..b3cc4ab2b96 100644 --- a/app/javascript/packages/document-capture/components/review-issues-step.tsx +++ b/app/javascript/packages/document-capture/components/review-issues-step.tsx @@ -88,6 +88,7 @@ function ReviewIssuesStep({ remainingAttempts={remainingAttempts} unknownFieldErrors={unknownFieldErrors} actionOnClick={onWarningPageDismissed} + hasDismissed={false} /> ); } @@ -103,6 +104,7 @@ function ReviewIssuesStep({ errors={errors} onChange={onChange} onError={onError} + hasDismissed /> ); } diff --git a/app/javascript/packages/document-capture/components/tip-list.tsx b/app/javascript/packages/document-capture/components/tip-list.tsx index 13a87755ed4..d2e2fbbc9de 100644 --- a/app/javascript/packages/document-capture/components/tip-list.tsx +++ b/app/javascript/packages/document-capture/components/tip-list.tsx @@ -1,11 +1,12 @@ interface TipListProps { title: string; items: string[]; + titleClassName?: string; } -function TipList({ title, items = [] }: TipListProps) { +function TipList({ title, items = [], titleClassName }: TipListProps) { return ( <> -

{title}

+

{title}

    {items.map((item) => (
  • {item}
  • diff --git a/app/javascript/packages/document-capture/components/unknown-error.tsx b/app/javascript/packages/document-capture/components/unknown-error.tsx index 96ec5e3cc17..dd7fdf4c7ca 100644 --- a/app/javascript/packages/document-capture/components/unknown-error.tsx +++ b/app/javascript/packages/document-capture/components/unknown-error.tsx @@ -1,12 +1,16 @@ import type { ComponentProps } from 'react'; +import { useContext } from 'react'; import { useI18n, HtmlTextWithStrongNoWrap } from '@18f/identity-react-i18n'; import { FormStepError } from '@18f/identity-form-steps'; +import { Link } from '@18f/identity-components'; +import MarketingSiteContext from '../context/marketing-site'; interface UnknownErrorProps extends ComponentProps<'p'> { unknownFieldErrors: FormStepError<{ front: string; back: string }>[]; isFailedDocType: boolean; remainingAttempts: number; altFailedDocTypeMsg?: string | null; + hasDismissed: boolean; } function UnknownError({ @@ -14,8 +18,15 @@ function UnknownError({ isFailedDocType = false, remainingAttempts, altFailedDocTypeMsg = null, + hasDismissed, }: UnknownErrorProps) { const { t } = useI18n(); + const { getHelpCenterURL } = useContext(MarketingSiteContext); + const helpCenterLink = getHelpCenterURL({ + category: 'verify-your-identity', + article: 'how-to-add-images-of-your-state-issued-id', + location: 'document_capture_review_issues', + }); const errs = !!unknownFieldErrors && unknownFieldErrors.filter((error) => !['front', 'back'].includes(error.field!)); @@ -33,9 +44,19 @@ function UnknownError({

    ); } - if (err) { + if (err && !hasDismissed) { return

    {err.message}

    ; } + if (err && hasDismissed) { + return ( +

    + {err.message}{' '} + + {t('doc_auth.info.review_examples_of_photos')} + +

    + ); + } return

    ; } diff --git a/config/locales/doc_auth/en.yml b/config/locales/doc_auth/en.yml index 3385fe7c07c..896682efd6b 100644 --- a/config/locales/doc_auth/en.yml +++ b/config/locales/doc_auth/en.yml @@ -188,6 +188,7 @@ en: privacy: '%{app_name} is a secure, government website that adheres to the highest standards in data protection. We use your data to verify your identity.' + review_examples_of_photos: Review examples of how to take clear photos of your ID. secure_account: We’ll encrypt your account with your password. Encryption means your data is protected and only you will be able to access or change your information. diff --git a/config/locales/doc_auth/es.yml b/config/locales/doc_auth/es.yml index 1f1f3407c73..c72483ac0ea 100644 --- a/config/locales/doc_auth/es.yml +++ b/config/locales/doc_auth/es.yml @@ -220,6 +220,7 @@ es: privacy: '%{app_name} es un sitio web gubernamental seguro que cumple con las normas más estrictas de protección de datos. Utilizamos sus datos para verificar su identidad.' + review_examples_of_photos: Revisa ejemplos de cómo hacer fotos nítidas de tu documento de identidad. secure_account: Vamos a encriptar su cuenta con su contraseña. La encriptación significa que sus datos están protegidos y solo usted podrá acceder o modificar su información. diff --git a/config/locales/doc_auth/fr.yml b/config/locales/doc_auth/fr.yml index 47fb28b8d34..bbc982b19ea 100644 --- a/config/locales/doc_auth/fr.yml +++ b/config/locales/doc_auth/fr.yml @@ -228,6 +228,7 @@ fr: privacy: '%{app_name} est un site gouvernemental sécurisé qui respecte les normes les plus strictes en matière de protection des données. Nous utilisons vos données pour vérifier votre identité.' + review_examples_of_photos: Examinez des exemples de photos claires de votre pièce d’identité. secure_account: Nous chiffrerons votre compte avec votre mot de passe. Le chiffrage signifie que vos données sont protégées et que vous êtes le/la seul(e) à pouvoir accéder à vos informations ou les modifier. diff --git a/config/locales/idv/en.yml b/config/locales/idv/en.yml index d331b10ef3d..2d0af4d1d7c 100644 --- a/config/locales/idv/en.yml +++ b/config/locales/idv/en.yml @@ -292,14 +292,14 @@ en: need_assistance: 'Need immediate assistance? Here’s how to get help:' options: contact_support: Contact %{app_name} Support - doc_capture_tips: More tips for adding photos of your ID + doc_capture_tips: Tips for taking clear photos of your ID get_help_at_sp: Get help at %{sp_name} learn_more_address_verification_options: Learn more about verifying by phone or mail learn_more_verify_by_mail: Learn more about verifying your address by mail learn_more_verify_by_phone: Learn more about what phone number to use learn_more_verify_by_phone_in_person: Learn more about verifying your phone number learn_more_verify_in_person: Learn more about verifying in person - supported_documents: See a list of accepted state‑issued IDs + supported_documents: Learn more about accepted IDs verify_by_mail: Verify your address by mail instead unavailable: exit_button: 'Exit %{app_name}' diff --git a/config/locales/idv/es.yml b/config/locales/idv/es.yml index 6056fcee9f2..7dfebd40c00 100644 --- a/config/locales/idv/es.yml +++ b/config/locales/idv/es.yml @@ -304,7 +304,7 @@ es: need_assistance: '¿Necesita ayuda inmediata? Así es como puede obtener ayuda:' options: contact_support: Póngase en contacto con el servicio de asistencia de %{app_name} - doc_capture_tips: Más consejos para agregar fotos de su identificación + doc_capture_tips: Sugerencias para obtener fotos nítidas de tu documento de identidad get_help_at_sp: Obtenga ayuda en %{sp_name} learn_more_address_verification_options: Obtenga más información sobre la verificación por teléfono o por correo learn_more_verify_by_mail: Obtenga más información sobre la verificación de su @@ -312,8 +312,7 @@ es: learn_more_verify_by_phone: Más información sobre qué número de teléfono usar learn_more_verify_by_phone_in_person: Más información sobre cómo verificar su número de teléfono learn_more_verify_in_person: Más información sobre la verificación en persona - supported_documents: Vea la lista de documentos de identidad emitidos por el - estado que son aceptados + supported_documents: Más información sobre los documentos de identidad aceptados verify_by_mail: Verifique su dirección por correo unavailable: exit_button: 'Salir de %{app_name}' diff --git a/config/locales/idv/fr.yml b/config/locales/idv/fr.yml index 5de4838c0e0..cdb49352b88 100644 --- a/config/locales/idv/fr.yml +++ b/config/locales/idv/fr.yml @@ -324,14 +324,14 @@ fr: obtenir de l’aide:' options: contact_support: Contacter le service d’assistance de %{app_name} - doc_capture_tips: Plus de conseils pour ajouter des photos de votre carte d’identité + doc_capture_tips: Conseils pour prendre des photos claires de votre pièce d’identité get_help_at_sp: Demandez de l’aide à %{sp_name} learn_more_address_verification_options: En savoir plus sur la vérification par téléphone ou par courrier learn_more_verify_by_mail: En savoir plus sur la vérification de votre adresse par courrier learn_more_verify_by_phone: Apprenez-en plus sur quel numéro de téléphone utiliser learn_more_verify_by_phone_in_person: En savoir plus sur la vérification de votre numéro de téléphone learn_more_verify_in_person: En savoir plus sur la vérification en personne - supported_documents: Voir la liste des pièces d’identité acceptées et délivrées par l’État + supported_documents: En savoir plus sur les pièces d’identité acceptées verify_by_mail: Vérifiez plutôt votre adresse par courrier unavailable: exit_button: 'Quitter %{app_name}' diff --git a/spec/features/idv/doc_auth/document_capture_spec.rb b/spec/features/idv/doc_auth/document_capture_spec.rb index 0a85d2e485a..1b55ebcddf7 100644 --- a/spec/features/idv/doc_auth/document_capture_spec.rb +++ b/spec/features/idv/doc_auth/document_capture_spec.rb @@ -23,22 +23,6 @@ complete_doc_auth_steps_before_document_capture_step end - it 'logs return to sp link click' do - new_window = window_opened_by do - click_on t('idv.troubleshooting.options.get_help_at_sp', sp_name: sp_name) - end - - within_window new_window do - expect(fake_analytics).to have_logged_event( - 'Return to SP: Failed to proof', - flow: nil, - location: 'document_capture', - redirect_url: instance_of(String), - step: 'document_capture', - ) - end - end - context 'wrong doc type is uploaded', allow_browser_log: true do it 'try again and page show doc type inline error message' do attach_images( diff --git a/spec/javascript/packages/document-capture/components/document-capture-review-issues-spec.jsx b/spec/javascript/packages/document-capture/components/document-capture-review-issues-spec.jsx index ef8f32deccf..f2f8e7a8476 100644 --- a/spec/javascript/packages/document-capture/components/document-capture-review-issues-spec.jsx +++ b/spec/javascript/packages/document-capture/components/document-capture-review-issues-spec.jsx @@ -48,11 +48,6 @@ describe('DocumentCaptureReviewIssues', () => { expect(h1).to.be.ok(); expect(getByText('general error')).to.be.ok(); - const h2 = screen.getByRole('heading', { - name: 'components.troubleshooting_options.default_heading', - level: 2, - }); - expect(h2).to.be.ok(); // tips header expect(getByText('doc_auth.tips.review_issues_id_header_text')).to.be.ok(); @@ -63,18 +58,7 @@ describe('DocumentCaptureReviewIssues', () => { tipListItem.forEach((li, idx) => { expect(li.textContent).to.equals(`doc_auth.tips.review_issues_id_text${idx + 1}`); }); - const troubleShootingList = lists[1]; - - expect(troubleShootingList).to.be.ok(); - const troubleShootingListItem = within(troubleShootingList).getAllByRole('listitem'); - expect(troubleShootingListItem).to.be.ok(); - const hrefLinks = within(troubleShootingList).getAllByRole('link'); - - hrefLinks.forEach((hrefLink) => { - expect(hrefLink.href).to.contains('location=post_submission_review'); - expect(hrefLink.href).to.contains('article='); - }); // front capture input const frontCapture = getByLabelText('doc_auth.headings.document_capture_front'); expect(frontCapture).to.be.ok(); @@ -123,18 +107,6 @@ describe('DocumentCaptureReviewIssues', () => { expect(getByText('doc_auth.errors.doc.wrong_id_type')).to.be.ok(); - const troubleShootingList = getByRole('list'); - - expect(troubleShootingList).to.be.ok(); - - const troubleShootingListItem = within(troubleShootingList).getAllByRole('listitem'); - expect(troubleShootingListItem).to.be.ok(); - const hrefLinks = within(troubleShootingList).getAllByRole('link'); - - hrefLinks.forEach((hrefLink) => { - expect(hrefLink.href).to.contains('location=post_submission_review'); - expect(hrefLink.href).to.contains('article='); - }); // front capture input const frontCapture = getByLabelText('doc_auth.headings.document_capture_front'); expect(frontCapture).to.be.ok(); diff --git a/spec/javascript/packages/document-capture/components/document-capture-spec.jsx b/spec/javascript/packages/document-capture/components/document-capture-spec.jsx index ccbcf60017c..87febba9db3 100644 --- a/spec/javascript/packages/document-capture/components/document-capture-spec.jsx +++ b/spec/javascript/packages/document-capture/components/document-capture-spec.jsx @@ -222,8 +222,9 @@ describe('document-capture/components/document-capture', () => { ); // Make sure that the first focusable element after a tab is what we expect it to be. + // Since the error wasn't related to an unsupported document type, user should see a help center link. await userEvent.tab(); - const firstFocusable = getByLabelText('doc_auth.headings.document_capture_front'); + const firstFocusable = getByText('doc_auth.info.review_examples_of_photos'); expect(document.activeElement).to.equal(firstFocusable); const hasValueSelected = !!getByLabelText('doc_auth.headings.document_capture_front'); diff --git a/spec/javascript/packages/document-capture/components/documents-step-spec.jsx b/spec/javascript/packages/document-capture/components/documents-step-spec.jsx index 88174889e0b..a5c5cb4d2a7 100644 --- a/spec/javascript/packages/document-capture/components/documents-step-spec.jsx +++ b/spec/javascript/packages/document-capture/components/documents-step-spec.jsx @@ -1,11 +1,7 @@ import userEvent from '@testing-library/user-event'; import sinon from 'sinon'; import { t } from '@18f/identity-i18n'; -import { - DeviceContext, - ServiceProviderContextProvider, - UploadContextProvider, -} from '@18f/identity-document-capture'; +import { DeviceContext, UploadContextProvider } from '@18f/identity-document-capture'; import DocumentsStep from '@18f/identity-document-capture/components/documents-step'; import { render } from '../../../support/document-capture'; import { getFixtureFile } from '../../../support/file'; @@ -50,26 +46,6 @@ describe('document-capture/components/documents-step', () => { expect(() => getByText('doc_auth.tips.document_capture_id_text4')).not.to.throw(); }); - it('renders troubleshooting options', () => { - const { getByRole } = render( - - - , - ); - - expect( - getByRole('heading', { name: 'components.troubleshooting_options.default_heading' }), - ).to.be.ok(); - expect( - getByRole('link', { name: 'idv.troubleshooting.options.get_help_at_sp links.new_tab' }).href, - ).to.equal('https://example.com/?step=document_capture&location=document_capture'); - }); - it('renders the hybrid flow warning if the flow is hybrid', () => { const { getByText } = render( diff --git a/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx b/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx index 45cb46e341f..5984f8f72d4 100644 --- a/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx +++ b/spec/javascript/packages/document-capture/components/review-issues-step-spec.jsx @@ -169,28 +169,6 @@ describe('document-capture/components/review-issues-step', () => { }); }); - it('renders troubleshooting options', async () => { - const { getByRole } = render( - - - , - ); - - await userEvent.click(getByRole('button', { name: 'idv.failure.button.warning' })); - - expect( - getByRole('heading', { name: 'components.troubleshooting_options.default_heading' }), - ).to.be.ok(); - expect( - getByRole('link', { name: 'idv.troubleshooting.options.get_help_at_sp links.new_tab' }).href, - ).to.equal('https://example.com/?step=document_capture&location=post_submission_review'); - }); - it('does not render sp help troubleshooting option for errored review', () => { const { queryByRole } = render( diff --git a/spec/javascript/packages/document-capture/components/tip-list-spec.jsx b/spec/javascript/packages/document-capture/components/tip-list-spec.jsx index 5a02d58fdee..2e6990d7b38 100644 --- a/spec/javascript/packages/document-capture/components/tip-list-spec.jsx +++ b/spec/javascript/packages/document-capture/components/tip-list-spec.jsx @@ -3,9 +3,10 @@ import TipList from '@18f/identity-document-capture/components/tip-list'; import { render } from '../../../support/document-capture'; describe('document-capture/components/tip-list', () => { + const title = 'doc_auth.tips.review_issues_id_header_text'; + const items = ['doc_auth.tips.review_issues_id_text1', 'doc_auth.tips.review_issues_id_text2']; + it('renders title and list', () => { - const title = 'doc_auth.tips.review_issues_id_header_text'; - const items = ['doc_auth.tips.review_issues_id_text1', 'doc_auth.tips.review_issues_id_text2']; const { getByRole, getAllByRole, getByText } = render( , ); @@ -17,4 +18,13 @@ describe('document-capture/components/tip-list', () => { 'doc_auth.tips.review_issues_id_text2', ]); }); + + it('formats the title based on titleClassName', () => { + const titleClassName = 'margin-bottom-0 margin-top-2'; + const { getByText } = render( + , + ); + const tipsTitle = getByText('doc_auth.tips.review_issues_id_header_text'); + expect(tipsTitle.className).to.eql(titleClassName); + }); }); diff --git a/spec/javascript/packages/document-capture/components/unknown-error-spec.jsx b/spec/javascript/packages/document-capture/components/unknown-error-spec.jsx index 6d4616ddcf5..19e143cdd10 100644 --- a/spec/javascript/packages/document-capture/components/unknown-error-spec.jsx +++ b/spec/javascript/packages/document-capture/components/unknown-error-spec.jsx @@ -6,91 +6,126 @@ import { within } from '@testing-library/dom'; import { render } from '../../../support/document-capture'; describe('UnknownError', () => { - it('render an empty paragraph when no errors', () => { - const { container } = render( - , - ); - expect(container.querySelector('p')).to.be.ok(); - }); + context('there is no doc type failure', () => { + it('render an empty paragraph when no errors', () => { + const { container } = render( + , + ); + expect(container.querySelector('p')).to.be.ok(); + }); - it('renders error message with errors but not a doc type failure', () => { - const { container } = render( - , - ); - const paragraph = container.querySelector('p'); - expect(within(paragraph).getByText('An unknown error occurred')).to.be.ok(); - }); + context('hasDismissed is true', () => { + it('renders error message with errors and a help center link', () => { + const { container } = render( + , + ); + const paragraph = container.querySelector('p'); + expect(within(paragraph).getByText('An unknown error occurred')).to.be.ok(); + const link = container.querySelector('a'); + expect(link.text).to.eql('doc_auth.info.review_examples_of_photoslinks.new_tab'); + }); + }); - it('renders error message with errors and is a doc type failure', () => { - const { container } = render( - One attempt remaining', - other: 'You have%{count} attempts remaining', + context('hasDismissed is false', () => { + it('renders error message with errors but no link', () => { + const { container, queryByRole } = render( + - - , - ); - const paragraph = container.querySelector('p'); - expect(within(paragraph).getByText(/An unknown error occurred/)).to.be.ok(); - expect(within(paragraph).getByText(/2 attempts/)).to.be.ok(); - expect(within(paragraph).getByText(/remaining/)).to.be.ok(); + ]} + isFailedDocType={false} + remainingAttempts={10} + hasDismissed={false} + />, + ); + + const paragraph = container.querySelector('p'); + expect(within(paragraph).getByText('An unknown error occurred')).to.be.ok(); + expect( + queryByRole('link', { + name: 'doc_auth.info.review_examples_of_photos', + }), + ).to.not.exist(); + }); + }); }); - it('renders alternative error message with errors and is a doc type failure', () => { - const { container } = render( - One attempt remaining', - other: 'You have %{count} attempts remaining', + context('there is a doc type failure', () => { + it('renders error message with errors and is a doc type failure', () => { + const { container } = render( + One attempt remaining', + other: 'You have%{count} attempts remaining', + }, + 'errors.doc_auth.doc_type_not_supported_heading': 'doc type not supported', + }, + }) + } + > + + , + ); + const paragraph = container.querySelector('p'); + expect(within(paragraph).getByText(/An unknown error occurred/)).to.be.ok(); + expect(within(paragraph).getByText(/2 attempts/)).to.be.ok(); + expect(within(paragraph).getByText(/remaining/)).to.be.ok(); + }); + + it('renders alternative error message with errors and is a doc type failure', () => { + const { container } = render( + One attempt remaining', + other: 'You have %{count} attempts remaining', + }, + }, + }) + } + > + - - , - ); - const paragraph = container.querySelector('p'); - expect(within(paragraph).getByText(/alternative message/)).to.be.ok(); + ]} + remainingAttempts={2} + isFailedDocType + altFailedDocTypeMsg="alternative message" + /> + , + ); + const paragraph = container.querySelector('p'); + expect(within(paragraph).getByText(/alternative message/)).to.be.ok(); + }); }); }); From 700fbee3caf26c9a263e6f0452180541a52926b5 Mon Sep 17 00:00:00 2001 From: Matt Gardner Date: Mon, 11 Sep 2023 13:58:50 -0400 Subject: [PATCH 03/11] 10787: Combine then extract FullAddressSearch component (#9179) * changelog: Internal, In-Person Proofing, Separate components * Move Into separate component * Lint --- .../components/full-address-search-input.tsx | 177 +++++++++++++++ .../in-person-full-address-search.spec.tsx | 32 ++- .../in-person-full-address-search.tsx | 204 ++++-------------- ...-address-entry-post-office-search-step.tsx | 36 +--- 4 files changed, 250 insertions(+), 199 deletions(-) create mode 100644 app/javascript/packages/document-capture/components/full-address-search-input.tsx diff --git a/app/javascript/packages/document-capture/components/full-address-search-input.tsx b/app/javascript/packages/document-capture/components/full-address-search-input.tsx new file mode 100644 index 00000000000..e81c5f9f771 --- /dev/null +++ b/app/javascript/packages/document-capture/components/full-address-search-input.tsx @@ -0,0 +1,177 @@ +import { TextInput, SelectInput } from '@18f/identity-components'; +import type { FormattedLocation, LocationQuery } from '@18f/identity-address-search/types'; +import type { RegisterFieldCallback } from '@18f/identity-form-steps'; +import { useDidUpdateEffect } from '@18f/identity-react-hooks'; +import { SpinnerButtonRefHandle, SpinnerButton } from '@18f/identity-spinner-button'; +import { ValidatedField } from '@18f/identity-validated-field'; +import { t } from '@18f/identity-i18n'; +import { useCallback, useContext, useEffect, useRef, useState } from 'react'; +import { InPersonContext } from '../context'; +import useValidatedUspsLocations from '../hooks/use-validated-usps-locations'; + +interface FullAddressSearchProps { + registerField?: RegisterFieldCallback; + onFoundLocations?: ( + address: LocationQuery | null, + locations: FormattedLocation[] | null | undefined, + ) => void; + onLoadingLocations?: (isLoading: boolean) => void; + onError?: (error: Error | null) => void; + disabled?: boolean; + locationsURL: string; +} + +export default function FullAddressSearchInput({ + registerField = () => undefined, + onFoundLocations = () => undefined, + onLoadingLocations = () => undefined, + onError = () => undefined, + disabled = false, + locationsURL, +}: FullAddressSearchProps) { + const spinnerButtonRef = useRef(null); + const [addressValue, setAddressValue] = useState(''); + const [cityValue, setCityValue] = useState(''); + const [stateValue, setStateValue] = useState(''); + const [zipCodeValue, setZipCodeValue] = useState(''); + const { + locationQuery, + locationResults, + uspsError, + isLoading, + handleLocationSearch: onSearch, + validatedAddressFieldRef, + validatedCityFieldRef, + validatedStateFieldRef, + validatedZipCodeFieldRef, + } = useValidatedUspsLocations(locationsURL); + + const inputChangeHandler = + (input) => + (event: React.ChangeEvent) => { + const { target } = event; + input(target.value); + }; + + type SelectChangeEvent = React.ChangeEvent; + + const onAddressChange = inputChangeHandler(setAddressValue); + const onCityChange = inputChangeHandler(setCityValue); + const onStateChange = (e: SelectChangeEvent) => setStateValue(e.target.value); + const onZipCodeChange = inputChangeHandler(setZipCodeValue); + + useEffect(() => { + spinnerButtonRef.current?.toggleSpinner(isLoading); + onLoadingLocations(isLoading); + }, [isLoading]); + + useEffect(() => { + uspsError && onError(uspsError); + }, [uspsError]); + + useDidUpdateEffect(() => { + onFoundLocations(locationQuery, locationResults); + }, [locationResults]); + + const handleSearch = useCallback( + (event) => { + onError(null); + onSearch(event, addressValue, cityValue, stateValue, zipCodeValue); + }, + [addressValue, cityValue, stateValue, zipCodeValue], + ); + + const { usStatesTerritories } = useContext(InPersonContext); + + return ( + <> + + + + + + + + + + {usStatesTerritories.map(([name, abbr]) => ( + + ))} + + + + + +

    + + {t('in_person_proofing.body.location.po_search.search_button')} + +
    + + ); +} diff --git a/app/javascript/packages/document-capture/components/in-person-full-address-search.spec.tsx b/app/javascript/packages/document-capture/components/in-person-full-address-search.spec.tsx index 713fe90b93b..d879b20da61 100644 --- a/app/javascript/packages/document-capture/components/in-person-full-address-search.spec.tsx +++ b/app/javascript/packages/document-capture/components/in-person-full-address-search.spec.tsx @@ -16,7 +16,13 @@ describe('FullAddressSearch', () => { const handleLocationsFound = sandbox.stub(); const { findByText, findAllByText } = render( new Map() }}> - + , ); @@ -32,7 +38,13 @@ describe('FullAddressSearch', () => { const handleLocationsFound = sandbox.stub(); const { findByText, findByLabelText, findAllByText } = render( new Map() }}> - + , ); @@ -64,7 +76,13 @@ describe('FullAddressSearch', () => { const handleLocationsFound = sandbox.stub(); const { findByText, findByLabelText, queryByText } = render( new Map() }}> - + , ); @@ -109,7 +127,13 @@ describe('FullAddressSearch', () => { const handleLocationsFound = sandbox.stub(); const { findByText, findByLabelText } = render( new Map() }}> - + , ); diff --git a/app/javascript/packages/document-capture/components/in-person-full-address-search.tsx b/app/javascript/packages/document-capture/components/in-person-full-address-search.tsx index 0f3d3639feb..1cbd826ed70 100644 --- a/app/javascript/packages/document-capture/components/in-person-full-address-search.tsx +++ b/app/javascript/packages/document-capture/components/in-person-full-address-search.tsx @@ -1,177 +1,55 @@ -import { TextInput, SelectInput } from '@18f/identity-components'; -import { useState, useRef, useEffect, useCallback, useContext } from 'react'; +import { Alert, PageHeading } from '@18f/identity-components'; +import { useState } from 'react'; import { t } from '@18f/identity-i18n'; -import ValidatedField from '@18f/identity-validated-field/validated-field'; -import SpinnerButton, { SpinnerButtonRefHandle } from '@18f/identity-spinner-button/spinner-button'; -import type { RegisterFieldCallback } from '@18f/identity-form-steps'; -import { useDidUpdateEffect } from '@18f/identity-react-hooks'; +import { InPersonLocations } from '@18f/identity-address-search'; import type { LocationQuery, FormattedLocation } from '@18f/identity-address-search/types'; -import { InPersonContext } from '../context'; -import useValidatedUspsLocations from '../hooks/use-validated-usps-locations'; - -interface FullAddressSearchProps { - registerField?: RegisterFieldCallback; - onFoundLocations?: ( - address: LocationQuery | null, - locations: FormattedLocation[] | null | undefined, - ) => void; - onLoadingLocations?: (isLoading: boolean) => void; - onError?: (error: Error | null) => void; - disabled?: boolean; - locationsURL: string; -} +import FullAddressSearchInput from './full-address-search-input'; function FullAddressSearch({ - registerField = () => undefined, - onFoundLocations = () => undefined, - onLoadingLocations = () => undefined, - onError = () => undefined, - disabled = false, + registerField, locationsURL, -}: FullAddressSearchProps) { - const spinnerButtonRef = useRef(null); - const [addressValue, setAddressValue] = useState(''); - const [cityValue, setCityValue] = useState(''); - const [stateValue, setStateValue] = useState(''); - const [zipCodeValue, setZipCodeValue] = useState(''); - const { - locationQuery, - locationResults, - uspsError, - isLoading, - handleLocationSearch: onSearch, - validatedAddressFieldRef, - validatedCityFieldRef, - validatedStateFieldRef, - validatedZipCodeFieldRef, - } = useValidatedUspsLocations(locationsURL); - - const inputChangeHandler = - (input) => - (event: React.ChangeEvent) => { - const { target } = event; - input(target.value); - }; - - type SelectChangeEvent = React.ChangeEvent; - - const onAddressChange = inputChangeHandler(setAddressValue); - const onCityChange = inputChangeHandler(setCityValue); - const onStateChange = (e: SelectChangeEvent) => setStateValue(e.target.value); - const onZipCodeChange = inputChangeHandler(setZipCodeValue); - - useEffect(() => { - spinnerButtonRef.current?.toggleSpinner(isLoading); - onLoadingLocations(isLoading); - }, [isLoading]); - - useEffect(() => { - uspsError && onError(uspsError); - }, [uspsError]); - - useDidUpdateEffect(() => { - onFoundLocations(locationQuery, locationResults); - }, [locationResults]); - - const handleSearch = useCallback( - (event) => { - onError(null); - onSearch(event, addressValue, cityValue, stateValue, zipCodeValue); - }, - [addressValue, cityValue, stateValue, zipCodeValue], + handleLocationSelect, + disabled, + onFoundLocations, +}) { + const [apiError, setApiError] = useState(null); + const [foundAddress, setFoundAddress] = useState(null); + const [locationResults, setLocationResults] = useState( + null, ); - - const { usStatesTerritories } = useContext(InPersonContext); + const [isLoadingLocations, setLoadingLocations] = useState(false); return ( <> - - - - - - - - - - {usStatesTerritories.map(([name, abbr]) => ( - - ))} - - - + {t('idv.failure.exceptions.post_office_search_error')} + + )} + {t('in_person_proofing.headings.po_search.location')} +

    {t('in_person_proofing.body.location.po_search.po_search_about')}

    + { + setFoundAddress(address); + setLocationResults(locations); + onFoundLocations(locations); }} - > - + {locationResults && foundAddress && !isLoadingLocations && ( + -
    -
    - - {t('in_person_proofing.body.location.po_search.search_button')} - -
    + )} ); } diff --git a/app/javascript/packages/document-capture/components/in-person-location-full-address-entry-post-office-search-step.tsx b/app/javascript/packages/document-capture/components/in-person-location-full-address-entry-post-office-search-step.tsx index 564c9fdcb0c..1ce97987a37 100644 --- a/app/javascript/packages/document-capture/components/in-person-location-full-address-entry-post-office-search-step.tsx +++ b/app/javascript/packages/document-capture/components/in-person-location-full-address-entry-post-office-search-step.tsx @@ -1,10 +1,8 @@ import { useState, useEffect, useCallback, useRef, useContext } from 'react'; -import { t } from '@18f/identity-i18n'; -import { Alert, PageHeading } from '@18f/identity-components'; import { request } from '@18f/identity-request'; import { forceRedirect } from '@18f/identity-url'; -import { transformKeys, snakeCase, InPersonLocations } from '@18f/identity-address-search'; -import type { LocationQuery, FormattedLocation } from '@18f/identity-address-search/types'; +import { transformKeys, snakeCase } from '@18f/identity-address-search'; +import type { FormattedLocation } from '@18f/identity-address-search/types'; import FullAddressSearch from './in-person-full-address-search'; import BackButton from './back-button'; import AnalyticsContext from '../context/analytics'; @@ -19,25 +17,14 @@ function InPersonLocationFullAddressEntryPostOfficeSearchStep({ }) { const { inPersonURL } = useContext(InPersonContext); const [inProgress, setInProgress] = useState(false); - const [isLoadingLocations, setLoadingLocations] = useState(false); const [autoSubmit, setAutoSubmit] = useState(false); const { trackEvent } = useContext(AnalyticsContext); const [locationResults, setLocationResults] = useState( null, ); - const [foundAddress, setFoundAddress] = useState(null); - const [apiError, setApiError] = useState(null); const [disabledAddressSearch, setDisabledAddressSearch] = useState(false); const { flowPath } = useContext(UploadContext); - const onFoundLocations = ( - address: LocationQuery | null, - locations: FormattedLocation[] | null | undefined, - ) => { - setFoundAddress(address); - setLocationResults(locations); - }; - // ref allows us to avoid a memory leak const mountedRef = useRef(false); @@ -105,28 +92,13 @@ function InPersonLocationFullAddressEntryPostOfficeSearchStep({ return ( <> - {apiError && ( - - {t('idv.failure.exceptions.post_office_search_error')} - - )} - {t('in_person_proofing.headings.po_search.location')} -

    {t('in_person_proofing.body.location.po_search.po_search_about')}

    - {locationResults && foundAddress && !isLoadingLocations && ( - - )} ); From 02a710812edf41462d859753d8a0e7cef70370ae Mon Sep 17 00:00:00 2001 From: Shannon A <20867088+svalexander@users.noreply.github.com> Date: Mon, 11 Sep 2023 14:29:24 -0400 Subject: [PATCH 04/11] LG-10759 Fix State Address Validation (#9134) * add address and city to validation and update spec * changelog: Bug Fixes, State id form, fix validation for address line 1 and city * check dav for addres1 and city validation * lint fix * disable lint message * update spec * update specs with capture_secondary_id_enabled * initialize capture_secondary_id_enabled and lint fixes * lint fix for state id step * lint fix in_person_helper * remove dav from in person helper and spec * remove dav from step helper and ssn spec * update state id step spec and lint --- app/forms/idv/state_id_form.rb | 6 +++- .../idv/steps/in_person/state_id_step.rb | 5 ++- app/validators/idv/form_state_id_validator.rb | 5 +++ spec/features/idv/in_person_spec.rb | 36 +++++++++++++------ spec/features/idv/steps/in_person/ssn_spec.rb | 2 +- .../idv/steps/in_person/state_id_step_spec.rb | 2 +- .../idv/steps/in_person/verify_info_spec.rb | 4 +++ spec/forms/idv/state_id_form_spec.rb | 6 +++- spec/support/features/idv_step_helper.rb | 2 +- spec/support/features/in_person_helper.rb | 10 +++--- 10 files changed, 57 insertions(+), 21 deletions(-) diff --git a/app/forms/idv/state_id_form.rb b/app/forms/idv/state_id_form.rb index 2225225344a..3f80e517ca3 100644 --- a/app/forms/idv/state_id_form.rb +++ b/app/forms/idv/state_id_form.rb @@ -13,8 +13,9 @@ def self.model_name ActiveModel::Name.new(self, nil, 'StateId') end - def initialize(pii) + def initialize(pii, capture_secondary_id_enabled:) @pii = pii + @capture_secondary_id_enabled = capture_secondary_id_enabled end def submit(params) @@ -35,6 +36,9 @@ def submit(params) private + attr_reader :capture_secondary_id_enabled + alias_method :capture_secondary_id_enabled?, :capture_secondary_id_enabled + def consume_params(params) params.each do |key, value| raise_invalid_state_id_parameter_error(key) unless ATTRIBUTES.include?(key.to_sym) diff --git a/app/services/idv/steps/in_person/state_id_step.rb b/app/services/idv/steps/in_person/state_id_step.rb index f87327898c1..872387abe32 100644 --- a/app/services/idv/steps/in_person/state_id_step.rb +++ b/app/services/idv/steps/in_person/state_id_step.rb @@ -104,7 +104,10 @@ def flow_params end def form - @form ||= Idv::StateIdForm.new(current_user) + @form ||= Idv::StateIdForm.new( + current_user, + capture_secondary_id_enabled: capture_secondary_id_enabled?, + ) end def form_submit diff --git a/app/validators/idv/form_state_id_validator.rb b/app/validators/idv/form_state_id_validator.rb index 6e409bd9916..cb993bae471 100644 --- a/app/validators/idv/form_state_id_validator.rb +++ b/app/validators/idv/form_state_id_validator.rb @@ -11,6 +11,11 @@ module FormStateIdValidator :state_id_number, presence: true + validates :identity_doc_address1, + :identity_doc_city, + presence: true, + if: :capture_secondary_id_enabled? + validates_with UspsInPersonProofing::TransliterableValidator, fields: [:first_name, :last_name, :identity_doc_city], reject_chars: /[^A-Za-z\-' ]/, diff --git a/spec/features/idv/in_person_spec.rb b/spec/features/idv/in_person_spec.rb index 99a3357d6c5..14fe80b0713 100644 --- a/spec/features/idv/in_person_spec.rb +++ b/spec/features/idv/in_person_spec.rb @@ -456,7 +456,7 @@ complete_location_step expect(page).to have_current_path(idv_in_person_step_path(step: :state_id), wait: 10) - fill_out_state_id_form_ok(double_address_verification: double_address_verification) + fill_out_state_id_form_ok(capture_secondary_id_enabled: capture_secondary_id_enabled) fill_in t('in_person_proofing.form.state_id.first_name'), with: 'T0mmy "Lee"' fill_in t('in_person_proofing.form.state_id.last_name'), with: 'Джейкоб' fill_in t('in_person_proofing.form.state_id.address1'), with: '#1 $treet' @@ -531,7 +531,7 @@ expect(page).to have_content(I18n.t('in_person_proofing.form.state_id.address2_hint')) # change state selection - fill_out_state_id_form_ok(double_address_verification: true) + fill_out_state_id_form_ok(capture_secondary_id_enabled: true) expect(page).not_to have_content(I18n.t('in_person_proofing.form.state_id.address1_hint')) expect(page).not_to have_content(I18n.t('in_person_proofing.form.state_id.address2_hint')) @@ -693,7 +693,9 @@ complete_location_step(user) end it 'successfully proceeds through the flow' do - complete_state_id_step(user, same_address_as_id: false, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: false, capture_secondary_id_enabled: true + ) complete_address_step(user, double_address_verification: true) @@ -732,7 +734,9 @@ end it 'skips the address page' do - complete_state_id_step(user, same_address_as_id: true, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: true, capture_secondary_id_enabled: true + ) # skip address step complete_ssn_step(user) # Ensure the page submitted successfully @@ -740,7 +744,9 @@ end it 'can redo the address page form even if that page is skipped' do - complete_state_id_step(user, same_address_as_id: true, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: true, capture_secondary_id_enabled: true + ) # skip address step complete_ssn_step(user) # click update address button on the verify page @@ -753,7 +759,9 @@ end it 'allows user to update their residential address as different from their state id' do - complete_state_id_step(user, same_address_as_id: true, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: true, capture_secondary_id_enabled: true + ) complete_ssn_step(user) # click "update residential address" @@ -797,7 +805,9 @@ it 'does not update their previous selection of "Yes, I live at the address on my state-issued ID"' do - complete_state_id_step(user, same_address_as_id: true, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: true, capture_secondary_id_enabled: true + ) # skip address step complete_ssn_step(user) # expect to be on verify page @@ -829,7 +839,9 @@ end it 'does not update their previous selection of "No, I live at a different address"' do - complete_state_id_step(user, same_address_as_id: false, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: false, capture_secondary_id_enabled: true + ) # expect to be on address page expect(page).to have_content(t('in_person_proofing.headings.address')) # complete address step @@ -863,7 +875,9 @@ end it 'updates their previous selection from "Yes" TO "No, I live at a different address"' do - complete_state_id_step(user, same_address_as_id: true, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: true, capture_secondary_id_enabled: true + ) # skip address step complete_ssn_step(user) # click update state ID button on the verify page @@ -898,7 +912,9 @@ it 'updates their previous selection from "No" TO "Yes, I live at the address on my state-issued ID"' do - complete_state_id_step(user, same_address_as_id: false, double_address_verification: true) + complete_state_id_step( + user, same_address_as_id: false, capture_secondary_id_enabled: true + ) # expect to be on address page expect(page).to have_content(t('in_person_proofing.headings.address')) # complete address step diff --git a/spec/features/idv/steps/in_person/ssn_spec.rb b/spec/features/idv/steps/in_person/ssn_spec.rb index ef7e6a5fbd3..011f4ed029e 100644 --- a/spec/features/idv/steps/in_person/ssn_spec.rb +++ b/spec/features/idv/steps/in_person/ssn_spec.rb @@ -111,7 +111,7 @@ # location page complete_location_step(user) # state ID page - fill_out_state_id_form_ok(double_address_verification: true, same_address_as_id: false) + fill_out_state_id_form_ok(same_address_as_id: false, capture_secondary_id_enabled: true) click_idv_continue fill_out_address_form_ok(double_address_verification: true, same_address_as_id: false) click_idv_continue diff --git a/spec/features/idv/steps/in_person/state_id_step_spec.rb b/spec/features/idv/steps/in_person/state_id_step_spec.rb index d0109843d9c..a265174d37f 100644 --- a/spec/features/idv/steps/in_person/state_id_step_spec.rb +++ b/spec/features/idv/steps/in_person/state_id_step_spec.rb @@ -23,7 +23,7 @@ complete_prepare_step(user) complete_location_step(user) expect(page).to have_current_path(idv_in_person_step_path(step: :state_id), wait: 10) - fill_out_state_id_form_ok(double_address_verification: true, same_address_as_id: true) + fill_out_state_id_form_ok(same_address_as_id: true, capture_secondary_id_enabled: true) # blank out the zip code field fill_in t('in_person_proofing.form.state_id.zipcode'), with: '' # try to enter invalid input into the zip code field diff --git a/spec/features/idv/steps/in_person/verify_info_spec.rb b/spec/features/idv/steps/in_person/verify_info_spec.rb index e0872eef1cc..c4a4be66b56 100644 --- a/spec/features/idv/steps/in_person/verify_info_spec.rb +++ b/spec/features/idv/steps/in_person/verify_info_spec.rb @@ -7,10 +7,14 @@ let(:user) { user_with_2fa } let(:fake_analytics) { FakeAnalytics.new(user: user) } + let(:capture_secondary_id_enabled) { false } + let(:enrollment) { InPersonEnrollment.new(capture_secondary_id_enabled:) } before do allow(IdentityConfig.store).to receive(:in_person_proofing_enabled).and_return(true) allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics) + allow(user).to receive(:enrollment). + and_return(enrollment) end it 'provides back buttons for address, state ID, and SSN that discard changes', diff --git a/spec/forms/idv/state_id_form_spec.rb b/spec/forms/idv/state_id_form_spec.rb index 7cb107f7db5..a2d371c481d 100644 --- a/spec/forms/idv/state_id_form_spec.rb +++ b/spec/forms/idv/state_id_form_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe Idv::StateIdForm do - let(:subject) { Idv::StateIdForm.new(pii) } + let(:subject) { Idv::StateIdForm.new(pii, capture_secondary_id_enabled:) } let(:valid_dob) do valid_d = Time.zone.today - IdentityConfig.store.idv_min_age_years.years - 1.day ActionController::Parameters.new( @@ -22,6 +22,7 @@ dob: valid_dob, identity_doc_address1: Faker::Address.street_address, identity_doc_address2: Faker::Address.secondary_address, + identity_doc_city: Faker::Address.city, identity_doc_zipcode: Faker::Address.zip_code, identity_doc_address_state: Faker::Address.state_abbr, same_address_as_id: 'true', @@ -36,6 +37,7 @@ dob: too_young_dob, identity_doc_address1: Faker::Address.street_address, identity_doc_address2: Faker::Address.secondary_address, + identity_doc_city: Faker::Address.city, identity_doc_zipcode: Faker::Address.zip_code, identity_doc_address_state: Faker::Address.state_abbr, same_address_as_id: 'true', @@ -51,6 +53,7 @@ dob: valid_dob, identity_doc_address1: Faker::Address.street_address, identity_doc_address2: Faker::Address.secondary_address, + identity_doc_city: Faker::Address.city, identity_doc_zipcode: Faker::Address.zip_code, identity_doc_address_state: Faker::Address.state_abbr, same_address_as_id: 'true', @@ -59,6 +62,7 @@ } end let(:pii) { nil } + let(:capture_secondary_id_enabled) { true } describe '#submit' do context 'when the form is valid' do it 'returns a successful form response' do diff --git a/spec/support/features/idv_step_helper.rb b/spec/support/features/idv_step_helper.rb index 4f14215840b..86b3b700e40 100644 --- a/spec/support/features/idv_step_helper.rb +++ b/spec/support/features/idv_step_helper.rb @@ -134,7 +134,7 @@ def complete_idv_steps_before_ssn(user = user_with_2fa) # location page complete_location_step(user) # state ID page - fill_out_state_id_form_ok(double_address_verification: true, same_address_as_id: true) + fill_out_state_id_form_ok(same_address_as_id: true, capture_secondary_id_enabled: true) click_idv_continue end diff --git a/spec/support/features/in_person_helper.rb b/spec/support/features/in_person_helper.rb index 672ea15dbec..8f856af732e 100644 --- a/spec/support/features/in_person_helper.rb +++ b/spec/support/features/in_person_helper.rb @@ -31,7 +31,7 @@ module InPersonHelper GOOD_IDENTITY_DOC_ZIPCODE = Idp::Constants::MOCK_IDV_APPLICANT_STATE_ID_ADDRESS[:identity_doc_zipcode] - def fill_out_state_id_form_ok(double_address_verification: false, same_address_as_id: false) + def fill_out_state_id_form_ok(same_address_as_id: false, capture_secondary_id_enabled: false) fill_in t('in_person_proofing.form.state_id.first_name'), with: GOOD_FIRST_NAME fill_in t('in_person_proofing.form.state_id.last_name'), with: GOOD_LAST_NAME year, month, day = GOOD_DOB.split('-') @@ -42,7 +42,7 @@ def fill_out_state_id_form_ok(double_address_verification: false, same_address_a from: t('in_person_proofing.form.state_id.state_id_jurisdiction') fill_in t('in_person_proofing.form.state_id.state_id_number'), with: GOOD_STATE_ID_NUMBER - if double_address_verification + if capture_secondary_id_enabled fill_in t('in_person_proofing.form.state_id.address1'), with: GOOD_IDENTITY_DOC_ADDRESS1 fill_in t('in_person_proofing.form.state_id.address2'), with: GOOD_IDENTITY_DOC_ADDRESS2 fill_in t('in_person_proofing.form.state_id.city'), with: GOOD_IDENTITY_DOC_CITY @@ -123,15 +123,15 @@ def complete_prepare_step(_user = nil) end def complete_state_id_step(_user = nil, same_address_as_id: true, - double_address_verification: false) + capture_secondary_id_enabled: false) # 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( - double_address_verification: double_address_verification, same_address_as_id: same_address_as_id, + capture_secondary_id_enabled: capture_secondary_id_enabled, ) click_idv_continue - unless double_address_verification && same_address_as_id + unless capture_secondary_id_enabled && same_address_as_id expect(page).to have_current_path(idv_in_person_step_path(step: :address), wait: 10) expect_in_person_step_indicator_current_step(t('step_indicator.flows.idv.verify_info')) end From e9885a4a0c53917e14eb933d7e0dca986b65b7d1 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Mon, 11 Sep 2023 16:55:44 -0400 Subject: [PATCH 05/11] Reduce size of Webpack i18n plugin template boilerplate (#9180) * Reduce size of Webpack i18n plugin template boilerplate changelog: Internal, Performance, Reduce size of translation JavaScript files * Drop semicolon * Golf: Avoid undefined fallback by changing merge order * Update spec fixtures * can't...stop...golfing --- .../rails-i18n-webpack-plugin.js | 3 +-- .../rails-i18n-webpack-plugin.spec.js | 12 ++++++------ .../spec/fixtures/expected1.en.js | 2 +- .../spec/fixtures/expected1.es.js | 2 +- .../spec/fixtures/expected1.fr.js | 2 +- .../spec/fixtures/expected452.en.js | 2 +- .../spec/fixtures/expected452.es.js | 2 +- .../spec/fixtures/expected452.fr.js | 2 +- .../spec/fixtures/expected946.en.js | 2 +- .../spec/fixtures/expected946.es.js | 2 +- .../spec/fixtures/expected946.fr.js | 2 +- 11 files changed, 16 insertions(+), 17 deletions(-) diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.js b/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.js index 975fafbffc0..520f48ab767 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.js @@ -113,8 +113,7 @@ const getKeyDomains = (keys) => uniq(keys.map(getKeyDomain)); class RailsI18nWebpackPlugin extends ExtractKeysWebpackPlugin { /** @type {RailsI18nWebpackPluginOptions} */ static DEFAULT_OPTIONS = { - template: - '!function(){var k,o=%j,l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}()', + template: '_locale_data=Object.assign(%j,this._locale_data)', configPath: path.resolve(process.cwd(), 'config/locales'), defaultLocale: 'en', onMissingString: () => {}, diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.spec.js b/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.spec.js index d46c64c362d..e89b4c9b8c2 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.spec.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/rails-i18n-webpack-plugin.spec.js @@ -161,22 +161,22 @@ describe('RailsI18nWebpackPlugin', () => { ); expect(manifest).to.deep.equal({ - 'actualmain-3b0c232b.en.js': 'actualmain-3b0c232b.en.js', - 'actualmain-a43216c8.es.js': 'actualmain-a43216c8.es.js', + 'actualmain-5b00aabc.en.js': 'actualmain-5b00aabc.en.js', + 'actualmain-941d1f5f.es.js': 'actualmain-941d1f5f.es.js', 'actualmain.js': 'actualmain.js', entrypoints: { main: { assets: { js: [ 'actualmain.js', - 'actualmain-3b0c232b.en.js', - 'actualmain-a43216c8.es.js', - 'actualmain-3b0c232b.fr.js', + 'actualmain-5b00aabc.en.js', + 'actualmain-941d1f5f.es.js', + 'actualmain-5b00aabc.fr.js', ], }, }, }, - 'main.js': 'actualmain-3b0c232b.fr.js', + 'main.js': 'actualmain-5b00aabc.fr.js', }); done(); } catch (error) { diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.en.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.en.js index efa2763e950..e9c79acc6a8 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.en.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.en.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.submit":"Submit","forms.messages":{"one":"One message","other":"%{count} messages"},"forms.key1":"value1-en","forms.key2":"value2-en","item.1":"First","item.2":"Second","item.3":"","forms.button.reset":"Reset"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.submit":"Submit","forms.messages":{"one":"One message","other":"%{count} messages"},"forms.key1":"value1-en","forms.key2":"value2-en","item.1":"First","item.2":"Second","item.3":"","forms.button.reset":"Reset"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.es.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.es.js index 814a80003c2..839199f2238 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.es.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.es.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.submit":"Enviar","forms.messages":{"one":"Un mensaje","other":"%{count} mensajes"},"forms.key1":"value1-es","forms.key2":"value2-es","item.1":"First","item.2":"Second","item.3":"","forms.button.reset":"Reiniciar"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.submit":"Enviar","forms.messages":{"one":"Un mensaje","other":"%{count} mensajes"},"forms.key1":"value1-es","forms.key2":"value2-es","item.1":"First","item.2":"Second","item.3":"","forms.button.reset":"Reiniciar"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.fr.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.fr.js index da9b0654b94..2b9c238a96e 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.fr.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected1.fr.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.submit":"Submit","forms.messages":{"one":"Un message","other":"%{count} messages"},"forms.key1":"value1-fr","forms.key2":"value2-fr","item.1":"Premier","item.2":"Second","item.3":"","forms.button.reset":"Réinitialiser"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.submit":"Submit","forms.messages":{"one":"Un message","other":"%{count} messages"},"forms.key1":"value1-fr","forms.key2":"value2-fr","item.1":"Premier","item.2":"Second","item.3":"","forms.button.reset":"Réinitialiser"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.en.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.en.js index 042002cfc7b..7da96c839e7 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.en.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.en.js @@ -1 +1 @@ -!function(){var k,o={"forms.dynamic":"Dynamic"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.dynamic":"Dynamic"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.es.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.es.js index d831638ab36..9a114ac5633 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.es.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.es.js @@ -1 +1 @@ -!function(){var k,o={"forms.dynamic":"Dinámico"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.dynamic":"Dinámico"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.fr.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.fr.js index 008b99685d7..878d87b2ffd 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.fr.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected452.fr.js @@ -1 +1 @@ -!function(){var k,o={"forms.dynamic":"Dynamique"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.dynamic":"Dynamique"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.en.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.en.js index 5d74f558a55..c2fdf8b71ed 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.en.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.en.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.cancel":"Cancel"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.cancel":"Cancel"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.es.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.es.js index b62d6b49124..b8e17f1742b 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.es.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.es.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.cancel":"Cancelar"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.cancel":"Cancelar"},this._locale_data) \ No newline at end of file diff --git a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.fr.js b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.fr.js index deae187476e..95b04979ff3 100644 --- a/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.fr.js +++ b/app/javascript/packages/rails-i18n-webpack-plugin/spec/fixtures/expected946.fr.js @@ -1 +1 @@ -!function(){var k,o={"forms.button.cancel":"Annuler"},l=window._locale_data=window._locale_data||{};for(k in o)l[k]=o[k]}() \ No newline at end of file +_locale_data=Object.assign({"forms.button.cancel":"Annuler"},this._locale_data) \ No newline at end of file From f595a72f31e0e5b175cc0bf138da993434a09505 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Tue, 12 Sep 2023 09:46:24 -0700 Subject: [PATCH 06/11] Fix typo in SuspendedEmail class (#9191) [skip changelog] --- app/models/suspended_email.rb | 2 +- app/models/user.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/suspended_email.rb b/app/models/suspended_email.rb index 2e0005c4c7e..5a1011bc884 100644 --- a/app/models/suspended_email.rb +++ b/app/models/suspended_email.rb @@ -8,7 +8,7 @@ def generate_email_digest(email) OpenSSL::Digest::SHA256.hexdigest(normalized_email) end - def create_from_email_adddress!(email_address) + def create_from_email_address!(email_address) create!( digested_base_email: generate_email_digest(email_address.email), email_address: email_address, diff --git a/app/models/user.rb b/app/models/user.rb index 4f48edb3853..653b3edd461 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -123,7 +123,7 @@ def suspend! update!(suspended_at: Time.zone.now, unique_session_id: nil) analytics.user_suspended(success: true) email_addresses.map do |email_address| - SuspendedEmail.create_from_email_adddress!(email_address) + SuspendedEmail.create_from_email_address!(email_address) end end From 321d67fd159cd63ad97214b6cab63c57d0f00c75 Mon Sep 17 00:00:00 2001 From: Jonathan Hooper Date: Tue, 12 Sep 2023 15:16:30 -0400 Subject: [PATCH 07/11] LG-10779 Log analytics events from the fraud review/reject script (#9194) We have a script that marks users pending fraud review as passed or rejected. Previously when users are passed or rejected in this way there was not event in events.log to log it. This commit adds logging so we can track these events in cloudwatch. [skip changelog] --- app/services/analytics_events.rb | 44 ++++++++++++++ lib/tasks/review_profile.rake | 68 +++++++++++++++++++--- spec/lib/tasks/review_profile_spec.rb | 82 +++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 8 deletions(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index efd3dc1df3f..de0670f125a 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -529,6 +529,50 @@ def forget_all_browsers_visited track_event('Forget All Browsers Visited') end + # @param [Boolean] success + # @param [Hash] errors + # @param [String] exception + # @param [String] profile_fraud_review_pending_at + # The user was passed by manual fraud review + def fraud_review_passed( + success:, + errors:, + exception:, + profile_fraud_review_pending_at:, + **extra + ) + track_event( + 'Fraud: Profile review passed', + success: success, + errors: errors, + exception: exception, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + **extra, + ) + end + + # @param [Boolean] success + # @param [Hash] errors + # @param [String] exception + # @param [String] profile_fraud_review_pending_at + # The user was rejected by manual fraud review + def fraud_review_rejected( + success:, + errors:, + exception:, + profile_fraud_review_pending_at:, + **extra + ) + track_event( + 'Fraud: Profile review rejected', + success: success, + errors: errors, + exception: exception, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + **extra, + ) + end + # @param [Boolean] success # @param [Boolean] address_edited # @param [Hash] pii_like_keypaths diff --git a/lib/tasks/review_profile.rake b/lib/tasks/review_profile.rake index 581e43cb611..a5d4dd00cc8 100644 --- a/lib/tasks/review_profile.rake +++ b/lib/tasks/review_profile.rake @@ -5,6 +5,11 @@ namespace :users do namespace :review do desc 'Pass a user that has a pending review' task pass: :environment do |_task, args| + user = nil + profile_fraud_review_pending_at = nil + success = false + exception = nil + STDOUT.sync = true STDOUT.print 'Enter the name of the investigator: ' investigator_name = STDIN.gets.chomp @@ -18,17 +23,18 @@ namespace :users do user = User.find_by(uuid: user_uuid) if !user - STDOUT.puts 'Error: Could not find user with that UUID' + error = 'Error: Could not find user with that UUID' next end if !user.fraud_review_pending? - STDOUT.puts 'Error: User does not have a pending fraud review' + error = 'Error: User does not have a pending fraud review' next end if FraudReviewChecker.new(user).fraud_review_eligible? profile = user.fraud_review_pending_profile + profile_fraud_review_pending_at = profile.fraud_review_pending_at profile.activate_after_passing_review if profile.active? @@ -41,17 +47,42 @@ namespace :users do sp_name: nil, ) + success = true STDOUT.puts "User's profile has been activated and the user has been emailed." else - STDOUT.puts "There was an error activating the user's profile. Please try again" + error = "There was an error activating the user's profile. Please try again" end else - STDOUT.puts 'User is past the 30 day review eligibility' + error = 'User is past the 30 day review eligibility' + end + rescue StandardError => e + success = false + exception = e + raise e + ensure + analytics_error_hash = nil + if error.present? + STDOUT.puts error + analytics_error_hash = { message: error } end + + Analytics.new( + user: user || AnonymousUser.new, request: nil, session: {}, sp: nil, + ).fraud_review_passed( + success:, + errors: analytics_error_hash, + exception: exception&.inspect, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + ) end desc 'Reject a user that has a pending review' task reject: :environment do |_task, args| + error = nil + user = nil + profile_fraud_review_pending_at = nil + success = false + STDOUT.sync = true STDOUT.print 'Enter the name of the investigator: ' investigator_name = STDIN.gets.chomp @@ -65,23 +96,44 @@ namespace :users do user = User.find_by(uuid: user_uuid) if !user - STDOUT.puts 'Error: Could not find user with that UUID' + error = 'Error: Could not find user with that UUID' next end if !user.fraud_review_pending? - STDOUT.puts 'Error: User does not have a pending fraud review' + error = 'Error: User does not have a pending fraud review' next end if FraudReviewChecker.new(user).fraud_review_eligible? profile = user.fraud_review_pending_profile - + profile_fraud_review_pending_at = profile.fraud_review_pending_at profile.reject_for_fraud(notify_user: true) + + success = true STDOUT.puts "User's profile has been deactivated due to fraud rejection." else - STDOUT.puts 'User is past the 30 day review eligibility' + error = 'User is past the 30 day review eligibility' end + rescue StandardError => e + success = false + exception = e + raise e + ensure + analytics_error_hash = nil + if error.present? + STDOUT.puts error + analytics_error_hash = { message: error } + end + + Analytics.new( + user: user || AnonymousUser.new, request: nil, session: {}, sp: nil, + ).fraud_review_rejected( + success:, + errors: analytics_error_hash, + exception: exception&.inspect, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + ) end end end diff --git a/spec/lib/tasks/review_profile_spec.rb b/spec/lib/tasks/review_profile_spec.rb index 4f8a9ec9933..95cb1f2c97e 100644 --- a/spec/lib/tasks/review_profile_spec.rb +++ b/spec/lib/tasks/review_profile_spec.rb @@ -5,6 +5,7 @@ let(:user) { create(:user, :fraud_review_pending) } let(:uuid) { user.uuid } let(:task_name) { nil } + let(:analytics) { FakeAnalytics.new } subject(:invoke_task) do Rake::Task[task_name].reenable @@ -22,6 +23,8 @@ uuid, ) stub_const('STDOUT', stdout) + + allow(Analytics).to receive(:new).and_return(analytics) end describe 'users:review:pass' do @@ -43,6 +46,20 @@ end end + it 'logs that the user was passed to analytics' do + profile_fraud_review_pending_at = user.profiles.first.fraud_review_pending_at + + invoke_task + + expect(analytics).to have_logged_event( + 'Fraud: Profile review passed', + success: true, + errors: nil, + exception: nil, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + ) + end + context 'when the user does not exist' do let(:user) { nil } let(:uuid) { 'not-a-real-uuid' } @@ -52,6 +69,18 @@ expect(stdout.string).to include('Error: Could not find user with that UUID') end + + it 'logs the error to analytics' do + invoke_task + + expect(analytics).to have_logged_event( + 'Fraud: Profile review passed', + success: false, + errors: { message: 'Error: Could not find user with that UUID' }, + exception: nil, + profile_fraud_review_pending_at: nil, + ) + end end context 'when the user has cancelled verification' do @@ -62,6 +91,21 @@ expect(user.reload.profiles.first.active).to eq(false) end + + it 'logs an error to analytics' do + profile_fraud_review_pending_at = user.profiles.first.fraud_review_pending_at + user.profiles.first.update!(gpo_verification_pending_at: user.created_at) + + expect { invoke_task }.to raise_error(RuntimeError) + + expect(analytics).to have_logged_event( + 'Fraud: Profile review passed', + success: false, + errors: nil, + exception: a_string_including('Attempting to activate profile with pending reason'), + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + ) + end end end @@ -78,6 +122,20 @@ expect { invoke_task }.to change(ActionMailer::Base.deliveries, :count).by(1) end + it 'logs that the user was rejected to analytics' do + profile_fraud_review_pending_at = user.profiles.first.fraud_review_pending_at + + invoke_task + + expect(analytics).to have_logged_event( + 'Fraud: Profile review rejected', + success: true, + errors: nil, + exception: nil, + profile_fraud_review_pending_at: profile_fraud_review_pending_at, + ) + end + context 'when the user does not exist' do let(:user) { nil } let(:uuid) { 'not-a-real-uuid' } @@ -87,6 +145,18 @@ expect(stdout.string).to include('Error: Could not find user with that UUID') end + + it 'logs the error to analytics' do + invoke_task + + expect(analytics).to have_logged_event( + 'Fraud: Profile review rejected', + success: false, + errors: { message: 'Error: Could not find user with that UUID' }, + exception: nil, + profile_fraud_review_pending_at: nil, + ) + end end context 'when the user profile has a nil fraud_review_pending_at' do @@ -103,6 +173,18 @@ expect(stdout.string).to include('Error: User does not have a pending fraud review') end + + it 'logs the error to analytics' do + invoke_task + + expect(analytics).to have_logged_event( + 'Fraud: Profile review rejected', + success: false, + errors: { message: 'Error: User does not have a pending fraud review' }, + exception: nil, + profile_fraud_review_pending_at: nil, + ) + end end end end From 871eff0b9f96e38e8181fadaf015f91d62051322 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 12 Sep 2023 16:13:19 -0400 Subject: [PATCH 08/11] LG-9821: Remove unused code (#9071) * changelog: Internal, Authentication, LG-9821: Clean up unused code * fix padding * remove unneeded fr * remove unneeded text * fix deletion error * remove phones controller --- app/controllers/users/phones_controller.rb | 78 ------ .../two_factor_authentication_controller.rb | 2 - .../otp_verification/show.html.erb | 2 +- app/views/users/phones/add.html.erb | 51 ---- config/locales/devise/en.yml | 3 - config/locales/devise/es.yml | 4 - config/locales/devise/fr.yml | 5 - config/routes.rb | 2 - .../users/phones_controller_spec.rb | 241 ------------------ spec/views/users/phones/add.html.erb_spec.rb | 23 -- 10 files changed, 1 insertion(+), 410 deletions(-) delete mode 100644 app/controllers/users/phones_controller.rb delete mode 100644 app/views/users/phones/add.html.erb delete mode 100644 spec/controllers/users/phones_controller_spec.rb delete mode 100644 spec/views/users/phones/add.html.erb_spec.rb diff --git a/app/controllers/users/phones_controller.rb b/app/controllers/users/phones_controller.rb deleted file mode 100644 index b5bd2834ef8..00000000000 --- a/app/controllers/users/phones_controller.rb +++ /dev/null @@ -1,78 +0,0 @@ -module Users - class PhonesController < ApplicationController - include PhoneConfirmation - include RecaptchaConcern - include ReauthenticationRequiredConcern - include MfaSetupConcern - - before_action :confirm_user_authenticated_for_2fa_setup - before_action :redirect_if_phone_vendor_outage - before_action :check_max_phone_numbers_per_account, only: %i[add create] - before_action :allow_csp_recaptcha_src, if: :recaptcha_enabled? - before_action :confirm_recently_authenticated_2fa - - helper_method :in_multi_mfa_selection_flow? - - def add - user_session[:phone_id] = nil - @new_phone_form = NewPhoneForm.new(user: current_user, analytics: analytics) - analytics.add_phone_setup_visit - end - - def create - @new_phone_form = NewPhoneForm.new(user: current_user, analytics: analytics) - result = @new_phone_form.submit(user_params) - analytics.multi_factor_auth_phone_setup(**result.to_h) - if result.success? - confirm_phone - elsif recoverable_recaptcha_error?(result) - render 'users/phone_setup/spam_protection' - else - render :add - end - end - - private - - def redirect_if_phone_vendor_outage - return unless OutageStatus.new.all_phone_vendor_outage? - redirect_to vendor_outage_path(from: :users_phones) - end - - def user_params - params.require(:new_phone_form).permit( - :phone, - :international_code, - :otp_delivery_preference, - :otp_make_default_number, - :recaptcha_token, - :recaptcha_version, - :recaptcha_mock_score, - ) - end - - def confirm_phone - flash[:info] = t('devise.registrations.phone_update_needs_confirmation') - prompt_to_confirm_phone( - id: user_session[:phone_id], - phone: @new_phone_form.phone, - selected_delivery_method: @new_phone_form.otp_delivery_preference, - selected_default_number: @new_phone_form.otp_make_default_number, - ) - end - - def check_max_phone_numbers_per_account - max_phones_count = IdentityConfig.store.max_phone_numbers_per_account - return if current_user.phone_configurations.count < max_phones_count - flash[:phone_error] = t('users.phones.error_message') - redirect_path = request.referer.match(account_two_factor_authentication_url) ? - account_two_factor_authentication_url(anchor: 'phones') : - account_url(anchor: 'phones') - redirect_to redirect_path - end - - def recaptcha_enabled? - FeatureManagement.phone_recaptcha_enabled? - end - end -end diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index b451223895f..14f823d11c3 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -206,8 +206,6 @@ def handle_telephony_result(method:, default:, otp_delivery_selection_result:) otp_make_default_number: default, ) elsif @telephony_result.error.is_a?(Telephony::OptOutError) - # clear message from https://github.com/18F/identity-idp/blob/7ad3feab24f6f9e0e45224d9e9be9458c0a6a648/app/controllers/users/phones_controller.rb#L40 - flash.delete(:info) opt_out = PhoneNumberOptOut.mark_opted_out(phone_to_deliver_to) redirect_to login_two_factor_sms_opt_in_path(opt_out_uuid: opt_out) else diff --git a/app/views/two_factor_authentication/otp_verification/show.html.erb b/app/views/two_factor_authentication/otp_verification/show.html.erb index fed58b72d88..bc98b579a25 100644 --- a/app/views/two_factor_authentication/otp_verification/show.html.erb +++ b/app/views/two_factor_authentication/otp_verification/show.html.erb @@ -39,7 +39,7 @@ checked: @presenter.remember_device_box_checked?, }, ) %> - <%= f.submit t('forms.buttons.submit.default'), class: 'display-block margin-y-5' %> + <%= f.submit t('forms.buttons.submit.default'), class: 'display-block margin-top-5 margin-bottom-2' %> <%= hidden_field_tag 'otp_make_default_number', @presenter.otp_make_default_number %> <%= render ButtonComponent.new( diff --git a/app/views/users/phones/add.html.erb b/app/views/users/phones/add.html.erb deleted file mode 100644 index b91627fabe5..00000000000 --- a/app/views/users/phones/add.html.erb +++ /dev/null @@ -1,51 +0,0 @@ -<%= title t('titles.add_info.phone') %> - -<%= render(VendorOutageAlertComponent.new(vendors: [:sms, :voice])) %> - -<%= render PageHeadingComponent.new.with_content(t('headings.add_info.phone')) %> - -

    - <%= t('two_factor_authentication.phone_info') %> -

    - -<%= simple_form_for( - @new_phone_form, - method: :post, - html: { autocomplete: 'off' }, - url: add_phone_path, - ) do |f| %> - - <%= render PhoneInputComponent.new( - form: f, - confirmed_phone: false, - required: true, - captcha_exempt_countries: FeatureManagement.phone_recaptcha_enabled? ? PhoneRecaptchaValidator.exempt_countries : nil, - class: 'margin-bottom-4', - ) %> - - <%= render 'users/shared/otp_delivery_preference_selection', - form_obj: @new_phone_form %> - <% if TwoFactorAuthentication::PhonePolicy.new(current_user).enabled? %> - <%= render 'users/shared/otp_make_default_number', - form_obj: @new_phone_form %> - <% end %> - <%= render CaptchaSubmitButtonComponent.new( - form: f, - action: PhoneRecaptchaValidator::RECAPTCHA_ACTION, - ).with_content(t('forms.buttons.send_one_time_code')) %> -<% end %> - -

    - <%= t('two_factor_authentication.phone_fee_disclosure') %> - <% if IdentityConfig.store.voip_block %> - <%= t('two_factor_authentication.two_factor_choice_options.phone_info_no_voip') %> - <% end %> -

    - -<%= render 'shared/cancel', link: account_path %> - - -<% if FeatureManagement.phone_recaptcha_enabled? %> - <%= render 'shared/recaptcha_disclosure' %> -<% end %> - diff --git a/config/locales/devise/en.yml b/config/locales/devise/en.yml index 905812cf78b..8aaae21b502 100644 --- a/config/locales/devise/en.yml +++ b/config/locales/devise/en.yml @@ -45,9 +45,6 @@ en: registrations: close_window: You can close this window if you’re done. destroyed: Your account has been successfully deleted. - phone_update_needs_confirmation: Your request to update your phone configuration - was processed, but we need to confirm your new number before you can use - it. Please follow the instructions below. start: accordion: You will also need bullet_1_html: An email address. diff --git a/config/locales/devise/es.yml b/config/locales/devise/es.yml index 8d75ecdfb40..b12b9758c5c 100644 --- a/config/locales/devise/es.yml +++ b/config/locales/devise/es.yml @@ -49,10 +49,6 @@ es: registrations: close_window: Puedes cerrar esta ventana si terminaste. destroyed: Su cuenta se ha eliminado exitosamente. - phone_update_needs_confirmation: Su solicitud para actualizar su número de - teléfono fue procesada, pero necesitamos confirmar primero su número - nuevo. Por favor, siga las siguientes instrucciones. Si no confirma su - nuevo número, seguiremos usando su número de teléfono antiguo. start: accordion: También necesitarás bullet_1_html: Una dirección de correo electrónico. diff --git a/config/locales/devise/fr.yml b/config/locales/devise/fr.yml index c8d7d9daaef..1fccfa56f8c 100644 --- a/config/locales/devise/fr.yml +++ b/config/locales/devise/fr.yml @@ -54,11 +54,6 @@ fr: registrations: close_window: Vous pouvez fermer cette fenêtre si vous avez terminé. destroyed: Votre compte a bien été supprimé. - phone_update_needs_confirmation: Votre demande de mise à jour de votre numéro de - téléphone a été traitée, mais nous devons d’abord confirmer votre - nouveau numéro. Veuillez suivre les instructions ci-dessous. Si vous ne - confirmez pas votre nouveau numéro, nous continuerons d’utiliser votre - ancien numéro de téléphone. start: accordion: Vous aurez aussi besoin bullet_1_html: Une adresse e-mail. diff --git a/config/routes.rb b/config/routes.rb index a0f8f899ecf..ad4ff4c4ed3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -237,8 +237,6 @@ get '/manage/email/confirm_delete/:id' => 'users/emails#confirm_delete', as: :manage_email_confirm_delete - get '/add/phone' => 'users/phones#add' - post '/add/phone' => 'users/phones#create' get '/manage/phone/:id' => 'users/edit_phone#edit', as: :manage_phone match '/manage/phone/:id' => 'users/edit_phone#update', via: %i[patch put] delete '/manage/phone/:id' => 'users/edit_phone#destroy' diff --git a/spec/controllers/users/phones_controller_spec.rb b/spec/controllers/users/phones_controller_spec.rb deleted file mode 100644 index 91af6cdbbe2..00000000000 --- a/spec/controllers/users/phones_controller_spec.rb +++ /dev/null @@ -1,241 +0,0 @@ -require 'rails_helper' - -RSpec.describe Users::PhonesController do - let(:user) { create(:user, :fully_registered, with: { phone: '+1 (202) 555-1234' }) } - before do - stub_sign_in(user) - - stub_analytics - allow(@analytics).to receive(:track_event) - end - - context 'user adds phone' do - it 'gives the user a form to enter a new phone number' do - get :add - - expect(response).to render_template(:add) - expect(response.request.flash[:alert]).to be_nil - end - - it 'tracks analytics' do - expect(@analytics).to receive(:track_event). - with('Phone Setup Visited') - get :add - end - end - - context 'user exceeds phone number limit' do - before do - user.phone_configurations.create(encrypted_phone: '4105555551') - user.phone_configurations.create(encrypted_phone: '4105555552') - user.phone_configurations.create(encrypted_phone: '4105555553') - user.phone_configurations.create(encrypted_phone: '4105555554') - end - - it 'displays error if phone number exceeds limit' do - controller.request.headers.merge({ HTTP_REFERER: account_url }) - - get :add - expect(response).to redirect_to(account_url(anchor: 'phones')) - expect(response.request.flash[:phone_error]).to_not be_nil - end - - it 'renders the #phone anchor when it exceeds limit' do - controller.request.headers.merge({ HTTP_REFERER: account_url }) - - get :add - expect(response.location).to include('#phone') - end - - it 'it redirects to two factor auth url if the referer was two factor auth' do - controller.request.headers.merge({ HTTP_REFERER: account_two_factor_authentication_url }) - - get :add - expect(response).to redirect_to(account_two_factor_authentication_url(anchor: 'phones')) - end - - it 'defaults to account url if the url is anything but two factor auth url' do - controller.request.headers.merge({ HTTP_REFERER: add_phone_url }) - - get :add - expect(response).to redirect_to(account_url(anchor: 'phones')) - end - end - - context 'phone vendor outage' do - before do - allow_any_instance_of(OutageStatus).to receive(:all_phone_vendor_outage?).and_return(true) - end - - it 'redirects to outage page' do - get :add - - expect(response).to redirect_to vendor_outage_path(from: :users_phones) - end - end - - describe 'recaptcha csp' do - before { stub_sign_in } - - it 'does not allow recaptcha in the csp' do - expect(subject).not_to receive(:allow_csp_recaptcha_src) - - get :add - end - - context 'recaptcha enabled' do - before do - allow(FeatureManagement).to receive(:phone_recaptcha_enabled?).and_return(true) - end - - it 'allows recaptcha in the csp' do - expect(subject).to receive(:allow_csp_recaptcha_src) - - get :add - end - end - end - - describe '#create' do - context 'with recoverable recaptcha error' do - it 'renders spam protection template' do - stub_sign_in - - allow(controller).to receive(:recoverable_recaptcha_error?) do |result| - result.is_a?(FormResponse) - end - - post :create, params: { new_phone_form: { international_code: 'CA' } } - - expect(response).to render_template('users/phone_setup/spam_protection') - end - end - - context 'invalid number' do - it 'tracks an event when the number is invalid' do - sign_in(user) - - stub_analytics - result = { - success: false, - errors: { - phone: [ - t('errors.messages.improbable_phone'), - t( - 'two_factor_authentication.otp_delivery_preference.voice_unsupported', - location: '', - ), - ], - }, - error_details: { - phone: [ - :improbable_phone, - t( - 'two_factor_authentication.otp_delivery_preference.voice_unsupported', - location: '', - ), - ], - }, - otp_delivery_preference: 'sms', - area_code: nil, - carrier: 'Test Mobile Carrier', - country_code: nil, - phone_type: :mobile, - types: [], - pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]], - } - - expect(@analytics).to receive(:track_event). - with('Multi-Factor Authentication: phone setup', result) - - post :create, params: { - new_phone_form: { - phone: '703-555-010', - international_code: 'US', - }, - } - - expect(response).to render_template(:add) - end - end - - context 'with SMS' do - it 'prompts to confirm the number' do - sign_in(user) - - stub_analytics - - result = { - success: true, - errors: {}, - otp_delivery_preference: 'sms', - area_code: '703', - carrier: 'Test Mobile Carrier', - country_code: 'US', - phone_type: :mobile, - types: [:fixed_or_mobile], - pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]], - } - - expect(@analytics).to receive(:track_event). - with('Multi-Factor Authentication: phone setup', result) - - post( - :create, - params: { - new_phone_form: { phone: '703-555-0100', - international_code: 'US' }, - }, - ) - - expect(response).to redirect_to( - otp_send_path( - otp_delivery_selection_form: { otp_delivery_preference: 'sms', - otp_make_default_number: false }, - ), - ) - - expect(subject.user_session[:context]).to eq 'confirmation' - end - end - - context 'without selection' do - it 'prompts to confirm via SMS by default' do - sign_in(user) - - stub_analytics - result = { - success: true, - errors: {}, - otp_delivery_preference: 'sms', - area_code: '703', - carrier: 'Test Mobile Carrier', - country_code: 'US', - phone_type: :mobile, - types: [:fixed_or_mobile], - pii_like_keypaths: [[:errors, :phone], [:error_details, :phone]], - } - - expect(@analytics).to receive(:track_event). - with('Multi-Factor Authentication: phone setup', result) - - patch( - :create, - params: { - new_phone_form: { phone: '703-555-0100', - international_code: 'US' }, - }, - ) - - expect(response).to redirect_to( - otp_send_path( - otp_delivery_selection_form: { otp_delivery_preference: 'sms', - otp_make_default_number: false }, - ), - ) - - expect(subject.user_session[:context]).to eq 'confirmation' - end - end - end -end diff --git a/spec/views/users/phones/add.html.erb_spec.rb b/spec/views/users/phones/add.html.erb_spec.rb deleted file mode 100644 index 30def5bdbb4..00000000000 --- a/spec/views/users/phones/add.html.erb_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'rails_helper' - -RSpec.describe 'users/phones/add.html.erb' do - include Devise::Test::ControllerHelpers - - subject(:rendered) { render } - - before do - user = build_stubbed(:user) - @new_phone_form = NewPhoneForm.new(user:) - end - - context 'phone vendor outage' do - before do - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).and_return(false) - allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms).and_return(true) - end - - it 'renders alert banner' do - expect(rendered).to have_selector('.usa-alert.usa-alert--error') - end - end -end From e44cfbb4d398baa2c786d36c228be409afb8fb53 Mon Sep 17 00:00:00 2001 From: Sonia Connolly Date: Tue, 12 Sep 2023 15:24:58 -0700 Subject: [PATCH 09/11] LG-10886 remove ssn from flow session (#9182) * Remove ssn from flow_session, after deploying PR to add it to idv_session Changelog: Internal, Identity Verification, move ssn to idv_session, part 2 of 2 * VerifyInfo show templates need access to @ssn separately from @pii now * Get ssn from idv_session for verify_info templates * Update IdvStepConcern#confirm_ssn_step_complete * Set @ssn in #show, not in #pii, to fix :confirm_ssn_step_complete before action * Set idv_session.ssn in in_person verify_info_controller_spec * Add ssn back to pii for proof_resolution job in VerifyInfoConcern * Remove specs for ssn in flow_session --------- Co-authored-by: Gina Yamada --- .../concerns/idv/verify_info_concern.rb | 11 +++--- app/controllers/concerns/idv_step_concern.rb | 2 +- .../concerns/rate_limit_concern.rb | 6 ++-- .../idv/in_person/ssn_controller.rb | 11 +++--- .../idv/in_person/verify_info_controller.rb | 2 +- .../idv/session_errors_controller.rb | 8 ++--- app/controllers/idv/ssn_controller.rb | 9 ++--- app/controllers/idv/verify_info_controller.rb | 2 +- .../concerns/rate_limit_concern_spec.rb | 10 ------ .../idv/in_person/ssn_controller_spec.rb | 33 +---------------- .../in_person/verify_info_controller_spec.rb | 1 + .../idv/session_errors_controller_spec.rb | 26 +------------- spec/controllers/idv/ssn_controller_spec.rb | 35 ++----------------- .../idv/verify_info_controller_spec.rb | 26 +++----------- 14 files changed, 28 insertions(+), 154 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index a4172f60aab..9169af3cf40 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -25,6 +25,7 @@ def shared_update idv_session.vendor_phone_confirmation = false idv_session.user_phone_confirmation = false + pii[:ssn] = idv_session.ssn # Required for proof_resolution job Idv::Agent.new(pii).proof_resolution( document_capture_session, should_proof_state_id: should_use_aamva?(pii), @@ -69,9 +70,8 @@ def resolution_rate_limiter end def ssn_rate_limiter - ssn = idv_session.ssn || pii[:ssn] @ssn_rate_limiter ||= RateLimiter.new( - target: Pii::Fingerprinter.fingerprint(ssn), + target: Pii::Fingerprinter.fingerprint(idv_session.ssn), rate_limit_type: :proof_ssn, ) end @@ -301,19 +301,18 @@ def log_idv_verification_submitted_event(success: false, failure_reason: nil) last_name: pii_from_doc[:last_name], date_of_birth: pii_from_doc[:dob], address: pii_from_doc[:address1], - ssn: idv_session.ssn || pii_from_doc[:ssn], + ssn: idv_session.ssn, failure_reason: failure_reason, ) end def check_ssn - ssn = idv_session.ssn || pii[:ssn] - Idv::SsnForm.new(current_user).submit(ssn: ssn) + Idv::SsnForm.new(current_user).submit(ssn: idv_session.ssn) end def move_applicant_to_idv_session idv_session.applicant = pii - idv_session.applicant[:ssn] ||= idv_session.ssn + idv_session.applicant[:ssn] = idv_session.ssn idv_session.applicant['uuid'] = current_user.uuid delete_pii end diff --git a/app/controllers/concerns/idv_step_concern.rb b/app/controllers/concerns/idv_step_concern.rb index 44ce2834e9d..157b6d8ab8b 100644 --- a/app/controllers/concerns/idv_step_concern.rb +++ b/app/controllers/concerns/idv_step_concern.rb @@ -52,7 +52,7 @@ def flow_path private def confirm_ssn_step_complete - return if pii.present? && (idv_session.ssn.present? || pii[:ssn].present?) + return if pii.present? && idv_session.ssn.present? redirect_to prev_url end diff --git a/app/controllers/concerns/rate_limit_concern.rb b/app/controllers/concerns/rate_limit_concern.rb index d5b3a9ae4e7..8ada2c10913 100644 --- a/app/controllers/concerns/rate_limit_concern.rb +++ b/app/controllers/concerns/rate_limit_concern.rb @@ -64,9 +64,7 @@ def idv_attempter_rate_limited?(rate_limit_type) end def pii_ssn - return unless defined?(flow_session) && defined?(idv_session) && user_session - pii_from_doc_ssn = idv_session&.ssn || flow_session[:pii_from_doc]&.[](:ssn) - return pii_from_doc_ssn if pii_from_doc_ssn - flow_session[:pii_from_user]&.[](:ssn) + return unless defined?(idv_session) && user_session + idv_session&.ssn end end diff --git a/app/controllers/idv/in_person/ssn_controller.rb b/app/controllers/idv/in_person/ssn_controller.rb index 8a717ba6968..6b9dad37590 100644 --- a/app/controllers/idv/in_person/ssn_controller.rb +++ b/app/controllers/idv/in_person/ssn_controller.rb @@ -14,8 +14,7 @@ class SsnController < ApplicationController attr_accessor :error_message def show - incoming_ssn = idv_session.ssn || flow_session.dig(:pii_from_user, :ssn) - @ssn_form = Idv::SsnFormatForm.new(current_user, incoming_ssn) + @ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn) analytics.idv_doc_auth_redo_ssn_submitted(**analytics_arguments) if updating_ssn? analytics.idv_doc_auth_ssn_visited(**analytics_arguments) @@ -28,8 +27,7 @@ def show def update @error_message = nil - incoming_ssn = idv_session.ssn || flow_session.dig(:pii_from_user, :ssn) - @ssn_form = Idv::SsnFormatForm.new(current_user, incoming_ssn) + @ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn) ssn = params.require(:doc_auth).permit(:ssn) form_response = @ssn_form.submit(ssn) @@ -42,7 +40,6 @@ def update ) if form_response.success? - flow_session[:pii_from_user][:ssn] = params[:doc_auth][:ssn] idv_session.ssn = params[:doc_auth][:ssn] idv_session.invalidate_steps_after_ssn! redirect_to idv_in_person_verify_info_url @@ -70,7 +67,7 @@ def flow_path end def confirm_repeat_ssn - return if !idv_session.ssn && !pii_from_user[:ssn] + return if !idv_session.ssn return if request.referer == idv_in_person_verify_info_url redirect_to idv_in_person_verify_info_url end @@ -86,7 +83,7 @@ def analytics_arguments end def updating_ssn? - idv_session.ssn.present? || flow_session.dig(:pii_from_user, :ssn).present? + idv_session.ssn.present? end def confirm_in_person_address_step_complete diff --git a/app/controllers/idv/in_person/verify_info_controller.rb b/app/controllers/idv/in_person/verify_info_controller.rb index e2853047e3c..6ce2fc38a91 100644 --- a/app/controllers/idv/in_person/verify_info_controller.rb +++ b/app/controllers/idv/in_person/verify_info_controller.rb @@ -12,7 +12,7 @@ class VerifyInfoController < ApplicationController def show @step_indicator_steps = step_indicator_steps - @ssn = idv_session.ssn || flow_session[:pii_from_user][:ssn] + @ssn = idv_session.ssn @capture_secondary_id_enabled = capture_secondary_id_enabled analytics.idv_doc_auth_verify_visited(**analytics_arguments) diff --git a/app/controllers/idv/session_errors_controller.rb b/app/controllers/idv/session_errors_controller.rb index 4dd98df5847..d7b9ca201c3 100644 --- a/app/controllers/idv/session_errors_controller.rb +++ b/app/controllers/idv/session_errors_controller.rb @@ -39,9 +39,9 @@ def failure def ssn_failure rate_limiter = nil - if ssn_from_doc + if idv_session&.ssn rate_limiter = RateLimiter.new( - target: Pii::Fingerprinter.fingerprint(ssn_from_doc), + target: Pii::Fingerprinter.fingerprint(idv_session.ssn), rate_limit_type: :proof_ssn, ) @expires_at = rate_limiter.expires_at @@ -59,10 +59,6 @@ def rate_limited private - def ssn_from_doc - idv_session&.ssn || user_session&.dig('idv/doc_auth', :pii_from_doc, :ssn) - end - def confirm_two_factor_authenticated_or_user_id_in_session return if session[:doc_capture_user_id].present? diff --git a/app/controllers/idv/ssn_controller.rb b/app/controllers/idv/ssn_controller.rb index ab41a5b0df5..f0e64000d91 100644 --- a/app/controllers/idv/ssn_controller.rb +++ b/app/controllers/idv/ssn_controller.rb @@ -13,8 +13,7 @@ class SsnController < ApplicationController attr_accessor :error_message def show - incoming_ssn = idv_session.ssn || flow_session.dig(:pii_from_doc, :ssn) - @ssn_form = Idv::SsnFormatForm.new(current_user, incoming_ssn) + @ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn) analytics.idv_doc_auth_redo_ssn_submitted(**analytics_arguments) if @ssn_form.updating_ssn? analytics.idv_doc_auth_ssn_visited(**analytics_arguments) @@ -28,8 +27,7 @@ def show def update @error_message = nil - incoming_ssn = idv_session.ssn || flow_session.dig(:pii_from_doc, :ssn) - @ssn_form = Idv::SsnFormatForm.new(current_user, incoming_ssn) + @ssn_form = Idv::SsnFormatForm.new(current_user, idv_session.ssn) form_response = @ssn_form.submit(params.require(:doc_auth).permit(:ssn)) analytics.idv_doc_auth_ssn_submitted( @@ -40,7 +38,6 @@ def update ) if form_response.success? - flow_session[:pii_from_doc][:ssn] = params[:doc_auth][:ssn] idv_session.ssn = params[:doc_auth][:ssn] idv_session.invalidate_steps_after_ssn! redirect_to next_url @@ -53,7 +50,7 @@ def update private def confirm_repeat_ssn - return if !idv_session.ssn && !pii_from_doc[:ssn] + return if !idv_session.ssn return if request.referer == idv_verify_info_url redirect_to idv_verify_info_url diff --git a/app/controllers/idv/verify_info_controller.rb b/app/controllers/idv/verify_info_controller.rb index 833143b4cb7..cd4f7e1c340 100644 --- a/app/controllers/idv/verify_info_controller.rb +++ b/app/controllers/idv/verify_info_controller.rb @@ -11,7 +11,7 @@ class VerifyInfoController < ApplicationController def show @step_indicator_steps = step_indicator_steps - @ssn = idv_session.ssn || pii_from_doc[:ssn] + @ssn = idv_session.ssn analytics.idv_doc_auth_verify_visited(**analytics_arguments) Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]). diff --git a/spec/controllers/concerns/rate_limit_concern_spec.rb b/spec/controllers/concerns/rate_limit_concern_spec.rb index 25bc2ee6482..f8e3b8b29b7 100644 --- a/spec/controllers/concerns/rate_limit_concern_spec.rb +++ b/spec/controllers/concerns/rate_limit_concern_spec.rb @@ -89,16 +89,6 @@ def update ).increment_to_limited! end - context 'ssn is in flow session' do - it 'redirects to proof_ssn rate limited error page' do - flow_session = { pii_from_doc: { ssn: ssn } } - allow(subject).to receive(:flow_session).and_return(flow_session) - get :show - - expect(response).to redirect_to idv_session_errors_ssn_failure_url - end - end - context 'ssn is in idv_session' do it 'redirects to proof_ssn rate limited error page' do subject.idv_session.ssn = ssn diff --git a/spec/controllers/idv/in_person/ssn_controller_spec.rb b/spec/controllers/idv/in_person/ssn_controller_spec.rb index a5ad306f80b..1bf95e7713f 100644 --- a/spec/controllers/idv/in_person/ssn_controller_spec.rb +++ b/spec/controllers/idv/in_person/ssn_controller_spec.rb @@ -81,31 +81,6 @@ expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }.from(nil) end - context 'with an ssn in flow_session' do - let(:referer) { idv_in_person_step_url(step: :address) } - before do - flow_session[:pii_from_user][:ssn] = ssn - request.env['HTTP_REFERER'] = referer - end - - context 'referer is not verify_info' do - it 'redirects to verify_info' do - get :show - - expect(response).to redirect_to(idv_in_person_verify_info_url) - end - end - - context 'referer is verify_info' do - let(:referer) { idv_in_person_verify_info_url } - it 'does not redirect' do - get :show - - expect(response).to render_template :show - end - end - end - context 'with an ssn in idv_session' do let(:referer) { idv_in_person_step_url(step: :address) } before do @@ -163,12 +138,6 @@ put :update, params: params end - it 'merges ssn into pii session value' do - put :update, params: params - - expect(flow_session[:pii_from_user][:ssn]).to eq(ssn) - end - it 'adds ssn to idv_session' do put :update, params: params @@ -190,7 +159,7 @@ end it 'does not change threatmetrix_session_id when updating ssn' do - flow_session[:pii_from_user][:ssn] = ssn + subject.idv_session.ssn = ssn put :update, params: params session_id = subject.idv_session.threatmetrix_session_id subject.threatmetrix_view_variables diff --git a/spec/controllers/idv/in_person/verify_info_controller_spec.rb b/spec/controllers/idv/in_person/verify_info_controller_spec.rb index 3cd9a7773dd..eeaca608221 100644 --- a/spec/controllers/idv/in_person/verify_info_controller_spec.rb +++ b/spec/controllers/idv/in_person/verify_info_controller_spec.rb @@ -20,6 +20,7 @@ before do allow(subject).to receive(:flow_session).and_return(flow_session) stub_sign_in(user) + subject.idv_session.ssn = Idp::Constants::MOCK_IDV_APPLICANT_SAME_ADDRESS_AS_ID[:ssn] allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end diff --git a/spec/controllers/idv/session_errors_controller_spec.rb b/spec/controllers/idv/session_errors_controller_spec.rb index 82c9da477c2..8fd17a30332 100644 --- a/spec/controllers/idv/session_errors_controller_spec.rb +++ b/spec/controllers/idv/session_errors_controller_spec.rb @@ -285,7 +285,7 @@ rate_limit_type: :proof_ssn, target: Pii::Fingerprinter.fingerprint(ssn), ).increment_to_limited! - controller.user_session['idv/doc_auth'] = { pii_from_doc: { ssn: ssn } } + allow(idv_session).to receive(:ssn).and_return(ssn) end it 'assigns expiration time' do @@ -304,30 +304,6 @@ ) get action end - - context 'with ssn in idv_session' do - before do - controller.user_session['idv/doc_auth'] = {} - allow(idv_session).to receive(:ssn).and_return(ssn) - end - - it 'assigns expiration time' do - get action - - expect(assigns(:expires_at)).not_to eq(Time.zone.now) - end - - it 'logs an event with attempts remaining' do - expect(@analytics).to receive(:track_event).with( - 'IdV: session error visited', - hash_including( - type: 'ssn_failure', - attempts_remaining: 0, - ), - ) - get action - end - end end end diff --git a/spec/controllers/idv/ssn_controller_spec.rb b/spec/controllers/idv/ssn_controller_spec.rb index e64e04f9f0c..d16a33c2bcd 100644 --- a/spec/controllers/idv/ssn_controller_spec.rb +++ b/spec/controllers/idv/ssn_controller_spec.rb @@ -90,31 +90,6 @@ expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }.from(nil) end - context 'with an ssn in flow_session' do - let(:referer) { idv_document_capture_url } - before do - flow_session[:pii_from_doc][:ssn] = ssn - request.env['HTTP_REFERER'] = referer - end - - context 'referer is not verify_info' do - it 'redirects to verify_info' do - get :show - - expect(response).to redirect_to(idv_verify_info_url) - end - end - - context 'referer is verify_info' do - let(:referer) { idv_verify_info_url } - it 'does not redirect' do - get :show - - expect(response).to render_template :show - end - end - end - context 'with an ssn in idv_session' do let(:referer) { idv_document_capture_url } before do @@ -178,12 +153,6 @@ }.merge(ab_test_args) end - it 'merges ssn into pii session value' do - put :update, params: params - - expect(flow_session[:pii_from_doc][:ssn]).to eq(ssn) - end - it 'updates idv_session.ssn' do expect { put :update, params: params }.to change { subject.idv_session.ssn }. from(nil).to(ssn) @@ -199,7 +168,7 @@ end it 'redirects to the verify info controller if a user is updating their SSN' do - flow_session[:pii_from_doc][:ssn] = ssn + subject.idv_session.ssn = ssn flow_session[:pii_from_doc][:state] = 'PR' put :update, params: params @@ -226,7 +195,7 @@ end it 'does not change threatmetrix_session_id when updating ssn' do - flow_session[:pii_from_doc][:ssn] = ssn + subject.idv_session.ssn = ssn put :update, params: params session_id = subject.idv_session.threatmetrix_session_id subject.threatmetrix_view_variables diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index 8e52b8a21a1..97cd1a4f177 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -4,7 +4,7 @@ include IdvHelper let(:flow_session) do - { pii_from_doc: Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN.dup } + { pii_from_doc: Idp::Constants::MOCK_IDV_APPLICANT.dup } end let(:user) { create(:user) } @@ -26,6 +26,7 @@ stub_attempts_tracker stub_idv_steps_before_verify_step(user) subject.idv_session.flow_path = 'standard' + subject.idv_session.ssn = Idp::Constants::MOCK_IDV_APPLICANT_WITH_SSN[:ssn] subject.user_session['idv/doc_auth'] = flow_session allow(subject).to receive(:ab_test_analytics_buckets).and_return(ab_test_args) end @@ -64,7 +65,7 @@ }.merge(ab_test_args) end - it 'renders the show template (with ssn in flow_session)' do + it 'renders the show template' do get :show expect(response).to render_template :show @@ -114,7 +115,6 @@ end it 'redirects to ssn controller when ssn info is missing' do - flow_session[:pii_from_doc][:ssn] = nil subject.idv_session.ssn = nil get :show @@ -122,15 +122,6 @@ expect(response).to redirect_to(idv_ssn_url) end - it 'renders show when ssn is in idv_session' do - subject.idv_session.ssn = flow_session[:pii_from_doc][:ssn] - flow_session[:pii_from_doc][:ssn] = nil - - get :show - - expect(response).to render_template :show - end - context 'when the user is ssn rate limited' do before do RateLimiter.new( @@ -141,16 +132,7 @@ ).increment_to_limited! end - it 'redirects to ssn failure url with ssn in flow session' do - get :show - - expect(response).to redirect_to idv_session_errors_ssn_failure_url - end - - it 'redirects to ssn failure url with ssn in idv_session' do - subject.idv_session.ssn = flow_session[:pii_from_doc][:ssn] - flow_session[:pii_from_doc][:ssn] = nil - + it 'redirects to ssn failure url' do get :show expect(response).to redirect_to idv_session_errors_ssn_failure_url From 2a58948d50b6d4cf2359461a1a1770a628d71bc4 Mon Sep 17 00:00:00 2001 From: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Date: Wed, 13 Sep 2023 08:18:45 -0700 Subject: [PATCH 10/11] Add preview for password reset email with outstanding GPO letter (#9197) [skip changelog] --- spec/mailers/previews/user_mailer_preview.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/mailers/previews/user_mailer_preview.rb b/spec/mailers/previews/user_mailer_preview.rb index c94c93819db..2da146b5174 100644 --- a/spec/mailers/previews/user_mailer_preview.rb +++ b/spec/mailers/previews/user_mailer_preview.rb @@ -32,6 +32,14 @@ def reset_password_instructions ) end + def reset_password_instructions_with_pending_gpo_letter + UserMailer.with( + user: user_with_pending_gpo_letter, email_address: email_address_record, + ).reset_password_instructions( + token: SecureRandom.hex, request_id: SecureRandom.hex, + ) + end + def password_changed UserMailer.with(user: user, email_address: email_address_record). password_changed(disavowal_token: SecureRandom.hex) From 50956f1ac09284c0005ff0fa1ec18577203900d7 Mon Sep 17 00:00:00 2001 From: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Date: Wed, 13 Sep 2023 08:36:01 -0700 Subject: [PATCH 11/11] LG-8681: Replace h4 with h1 in emails (#9167) * LG-8681: Replace h4 with h2 in emails per VPAT findings changelog: User-Facing Improvements, Accessibility, Use h1 tag for main heading in emails * LG-8681: Use H1 tag per design and content guidance * LG-8681: Update email heading styles to better meet desktop LGDS standards * LG-8681: Format CSS * LG-8681: Minor HTML/ERB formatting cleanup --- app/assets/stylesheets/email.css.scss | 10 +++++++--- app/assets/stylesheets/variables/_email.scss | 16 ++++++++-------- app/views/layouts/user_mailer.html.erb | 3 +-- .../user_mailer/in_person_verified.html.erb | 2 +- .../shared/_in_person_ready_to_verify.html.erb | 4 ++-- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/app/assets/stylesheets/email.css.scss b/app/assets/stylesheets/email.css.scss index 20a619cab65..2015a1661d0 100644 --- a/app/assets/stylesheets/email.css.scss +++ b/app/assets/stylesheets/email.css.scss @@ -39,9 +39,13 @@ line-height: 60px; } -h4 { - font-weight: bold; - margin-bottom: 16px; +h1, +h2, +h3, +h4, +h5, +h6 { + line-height: 1.4; } .button.large.expanded table a { diff --git a/app/assets/stylesheets/variables/_email.scss b/app/assets/stylesheets/variables/_email.scss index 134d4856e85..aca2ac2c8f6 100644 --- a/app/assets/stylesheets/variables/_email.scss +++ b/app/assets/stylesheets/variables/_email.scss @@ -66,14 +66,14 @@ $global-line-height: 1.5; $global-font-size: 16px; $body-line-height: $global-line-height; $header-font-family: $body-font-family; -$header-font-weight: $global-font-weight; -$h1-font-size: 34px; -$h2-font-size: 30px; -$h3-font-size: 28px; -$h4-font-size: 24px; -$h5-font-size: 20px; -$h6-font-size: 18px; -$header-margin-bottom: 10px; +$header-font-weight: 700; +$h1-font-size: 28px; +$h2-font-size: 22px; +$h3-font-size: 18px; +$h4-font-size: 16px; +$h5-font-size: 14px; +$h6-font-size: 12px; +$header-margin-bottom: 16px; $paragraph-margin-bottom: 10px; $small-font-size: 80%; $small-font-color: $medium-gray; diff --git a/app/views/layouts/user_mailer.html.erb b/app/views/layouts/user_mailer.html.erb index a4005ae384b..24a1b1d6aff 100644 --- a/app/views/layouts/user_mailer.html.erb +++ b/app/views/layouts/user_mailer.html.erb @@ -73,8 +73,7 @@ <% unless @hide_title %> -

    <%= @header || message.subject %> -

    +

    <%= @header || message.subject %>

    <% end %> <%= yield %> diff --git a/app/views/user_mailer/in_person_verified.html.erb b/app/views/user_mailer/in_person_verified.html.erb index f5df6949ede..e1378c107f5 100644 --- a/app/views/user_mailer/in_person_verified.html.erb +++ b/app/views/user_mailer/in_person_verified.html.erb @@ -5,7 +5,7 @@ alt: '', class: 'float-center padding-bottom-4', ) %> -

    <%= message.subject %>

    +

    <%= message.subject %>

    <%= t('user_mailer.in_person_verified.greeting') %>
    <%= t( diff --git a/app/views/user_mailer/shared/_in_person_ready_to_verify.html.erb b/app/views/user_mailer/shared/_in_person_ready_to_verify.html.erb index 1a2431f284c..ae55ab5aa3b 100644 --- a/app/views/user_mailer/shared/_in_person_ready_to_verify.html.erb +++ b/app/views/user_mailer/shared/_in_person_ready_to_verify.html.erb @@ -9,9 +9,9 @@

    -

    +

    <%= t('in_person_proofing.headings.barcode') %> -

    + <% end %>
    <%= render 'idv/shared/mini_logo', filename: 'logo.png' %>