summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorZ.J. van de Weg <git@zjvandeweg.nl>2017-03-01 20:34:29 +0100
committerZ.J. van de Weg <git@zjvandeweg.nl>2017-03-02 10:21:29 +0100
commit52c4a7866ed010d8db67e5ca976d8c73d4084784 (patch)
treea1cbb2d3910f9433e23dec22cc0f8c94c2e8675c
parentf6247600a3f5d500952b0ba32e6915a2d045e392 (diff)
downloadgitlab-ce-52c4a7866ed010d8db67e5ca976d8c73d4084784.tar.gz
Improve UX
-rw-r--r--app/controllers/groups_controller.rb8
-rw-r--r--app/models/group.rb10
-rw-r--r--app/services/groups/base_service.rb6
-rw-r--r--app/services/groups/create_service.rb12
-rw-r--r--app/services/groups/update_service.rb14
-rw-r--r--app/services/mattermost/create_team_service.rb5
-rw-r--r--db/migrate/20170120131253_create_chat_teams.rb4
-rw-r--r--db/schema.rb14
-rw-r--r--lib/mattermost/team.rb20
-rw-r--r--spec/services/groups/create_service_spec.rb6
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' })