MSFT Sync Job: create a team for the group

This commit also renames the InvalidStatusCode error and adds subclasses
for cases we semi-expect certain statuses.

The sleep is temporary; I am working right now on a framework to split
the job into steps with retry (and yet, run it synchronously with sleeps
if we want).

refs INTEROP-6571
flag=microsoft_group_enrollments_syncing

Test plan:
- Create a course with no enrollments
- Create the MicrosoftGroup and run the sync:
  g = MicrosoftSync::Group.create!(course: mycourse)
  g.sync!
- Check that the sync runs with no errors. Check in the
  Microsoft admin console that a group was created for the course
- Add at least one teacher/TA/designer to the course
- Sync again. This time the team should be created.

Change-Id: If85ae3f1ad7f24a7e25f1811aca4e35043131326
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/262074
QA-Review: Wagner Goncalves <wagner.goncalves@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>
Reviewed-by: Wagner Goncalves <wagner.goncalves@instructure.com>

lib/microsoft_sync/errors.rb

@@ -43,16 +43,38 @@ module MicrosoftSync
     end
 
     class InvalidRemoteState < PublicError; end
+    class GroupHasNoOwners < PublicError; end
 
     # Makes public the status code but not anything about the response body.
     # The internal error message has the response body (truncated)
-    class InvalidStatusCode < PublicError
+    # Use for() instead of new; this creates a subclass based on the status
+    # code, which allows consumers to select semi-expected cases if they need to.
+    class HTTPInvalidStatus < PublicError
       attr_reader :public_message
+      attr_reader :response_body
+      attr_reader :code
+
+      def self.subclasses_by_status_code
+        @subclasses_by_status_code ||= {
+          400 => HTTPBadRequest,
+          404 => HTTPNotFound
+        }
+      end
+
+      def self.for(service:, response:, tenant:)
+        klass = subclasses_by_status_code[response.code] || self
+        klass.new(service: service, response: response, tenant: tenant)
+      end
 
       def initialize(service:, response:, tenant:)
+        @response_body = response.body
+        @code = response.code
         @public_message = "#{service.capitalize} service returned #{response.code} for tenant #{tenant}"
         super("#{@public_message}, full body: #{response.body.inspect.truncate(1000)}")
       end
     end
+
+    class HTTPNotFound < HTTPInvalidStatus; end
+    class HTTPBadRequest < HTTPInvalidStatus; end
   end
 end

lib/microsoft_sync/graph_service.rb

@@ -97,6 +97,34 @@ module MicrosoftSync
       request(:delete, "groups/#{group_id}/owners/#{user_aad_id}/$ref")
     end
 
+    # === Teams ===
+    def get_team(team_id, options={})
+      request(:get, "teams/#{team_id}", query: expand_options(**options))
+    end
+
+    def team_exists?(team_id)
+      get_team(team_id)
+      true
+    rescue MicrosoftSync::Errors::HTTPNotFound
+      false
+    end
+
+    def create_education_class_team(group_id)
+      body = {
+        "template@odata.bind" =>
+          "https://graph.microsoft.com/v1.0/teamsTemplates('educationClass')",
+        "group@odata.bind" =>
+          "https://graph.microsoft.com/v1.0/groups(#{quote_value(group_id)})"
+      }
+      request(:post, 'teams', body: body)
+    rescue MicrosoftSync::Errors::HTTPBadRequest => e
+      if e.response_body =~ /must have one or more owners in order to create a Team/
+        raise MicrosoftSync::Errors::GroupHasNoOwners
+      end
+
+      raise
+    end
+
     # === Users ===
 
     def list_users(options={}, &blk)
@@ -121,7 +149,7 @@ module MicrosoftSync
       end
 
       unless (200..299).cover?(response.code)
-        raise MicrosoftSync::Errors::InvalidStatusCode.new(
+        raise MicrosoftSync::Errors::HTTPInvalidStatus.for(
           service: 'graph', tenant: tenant, response: response
         )
       end
@@ -160,15 +188,15 @@ module MicrosoftSync
     def filter_clause(filter)
       filter.map do |filter_key, filter_value|
         if filter_value.is_a?(Array)
-          quoted_values = filter_value.map{|v| filter_quote_value(v)}
+          quoted_values = filter_value.map{|v| quote_value(v)}
           "#{filter_key} in (#{quoted_values.join(', ')})"
         else
-          "#{filter_key} eq #{filter_quote_value(filter_value)}"
+          "#{filter_key} eq #{quote_value(filter_value)}"
         end
       end.join(' and ')
     end
 
-    def filter_quote_value(str)
+    def quote_value(str)
       "'#{str.gsub("'", "''")}'"
     end
   end

lib/microsoft_sync/login_service.rb

@@ -56,7 +56,7 @@ module MicrosoftSync
         unless (200..299).cover?(response.code)
           # Probably the key itself is bad. As of 3/2021, it seems like if the tenant
           # hasn't granted permission, we get a token but then a 401 from the Graph API
-          raise MicrosoftSync::Errors::InvalidStatusCode.new(
+          raise MicrosoftSync::Errors::HTTPInvalidStatus.for(
             service: 'login', tenant: tenant, response: response
           )
         end

lib/microsoft_sync/syncer.rb

@@ -39,6 +39,8 @@ module MicrosoftSync
       ensure_enrollments_user_mappings_filled
       diff = generate_diff
       execute_diff(diff)
+      sleep 15
+      ensure_team_exists
 
       group.update_workflow_state_unless_deleted(:completed, last_error: nil)
     rescue => e
@@ -135,6 +137,21 @@ module MicrosoftSync
       end
     end
 
+    def ensure_team_exists
+      return unless course.enrollments.where(type: MembershipDiff::OWNER_ENROLLMENT_TYPES).any?
+      return if graph_service.team_exists?(group.ms_group_id)
+
+      graph_service.create_education_class_team(group.ms_group_id)
+    rescue MicrosoftSync::Errors::GroupHasNoOwners
+      # It's possible (though unlikely) for the course to have owners (as found
+      # by the guard above) but they are not synced to Microsoft yet. Ignore
+      # this case, as we will retry when we sync again.
+      # It may also be possible for the group to have owners on the MS side but
+      # it isn't consistent yet, although I haven't seen that behavior. When
+      # implementing retry or a team_exists on the field, we can change it to
+      # retry once here.
+    end
+
     def tenant
       @tenant ||= group.root_account.settings[:microsoft_sync_tenant]
     end

spec/lib/microsoft_sync/errors_spec.rb

@@ -43,21 +43,22 @@ describe MicrosoftSync::Errors do
     end
   end
 
-  describe described_class::InvalidStatusCode do
+  describe described_class::HTTPInvalidStatus do
     subject do
-      described_class.new(
-        service: 'my api', response: double(code: 404, body: body), tenant: 'mytenant'
+      described_class.for(
+        service: 'my api', response: double(code: code, body: body), tenant: 'mytenant'
       )
     end
 
+    let(:code) { 422 }
     let(:body) { 'abc' }
 
     it 'gives a public message with the service name, status code, and tenant' do
-      expect(subject.public_message).to eq('My api service returned 404 for tenant mytenant')
+      expect(subject.public_message).to eq('My api service returned 422 for tenant mytenant')
     end
 
     it 'gives an internal message with the public message plus full response body' do
-      expect(subject.message).to eq('My api service returned 404 for tenant mytenant, full body: "abc"')
+      expect(subject.message).to eq('My api service returned 422 for tenant mytenant, full body: "abc"')
     end
 
     context 'when the body is very long' do
@@ -74,7 +75,22 @@ describe MicrosoftSync::Errors do
 
       it 'gives a message showing a nil body' do
         expect(subject.message).to \
-          eq('My api service returned 404 for tenant mytenant, full body: nil')
+          eq('My api service returned 422 for tenant mytenant, full body: nil')
+      end
+    end
+
+    describe '.for' do
+      {
+        400 => MicrosoftSync::Errors::HTTPBadRequest,
+        404 => MicrosoftSync::Errors::HTTPNotFound
+      }.each do |status_code, error_class|
+        context "when the response status code is #{status_code}" do
+          let(:code) { status_code }
+
+          it "returns a #{error_class}" do
+            expect(subject).to be_a(error_class)
+          end
+        end
       end
     end
   end

spec/lib/microsoft_sync/graph_service_spec.rb

@@ -45,13 +45,26 @@ describe MicrosoftSync::GraphService do
 
   # http_method, url, with_params, and reponse_body will be defined with let()s below
 
-  shared_examples_for 'a graph service endpoint' do
+  shared_examples_for 'a graph service endpoint' do |opts={}|
+    unless opts[:ignore_404]
+      context 'with a 404 status code' do
+        let(:response) { json_response(404, error: {message: 'uh-oh!'}) }
+
+        it 'raises an HTTPNotFound error' do
+          expect { subject }.to raise_error(
+            MicrosoftSync::Errors::HTTPNotFound,
+            /Graph service returned 404 for tenant mytenant.*uh-oh!/
+          )
+        end
+      end
+    end
+
     context 'with a non-200 status code' do
       let(:response) { json_response(403, error: {message: 'uh-oh!'}) }
 
-      it 'raises an InvalidStatusCodewith the code and message' do
+      it 'raises an HTTPInvalidStatus with the code and message' do
         expect { subject }.to raise_error(
-          MicrosoftSync::Errors::InvalidStatusCode,
+          MicrosoftSync::Errors::HTTPInvalidStatus,
           /Graph service returned 403 for tenant mytenant.*uh-oh!/
         )
       end
@@ -290,14 +303,6 @@ describe MicrosoftSync::GraphService do
     it_behaves_like 'a paginated list endpoint'
   end
 
-  describe '#list_users' do
-    let(:url) { 'https://graph.microsoft.com/v1.0/users' }
-    let(:method_name) { :list_users }
-    let(:method_args) { [] }
-
-    it_behaves_like 'a paginated list endpoint'
-  end
-
   describe '#remove_group_member' do
     subject { service.remove_group_member('mygroup', 'myuser') }
 
@@ -321,4 +326,82 @@ describe MicrosoftSync::GraphService do
 
     it_behaves_like 'a graph service endpoint'
   end
+
+  describe '#get_team' do
+    subject { service.get_team('mygroupid') }
+
+    let(:http_method) { :get }
+    let(:url) { 'https://graph.microsoft.com/v1.0/teams/mygroupid' }
+    let(:response_body) { {'foo' => 'bar'} }
+
+    it { is_expected.to eq('foo' => 'bar') }
+
+    it_behaves_like 'a graph service endpoint'
+  end
+
+  describe '#team_exists?' do
+    subject { service.team_exists?('mygroupid') }
+
+    let(:http_method) { :get }
+    let(:url) { 'https://graph.microsoft.com/v1.0/teams/mygroupid' }
+
+    context 'when the team exists' do
+      let(:response_body) { {'foo' => 'bar'} }
+
+      it { is_expected.to eq(true) }
+
+      it_behaves_like 'a graph service endpoint', ignore_404: true
+    end
+
+    context "when the team doesn't exist" do
+      let(:response) { json_response(404, error: {code: 'NotFound', message: 'Does not exist'}) }
+
+      it { is_expected.to eq(false) }
+
+      it_behaves_like 'a graph service endpoint', ignore_404: true
+    end
+  end
+
+  describe '#create_education_class_team' do
+    subject { service.create_education_class_team("Evan's group id") }
+
+    let(:http_method) { :post }
+    let(:url) { 'https://graph.microsoft.com/v1.0/teams' }
+    let(:req_body) do
+      {
+        "template@odata.bind" =>
+          "https://graph.microsoft.com/v1.0/teamsTemplates('educationClass')",
+        "group@odata.bind" => "https://graph.microsoft.com/v1.0/groups('Evan''s group id')"
+      }
+    end
+    let(:with_params) { {body: req_body} }
+    let(:response) { {status: 204, body: ''} }
+
+    it { is_expected.to eq(nil) }
+
+    it_behaves_like 'a graph service endpoint'
+
+    context 'when Microsoft returns a 400 saying "must have one or more owners"' do
+      let(:response) do
+        {
+          status: 400,
+          # this is an actual error from them (ids changed)
+          body: "{\r\n  \"error\": {\r\n    \"code\": \"BadRequest\",\r\n    \"message\": \"Failed to execute Templates backend request CreateTeamFromGroupWithTemplateRequest. Request Url: https://teams.microsoft.com/fabric/amer/templates/api/groups/abcdef01-1212-1212-1212-121212121212/team, Request Method: PUT, Response Status Code: BadRequest, Response Headers: Strict-Transport-Security: max-age=2592000\\r\\nx-operationid: 23457812489473234789372498732493\\r\\nx-telemetryid: 00-31424324322423432143421433242344-4324324234123412-43\\r\\nX-MSEdge-Ref: Ref A: 34213432213432413243422134344322 Ref B: DM1EDGE1111 Ref C: 2021-04-01T20:11:11Z\\r\\nDate: Thu, 01 Apr 2021 20:11:11 GMT\\r\\n, ErrorMessage : {\\\"errors\\\":[{\\\"message\\\":\\\"Group abcdef01-1212-1212-1212-12121212121 must have one or more owners in order to create a Team.\\\",\\\"errorCode\\\":\\\"Unknown\\\"}],\\\"operationId\\\":\\\"23457812489473234789372498732493\\\"}\",\r\n    \"innerError\": {\r\n      \"date\": \"2021-04-01T20:11:11\",\r\n      \"request-id\": \"11111111-1111-1111-1111-111111111111\",\r\n      \"client-request-id\": \"11111111-1111-1111-1111-111111111111\"\r\n    }\r\n  }\r\n}"
+        }
+      end
+
+      it 'raises a GroupHasNoOwners error' do
+        expect { subject }.to raise_error(MicrosoftSync::Errors::GroupHasNoOwners)
+      end
+    end
+  end
+
+  describe '#list_users' do
+    let(:http_method) { :get }
+    let(:url) { 'https://graph.microsoft.com/v1.0/users' }
+    let(:method_name) { :list_users }
+    let(:method_args) { [] }
+
+    it_behaves_like 'a paginated list endpoint'
+  end
 end

spec/lib/microsoft_sync/login_service_spec.rb

@@ -81,9 +81,9 @@ describe MicrosoftSync::LoginService do
         let(:response_status) { 401 }
         let(:response_body) { {} }
 
-        it 'raises an InvalidStatusCode' do
+        it 'raises an HTTPInvalidStatus' do
           expect { subject }.to raise_error(
-            MicrosoftSync::Errors::InvalidStatusCode,
+            MicrosoftSync::Errors::HTTPInvalidStatus,
             /Login service returned 401 for tenant mytenant/
           )
         end

spec/lib/microsoft_sync/syncer_spec.rb

@@ -71,6 +71,8 @@ describe MicrosoftSync::Syncer do
       mock_diff = double('diff')
       expect(syncer).to receive(:generate_diff).and_return(mock_diff)
       expect(syncer).to receive(:execute_diff).with(mock_diff)
+      expect(syncer).to receive(:sleep).with(15)
+      expect(syncer).to receive(:ensure_team_exists)
       expect { syncer.sync! }.to \
         change { group.reload.workflow_state }.from('pending').to('completed')
     end
@@ -275,4 +277,62 @@ describe MicrosoftSync::Syncer do
       syncer.execute_diff(mc)
     end
   end
+
+  describe '#ensure_team_exists' do
+    subject { syncer.ensure_team_exists }
+
+    before { group.update!(ms_group_id: 'mygroupid') }
+
+    context 'when there are no teacher/ta/designer enrollments' do
+      it "doesn't create a team" do
+        course.enrollments.to_a.each do |e|
+          e.destroy if e.type =~ /^Teacher|Ta|Designer/
+        end
+        expect(graph_service).to_not receive(:team_exists?)
+        expect(graph_service).to_not receive(:create_team)
+        subject
+      end
+    end
+
+    context 'when the team already exists' do
+      it "doesn't create a team" do
+        expect(graph_service).to receive(:team_exists?).with('mygroupid').and_return(true)
+        expect(graph_service).to_not receive(:create_team)
+        subject
+      end
+    end
+
+    context "when the team doesn't exist" do
+      before do
+        allow(graph_service).to receive(:team_exists?).with('mygroupid').and_return(false)
+      end
+
+      it 'creates the team' do
+        expect(graph_service).to receive(:create_education_class_team).with('mygroupid')
+        subject
+      end
+
+      context 'when the Microsoft API errors with "group has no owners"' do
+        it "doesn't raise an error" do
+          expect(graph_service).to receive(:create_education_class_team).with('mygroupid').
+            and_raise(MicrosoftSync::Errors::GroupHasNoOwners)
+          subject
+        end
+      end
+
+      context 'when the Microsoft API errors with some other error' do
+        it "bubbles up the error" do
+          expect(graph_service).to \
+            receive(:create_education_class_team).with('mygroupid').
+            and_raise(
+              # if we need to do lots of this, we can make a spec helper to make errors
+              MicrosoftSync::Errors::HTTPBadRequest.new(
+                response: double(code: 400, body: ''), tenant: '', service: ''
+              )
+            )
+          expect { subject }.to raise_error(MicrosoftSync::Errors::HTTPBadRequest)
+        end
+      end
+    end
+  end
 end