From 444d71e043eb19979ec1b08504b2760910cb2a47 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Mon, 20 Feb 2017 13:41:50 +0100 Subject: Transactional mattermost team creation Before this commit, but still on this feature branch, the creation of mattermost teams where a background job. However, it was decided it was better that these happened as transaction so feedback could be displayed to the user. --- app/controllers/groups_controller.rb | 1 - app/models/group.rb | 1 - app/models/namespace.rb | 1 + app/services/groups/create_service.rb | 21 ++++++++----- app/services/mattermost/create_team_service.rb | 15 +++++++++ app/workers/mattermost/create_team_worker.rb | 24 --------------- lib/mattermost/team.rb | 8 ++--- spec/services/groups/create_service_spec.rb | 11 +++++-- spec/workers/mattermost/create_team_worker_spec.rb | 36 ---------------------- 9 files changed, 42 insertions(+), 76 deletions(-) create mode 100644 app/services/mattermost/create_team_service.rb delete mode 100644 app/workers/mattermost/create_team_worker.rb delete mode 100644 spec/workers/mattermost/create_team_worker_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index b2a18f0e65d..0e421b915cf 100644 --- a/app/controllers/groups_controller.rb +++ b/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? diff --git a/app/models/group.rb b/app/models/group.rb index 27dc4e8f319..240a17f1dc1 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 6de4d08fc28..461a38d3e8e 100644 --- a/app/models/namespace.rb +++ b/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, diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb index 13d1b545498..3028025fc6e 100644 --- a/app/services/groups/create_service.rb +++ b/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 diff --git a/app/services/mattermost/create_team_service.rb b/app/services/mattermost/create_team_service.rb new file mode 100644 index 00000000000..199d15aee92 --- /dev/null +++ b/app/services/mattermost/create_team_service.rb @@ -0,0 +1,15 @@ +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 diff --git a/app/workers/mattermost/create_team_worker.rb b/app/workers/mattermost/create_team_worker.rb deleted file mode 100644 index 95b63dac8ab..00000000000 --- a/app/workers/mattermost/create_team_worker.rb +++ /dev/null @@ -1,24 +0,0 @@ -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 diff --git a/lib/mattermost/team.rb b/lib/mattermost/team.rb index d5648f7167f..52486fd1a54 100644 --- a/lib/mattermost/team.rb +++ b/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 diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 235fa5c5188..d8181f73110 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/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 diff --git a/spec/workers/mattermost/create_team_worker_spec.rb b/spec/workers/mattermost/create_team_worker_spec.rb deleted file mode 100644 index d3230f535c2..00000000000 --- a/spec/workers/mattermost/create_team_worker_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -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 -- cgit v1.2.1