Update transfer_ownership task to use OwnershipTransfer::Create

app/models/ownership_transfer.rb

@@ -5,6 +5,8 @@ class OwnershipTransfer < ApplicationRecord
 
   belongs_to :school
 
+  attr_accessor :skip_request_email
+
   enum :status, {
     pending: 'pending', completed: 'completed', rejected: 'rejected', cancelled: 'cancelled'
   }, default: :pending, validate: true
@@ -18,7 +20,7 @@ class OwnershipTransfer < ApplicationRecord
             on: :create
   validate :nominee_has_the_school_teacher_role_for_the_school
 
-  after_create_commit :send_ownership_transfer_request_email
+  after_create_commit :send_ownership_transfer_request_email, unless: :skip_request_email
   after_update_commit :send_ownership_transfer_cancelled_email,
                       if: -> { saved_change_to_status?(from: 'pending', to: 'cancelled') }
   after_update_commit :send_ownership_transfer_completed_email,

lib/concepts/ownership_transfer/create.rb

@@ -3,9 +3,10 @@
 class OwnershipTransfer
   class Create
     class << self
-      def call(school:, nominated_user_id:, requested_by_user_id:)
+      def call(school:, nominated_user_id:, requested_by_user_id:, skip_request_email: false)
         response = OperationResponse.new
         ownership_transfer = build_ownership_transfer(school:, nominated_user_id:, requested_by_user_id:)
+        ownership_transfer.skip_request_email = skip_request_email
 
         if ownership_transfer.save
           response[:ownership_transfer] = ownership_transfer

lib/tasks/school_management.rake

@@ -3,6 +3,8 @@
 namespace :school_management do
   desc 'Transfer ownership of a school'
   task :transfer_ownership, %i[old_owner_email new_owner_email keep_old_owner_as_teacher] => :environment do |_task, args|
+    keep_old_owner_as_teacher = ActiveModel::Type::Boolean.new.cast(args[:keep_old_owner_as_teacher])
+
     old_owner = UserInfoApiClient.find_user_by_email(args[:old_owner_email])
     new_owner = UserInfoApiClient.find_user_by_email(args[:new_owner_email])
 
@@ -21,12 +23,37 @@ namespace :school_management do
       next
     end
 
-    school = Role.find_by(roles: { user_id: old_owner[:id], role: 'owner' }).school
+    school = Role.find_by(user_id: old_owner[:id], role: 'owner')&.school
+
+    unless school
+      Rails.logger.error("User #{old_owner[:id]} is not the owner of a school")
+      next
+    end
+
+    result = nil
 
     school.transaction do
-      remove_old_owner(school, old_owner[:id], args[:keep_old_owner_as_teacher])
-      assign_roles_to_new_owner(school, new_owner[:id])
+      teacher_role = Role.unscoped.teacher.find_or_initialize_by(school:, user_id: new_owner[:id])
+      teacher_role.update!(archived_at: nil)
+
+      result = OwnershipTransfer::Create.call(
+        school:,
+        nominated_user_id: new_owner[:id],
+        requested_by_user_id: old_owner[:id],
+        skip_request_email: true
+      )
+
+      raise ActiveRecord::Rollback unless result.success?
+    end
 
+    unless result.success?
+      Rails.logger.error("Could not create ownership transfer: #{result[:error].inspect}")
+      next
+    end
+
+    school.transaction do
+      OwnershipTransfer::Accept.call(ownership_transfer: result[:ownership_transfer])
+      school.roles.teacher.find_by(user_id: old_owner[:id])&.archive! unless keep_old_owner_as_teacher
       school.update!(creator_agree_to_ux_contact: false)
     end
 
@@ -35,14 +62,4 @@ namespace :school_management do
     Rails.logger.warn 'Open a bash console on the rpf-profile app: `heroku run bash -a rpf-profile`'
     Rails.logger.warn "Remove the owner safeguarding flag from the old owner: `node profile-cli remove-safeguarding-flag #{args[:old_owner_email]} school:owner`"
   end
-
-  def remove_old_owner(school, user_id, keep_old_owner_as_teacher)
-    school.roles.owner.find_by(user_id: user_id).destroy!
-    school.roles.teacher.find_by(user_id: user_id)&.destroy! unless keep_old_owner_as_teacher
-  end
-
-  def assign_roles_to_new_owner(school, user_id)
-    school.roles.create!(user_id: user_id, role: 'owner')
-    school.roles.find_or_create_by!(user_id: user_id, role: 'teacher')
-  end
 end

spec/lib/tasks/school_management_spec.rb

@@ -8,7 +8,7 @@
     let(:task) { Rake::Task['school_management:transfer_ownership'] }
     let(:old_user_id) { SecureRandom.uuid }
     let(:new_user_id) { SecureRandom.uuid }
-    let(:school) { create(:school) }
+    let(:school) { create(:school, creator_agree_to_ux_contact: true) }
 
     before do
       stub_user_info_api_find_by_email(
@@ -21,6 +21,11 @@
         user: { id: new_user_id, email: 'new_owner@example.com' }
       )
 
+      stub_user_info_api_fetch_by_ids(
+        user_ids: [new_user_id],
+        users: [{ id: new_user_id, email: 'new_owner@example.com' }]
+      )
+
       create(:owner_role, school:, user_id: old_user_id)
       create(:teacher_role, school:, user_id: old_user_id)
     end
@@ -53,43 +58,114 @@
       expect(school.roles.owner.pluck(:user_id)).to include(old_user_id)
     end
 
-    it 'creates owner and teacher roles for the new owner' do
+    it 'exits early if old owner is not the owner of a school' do
+      school.roles.owner.find_by(user_id: old_user_id).destroy!
+
       task.invoke('old_owner@example.com', 'new_owner@example.com')
 
-      owners = school.roles.owner
-      owner_user_ids = owners.map(&:user_id)
-      teachers = school.roles.teacher
-      teacher_user_ids = teachers.map(&:user_id)
+      expect(OwnershipTransfer.count).to eq(0)
+    end
 
-      expect(owner_user_ids).to eq([new_user_id])
-      expect(teacher_user_ids).to include(new_user_id)
+    it 'creates owner and teacher roles for the new owner' do
+      task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+      expect(school.roles.owner.pluck(:user_id)).to eq([new_user_id])
+      expect(school.roles.teacher.pluck(:user_id)).to include(new_user_id)
     end
 
     it 'does not error if new owner already has teacher role' do
       create(:teacher_role, school:, user_id: new_user_id)
+
+      task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+      expect(school.roles.owner.pluck(:user_id)).to eq([new_user_id])
+      expect(school.roles.teacher.pluck(:user_id)).to include(new_user_id)
+    end
+
+    it 'restores an archived teacher role for the new owner' do
+      create(:teacher_role, school:, user_id: new_user_id, archived_at: 1.day.ago)
+
       task.invoke('old_owner@example.com', 'new_owner@example.com')
 
-      owners = school.roles.owner
-      owner_user_ids = owners.map(&:user_id)
-      teachers = school.roles.teacher
-      teacher_user_ids = teachers.map(&:user_id)
+      expect(school.roles.owner.pluck(:user_id)).to eq([new_user_id])
+      expect(Role.unscoped.teacher.where(school:, user_id: new_user_id).sole.archived_at).to be_nil
+    end
+
+    it 'archives the old owner role' do
+      task.invoke('old_owner@example.com', 'new_owner@example.com')
 
-      expect(owner_user_ids).to eq([new_user_id])
-      expect(teacher_user_ids).to include(new_user_id)
+      old_owner_role = Role.unscoped.owner.find_by(school:, user_id: old_user_id)
+      expect(old_owner_role.archived_at).to be_present
+    end
+
+    it 'records a completed ownership transfer' do
+      task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+      expect(school.ownership_transfers.sole).to have_attributes(
+        status: 'completed',
+        nominated_user_id: new_user_id,
+        requested_by_user_id: old_user_id,
+        email_address: 'new_owner@example.com'
+      )
+    end
+
+    it 'does not send the ownership transfer request email' do
+      expect do
+        task.invoke('old_owner@example.com', 'new_owner@example.com')
+      end.not_to have_enqueued_mail(SchoolOwnershipMailer, :request_ownership_transfer)
+    end
+
+    it 'sends the ownership transfer completed email to the new owner' do
+      expect do
+        task.invoke('old_owner@example.com', 'new_owner@example.com')
+      end.to have_enqueued_mail(SchoolOwnershipMailer, :complete_ownership_transfer)
+    end
+
+    it 'sends the ownership transfer completed email to the previous owner' do
+      expect do
+        task.invoke('old_owner@example.com', 'new_owner@example.com')
+      end.to have_enqueued_mail(SchoolOwnershipMailer, :complete_ownership_transfer_previous_owner)
+    end
+
+    it 'resets creator_agree_to_ux_contact on the school' do
+      task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+      expect(school.reload.creator_agree_to_ux_contact).to be(false)
     end
 
     it 'keeps the old owner as a teacher if keep parameter is true' do
       task.invoke('old_owner@example.com', 'new_owner@example.com', 'true')
-      teachers = school.roles.teacher
-      teacher_user_ids = teachers.map(&:user_id)
-      expect(teacher_user_ids).to include(old_user_id)
+
+      expect(school.roles.teacher.pluck(:user_id)).to include(old_user_id)
+    end
+
+    it 'removes the old owner as a teacher if keep parameter is false' do
+      task.invoke('old_owner@example.com', 'new_owner@example.com', 'false')
+
+      expect(school.roles.teacher.pluck(:user_id)).not_to include(old_user_id)
     end
 
     it 'removes the old owner as a teacher by default' do
       task.invoke('old_owner@example.com', 'new_owner@example.com')
-      teachers = school.roles.teacher
-      teacher_user_ids = teachers.map(&:user_id)
-      expect(teacher_user_ids).not_to include(old_user_id)
+
+      expect(school.roles.teacher.pluck(:user_id)).not_to include(old_user_id)
+    end
+
+    context 'when the school already has a pending ownership transfer' do
+      before { create(:ownership_transfer, school:) }
+
+      it 'does not change the owner' do
+        task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+        expect(school.roles.owner.pluck(:user_id)).to include(old_user_id)
+        expect(school.roles.owner.pluck(:user_id)).not_to include(new_user_id)
+      end
+
+      it 'rolls back the teacher role created for the new owner' do
+        task.invoke('old_owner@example.com', 'new_owner@example.com')
+
+        expect(school.roles.teacher.pluck(:user_id)).not_to include(new_user_id)
+      end
     end
   end
 end

spec/models/ownership_transfer_spec.rb

@@ -172,6 +172,14 @@
         SchoolOwnershipMailer, :request_ownership_transfer, params: { ownership_transfer: }
       )
     end
+
+    it 'is not enqueued when skip_request_email is set' do
+      ownership_transfer.skip_request_email = true
+
+      expect do
+        ownership_transfer.save!
+      end.not_to have_enqueued_mail(SchoolOwnershipMailer, :request_ownership_transfer)
+    end
   end
 
   describe 'the cancellation email' do