diff --git a/lib/microsoft_sync/errors.rb b/lib/microsoft_sync/errors.rb index ed36288f87d..7fb2d69f2d2 100644 --- a/lib/microsoft_sync/errors.rb +++ b/lib/microsoft_sync/errors.rb @@ -28,8 +28,9 @@ module MicrosoftSync module Errors def self.user_facing_message(error) error_name = error.class.name.underscore.split(%r{[_/]}).map(&:capitalize).join(' ') - case error - when MicrosoftSync::Errors::PublicError + + if error.is_a?(MicrosoftSync::Errors::PublicError) && error.public_message.present? && + error.public_message != error.class.name "#{error_name}: #{error.public_message}" else error_name diff --git a/lib/microsoft_sync/membership_diff.rb b/lib/microsoft_sync/membership_diff.rb index eb2a734a8a0..fc111588148 100644 --- a/lib/microsoft_sync/membership_diff.rb +++ b/lib/microsoft_sync/membership_diff.rb @@ -24,6 +24,8 @@ module MicrosoftSync class MembershipDiff + attr_reader :local_owners + def initialize(remote_members, remote_owners) @remote_members = Set.new(remote_members) @remote_owners = Set.new(remote_owners) diff --git a/lib/microsoft_sync/syncer_steps.rb b/lib/microsoft_sync/syncer_steps.rb index d812b59573c..0c0a485829a 100644 --- a/lib/microsoft_sync/syncer_steps.rb +++ b/lib/microsoft_sync/syncer_steps.rb @@ -36,6 +36,16 @@ module MicrosoftSync ENROLLMENTS_UPN_FETCHING_BATCH_SIZE = 750 STANDARD_RETRY_DELAY = {delay_amount: [5, 20, 100].freeze}.freeze + # SyncCanceled errors are semi-expected errors -- so we raise them they will + # cleanup_after_failure but not produce a failed job. + class SyncCanceled < Errors::PublicError + include StateMachineJob::GracefulCancelErrorMixin + end + + class MissingOwners < SyncCanceled; end + # Can happen when User disables sync on account-level when jobs are running: + class TenantMissingOrSyncDisabled < SyncCanceled; end + attr_reader :group delegate :course, to: :group @@ -64,13 +74,6 @@ module MicrosoftSync group.update!(last_synced_at: Time.zone.now) end - # This is semi-expected (user disables sync on account-level when jobs are - # running), so we raise this which will cleanup_after_failure but not - # produce a failed job. - class TenantMissingOrSyncDisabled < StandardError - include StateMachineJob::GracefulCancelErrorMixin - end - # First step of a full sync. Create group on the Microsoft side. def step_ensure_class_group_exists(_mem_data, _job_state_data) # TODO: as we continue building the job we could possibly just use the @@ -156,6 +159,14 @@ module MicrosoftSync # Run the API calls to add/remove users. def step_execute_diff(diff, _job_state_data) + # TODO: If there are no instructor enrollments, we actually want to + # remove the group on the Microsoft side (INTEROP-6672) + if diff.local_owners.empty? + raise MissingOwners, 'A Microsoft 365 Group must have owners, and no users ' \ + 'corresponding to the instructors of the Canvas course could be found on the ' \ + 'Microsoft side.' + end + batch_size = GraphService::GROUP_USERS_ADD_BATCH_SIZE diff.additions_in_slices_of(batch_size) do |members_and_owners| graph_service.add_users_to_group(group.ms_group_id, members_and_owners) @@ -163,9 +174,7 @@ module MicrosoftSync # Microsoft will not let you remove the last owner in a group, so it's # slightly safer to remove owners last in case we need to completely - # change owners. TODO: A class could still have all of its teacher - # enrollments removed, need to remove the group when this happens - # (INTEROP-6672) + # change owners. diff.members_to_remove.each do |aad| graph_service.remove_group_member(group.ms_group_id, aad) end diff --git a/spec/lib/microsoft_sync/errors_spec.rb b/spec/lib/microsoft_sync/errors_spec.rb index fb52f1fe9ab..1ac54ef541f 100644 --- a/spec/lib/microsoft_sync/errors_spec.rb +++ b/spec/lib/microsoft_sync/errors_spec.rb @@ -28,11 +28,37 @@ describe MicrosoftSync::Errors do end end + class MicrosoftSync::Errors::TestError2 < MicrosoftSync::Errors::PublicError; end + it 'shows the error class name and the public_message' do error = MicrosoftSync::Errors::TestError.new("abc") expect(described_class.user_facing_message(error)).to \ eq("Microsoft Sync Errors Test Error: the public message") end + + context 'when there is no public_message but there is a message' do + it 'shows the error class name and the message' do + error = MicrosoftSync::Errors::TestError2.new('some message') + expect(described_class.user_facing_message(error)).to \ + eq("Microsoft Sync Errors Test Error2: some message") + end + end + + context 'when there is no public message or message' do + it 'shows just the error class name' do + error = MicrosoftSync::Errors::TestError2.new + expect(described_class.user_facing_message(error)).to \ + eq("Microsoft Sync Errors Test Error2") + end + end + + context 'when the public message is just the error class name' do + it 'shows just the error class name' do + error = MicrosoftSync::Errors::TestError2.new('MicrosoftSync::Errors::TestError2') + expect(described_class.user_facing_message(error)).to \ + eq("Microsoft Sync Errors Test Error2") + end + end end context 'with a non-PublicError error' do diff --git a/spec/lib/microsoft_sync/membership_diff_spec.rb b/spec/lib/microsoft_sync/membership_diff_spec.rb index 66edf83d497..ef56d96802e 100644 --- a/spec/lib/microsoft_sync/membership_diff_spec.rb +++ b/spec/lib/microsoft_sync/membership_diff_spec.rb @@ -166,4 +166,13 @@ describe MicrosoftSync::MembershipDiff do eq(%w[teacher1 teacher3]) end end + + describe '#local_owners' do + it 'returns the local owners' do + set_local_members 'teacher', [1], member_enrollment_type + set_local_members 'teacher', [2, 4], owner_enrollment_type + set_local_members 'teacher', [4, 5, 6], owner_enrollment_type + expect(subject.local_owners).to eq(Set.new(%w[teacher2 teacher4 teacher5 teacher6])) + end + end end diff --git a/spec/lib/microsoft_sync/syncer_steps_spec.rb b/spec/lib/microsoft_sync/syncer_steps_spec.rb index 111998dc195..ed55ec6e18a 100644 --- a/spec/lib/microsoft_sync/syncer_steps_spec.rb +++ b/spec/lib/microsoft_sync/syncer_steps_spec.rb @@ -184,12 +184,10 @@ describe MicrosoftSync::SyncerSteps do it 'raises a graceful cleanup error with a end-user-friendly name' do expect(MicrosoftSync::GraphServiceHelpers).to_not receive(:new) expect(syncer_steps).to_not receive(:ensure_class_group_exists) - begin - subject - rescue => e + expect { subject }.to raise_error do |e| + expect(e).to be_a(described_class::TenantMissingOrSyncDisabled) + expect(e).to be_a(MicrosoftSync::StateMachineJob::GracefulCancelErrorMixin) end - expect(e).to be_a(described_class::TenantMissingOrSyncDisabled) - expect(e).to be_a(MicrosoftSync::StateMachineJob::GracefulCancelErrorMixin) end end @@ -392,6 +390,11 @@ describe MicrosoftSync::SyncerSteps do receive(:new).with(%w[m1 m2], %w[o1 o2]).and_return(diff) members_and_enrollment_types = [] expect(diff).to receive(:set_local_member) { |*args| members_and_enrollment_types << args } + allow(diff).to receive(:local_owners) do + members_and_enrollment_types.select do |me| + MicrosoftSync::MembershipDiff::OWNER_ENROLLMENT_TYPES.include?(me.last) + end.map(&:first) + end expect_next_step(subject, :step_execute_diff, diff) expect(members_and_enrollment_types).to eq([['0', 'TeacherEnrollment']]) @@ -427,6 +430,8 @@ describe MicrosoftSync::SyncerSteps do with(MicrosoftSync::GraphService::GROUP_USERS_ADD_BATCH_SIZE). and_yield(owners: %w[o3], members: %w[o1 o2]). and_yield(members: %w[o3]) + + allow(diff).to receive(:local_owners).and_return Set.new(%w[o3]) end it 'adds/removes users based on the diff' do @@ -450,6 +455,19 @@ describe MicrosoftSync::SyncerSteps do ) end end + + context 'when there are no local owners (course teacher enrollments)' do + it 'raises a graceful exit error informing the user' do + expect(diff).to receive(:local_owners).and_return Set.new + expect { subject }.to raise_error do |error| + expect(error).to be_a(MicrosoftSync::Errors::PublicError) + expect(error.public_message).to match( + /no users corresponding to the instructors of the Canvas course could be found/ + ) + expect(error).to be_a(MicrosoftSync::StateMachineJob::GracefulCancelErrorMixin) + end + end + end end describe '#step_check_team_exists' do