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.

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
180ada1
bounce from page when there is no request
Feb 20, 2025
c4c9a96
create shared test spec
Feb 24, 2025
8b27d28
update tests
Feb 24, 2025
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
2 changes: 1 addition & 1 deletion app/controllers/state_file/archived_intakes/archived_intake_controller.rb
View file
Open in desktop
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
52 changes: 52 additions & 0 deletions spec/controllers/state_file/archived_intake/archived_intake_controller_spec.rb
View file
Open in desktop
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
Loading