Add tests for Mattermost team creation

app/controllers/groups_controller.rb

@@ -143,6 +143,7 @@ class GroupsController < Groups::ApplicationController
       :share_with_group_lock,
       :visibility_level,
       :parent_id
+      :create_chat_team
     ]
   end
 

app/models/chat_team.rb

@@ -0,0 +1,5 @@
+class ChatTeam < ActiveRecord::Base
+  validates :team_id, presence: true
+
+  belongs_to :namespace
+end

app/services/groups/create_service.rb

@@ -5,6 +5,8 @@ module Groups
     end
 
     def execute
+      create_chat_team = params.delete(:create_chat_team)
+
       @group = Group.new(params)
 
       unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
@@ -23,7 +25,7 @@ module Groups
       @group.save
       @group.add_owner(current_user)
 
-      if params[:create_chat_team] && Gitlab.config.mattermost.enabled
+      if create_chat_team && Gitlab.config.mattermost.enabled
         Mattermost::CreateTeamWorker.perform_async(@group.id, current_user.id)
       end
 

app/workers/mattermost/create_team_worker.rb

@@ -3,26 +3,21 @@ module Mattermost
     include Sidekiq::Worker
     include DedicatedSidekiqQueue
 
-    def perform(group_id, current_user_id, options = {})
-      @group = Group.find(group_id)
-      current_user = User.find(current_user_id)
+    sidekiq_options retry: 5
 
-      options = team_params.merge(options)
-
-      # The user that creates the team will be Team Admin
-      response = Mattermost::Team.new(current_user).create(options)
-
-      ChatTeam.create!(namespace: @group, name: response['name'], team_id: response['id'])
+    # Add 5 seconds so the first retry isn't 1 second later
+    sidekiq_retry_in do |count|
+      5 + 5 ** n
     end
 
-    private
+    def perform(group_id, current_user_id, options = {})
+      group = Group.find(group_id)
+      current_user = User.find(current_user_id)
 
-    def team_params
-      {
-        name: @group.path[0..59],
-        display_name: @group.name[0..59],
-        type: @group.public? ? 'O' : 'I' # Open vs Invite-only
-      }
+      # The user that creates the team will be Team Admin
+      response = Mattermost::Team.new(current_user).create(group, options)
+
+      ChatTeam.create(namespace: group, name: response['name'], team_id: response['id'])
     end
   end
 end

lib/mattermost/client.rb

@@ -26,7 +26,7 @@ module Mattermost
 
     def session_get(path, options = {})
       with_session do |session|
-        get(session, path, options)  
+        get(session, path, options)
       end
     end
 

lib/mattermost/team.rb

@@ -5,8 +5,22 @@ module Mattermost
       session_get('/api/v3/teams/all')
     end
 
-    def create(params)
-      session_post('/api/v3/teams/create', body: params.to_json)
+    # Creates a team on the linked Mattermost instance, the team admin will be the
+    # `current_user` passed to the Mattermost::Client instance
+    def create(group, params)
+      session_post('/api/v3/teams/create', body: new_team_params(group, params).to_json)
+    end
+
+    private
+
+    MATTERMOST_TEAM_LENGTH_MAX = 59
+
+    def new_team_params(group, options)
+      {
+        name: group.path[0..MATTERMOST_TEAM_LENGTH_MAX],
+        display_name: group.name[0..MATTERMOST_TEAM_LENGTH_MAX],
+        type: group.public? ? 'O' : 'I' # Open vs Invite-only
+      }.merge(options)
     end
   end
 end

spec/models/chat_team_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe ChatTeam, type: :model do
   # Associations
-  it { is_expected.to belong_to(:group) }
+  it { is_expected.to belong_to(:namespace) }
 
   # Fields
   it { is_expected.to respond_to(:name) }

spec/services/groups/create_service_spec.rb

@@ -4,11 +4,11 @@ describe Groups::CreateService, '#execute', services: true do
   let!(:user) { create(:user) }
   let!(:group_params) { { path: "group_path", visibility_level: Gitlab::VisibilityLevel::PUBLIC } }
 
+  subject { service.execute }
+
   describe 'visibility level restrictions' do
     let!(:service) { described_class.new(user, group_params) }
 
-    subject { service.execute }
-
     context "create groups without restricted visibility level" do
       it { is_expected.to be_persisted }
     end
@@ -24,8 +24,6 @@ describe Groups::CreateService, '#execute', services: true do
     let!(:group) { create(:group) }
     let!(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) }
 
-    subject { service.execute }
-
     context 'as group owner' do
       before { group.add_owner(user) }
 
@@ -40,4 +38,15 @@ describe Groups::CreateService, '#execute', services: true do
       end
     end
   end
+
+  describe 'creating a mattermost team' do
+    let!(:params) { group_params.merge(create_chat_team: true) }
+    let!(:service) { described_class.new(user, params) }
+
+    it 'queues a background job' do
+      expect(Mattermost::CreateTeamWorker).to receive(:perform_async)
+
+      subject
+    end
+  end
 end

spec/workers/mattermost/create_team_worker_spec.rb

@@ -7,15 +7,30 @@ describe Mattermost::CreateTeamWorker do
   describe '.perform' do
     subject { described_class.new.perform(group.id, admin.id) }
 
-    before do
-      allow_any_instance_of(Mattermost::Team).
-        to receive(:create).
-        with(name: "path", display_name: "name", type: "O").
-        and_return('name' => 'my team', 'id' => 'sjfkdlwkdjfwlkfjwf')
+    context 'succesfull request to mattermost' do
+      before do
+        allow_any_instance_of(Mattermost::Team).
+          to receive(:create).
+          with(group, {}).
+          and_return('name' => 'my team', 'id' => 'sjfkdlwkdjfwlkfjwf')
+      end
+
+      it 'creates a new chat team' do
+        expect { subject }.to change { ChatTeam.count }.from(0).to(1)
+      end
     end
 
-    it 'creates a new chat team' do
-      expect { subject }.to change { ChatTeam.count }.from(0).to(1)
+    context 'connection trouble' do
+      before do
+        allow_any_instance_of(Mattermost::Team).
+          to receive(:create).
+          with(group, {}).
+          and_raise(Mattermost::ClientError.new('Undefined error'))
+      end
+
+      it 'does not rescue the error' do
+        expect { subject }.to raise_error(Mattermost::ClientError)
+      end
     end
   end
 end