Transactional mattermost team creation

app/controllers/groups_controller.rb

@@ -29,7 +29,6 @@ class GroupsController < Groups::ApplicationController
   end
 
   def create
-    byebug
     @group = Groups::CreateService.new(current_user, group_params).execute
 
     if @group.persisted?

app/models/group.rb

@@ -21,7 +21,6 @@ class Group < Namespace
   has_many :shared_projects, through: :project_group_links, source: :project
   has_many :notification_settings, dependent: :destroy, as: :source
   has_many :labels, class_name: 'GroupLabel'
-  has_one :chat_team, dependent: :destroy
 
   validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
   validate :visibility_level_allowed_by_projects

app/models/namespace.rb

@@ -20,6 +20,7 @@ class Namespace < ActiveRecord::Base
 
   belongs_to :parent, class_name: "Namespace"
   has_many :children, class_name: "Namespace", foreign_key: :parent_id
+  has_one :chat_team, dependent: :destroy
 
   validates :owner, presence: true, unless: ->(n) { n.type == "Group" }
   validates :name,

app/services/groups/create_service.rb

@@ -2,12 +2,10 @@ module Groups
   class CreateService < Groups::BaseService
     def initialize(user, params = {})
       @current_user, @params = user, params.dup
+      @chat_team = @params.delete(:create_chat_team)
     end
 
     def execute
-      create_chat_team = params.delete(:create_chat_team)
-      team_name = params.delete(:chat_team_name)
-
       @group = Group.new(params)
 
       unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
@@ -23,15 +21,22 @@ module Groups
       end
 
       @group.name ||= @group.path.dup
-      @group.save
-      @group.add_owner(current_user)
 
-      if create_chat_team && Gitlab.config.mattermost.enabled
-        options = team_name ? { name: team_name } : {}
-        Mattermost::CreateTeamWorker.perform_async(@group.id, current_user.id, options)
+      if create_chat_team?
+        Mattermost::CreateTeamService.new(@group, current_user).execute
+
+        return @group if @group.errors.any?
       end
 
+      @group.save
+      @group.add_owner(current_user)
       @group
     end
+
+    private
+
+    def create_chat_team?
+      @chat_team && Gitlab.config.mattermost.enabled
+    end
   end
 end

app/services/mattermost/create_team_service.rb

+module Mattermost
+  class CreateTeamService < ::BaseService
+    def initialize(group, current_user)
+      @group, @current_user = group, current_user
+    end
+
+    def execute
+      # The user that creates the team will be Team Admin
+      response = Mattermost::Team.new(current_user).create(@group)
+      @group.build_chat_team(name: response['name'], team_id: response['id'])
+    rescue Mattermost::ClientError => e
+      @group.errors.add(:chat_team, e.message)
+    end
+  end
+end

app/workers/mattermost/create_team_worker.rb

-module Mattermost
-  class CreateTeamWorker
-    include Sidekiq::Worker
-    include DedicatedSidekiqQueue
-
-    sidekiq_options retry: 5
-
-    # Add 5 seconds so the first retry isn't 1 second later
-    sidekiq_retry_in do |count|
-      5 + 5**n
-    end
-
-    def perform(group_id, current_user_id, options = {})
-      group = Group.find_by(id: group_id)
-      current_user = User.find_by(id: current_user_id)
-      return unless group && current_user
-
-      # The user that creates the team will be Team Admin
-      response = Mattermost::Team.new(current_user).create(group, options)
-
-      group.create_chat_team(name: response['name'], team_id: response['id'])
-    end
-  end
-end

lib/mattermost/team.rb

@@ -7,20 +7,20 @@ module Mattermost
 
     # 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)
+    def create(group)
+      session_post('/api/v3/teams/create', body: new_team_params(group).to_json)
     end
 
     private
 
     MATTERMOST_TEAM_LENGTH_MAX = 59
 
-    def new_team_params(group, options)
+    def new_team_params(group)
       {
         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/services/groups/create_service_spec.rb

@@ -43,10 +43,17 @@ describe Groups::CreateService, '#execute', services: true 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)
+    it 'triggers the service' do
+      expect_any_instance_of(Mattermost::CreateTeamService).to receive(:execute)
 
       subject
     end
+
+    it 'create the chat team with the group' do
+      allow_any_instance_of(Mattermost::Team).to receive(:create)
+        .and_return({ 'name' => 'tanuki', 'id' => 'lskdjfwlekfjsdifjj' })
+
+      expect { subject }.to change { ChatTeam.count }.from(0).to(1)
+    end
   end
 end

spec/workers/mattermost/create_team_worker_spec.rb

-require 'spec_helper'
-
-describe Mattermost::CreateTeamWorker do
-  let(:group) { create(:group, path: 'path', name: 'name') }
-  let(:admin) { create(:admin) }
-
-  describe '.perform' do
-    subject { described_class.new.perform(group.id, admin.id) }
-
-    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
-
-    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