diff --git a/app/models/ownership_transfer.rb b/app/models/ownership_transfer.rb index 05b4e00c3..9f581c760 100644 --- a/app/models/ownership_transfer.rb +++ b/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, diff --git a/lib/concepts/ownership_transfer/create.rb b/lib/concepts/ownership_transfer/create.rb index a9463529c..7ee1f4718 100644 --- a/lib/concepts/ownership_transfer/create.rb +++ b/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 diff --git a/lib/tasks/school_management.rake b/lib/tasks/school_management.rake index f298328a3..42c2240a8 100644 --- a/lib/tasks/school_management.rake +++ b/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 diff --git a/spec/lib/tasks/school_management_spec.rb b/spec/lib/tasks/school_management_spec.rb index 08b5e9df1..2307b5f6c 100644 --- a/spec/lib/tasks/school_management_spec.rb +++ b/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 diff --git a/spec/models/ownership_transfer_spec.rb b/spec/models/ownership_transfer_spec.rb index 2cfa90860..9120aa407 100644 --- a/spec/models/ownership_transfer_spec.rb +++ b/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