MSFT partial sync: expect removing last owner

I see a case of the user removing the last owner. This causes a generic
BatchRequestFailed error currently, which will confuse the user, but
more importantly clog up error logs / Sentry with something which is
expected.

We always add new owners before removing old ones in execute_diff so if
the user adds a different owner the sync will succeed.

flag=microsoft_group_enrollments_syncing
refs INTEROP-6970

Test plan:
- have a course with one owner that is synced
- remove last owner in a course. Note: if you have multiple owners and
  remove them in one sync, you will likely not get an error, because the
  MS API is eventually-consistent and we remove all at the same time and
  it won't know that you have removed all owners
- run a partial sync, e.g. g.syncer_job.run_synchronously(:partial)
- note in log output that there is no big message about making a Canvas
  ErrorReport
- check errors in Course Settings -> Integrations. You should see the
  friendly message about the course having no owners
- add a new owner and re-run partial sync
- it should succeed

Change-Id: I3aad866a5f0e11234ae1402fc7ae401ce566e609
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271148
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Sean Scally <sean.scally@instructure.com>
QA-Review: Evan Battaglia <ebattaglia@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>

lib/microsoft_sync/errors.rb

@@ -107,6 +107,16 @@ module MicrosoftSync
       end
     end
 
+    # When we know the team has been created so we know there are really no teachers
+    # (or possibly, in the case of partial sync, some have been manually removed)
+    class MissingOwners < Errors::GracefulCancelError
+      def self.public_message
+        I18n.t '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
+    end
+
     class TeamAlreadyExists < StandardError; end
 
     # Makes public the status code but not anything about the response body.

lib/microsoft_sync/graph_service.rb

@@ -87,6 +87,9 @@ module MicrosoftSync
         group_remove_user_requests(group_id, members, 'members') +
         group_remove_user_requests(group_id, owners, 'owners')
       quota = [reqs.count, reqs.count]
+
+      expected_error = nil
+
       failed_req_ids = run_batch('group_remove_users', reqs, quota: quota) do |resp|
         (
           resp['status'] == 404 && resp['body'].to_s =~
@@ -95,8 +98,16 @@ module MicrosoftSync
           # This variant seems to happen right after removing a user with the UI
           resp['status'] == 400 && resp['body'].to_s =~
             /One or more removed object references do not exist for the following modified/i
+        ) || (
+          # Check for this here so run_batch doesn't increment error counters. Record
+          # the failure in expected_error and raise below.
+          resp['status'] == 400 && resp['body'].to_s =~
+            /must have at least one owner, hence this owner cannot be removed./ &&
+          (expected_error = :missing_owners)
         )
       end
+
+      raise Errors::MissingOwners if expected_error == :missing_owners
       split_request_ids_to_hash(failed_req_ids)
     end
 

lib/microsoft_sync/syncer_steps.rb

@@ -60,14 +60,6 @@ module MicrosoftSync
     STATSD_NAME_SKIPPED_BATCHES = "#{STATSD_NAME}.skipped_batches"
     STATSD_NAME_SKIPPED_TOTAL = "#{STATSD_NAME}.skipped_total"
 
-    class MissingOwners < Errors::GracefulCancelError
-      def self.public_message
-        I18n.t '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
-    end
-
     # Can happen when User disables sync on account-level when jobs are running:
     class TenantMissingOrSyncDisabled < Errors::GracefulCancelError
       def self.public_message
@@ -257,7 +249,7 @@ module MicrosoftSync
     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)
-      raise MissingOwners if diff.local_owners.empty?
+      raise Errors::MissingOwners if diff.local_owners.empty?
 
       raise MaxMemberEnrollmentsReached if diff.max_enrollment_members_reached?
       raise MaxOwnerEnrollmentsReached if diff.max_enrollment_owners_reached?

spec/lib/microsoft_sync/graph_service_spec.rb

@@ -739,6 +739,11 @@ describe MicrosoftSync::GraphService do
       {id: id, status: 400, body: {error: {code:"Request_BadRequest", message: msg}}}
     end
 
+    def last_owner_removed(id)
+      msg = "The group must have at least one owner, hence this owner cannot be removed."
+      {id: id, status: 400, body: {error: {code: "Request_BadRequest", message: msg}}}
+    end
+
     context 'when all are successfully removed' do
       let(:batch_responses) do
         [succ('members_m1'), succ('members_m2'), succ('owners_o1'), succ('owners_o2')]
@@ -793,6 +798,18 @@ describe MicrosoftSync::GraphService do
       end
     end
 
+    context 'when the last owner in a group is removed' do
+      let(:batch_responses) do
+        [succ('members_m1'), succ('members_m2'), last_owner_removed('owners_o1'), succ('owners_o2')]
+      end
+
+      it 'raises an MissingOwners' do
+        expect { subject }.to raise_microsoft_sync_graceful_cancel_error(
+          MicrosoftSync::Errors::MissingOwners, /must have owners/
+        )
+      end
+    end
+
     context 'when more than 20 users are given' do
       it 'raises an ArgumentError' do
         expect {
@@ -807,6 +824,7 @@ describe MicrosoftSync::GraphService do
       let(:endpoint_name) { 'group_remove_users' }
       let(:ignored_members_m1_response) { missing('members_m1') }
     end
+
   end
 
   describe '#team_exists?' do

spec/lib/microsoft_sync/syncer_steps_spec.rb

@@ -296,7 +296,7 @@ describe MicrosoftSync::SyncerSteps do
           expect(MicrosoftSync::GraphServiceHelpers).to_not receive(:new)
           expect(syncer_steps).to_not receive(:ensure_class_group_exists)
           klass = described_class::TenantMissingOrSyncDisabled
-          msg = 
+          msg =
             'Tenant missing or sync disabled. ' \
             'Check the Microsoft sync integration settings for the course and account.'
           expect { subject }.to raise_microsoft_sync_graceful_cancel_error(klass, msg)
@@ -630,7 +630,7 @@ describe MicrosoftSync::SyncerSteps do
     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
-        klass = described_class::MissingOwners
+        klass = MicrosoftSync::Errors::MissingOwners
         msg = /no users corresponding to the instructors of the Canvas course could be found/
         expect { subject }.to raise_microsoft_sync_graceful_cancel_error(klass, msg)
       end