MSFT Sync: clarify if owners don't have MS users

Give less confusing error when course owners do not have Microsoft users
associated with them. This has happened locally and been confusing, but
it will also give us a heads up if, for real customers, the way of
linking a Canvas user to Microsoft user is not working how they expect.

refs INTEROP-6556
flag=microsoft_group_enrollments_syncing

Test plan:
- Have a course with teachers but no users which correspond to users
  in our test tenant,
- Run the sync. It should fail with an error saying 'A Microsoft 365
  Group must have owners, and no users to the instructors of the Canvas
  course could be found on the Microsoft side'. This can be seen on the
  MicrosoftGroup in the last_error field.
- Run a full sync with valid teachers just to make sure it still works.

Change-Id: I9a8c4f4421d04ed4c17e541fffb2ce69154c9cdb
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/263263
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Xander Moffatt <xmoffatt@instructure.com>
QA-Review: Xander Moffatt <xmoffatt@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>

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

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)

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

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

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

spec/lib/microsoft_sync/syncer_steps_spec.rb

@@ -184,14 +184,12 @@ 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
-          end
+          expect { subject }.to raise_error do |e|
             expect(e).to be_a(described_class::TenantMissingOrSyncDisabled)
             expect(e).to be_a(MicrosoftSync::StateMachineJob::GracefulCancelErrorMixin)
           end
         end
+      end
 
       context 'when the tenant is not set in the account settings' do
         let(:tenant) { nil }
@@ -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