Improve UX

10 files changed, 47 insertions, 52 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 5b1898b0ee1..5a6ba3f2054 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -32,7 +32,13 @@ class GroupsController < Groups::ApplicationController
     @group = Groups::CreateService.new(current_user, group_params).execute
 
     if @group.persisted?
-      redirect_to @group, notice: "Group '#{@group.name}' was successfully created."
+      notice = if @group.chat_team.present?
+                 "Group '#{@group.name}' and its Mattermost team were successfully created."
+               else
+                  "Group '#{@group.name}' was successfully created."
+               end
+
+      redirect_to @group, notice: notice
     else
       render action: "new"
     end
diff --git a/app/models/group.rb b/app/models/group.rb
index 240a17f1dc1..ff340859cb7 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -212,4 +212,14 @@ class Group < Namespace
   def users_with_parents
     User.where(id: members_with_parents.select(:user_id))
   end
+
+  def mattermost_team_params
+    max_length = 59
+
+    {
+      name: path[0..max_length],
+      display_name: name[0..max_length],
+      type: public? ? 'O' : 'I' # Open vs Invite-only
+    }
+  end
 end
diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb
index 47476aaa05e..a8fa098246a 100644
--- a/app/services/groups/base_service.rb
+++ b/app/services/groups/base_service.rb
@@ -5,11 +5,5 @@ module Groups
     def initialize(group, user, params = {})
       @group, @current_user, @params = group, user, params.dup
     end
-
-    private
-
-    def create_chat_team?
-      @chat_team == true && Gitlab.config.mattermost.enabled
-    end
   end
 end
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index 4ed2cb7c8af..6ef78c3e677 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -23,7 +23,11 @@ module Groups
       @group.name ||= @group.path.dup
 
       if create_chat_team?
-        Mattermost::CreateTeamService.new(@group, current_user).execute
+        begin
+          response = Mattermost::CreateTeamService.new(@group, current_user).execute
+
+          @group.build_chat_team(name: response['name'], team_id: response['id'])
+        end
 
         return @group if @group.errors.any?
       end
@@ -32,5 +36,11 @@ module Groups
       @group.add_owner(current_user)
       @group
     end
+
+    private
+
+    def create_chat_team?
+      Gitlab.config.mattermost.enabled && @chat_team && group.chat_team.nil?
+    end
   end
 end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index e8ad6d41ed4..4e878ec556a 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -1,8 +1,6 @@
 module Groups
   class UpdateService < Groups::BaseService
     def execute
-      @chat_team = params.delete(:create_chat_team)
-
       # check that user is allowed to set specified visibility_level
       new_visibility = params[:visibility_level]
       if new_visibility && new_visibility.to_i != group.visibility_level
@@ -16,12 +14,6 @@ module Groups
 
       group.assign_attributes(params)
 
-      if create_chat_team?
-        Mattermost::CreateTeamService.new(group, current_user).execute
-
-        return group if group.errors.any?
-      end
-
       begin
         group.save
       rescue Gitlab::UpdatePathError => e
@@ -30,11 +22,5 @@ module Groups
         false
       end
     end
-
-    private
-
-    def create_chat_team?
-      super && group.chat_team.nil?
-    end
   end
 end
diff --git a/app/services/mattermost/create_team_service.rb b/app/services/mattermost/create_team_service.rb
index 199d15aee92..e3206810f3a 100644
--- a/app/services/mattermost/create_team_service.rb
+++ b/app/services/mattermost/create_team_service.rb
@@ -6,10 +6,9 @@ module Mattermost
 
     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'])
+      Mattermost::Team.new(current_user).create(@group.mattermost_team_params)
     rescue Mattermost::ClientError => e
-      @group.errors.add(:chat_team, e.message)
+      @group.errors.add(:mattermost_team, e.message)
     end
   end
 end
diff --git a/db/migrate/20170120131253_create_chat_teams.rb b/db/migrate/20170120131253_create_chat_teams.rb
index 2d9341d235f..699226d60c9 100644
--- a/db/migrate/20170120131253_create_chat_teams.rb
+++ b/db/migrate/20170120131253_create_chat_teams.rb
@@ -4,6 +4,8 @@ class CreateChatTeams < ActiveRecord::Migration
   DOWNTIME = true
   DOWNTIME_REASON = "Adding a foreign key"
 
+  disable_ddl_transaction!
+
   def change
     create_table :chat_teams do |t|
       t.integer :namespace_id, index: true
@@ -13,6 +15,6 @@ class CreateChatTeams < ActiveRecord::Migration
       t.timestamps null: false
     end
 
-    add_concurrent_foreign_key :chat_teams, :namespaces, on_delete: :cascade
+    add_concurrent_foreign_key :chat_teams, :namespaces, column: :namespace_id
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 39c767a5595..34aa12814c0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170216141440) do
+ActiveRecord::Schema.define(version: 20170217151947) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -61,7 +61,6 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.boolean "shared_runners_enabled", default: true, null: false
     t.integer "max_artifacts_size", default: 100, null: false
     t.string "runners_registration_token"
-    t.integer "max_pages_size", default: 100, null: false
     t.boolean "require_two_factor_authentication", default: false
     t.integer "two_factor_grace_period", default: 48
     t.boolean "metrics_enabled", default: false
@@ -110,7 +109,9 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.boolean "html_emails_enabled", default: true
     t.string "plantuml_url"
     t.boolean "plantuml_enabled"
+    t.integer "max_pages_size", default: 100, null: false
     t.integer "terminal_max_session_time", default: 0, null: false
+    t.string "default_artifacts_expire_in", default: "0", null: false
   end
 
   create_table "audit_events", force: :cascade do |t|
@@ -698,7 +699,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.integer "updated_by_id"
     t.text "merge_error"
     t.text "merge_params"
-    t.boolean "merge_when_build_succeeds", default: false, null: false
+    t.boolean "merge_when_pipeline_succeeds", default: false, null: false
     t.integer "merge_user_id"
     t.string "merge_commit_sha"
     t.datetime "deleted_at"
@@ -765,8 +766,8 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.integer "visibility_level", default: 20, null: false
     t.boolean "request_access_enabled", default: false, null: false
     t.datetime "deleted_at"
-    t.text "description_html"
     t.boolean "lfs_enabled"
+    t.text "description_html"
     t.integer "parent_id"
   end
 
@@ -981,7 +982,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.boolean "last_repository_check_failed"
     t.datetime "last_repository_check_at"
     t.boolean "container_registry_enabled"
-    t.boolean "only_allow_merge_if_build_succeeds", default: false, null: false
+    t.boolean "only_allow_merge_if_pipeline_succeeds", default: false, null: false
     t.boolean "has_external_issue_tracker"
     t.string "repository_storage", default: "default", null: false
     t.boolean "request_access_enabled", default: false, null: false
@@ -1291,6 +1292,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
     t.string "organization"
     t.boolean "authorized_projects_populated"
     t.boolean "notified_of_own_activity", default: false, null: false
+    t.boolean "ghost"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
@@ -1341,7 +1343,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
 
   add_foreign_key "boards", "projects"
-  add_foreign_key "chat_teams", "namespaces", on_delete: :cascade
+  add_foreign_key "chat_teams", "namespaces", name: "fk_3b543909cb", on_delete: :cascade
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
   add_foreign_key "label_priorities", "labels", on_delete: :cascade
   add_foreign_key "label_priorities", "projects", on_delete: :cascade
diff --git a/lib/mattermost/team.rb b/lib/mattermost/team.rb
index 52486fd1a54..69b0a3edf6f 100644
--- a/lib/mattermost/team.rb
+++ b/lib/mattermost/team.rb
@@ -7,20 +7,12 @@ 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)
-      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)
-      {
-        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
-      }
+    def create(name:, display_name:, type:)
+      session_post('/api/v3/teams/create', body: {
+        name: name,
+        display_name: display_name,
+        type: type
+      }.to_json)
     end
   end
 end
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
index b11483fb407..ec89b540e6a 100644
--- a/spec/services/groups/create_service_spec.rb
+++ b/spec/services/groups/create_service_spec.rb
@@ -47,12 +47,6 @@ describe Groups::CreateService, '#execute', services: true do
       Settings.mattermost['enabled'] = true
     end
 
-    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' })