Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions app/controllers/test/ipp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@ module Test
class IppController < ApplicationController
layout 'no_card'

before_action :render_not_found_in_production
before_action :authorize
before_action :confirm_two_factor_authenticated

def index
@enrollments = InPersonEnrollment
.order(created_at: :desc)
.limit(10)
@enrollments = Rails.env.development? ? all_enrollments : enrollments_for_current_user

@enrollments_with_actions = @enrollments.map do |e|
case e.status
Expand All @@ -20,18 +19,44 @@ def index
end

def update
enrollment_id = params['enrollment'].to_i
enrollment = InPersonEnrollment.find(enrollment_id)
enrollment = Rails.env.development? ? enrollment_for_id : enrollment_for_current_user

if enrollment.present?
approve_enrollment(enrollment)
else
flash[:error] = "Could not find pending IPP enrollment with ID #{enrollment_id}"
end

redirect_to test_ipp_url
end

private

def enrollments_for_current_user
InPersonEnrollment
.order(created_at: :desc)
.where(user_id: current_user&.id)
end

def all_enrollments
InPersonEnrollment
.includes(:user)
.order(created_at: :desc)
.limit(10)
end

def enrollment_for_current_user
InPersonEnrollment.find_by(id: enrollment_id, user_id: current_user&.id)
end

def enrollment_for_id
InPersonEnrollment.find_by(id: enrollment_id)
end

def enrollment_id
params['enrollment'].to_i
end

def approve_enrollment(enrollment)
return if !enrollment.pending?

Expand All @@ -52,8 +77,9 @@ def approve_enrollment(enrollment)
job.send(:process_enrollment_response, enrollment, res)
end

def render_not_found_in_production
return unless Rails.env.production?
def authorize
return if FeatureManagement.allow_ipp_enrollment_approval?

render_not_found
end
end
Expand Down
8 changes: 4 additions & 4 deletions app/views/test/ipp/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@

<div class="grid-row grid-gap margin-top-2 flex-align-stretch">
<div class="tablet:grid-col-12">
<% if @enrollments.count == 0 %>
<% if @enrollments.size == 0 %>
<p>
There are no recent in-person enrollments.
</p>
<% else %>
<p>
Listed below
<%= @enrollments.count == 1 ? 'is' : 'are' %>
the <%= @enrollments.count %> most recent
in-person enrollment<%= @enrollments.count == 1 ? '' : 's' %>.
<%= @enrollments.size == 1 ? 'is' : 'are' %>
the <%= @enrollments.size %> most recent
in-person enrollment<%= @enrollments.size == 1 ? '' : 's' %>.
</p>

<table class="bg-white" border="0" cellpadding="0" cellspacing="0">
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ in_person_email_reminder_early_benchmark_in_days: 11
in_person_email_reminder_final_benchmark_in_days: 1
in_person_email_reminder_late_benchmark_in_days: 4
in_person_enrollment_validity_in_days: 30
in_person_enrollments_immediate_approval_enabled: false
in_person_enrollments_ready_job_cron: '0/10 * * * *'
in_person_enrollments_ready_job_email_body_pattern: '\A\s*(?<enrollment_code>\d{16})\s*\Z'
in_person_enrollments_ready_job_enabled: false
Expand Down Expand Up @@ -484,6 +485,7 @@ development:
hmac_fingerprinter_key: a2c813d4dca919340866ba58063e4072adc459b767a74cf2666d5c1eef3861db26708e7437abde1755eb24f4034386b0fea1850a1cb7e56bff8fae3cc6ade96c
hmac_fingerprinter_key_queue: '["11111111111111111111111111111111", "22222222222222222222222222222222"]'
identity_pki_local_dev: true
in_person_enrollments_immediate_approval_enabled: true
in_person_proofing_enabled: true
in_person_proofing_enforce_tmx: true
in_person_proofing_opt_in_enabled: true
Expand Down
7 changes: 7 additions & 0 deletions lib/feature_management.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,11 @@ def self.idv_by_mail_only?
outage_status.any_phone_vendor_outage? ||
outage_status.phone_finder_outage?
end

# This feature allows pending IPP enrollments to be approved immediately, as
# opposed to having to wait close to 2 hours, which is not ideal when testing.
# See test/ipp_controller.rb
def self.allow_ipp_enrollment_approval?
IdentityConfig.store.in_person_enrollments_immediate_approval_enabled
end
end
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ def self.store
config.add(:in_person_email_reminder_final_benchmark_in_days, type: :integer)
config.add(:in_person_email_reminder_late_benchmark_in_days, type: :integer)
config.add(:in_person_enrollment_validity_in_days, type: :integer)
config.add(:in_person_enrollments_immediate_approval_enabled, type: :boolean)
config.add(:in_person_enrollments_ready_job_cron, type: :string)
config.add(:in_person_enrollments_ready_job_email_body_pattern, type: :string)
config.add(:in_person_enrollments_ready_job_enabled, type: :boolean)
Expand Down
124 changes: 124 additions & 0 deletions spec/controllers/test/ipp_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
require 'rails_helper'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend moving setup, such as stubbing, allow statements, and factory bot usage, out of it blocks.


RSpec.describe Test::IppController do
describe 'GET index' do
context 'when allow_ipp_enrollment_approval? is true' do
it 'renders enrollments' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)

stub_sign_in
get :index

expect(response.status).to eq 200
expect(response).to_not be_redirect
expect(subject).to render_template(:index)
end
end

context 'when allow_ipp_enrollment_approval? is false' do
it 'renders 404' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(false)

stub_sign_in
get :index

expect(response.status).to eq 404
expect(response).to render_template('pages/page_not_found')
end
end
end

describe 'PUT update' do
context 'when allow_ipp_enrollment_approval? is true and pending enrollment is found' do
it 'updates the enrollment via a background job' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)
allow(Rails.env).to receive(:development?).and_return(true)

create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
job = instance_double(GetUspsProofingResultsJob)
allow(GetUspsProofingResultsJob).to receive(:new).and_return(job)
allow(job).to receive(:send).and_return(true)

stub_sign_in
put :update, params: { enrollment: InPersonEnrollment.last.id.to_s }

expect(response).to redirect_to test_ipp_url
expect(job).to have_received(:send)
end
end

context 'when allow_ipp_enrollment_approval? is true but enrollment is not found' do
it 'redirects to ipp_test_url with flash error' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)
allow(Rails.env).to receive(:development?).and_return(true)

stub_sign_in
put :update, params: { enrollment: '1' }

expect(response).to redirect_to test_ipp_url
expect(flash[:error]).to eq 'Could not find pending IPP enrollment with ID 1'
end
end

context 'when Rails env is not development and enrollment id belongs to current user' do
it 'updates the enrollment via a background job' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)
allow(Rails.env).to receive(:development?).and_return(false)

user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
job = instance_double(GetUspsProofingResultsJob)
allow(GetUspsProofingResultsJob).to receive(:new).and_return(job)
allow(job).to receive(:send).and_return(true)

stub_sign_in(user)
put :update, params: { enrollment: InPersonEnrollment.last.id.to_s }

expect(response).to redirect_to test_ipp_url
expect(job).to have_received(:send)
end
end

context 'when Rails env is not development and enrollment id does not belong to current user' do
it 'does not update the enrollment' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)
allow(Rails.env).to receive(:development?).and_return(false)

first_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
second_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
job = instance_double(GetUspsProofingResultsJob)
allow(GetUspsProofingResultsJob).to receive(:new).and_return(job)
allow(job).to receive(:send).and_return(true)

enrollment_id_for_first_user = InPersonEnrollment.find_by(user_id: first_user.id).id

stub_sign_in(second_user)
put :update, params: { enrollment: enrollment_id_for_first_user }

expect(response).to redirect_to test_ipp_url
expect(job).not_to have_received(:send)
expect(flash[:error])
.to eq "Could not find pending IPP enrollment with ID #{enrollment_id_for_first_user}"
end
end

context 'when allow_ipp_enrollment_approval? is false' do
it 'renders 404' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(false)

stub_sign_in
put :update

expect(response.status).to eq 404
expect(response).to render_template('pages/page_not_found')
end
end
end
end
42 changes: 42 additions & 0 deletions spec/features/test/approving_pending_ipp_enrollments_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
require 'rails_helper'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend moving setup, such as allow statements and factory bot usage, out of it blocks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you mean putting them in a before block, I disagree with that. I follow the school of thought that everything related to a test should be all within the test. This makes it easier to see exactly what is going on in each test. This is known as the Four-Phase test: setup, exercise, verify, teardown. Although teardown is handled automatically with RSpec.

Here are relevant resources that I agree with:

https://thoughtbot.com/blog/a-journey-towards-better-testing-practices
https://thoughtbot.com/blog/four-phase-test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I realize that the testing practices I advocate for represent one side of the story, so I'd love to learn more about your recommendation to move the setup outside of the it block. Is that a standard practice for this repo that's documented somewhere, or is it more of a personal preference, or perhaps the way you've always done it or seen it done?

Either way, I'd be curious to understand the reasoning behind moving stuff outside the it block, and how that makes the test easier to read and understand, and to maintain.

And I'd be curious what you think of the articles above.


RSpec.describe 'Approving Pending IPP Enrollments' do
context 'when Rails env is not development' do
it 'only shows pending enrollments for the current user' do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)

first_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
second_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)

sign_in_and_2fa_user(first_user)
visit test_ipp_path

expect(page).to have_content(first_user.uuid)
expect(page).not_to have_content(second_user.uuid)
end
end

context 'when Rails env is development' do
it 'shows all pending enrollments', allow_browser_log: true do
allow(FeatureManagement).to receive(:allow_ipp_enrollment_approval?).and_return(true)
allow(Rails.env).to receive(:development?).and_return(true)

first_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)
second_user = create(
:user, :with_phone, :with_pending_in_person_enrollment, password: 'p@assword!'
)

sign_in_and_2fa_user(second_user)
visit test_ipp_path

expect(page).to have_content(first_user.uuid)
expect(page).to have_content(second_user.uuid)
end
end
end
22 changes: 22 additions & 0 deletions spec/lib/feature_management_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -537,4 +537,26 @@
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend moving setup out of it blocks.

end
end

describe 'allow_ipp_enrollment_approval?' do
context 'when IdentityConfig.store.in_person_enrollments_immediate_approval_enabled is true' do
it 'returns true' do
allow(IdentityConfig.store).to receive(:in_person_enrollments_immediate_approval_enabled)
.and_return(true)
allow(Rails.env).to receive(:production?).and_return(true)

expect(FeatureManagement.allow_ipp_enrollment_approval?).to eq true
end
end

context 'when IdentityConfig.store.in_person_enrollments_immediate_approval_enabled is false' do
it 'returns false' do
allow(IdentityConfig.store).to receive(:in_person_enrollments_immediate_approval_enabled)
.and_return(false)
allow(Rails.env).to receive(:production?).and_return(true)

expect(FeatureManagement.allow_ipp_enrollment_approval?).to eq false
end
end
end
end