diff --git a/app/controllers/api/ownership_transfers_controller.rb b/app/controllers/api/ownership_transfers_controller.rb index 5c4775585..0a3e9f839 100644 --- a/app/controllers/api/ownership_transfers_controller.rb +++ b/app/controllers/api/ownership_transfers_controller.rb @@ -28,8 +28,33 @@ def create end end + def accept + resolve!(:completed) + end + + def decline + resolve!(:rejected) + end + private + # Wrapped in a transaction so the row lock below is held across the + # read-and-update, preventing a concurrent accept/decline on the same + # transfer from also finding it pending. + def resolve!(status) + OwnershipTransfer.transaction do + transfer = pending_ownership_transfer + + if transfer.blank? || cannot?(action_name.to_sym, transfer) + head :not_found + elsif transfer.update(status:) + head :ok + else + render json: { error: transfer.errors }, status: :unprocessable_content + end + end + end + def ownership_transfer_params params.expect(ownership_transfer: [:nominated_user_id]) end @@ -42,6 +67,10 @@ def most_recent_ownership_transfer @school.ownership_transfers.order(created_at: :desc).first end + def pending_ownership_transfer + @school.ownership_transfers.lock.pending.first + end + def current_user_is_requester? @ownership_transfer.requested_by_user_id == current_user.id end diff --git a/app/models/ability.rb b/app/models/ability.rb index ac365cee3..8ccd2dd70 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -42,6 +42,9 @@ def define_authenticated_abilities(user) can :read, OwnershipTransfer do |transfer| user.id == transfer.requested_by_user_id || user.id == transfer.nominated_user_id end + can %i[accept decline], OwnershipTransfer do |transfer| + user.id == transfer.nominated_user_id + end end def define_authenticated_non_student_abilities(user) @@ -85,7 +88,7 @@ def define_school_owner_abilities(school:) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) can(%i[read create destroy], :school_teacher) - can(%i[read create], :ownership_transfer) + can(%i[read create accept decline], :ownership_transfer) can(%i[read create create_batch update destroy destroy_batch], :school_student) can(%i[create create_copy], Lesson, school_id: school.id) can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public]) @@ -102,7 +105,7 @@ def define_school_teacher_abilities(user:, school:) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id }, teachers: { teacher_id: user.id } }) can(%i[read], :school_owner) can(%i[read], :school_teacher) - can(:read, :ownership_transfer) + can(%i[read accept decline], :ownership_transfer) can(%i[read create create_batch update], :school_student) can(%i[create update destroy], Lesson) do |lesson| school_teacher_can_manage_lesson?(user:, school:, lesson:) diff --git a/config/routes.rb b/config/routes.rb index ba8945c18..9e7598f08 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -87,7 +87,10 @@ resources :owners, only: %i[index], controller: 'school_owners' resources :teachers, only: %i[index create], controller: 'school_teachers' - resource :ownership_transfer, only: %i[show create], controller: 'ownership_transfers' + resource :ownership_transfer, only: %i[show create], controller: 'ownership_transfers' do + put :accept + put :decline + end resources :students, only: %i[index create update destroy], controller: 'school_students' do post :batch, on: :collection, to: 'school_students#create_batch' delete :batch, on: :collection, to: 'school_students#destroy_batch' diff --git a/spec/features/ownership_transfer/accepting_an_ownership_transfer_spec.rb b/spec/features/ownership_transfer/accepting_an_ownership_transfer_spec.rb new file mode 100644 index 000000000..7e2264936 --- /dev/null +++ b/spec/features/ownership_transfer/accepting_an_ownership_transfer_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Accepting an ownership transfer', type: :request do + include_context 'with a school owner and nominated teacher' + + it 'responds 401 Unauthorized when no token is given' do + put("/api/schools/#{school.id}/ownership_transfer/accept") + expect(response).to have_http_status(:unauthorized) + end + + it 'responds 403 Forbidden when the user is a school-student' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:forbidden) + end + + context 'when the school has never had an ownership transfer' do + before { authenticated_in_hydra_as(nominee) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when there is a pending transfer for the school' do + let!(:ownership_transfer) do + create( + :ownership_transfer, + school:, + nominated_user_id: nominee.id, + requested_by_user_id: owner.id, + email_address: nominee.email + ) + end + + context 'when the current user is the nominee' do + before { authenticated_in_hydra_as(nominee) } + + it 'responds 200 OK' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:ok) + end + + it 'marks the transfer as completed' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(ownership_transfer.reload.status).to eq('completed') + end + end + + context 'when the current user is the school owner who requested the transfer' do + before { authenticated_in_hydra_as(owner) } + + it 'responds 404 Not Found, since only the nominee can accept' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:not_found) + end + + it 'does not change the transfer status' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(ownership_transfer.reload.status).to eq('pending') + end + end + + context 'when the current user is a different teacher at the school' do + let(:other_teacher) { create(:teacher, school:) } + + before { authenticated_in_hydra_as(other_teacher) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when the transfer is no longer pending' do + before do + ownership_transfer.update!(status: :completed) + authenticated_in_hydra_as(nominee) + end + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/accept", headers:) + expect(response).to have_http_status(:not_found) + end + end + end +end diff --git a/spec/features/ownership_transfer/declining_an_ownership_transfer_spec.rb b/spec/features/ownership_transfer/declining_an_ownership_transfer_spec.rb new file mode 100644 index 000000000..aaef60eb1 --- /dev/null +++ b/spec/features/ownership_transfer/declining_an_ownership_transfer_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Declining an ownership transfer', type: :request do + include_context 'with a school owner and nominated teacher' + + it 'responds 401 Unauthorized when no token is given' do + put("/api/schools/#{school.id}/ownership_transfer/decline") + expect(response).to have_http_status(:unauthorized) + end + + it 'responds 403 Forbidden when the user is a school-student' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:forbidden) + end + + context 'when the school has never had an ownership transfer' do + before { authenticated_in_hydra_as(nominee) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when there is a pending transfer for the school' do + let!(:ownership_transfer) do + create( + :ownership_transfer, + school:, + nominated_user_id: nominee.id, + requested_by_user_id: owner.id, + email_address: nominee.email + ) + end + + context 'when the current user is the nominee' do + before { authenticated_in_hydra_as(nominee) } + + it 'responds 200 OK' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:ok) + end + + it 'marks the transfer as rejected' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(ownership_transfer.reload.status).to eq('rejected') + end + end + + context 'when the current user is the school owner who requested the transfer' do + before { authenticated_in_hydra_as(owner) } + + it 'responds 404 Not Found, since only the nominee can decline' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:not_found) + end + + it 'does not change the transfer status' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(ownership_transfer.reload.status).to eq('pending') + end + end + + context 'when the current user is a different teacher at the school' do + let(:other_teacher) { create(:teacher, school:) } + + before { authenticated_in_hydra_as(other_teacher) } + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:not_found) + end + end + + context 'when the transfer is no longer pending' do + before do + ownership_transfer.update!(status: :completed) + authenticated_in_hydra_as(nominee) + end + + it 'responds 404 Not Found' do + put("/api/schools/#{school.id}/ownership_transfer/decline", headers:) + expect(response).to have_http_status(:not_found) + end + end + end +end