From 5c5ca14c2a7309c4dd37e156ace79fe906f94321 Mon Sep 17 00:00:00 2001 From: Nathan Richards Date: Fri, 18 Sep 2026 15:56:48 +0200 Subject: [PATCH 1/3] feat: add endpoint for the nominee to accept an ownership transfer PUT /api/schools/:school_id/ownership_transfer/accept marks the school's pending transfer as completed. Only the nominee can accept - neither the requester nor anyone else at the school - and only while a transfer is actually pending; anything else responds 404, matching the same "don't reveal what you can't see" shape the show endpoint already uses rather than distinguishing "no transfer" from "not your transfer". --- .../api/ownership_transfers_controller.rb | 20 ++++ app/models/ability.rb | 7 +- config/routes.rb | 4 +- .../accepting_an_ownership_transfer_spec.rb | 92 +++++++++++++++++++ 4 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 spec/features/ownership_transfer/accepting_an_ownership_transfer_spec.rb diff --git a/app/controllers/api/ownership_transfers_controller.rb b/app/controllers/api/ownership_transfers_controller.rb index 5c4775585..e1f68082b 100644 --- a/app/controllers/api/ownership_transfers_controller.rb +++ b/app/controllers/api/ownership_transfers_controller.rb @@ -28,8 +28,24 @@ def create end end + def accept + resolve!(:completed) + end + private + def resolve!(status) + 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 + def ownership_transfer_params params.expect(ownership_transfer: [:nominated_user_id]) end @@ -42,6 +58,10 @@ def most_recent_ownership_transfer @school.ownership_transfers.order(created_at: :desc).first end + def pending_ownership_transfer + @school.ownership_transfers.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..01f9144c6 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 :accept, 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], :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], :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..e7763b269 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -87,7 +87,9 @@ 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 + 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 From 31b20b09eae743ebed76d0e5385109224e9a8728 Mon Sep 17 00:00:00 2001 From: Nathan Richards Date: Fri, 18 Sep 2026 15:59:05 +0200 Subject: [PATCH 2/3] feat: add endpoint for the nominee to decline an ownership transfer PUT /api/schools/:school_id/ownership_transfer/decline marks the school's pending transfer as rejected. Reuses accept's resolve!/ pending_ownership_transfer helpers - the two actions differ only in which status they set - and the same authorization shape (nominee only, pending only, 404 otherwise). --- .../api/ownership_transfers_controller.rb | 4 + app/models/ability.rb | 6 +- config/routes.rb | 1 + .../declining_an_ownership_transfer_spec.rb | 92 +++++++++++++++++++ 4 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 spec/features/ownership_transfer/declining_an_ownership_transfer_spec.rb diff --git a/app/controllers/api/ownership_transfers_controller.rb b/app/controllers/api/ownership_transfers_controller.rb index e1f68082b..dbfd0fc41 100644 --- a/app/controllers/api/ownership_transfers_controller.rb +++ b/app/controllers/api/ownership_transfers_controller.rb @@ -32,6 +32,10 @@ def accept resolve!(:completed) end + def decline + resolve!(:rejected) + end + private def resolve!(status) diff --git a/app/models/ability.rb b/app/models/ability.rb index 01f9144c6..8ccd2dd70 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -42,7 +42,7 @@ 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 :accept, OwnershipTransfer do |transfer| + can %i[accept decline], OwnershipTransfer do |transfer| user.id == transfer.nominated_user_id end end @@ -88,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 accept], :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]) @@ -105,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(%i[read accept], :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 e7763b269..9e7598f08 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -89,6 +89,7 @@ resources :teachers, only: %i[index create], controller: 'school_teachers' 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' 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 From 609f4505b022d67b1de11451560b81c9616d23f7 Mon Sep 17 00:00:00 2001 From: Nathan Richards Date: Fri, 18 Sep 2026 19:01:35 +0200 Subject: [PATCH 3/3] fix: lock the pending transfer row when accepting or declining --- .../api/ownership_transfers_controller.rb | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/ownership_transfers_controller.rb b/app/controllers/api/ownership_transfers_controller.rb index dbfd0fc41..0a3e9f839 100644 --- a/app/controllers/api/ownership_transfers_controller.rb +++ b/app/controllers/api/ownership_transfers_controller.rb @@ -38,15 +38,20 @@ def decline 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) - 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 + 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 @@ -63,7 +68,7 @@ def most_recent_ownership_transfer end def pending_ownership_transfer - @school.ownership_transfers.pending.first + @school.ownership_transfers.lock.pending.first end def current_user_is_requester?