Skip to content

Improve IdV verify feature specs #6555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 8 commits into from
Jul 8, 2022
Merged

Improve IdV verify feature specs #6555

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4535ce5
Always stub ApplicationController#analytics
aduth Jul 7, 2022
a250f4d
Remove duplicate SSN toggle spec
aduth Jul 7, 2022
54ef218
Remove duplicate resolution failure spec
aduth Jul 7, 2022
82933b9
Call AAMVA assertions consistently and on the correct user
aduth Jul 7, 2022
ad5e4c2
Consolidate content specs
aduth Jul 7, 2022
37c1ff3
Move setup behavior into test cases
aduth Jul 7, 2022
22bce78
Add changelog
aduth Jul 7, 2022
5807a3a
Add missing prep behavior
aduth Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 52 additions & 83 deletions spec/features/idv/doc_auth/verify_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,32 @@
let(:skip_step_completion) { false }
let(:max_attempts) { Throttle.max_attempts(:idv_resolution) }
let(:fake_analytics) { FakeAnalytics.new }
let(:user) { create(:user, :signed_up) }
before do
unless skip_step_completion
sign_in_and_2fa_user(user)
complete_doc_auth_steps_before_verify_step
end
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)
end

it 'is on the correct page' do
it 'displays the expected content' do
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step

expect(page).to have_current_path(idv_doc_auth_verify_step)
expect(page).to have_content(t('headings.verify'))
expect(page).to have_css(
'.step-indicator__step--current',
text: t('step_indicator.flows.idv.verify_info'),
)
end

it 'masks the ssn until toggled visible' do
# SSN is masked until revealed
expect(page).to have_text('9**-**-***4')
expect(page).not_to have_text(DocAuthHelper::GOOD_SSN)

check t('forms.ssn.show')

expect(page).not_to have_text('9**-**-***4')
expect(page).to have_text(DocAuthHelper::GOOD_SSN)
end

it 'proceeds to the next page upon confirmation' do
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)

sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step
click_idv_continue

expect(page).to have_current_path(idv_phone_path)
Expand All @@ -52,6 +48,8 @@
end

it 'proceeds to address page prepopulated with defaults if the user clicks change address' do
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step
click_button t('idv.buttons.change_address_label')

expect(page).to have_current_path(idv_address_path)
Expand All @@ -61,7 +59,8 @@
end

it 'tracks when the user edits their address' do
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step

click_button t('idv.buttons.change_address_label')
fill_out_address_form_ok
Expand All @@ -76,6 +75,9 @@
end

it 'proceeds to the ssn page if the user clicks change ssn and allows user to go back' do
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step

click_button t('idv.buttons.change_ssn_label')

expect(page).to have_current_path(idv_doc_auth_ssn_step)
Expand All @@ -86,8 +88,6 @@
end

it 'does not proceed to the next page if resolution fails' do
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)

sign_in_and_2fa_user
complete_doc_auth_steps_before_ssn_step
fill_out_ssn_form_with_ssn_that_fails_resolution
Expand All @@ -107,8 +107,6 @@
end

it 'does not proceed to the next page if resolution raises an exception' do
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)

sign_in_and_2fa_user
complete_doc_auth_steps_before_ssn_step
fill_out_ssn_form_with_ssn_that_raises_exception
Expand All @@ -128,7 +126,6 @@
end

it 'throttles resolution and continues when it expires' do
allow_any_instance_of(ApplicationController).to receive(:analytics).and_return(fake_analytics)
sign_in_and_2fa_user
complete_doc_auth_steps_before_ssn_step
fill_out_ssn_form_with_ssn_that_fails_resolution
Expand Down Expand Up @@ -160,19 +157,21 @@
allow(IdentityConfig.store).to receive(:aamva_supported_jurisdictions).and_return(
[Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]],
)

sign_in_and_2fa_user
expect_any_instance_of(Idv::Agent).
to receive(:proof_resolution).
with(
anything,
should_proof_state_id: true,
trace_id: anything,
).
and_call_original

user = create(:user, :signed_up)
sign_in_and_2fa_user(user)
complete_doc_auth_steps_before_verify_step
agent = instance_double(Idv::Agent)
allow(Idv::Agent).to receive(:new).and_return(agent)
allow(agent).to receive(:proof_resolution).and_return(
success: true, errors: {}, context: { stages: [] },
)
click_idv_continue

expect(agent).to have_received(:proof_resolution).with(
anything, should_proof_state_id: true, trace_id: anything
)
expect(DocAuthLog.find_by(user_id: user.id).aamva).not_to be_nil
end
end

Expand All @@ -182,19 +181,20 @@
IdentityConfig.store.aamva_supported_jurisdictions -
[Idp::Constants::MOCK_IDV_APPLICANT[:state_id_jurisdiction]],
)

sign_in_and_2fa_user
expect_any_instance_of(Idv::Agent).
to receive(:proof_resolution).
with(
anything,
should_proof_state_id: false,
trace_id: anything,
).
and_call_original

user = create(:user, :signed_up)
sign_in_and_2fa_user(user)
complete_doc_auth_steps_before_verify_step
agent = instance_double(Idv::Agent)
allow(Idv::Agent).to receive(:new).and_return(agent)
allow(agent).to receive(:proof_resolution).and_return(
success: true, errors: {}, context: { stages: [] },
)
click_idv_continue

expect(agent).to have_received(:proof_resolution).with(
anything, should_proof_state_id: false, trace_id: anything
)
expect(DocAuthLog.find_by(user_id: user.id).aamva).to be_nil
end
end
Expand All @@ -203,20 +203,22 @@
it 'does not perform the state ID check' do
allow(IdentityConfig.store).to receive(:aamva_sp_banlist_issuers).
and_return('["urn:gov:gsa:openidconnect:sp:server"]')
expect_any_instance_of(Idv::Agent).
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect_any_instance_of is not my favorite, but given that these are feature specs we don't have much of a choice 😬 and I'd rather have this than stub .new 🤷

Copy link
Copy Markdown
Contributor Author

@aduth aduth Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect_any_instance_of is not my favorite

Curious: Could you elaborate on why you'd avoid it?

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like as is, this is just syntactic sugar over stubbing .new,

In something like a unit test, I'd be able to inject a specific instance to the class we're testing, so I could test that instance, to prevent confusion or test pollution when like, there should maybe be separate Idv::Agent instances or something.

A little testing aphorism I like is "don't stub what you don't own", so if this was a unit test I'd write it like...

step = VerifyStep.new
allow(step).to receive(:idv_agent).and_return(double('Idv::Agent'))

expect(idv_agent).to receive().....
step.call(....)

Copy link
Copy Markdown
Contributor Author

@aduth aduth Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean now, and I see how feature specs in particular may not be particularly well suited for the ideal implementation. Thanks for explaining 👍

to receive(:proof_resolution).
with(
anything,
should_proof_state_id: false,
trace_id: anything,
).
and_call_original

visit_idp_from_sp_with_ial1(:oidc)
sign_in_and_2fa_user
user = create(:user, :signed_up)
sign_in_and_2fa_user(user)
complete_doc_auth_steps_before_verify_step
agent = instance_double(Idv::Agent)
allow(Idv::Agent).to receive(:new).and_return(agent)
allow(agent).to receive(:proof_resolution).and_return(
success: true, errors: {}, context: { stages: [] },
)
Comment on lines -210 to -214
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thank you for removing all these stubs on .new

click_idv_continue

expect(agent).to have_received(:proof_resolution).with(
anything, should_proof_state_id: false, trace_id: anything
)
expect(DocAuthLog.find_by(user_id: user.id).aamva).to be_nil
end
end

Expand All @@ -225,9 +227,6 @@
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step

allow_any_instance_of(ApplicationController).
to receive(:analytics).and_return(fake_analytics)

allow(DocumentCaptureSession).to receive(:find_by).
and_return(nil)

Expand All @@ -241,43 +240,13 @@
end
end

it 'can toggle viewing the ssn' do
expect(page).to have_text('9**-**-***4')
expect(page).not_to have_text(DocAuthHelper::GOOD_SSN)

check t('forms.ssn.show')
expect(page).to have_text(DocAuthHelper::GOOD_SSN)
expect(page).not_to have_text('9**-**-***4')

uncheck t('forms.ssn.show')
expect(page).to have_text('9**-**-***4')
expect(page).not_to have_text(DocAuthHelper::GOOD_SSN)
end

context 'resolution failure' do
let(:skip_step_completion) { true }

it 'does not proceed to the next page' do
sign_in_and_2fa_user
complete_doc_auth_steps_before_ssn_step
fill_out_ssn_form_with_ssn_that_fails_resolution
click_idv_continue
click_idv_continue

expect(page).to have_current_path(idv_session_errors_warning_path)

click_on t('idv.failure.button.warning')

expect(page).to have_current_path(idv_doc_auth_verify_step)
end
end

context 'async timed out' do
it 'allows resubmitting form' do
sign_in_and_2fa_user
complete_doc_auth_steps_before_verify_step

allow(DocumentCaptureSession).to receive(:find_by).
and_return(nil)
allow_any_instance_of(ApplicationController).
to receive(:analytics).and_return(fake_analytics)

click_idv_continue
expect(page).to have_content(t('idv.failure.timeout'))
Expand Down