Cancel ownership transfer

app/controllers/api/ownership_transfer_resolutions_controller.rb

@@ -14,26 +14,38 @@ def decline
       resolve!(:rejected)
     end
 
+    def cancel
+      resolve!(:cancelled)
+    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. Authorizing against the loaded
+    # read-and-update, preventing a concurrent accept/decline/cancel on the
+    # same transfer from also finding it pending. Authorizing against the loaded
     # transfer (rather than checking cannot? by hand) means an unauthorized
     # attempt raises and is rescued below into the same 404 a nonexistent
     # transfer gets, instead of leaking that a pending transfer exists.
+    #
+    # head/render happen after the transaction returns, not inside it, so a
+    # commit-time callback failure (e.g. enqueuing a notification) surfaces as
+    # a clean 500 instead of racing an already-performed response.
     def resolve!(status)
-      OwnershipTransfer.transaction do
-        transfer = pending_ownership_transfer
-        return head(:not_found) if transfer.blank?
+      transfer = OwnershipTransfer.transaction do
+        loaded = pending_ownership_transfer
+        next if loaded.blank?
 
-        authorize!(action_name.to_sym, transfer)
+        authorize!(action_name.to_sym, loaded)
+        loaded.update(status:)
+        loaded
+      end
 
-        if transfer.update(status:)
-          head :ok
-        else
-          render json: { error: transfer.errors }, status: :unprocessable_content
-        end
+      if transfer.nil?
+        head :not_found
+      elsif transfer.errors.empty?
+        head :ok
+      else
+        render json: { error: transfer.errors }, status: :unprocessable_content
       end
     rescue CanCan::AccessDenied
       head :not_found

app/controllers/api/ownership_transfers_controller.rb

@@ -11,7 +11,7 @@ def show
 
       if @ownership_transfer.blank? || cannot?(:read, @ownership_transfer)
         head :not_found
-      elsif current_user_is_requester?
+      elsif current_user_is_an_owner?
         render json: { status: @ownership_transfer.status, you_are: 'owner', nominee_name:, nominee_email: }, status: :ok
       else
         render json: { status: @ownership_transfer.status, you_are: 'nominee' }, status: :ok
@@ -42,8 +42,8 @@ def most_recent_ownership_transfer
       @school.ownership_transfers.order(created_at: :desc).first
     end
 
-    def current_user_is_requester?
-      @ownership_transfer.requested_by_user_id == current_user.id
+    def current_user_is_an_owner?
+      current_user.school_owner?(@school)
     end
 
     def nominee_name

app/mailers/school_ownership_mailer.rb

@@ -6,15 +6,25 @@ class SchoolOwnershipMailer < ApplicationMailer
   def request_ownership_transfer
     @school = ownership_transfer.school
 
-    @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name
-    @requested_owner_name = users_by_id[ownership_transfer.requested_by_user_id]&.name
+    @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name.presence || 'there'
+    @requested_owner_name = users_by_id[ownership_transfer.requested_by_user_id]&.name.presence || 'The school owner'
 
     mail(to: ownership_transfer.email_address,
          subject: "You've been nominated to be an owner of #{@school.name}",
          track_opens: 'true',
          message_stream: 'outbound')
   end
 
+  def cancel_ownership_transfer
+    @school = ownership_transfer.school
+    @nominee_name = users_by_id[ownership_transfer.nominated_user_id]&.name.presence || 'there'
+
+    mail(to: ownership_transfer.email_address,
+         subject: "The ownership nomination for #{@school.name} has been cancelled",
+         track_opens: 'true',
+         message_stream: 'outbound')
+  end
+
   private
 
   def ownership_transfer

app/models/ability.rb

@@ -13,7 +13,7 @@ def initialize(user)
     user.schools.active.each do |school|
       define_school_student_abilities(user:, school:) if user.school_student?(school)
       define_school_teacher_abilities(user:, school:) if user.school_teacher?(school)
-      define_school_owner_abilities(user:, school:) if user.school_owner?(school)
+      define_school_owner_abilities(school:) if user.school_owner?(school)
     end
 
     define_editor_admin_abilities(user)
@@ -68,7 +68,7 @@ def define_authenticated_non_student_abilities(user)
     can %i[read update destroy], Component, project: { user_id: user.id }
   end
 
-  def define_school_owner_abilities(user:, school:)
+  def define_school_owner_abilities(school:)
     can(%i[read update], School, id: school.id)
     can(%i[read], :school_member)
     can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id })
@@ -83,7 +83,8 @@ def define_school_owner_abilities(user:, school:)
     can(%i[read create destroy], :school_owner)
     can(%i[read create destroy], :school_teacher)
     can(%i[read create], :ownership_transfer)
-    can(:read, OwnershipTransfer, school_id: school.id, requested_by_user_id: user.id)
+    can(:read, OwnershipTransfer, school_id: school.id)
+    can(:cancel, OwnershipTransfer, school_id: school.id, status: 'pending')
     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])

app/models/ownership_transfer.rb

@@ -19,6 +19,8 @@ class OwnershipTransfer < ApplicationRecord
   validate :nominee_has_the_school_teacher_role_for_the_school
 
   after_create_commit :send_ownership_transfer_request_email
+  after_update_commit :send_ownership_transfer_cancelled_email,
+                      if: -> { saved_change_to_status?(from: 'pending', to: 'cancelled') }
   encrypts :email_address
 
   private
@@ -35,4 +37,8 @@ def nominee_has_the_school_teacher_role_for_the_school
   def send_ownership_transfer_request_email
     SchoolOwnershipMailer.with(ownership_transfer: self).request_ownership_transfer.deliver_later
   end
+
+  def send_ownership_transfer_cancelled_email
+    SchoolOwnershipMailer.with(ownership_transfer: self).cancel_ownership_transfer.deliver_later
+  end
 end

app/views/school_ownership_mailer/cancel_ownership_transfer.text.erb

@@ -0,0 +1,10 @@
+Hi <%= @nominee_name %>,
+
+The nomination for you to become the owner of the Code Classroom account for <%= @school.name %> has been cancelled by the school owner.
+
+No action is needed from you, and ownership remains unchanged. If you were expecting to take over this Code Classroom, please reach out to the school owner directly.
+
+If you have any questions, please contact us at websupport@raspberrypi.org.
+
+Kind Regards,
+The Code Editor team

config/routes.rb

@@ -90,6 +90,7 @@
       resource :ownership_transfer, only: %i[show create], controller: 'ownership_transfers' do
         put :accept, to: 'ownership_transfer_resolutions#accept'
         put :decline, to: 'ownership_transfer_resolutions#decline'
+        put :cancel, to: 'ownership_transfer_resolutions#cancel'
       end
       resources :students, only: %i[index create update destroy], controller: 'school_students' do
         post :batch, on: :collection, to: 'school_students#create_batch'

lib/user_info_api_client.rb

@@ -13,7 +13,7 @@ def fetch_by_ids(user_ids)
         r.url '/users'
         r.body = { userIds: user_ids }
       end
-      return if response.body.blank?
+      return [] if response.body.blank?
 
       transform_result(response.body.fetch('users', []))
     end

spec/features/ownership_transfer/cancelling_an_ownership_transfer_spec.rb

@@ -0,0 +1,141 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Cancelling an ownership transfer', type: :request do
+  include ActionMailer::TestHelper
+
+  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/cancel")
+    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/cancel", headers:)
+    expect(response).to have_http_status(:forbidden)
+  end
+
+  it 'responds 403 Forbidden when the user is the owner of a different school' do
+    other_owner = create(:owner, school: create(:verified_school))
+    authenticated_in_hydra_as(other_owner)
+
+    put("/api/schools/#{school.id}/ownership_transfer/cancel", 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(owner) }
+
+    it 'responds 404 Not Found' do
+      put("/api/schools/#{school.id}/ownership_transfer/cancel", 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 owner who requested the transfer' do
+      before { authenticated_in_hydra_as(owner) }
+
+      it 'responds 200 OK' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'marks the transfer as cancelled' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+        expect(ownership_transfer.reload.status).to eq('cancelled')
+      end
+
+      it 'sends the cancellation email' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+
+        assert_enqueued_email_with(
+          SchoolOwnershipMailer, :cancel_ownership_transfer, params: { ownership_transfer: }
+        )
+      end
+    end
+
+    context 'when the current user is a different owner of the same school' do
+      let(:other_owner) { create(:owner, school:) }
+
+      before { authenticated_in_hydra_as(other_owner) }
+
+      it 'responds 200 OK, since any current owner can cancel, not only the one who requested it' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+        expect(response).to have_http_status(:ok)
+      end
+    end
+
+    context 'when the current user is the nominee' do
+      before { authenticated_in_hydra_as(nominee) }
+
+      it 'responds 404 Not Found, since only an owner of the school can cancel' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", 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/cancel", 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/cancel", 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(owner)
+      end
+
+      it 'responds 404 Not Found' do
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context 'when the transfer has already been cancelled' do
+      before do
+        authenticated_in_hydra_as(owner)
+        put("/api/schools/#{school.id}/ownership_transfer/cancel", headers:)
+      end
+
+      it 'allows the owner to start a new transfer' do
+        other_teacher = create(:teacher, school:)
+        stub_user_info_api_for(other_teacher)
+
+        post(
+          "/api/schools/#{school.id}/ownership_transfer",
+          params: { ownership_transfer: { nominated_user_id: other_teacher.id } },
+          headers:
+        )
+
+        expect(response).to have_http_status(:created)
+      end
+    end
+  end
+end

spec/features/ownership_transfer/viewing_ownership_transfer_status_spec.rb

@@ -78,6 +78,25 @@
       end
     end
 
+    context 'when the current user is a different owner of the same school' do
+      let(:other_owner) { create(:owner, school:) }
+
+      before do
+        stub_user_info_api_for(nominee)
+        authenticated_in_hydra_as(other_owner)
+      end
+
+      it 'responds 200 OK, identifying them as an owner even though they did not request the transfer' do
+        get("/api/schools/#{school.id}/ownership_transfer", headers:)
+
+        expect(response).to have_http_status(:ok)
+        json = JSON.parse(response.body)
+        expect(json).to include(
+          'status' => 'pending', 'you_are' => 'owner', 'nominee_name' => nominee.name, 'nominee_email' => nominee.email
+        )
+      end
+    end
+
     context 'when the current user is the nominee' do
       before { authenticated_in_hydra_as(nominee) }
 
@@ -155,6 +174,22 @@
       end
     end
 
+    context 'when the transfer was cancelled by the owner' do
+      before do
+        ownership_transfer.update!(status: :cancelled)
+        stub_user_info_api_for(nominee)
+        authenticated_in_hydra_as(owner)
+      end
+
+      it 'responds 200 OK, still visible to the owner who cancelled it' do
+        get("/api/schools/#{school.id}/ownership_transfer", headers:)
+
+        expect(response).to have_http_status(:ok)
+        json = JSON.parse(response.body)
+        expect(json).to include('status' => 'cancelled', 'you_are' => 'owner', 'nominee_name' => nominee.name)
+      end
+    end
+
     context 'when a resolved transfer is viewed by someone who was never involved' do
       let(:other_teacher) { create(:teacher, school:) }
 

spec/lib/user_info_api_client_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe UserInfoApiClient do
+  describe '.fetch_by_ids' do
+    subject(:result) { described_class.fetch_by_ids(user_ids) }
+
+    let(:user_ids) { [SecureRandom.uuid] }
+
+    it 'returns an empty Array when the ids are blank' do
+      expect(described_class.fetch_by_ids([])).to eq []
+    end
+
+    context 'when the API responds with a blank body' do
+      before do
+        stub_request(:get, "#{described_class::API_URL}/users")
+          .with(headers: { Authorization: "Bearer #{described_class::API_KEY}" })
+          .to_return(status: 200, body: '')
+      end
+
+      it 'returns an empty Array rather than nil' do
+        expect(result).to eq []
+      end
+    end
+  end
+end