Skip to content
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.

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

FYST-1809 bounce from page when there is no request #5616

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def check_feature_flag
end

def is_request_locked
if current_request.access_locked? || current_archived_intake&.permanently_locked_at.present?
if current_request.nil? || current_request.access_locked? || current_archived_intake&.permanently_locked_at.present?
redirect_to state_file_archived_intakes_verification_error_path
end
end
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

[pebble] i think we typically use things like .not_to be_redirect instead of expecting it to receive redirect_to? seems a little more precise (because it checks the response as opposed to just checking that redirect_to was called, which it can be without actually redirecting) though i think they serve a very similar purpose. i will give a couple of examples below

Copy link
Contributor

Choose a reason for hiding this comment

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

having said that i just realized that since there's no request being made in this spec, you can't really check the response :/ nvm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah its def a little tricky to test, lmk if you have a better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

but just to elaborate: i think a more typical (and you could argue, more thorough) test would involve testing an action on a controller (in this case, either a child of ArchivedIntakeController or a fake child controller created in the test file) and then having something like this:

it 'redirects to verification error page' do
  get :edit
  expect(response).to redirect_to state_file_archived_intakes_verification_error_path
end

it 'does not redirect' do
  get :edit
  expect(response).not_to be_redirect
end

Copy link
Contributor

Choose a reason for hiding this comment

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

oh i totally missed your comment in the middle there. i was leaving several in a row so probably had to refresh the page. my real suggestion is in the approval comment!

Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,57 @@
end
end
end

describe '#is_request_locked' do
before do
allow(controller).to receive(:current_request).and_return(request_instance)
allow(controller).to receive(:current_archived_intake).and_return(archived_intake)
end

context 'when the request is nil' do
before do
allow(controller).to receive(:current_request).and_return(nil)
end

it 'redirects to verification error page' do
expect(controller).to receive(:redirect_to).with(state_file_archived_intakes_verification_error_path)
controller.is_request_locked
end
end

context 'when the request is locked' do
before do
allow(request_instance).to receive(:access_locked?).and_return(true)
end

it 'redirects to verification error page' do
expect(controller).to receive(:redirect_to).with(state_file_archived_intakes_verification_error_path)
controller.is_request_locked
end
end

context 'when the archived intake is permanently locked' do
before do
allow(archived_intake).to receive(:permanently_locked_at).and_return(Time.current)
end

it 'redirects to verification error page' do
expect(controller).to receive(:redirect_to).with(state_file_archived_intakes_verification_error_path)
controller.is_request_locked
end
end

context 'when the request is valid and not locked' do
before do
allow(request_instance).to receive(:access_locked?).and_return(false)
allow(archived_intake).to receive(:permanently_locked_at).and_return(nil)
end

it 'does not redirect' do
expect(controller).not_to receive(:redirect_to)
controller.is_request_locked
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
before do
Flipper.enable(:get_your_pdf)
end

describe "GET #edit" do
it "renders the edit template with a new EmailAddressForm" do
get :edit
Expand All @@ -23,9 +24,9 @@
end

context "when the form is valid" do
context "and a archived intake exists with the email address" do
context "and an archived intake exists with the email address" do
let!(:archived_intake) { create :state_file_archived_intake, email_address: valid_email_address }
it "creates an access log create a request and redirects to the verification code page" do
it "creates an access log, creates a request, and redirects to the verification code page" do
post :update, params: {
state_file_archived_intakes_email_address_form: { email_address: valid_email_address }
}
Expand All @@ -46,8 +47,8 @@
end
end

context "and a archived does not exist with the email address" do
it "creates an access log create a request and redirects to the verification code page" do
context "and an archived intake does not exist with the email address" do
it "creates an access log, creates a request, and redirects to the verification code page" do
post :update, params: {
state_file_archived_intakes_email_address_form: { email_address: valid_email_address }
}
Expand All @@ -68,6 +69,7 @@
end
end
end

context "when the form is invalid" do
it "renders the edit template" do
post :update, params: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
let(:invalid_ssn) { "212345678" }
let(:intake_request_email) { "[email protected]" }
let!(:archived_intake) { build(:state_file_archived_intake, hashed_ssn: hashed_ssn ) }
let!(:intake_request) do
let!(:current_request) do
create(:state_file_archived_intake_request,
state_file_archived_intake: archived_intake,
ip_address: ip_address,
Expand All @@ -21,32 +21,21 @@
allow(Flipper).to receive(:enabled?).with(:get_your_pdf).and_return(true)
session[:code_verified] = true
allow(controller).to receive(:ip_for_irs).and_return(ip_address)
allow(controller).to receive(:current_request).and_return(current_request)
session[:email_address] = "[email protected]"
end

describe "GET #edit" do
it_behaves_like 'archived intake request locked', action: :edit, method: :get

it "renders the edit template with a new IdentificationNumberForm" do
get :edit

expect(assigns(:form)).to be_a(StateFile::ArchivedIntakes::IdentificationNumberForm)
expect(response).to render_template(:edit)
end

it "redirects to the lockout path when the request is locked" do
intake_request.lock_access!
get :edit

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end

it "redirects to the lockout path when the request is permantly locked" do
archived_intake.update(permanently_locked_at: Time.now)
get :edit

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end

it "redirect to root if code verification was not completed" do
it "redirects to root if code verification was not completed" do
session[:code_verified] = nil
get :edit

Expand All @@ -57,30 +46,30 @@

describe "PATCH #update" do
context "with a valid ssn" do
it "creates an access log and redirects to the root path" do
it "creates an access log and redirects to the mailing address validation page" do
post :update, params: {
state_file_archived_intakes_identification_number_form: { ssn: intake_ssn}
}
expect(assigns(:form)).to be_valid

access_log = StateFileArchivedIntakeAccessLog.last
expect(access_log.state_file_archived_intake_request).to eq(intake_request)
expect(access_log.state_file_archived_intake_request).to eq(current_request)
expect(access_log.event_type).to eq("correct_ssn_challenge")
expect(session[:ssn_verified]).to eq(true)
expect(intake_request.reload.failed_attempts).to eq(0)
expect(current_request.reload.failed_attempts).to eq(0)

expect(response).to redirect_to(state_file_archived_intakes_edit_mailing_address_validation_path)
end

it "resets failed attempts to zero even if one failed attempt has already been made" do
intake_request.update!(failed_attempts: 1)
current_request.update!(failed_attempts: 1)

post :update, params: {
state_file_archived_intakes_identification_number_form: { ssn: intake_ssn}
}

expect(assigns(:form)).to be_valid
expect(intake_request.reload.failed_attempts).to eq(0)
expect(current_request.reload.failed_attempts).to eq(0)
end
end

Expand All @@ -97,12 +86,12 @@
log = StateFileArchivedIntakeAccessLog.last
expect(log.event_type).to eq("incorrect_ssn_challenge")

expect(intake_request.reload.failed_attempts).to eq(1)
expect(current_request.reload.failed_attempts).to eq(1)
expect(response).to render_template(:edit)
end

it "locks the account and redirects to root path after multiple failed attempts" do
intake_request.update!(failed_attempts: 1)
current_request.update!(failed_attempts: 1)

expect {
post :update, params: { state_file_archived_intakes_identification_number_form: { ssn: invalid_ssn } }
Expand All @@ -111,10 +100,10 @@
log = StateFileArchivedIntakeAccessLog.last
expect(log.event_type).to eq("client_lockout_begin")

expect(intake_request.reload.failed_attempts).to eq(2)
expect(intake_request.reload.access_locked?).to be_truthy
expect(current_request.reload.failed_attempts).to eq(2)
expect(current_request.reload.access_locked?).to be_truthy
expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
require "rails_helper"

RSpec.describe StateFile::ArchivedIntakes::MailingAddressValidationController, type: :controller do
let(:intake) { create(:state_file_archived_intake, mailing_state: "NY") }
let(:current_request) { create(:state_file_archived_intake_request, email_address:email_address, failed_attempts: 0, state_file_archived_intake: intake) }
let(:controller_instance) { described_class.new }
let(:archived_intake) { create(:state_file_archived_intake, mailing_state: "NY") }
let(:current_request) { create(:state_file_archived_intake_request, email_address: email_address, failed_attempts: 0, state_file_archived_intake: archived_intake) }
let(:email_address) { "[email protected]" }
let(:valid_verification_code) { "123456" }
let(:invalid_verification_code) { "654321" }
Expand All @@ -17,16 +16,12 @@
end

describe "GET #edit" do
it_behaves_like 'archived intake request locked', action: :edit, method: :get

context "when the request is locked" do
before do
allow(current_request).to receive(:access_locked?).and_return(true)
end

it "redirects to error page" do
get :edit

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end

context "when the request is not locked" do
Expand All @@ -42,7 +37,7 @@
end
end

it "redirect to root if code verification was not completed" do
it "redirects to root if code verification was not completed" do
session[:code_verified] = nil
session[:ssn_verified] = true
get :edit
Expand All @@ -51,7 +46,7 @@
expect(StateFileArchivedIntakeAccessLog.last.event_type).to eq("unauthorized_mailing_attempt")
end

it "redirect to root if ssn verification was not completed" do
it "redirects to root if ssn verification was not completed" do
session[:code_verified] = true
session[:ssn_verified] = nil
get :edit
Expand All @@ -65,7 +60,7 @@
context "with a valid chosen address" do
it "creates an access log and redirects to the download page" do
post :update, params: {
state_file_archived_intakes_mailing_address_validation_form: { selected_address: intake.full_address, addresses: current_request.address_challenge_set}
state_file_archived_intakes_mailing_address_validation_form: { selected_address: archived_intake.full_address, addresses: current_request.address_challenge_set}
}
expect(assigns(:form)).to be_valid

Expand All @@ -79,7 +74,7 @@
end

context "with an invalid chosen address" do
it "creates an access log and redirects to the root path and locks the request" do
it "creates an access log, locks the request, and redirects to the verification error path" do
post :update, params: {
state_file_archived_intakes_mailing_address_validation_form: { selected_address: current_request.fake_address_1, addresses: current_request.address_challenge_set}
}
Expand All @@ -89,15 +84,14 @@
expect(access_log.state_file_archived_intake_request).to eq(current_request)
expect(access_log.event_type).to eq("incorrect_mailing_address")
expect(session[:mailing_verified]).to eq(nil)
expect(intake.permanently_locked_at).to be_present
expect(archived_intake.permanently_locked_at).to be_present
expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end

context "without a chosen address" do
it "creates an access log and redirects to the root path" do
post :update, params: {
}
it "creates an access log and re-renders the edit template" do
post :update, params: {}
expect(assigns(:form)).not_to be_valid

expect(response).to render_template(:edit)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@

RSpec.describe StateFile::ArchivedIntakes::PdfsController, type: :controller do
let(:email_address) { "[email protected]" }
let!(:intake) { create(:state_file_archived_intake, state_code: "NY", mailing_state: "NY", email_address: email_address) }
let(:current_request) { create(:state_file_archived_intake_request, email_address: email_address, failed_attempts: 0, state_file_archived_intake: intake) }
let(:controller_instance) { described_class.new }
let!(:archived_intake) { create(:state_file_archived_intake, state_code: "NY", mailing_state: "NY", email_address: email_address) }
let(:current_request) { create(:state_file_archived_intake_request, email_address: email_address, failed_attempts: 0, state_file_archived_intake: archived_intake) }
let(:valid_verification_code) { "123456" }
let(:invalid_verification_code) { "654321" }

Expand All @@ -19,58 +18,12 @@
end

describe "GET #index" do
context "request is locked" do
before do
allow(current_request).to receive(:access_locked?).and_return(true)
end

it "redirects to error page" do
get :index

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end

context "email address is not verified" do
before do
session[:code_verified] = nil
end

it "redirects to the email verification page" do
get :index

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end

context "ssn is not verified" do
before do
session[:ssn_verified] = nil
end

it "redirects to the ssn verification page" do
get :index

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end

context "mailing address is not verified" do
before do
session[:mailing_verified] = nil
end

it "redirects to the ssn verification page" do
get :index

expect(response).to redirect_to(state_file_archived_intakes_verification_error_path)
end
end
it_behaves_like 'archived intake request locked', action: :index, method: :get

context "by default" do
it "renders" do
get :index
expect(assigns(:state_code)).to eq(intake.state_code)
expect(assigns(:state_code)).to eq(archived_intake.state_code)
expect(response).to render_template(:index)
end
end
Expand All @@ -93,4 +46,4 @@
expect(response).to redirect_to(pdf_url)
end
end
end
end
Loading
Loading