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
10 changes: 9 additions & 1 deletion app/controllers/concerns/effective_user.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
module EffectiveUser
def effective_user
return current_user if effective_user_id == current_user&.id
return User.find_by(id: effective_user_id) if effective_user_id

user = User.find_by(id: effective_user_id) if effective_user_id

if user.nil?
session.delete(:doc_capture_user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it a little strange we delete this after-the-fact, and as a side-effect of a getter method.

Should we delete this at the time that it becomes invalid? As in your pull request comment: "If the user deletes their account".

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, we considered that at first. But I think keeping it here means that things are slightly less "spread out" and easier to keep track of. Hopefully it will be academic because I am trying to get a ticket to remove effective_user refined and on Team Ada's plate.

return current_user
end

user
end

private
Expand Down
57 changes: 49 additions & 8 deletions spec/controllers/concerns/effective_user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,31 @@
it 'returns nil' do
expect(subject).to be_nil
end

context 'with valid doc capture session user id' do
before do
session[:doc_capture_user_id] = user.id
end

it 'returns session user id' do
expect(subject).to eq user
end
end

context 'with invalid doc capture session user id' do
before do
session[:doc_capture_user_id] = -1
end

it 'returns session user id' do
expect(subject).to be_nil
end

it 'deletes the session key' do
subject
expect(session).not_to include(:doc_capture_user_id)
end
end
end

context 'non-existent user' do
Expand All @@ -22,23 +47,39 @@
end
end

context 'logged out with doc capture session user id' do
context 'logged in' do
before do
session[:doc_capture_user_id] = user.id
stub_sign_in user
end

it 'returns session user id' do
expect(subject).to eq user
end
end

context 'logged in' do
before do
stub_sign_in user
context 'with valid doc capture session user id that is not the logged-in user' do
let(:doc_capture_user) { create(:user) }

before do
session[:doc_capture_user_id] = doc_capture_user.id
end

it 'returns doc capture user id' do
expect(subject).to eq(doc_capture_user)
end
end

it 'returns session user id' do
expect(subject).to eq user
context 'with invalid doc capture session user id' do
before do
session[:doc_capture_user_id] = -1
end

it 'returns logged in user' do
expect(subject).to eql(user)
end

it 'deletes the session key' do
expect { subject }.to(change { session[:doc_capture_user_id] }.to(nil))
end
end
end
end
Expand Down