From 34295036e2a9ecf18ca5440a5dd6dbb0c7f05643 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Mon, 19 Dec 2016 23:53:19 +0100
Subject: Improve sources

- Add proper error handling,
- Use flash[:alert] and flash[:notice],
- Use `resource` instead of `resources`,
---
 app/controllers/projects/mattermost_controller.rb  | 39 --------------------
 app/controllers/projects/mattermosts_controller.rb | 41 ++++++++++++++++++++++
 app/helpers/projects_helper.rb                     |  9 +++--
 .../mattermost_slash_commands_service.rb           | 35 ++++++------------
 app/views/projects/mattermost/_no_teams.html.haml  | 12 -------
 .../projects/mattermost/_team_selection.html.haml  | 41 ----------------------
 app/views/projects/mattermost/new.html.haml        |  8 -----
 app/views/projects/mattermosts/_no_teams.html.haml | 12 +++++++
 .../projects/mattermosts/_team_selection.html.haml | 41 ++++++++++++++++++++++
 app/views/projects/mattermosts/new.html.haml       |  8 +++++
 .../_installation_info.html.haml                   | 27 +++++++-------
 config/routes/project.rb                           |  2 +-
 lib/mattermost/command.rb                          | 13 ++++---
 lib/mattermost/session.rb                          |  7 +++-
 lib/mattermost/team.rb                             | 10 +++++-
 15 files changed, 152 insertions(+), 153 deletions(-)
 delete mode 100644 app/controllers/projects/mattermost_controller.rb
 create mode 100644 app/controllers/projects/mattermosts_controller.rb
 delete mode 100644 app/views/projects/mattermost/_no_teams.html.haml
 delete mode 100644 app/views/projects/mattermost/_team_selection.html.haml
 delete mode 100644 app/views/projects/mattermost/new.html.haml
 create mode 100644 app/views/projects/mattermosts/_no_teams.html.haml
 create mode 100644 app/views/projects/mattermosts/_team_selection.html.haml
 create mode 100644 app/views/projects/mattermosts/new.html.haml

diff --git a/app/controllers/projects/mattermost_controller.rb b/app/controllers/projects/mattermost_controller.rb
deleted file mode 100644
index a0eaec262ee..00000000000
--- a/app/controllers/projects/mattermost_controller.rb
+++ /dev/null
@@ -1,39 +0,0 @@
-class Projects::MattermostController < Projects::ApplicationController
-  layout 'project_settings'
-  before_action :authorize_admin_project!
-  before_action :service
-  before_action :teams, only: [:new]
-
-  def new
-  end
-
-  def create
-    message = @service.configure(current_user, configure_params)
-    notice = message.is_a?(String) ? message : 'This service is now configured'
-
-    redirect_to(
-      new_namespace_project_mattermost_path(@project.namespace, @project),
-      notice: notice
-    )
-  rescue NoSessionError
-    redirect_to(
-      new_namespace_project_mattermost_path(@project.namespace, @project),
-      alert: 'No session could be set up, is Mattermost configured with Single Sign on?'
-    )
-  end
-
-  private
-
-  def configure_params
-    params.permit(:trigger, :team_id).
-      merge(url: service_trigger_url(@service), icon_url: asset_url('gitlab_logo.png'))
-  end
-
-  def service
-    @service ||= @project.find_or_initialize_service('mattermost_slash_commands')
-  end
-
-  def teams
-    @teams = @service.list_teams(current_user)
-  end
-end
diff --git a/app/controllers/projects/mattermosts_controller.rb b/app/controllers/projects/mattermosts_controller.rb
new file mode 100644
index 00000000000..c6b39add7ad
--- /dev/null
+++ b/app/controllers/projects/mattermosts_controller.rb
@@ -0,0 +1,41 @@
+class Projects::MattermostsController < Projects::ApplicationController
+  include TriggersHelper
+  include ActionView::Helpers::AssetUrlHelper
+
+  layout 'project_settings'
+
+  before_action :authorize_admin_project!
+  before_action :service
+  before_action :teams, only: [:new]
+
+  def new
+  end
+
+  def create
+    @service.configure!(current_user, configure_params)
+
+    flash[:notice] = 'This service is now configured'
+    redirect_to edit_namespace_project_service_path(@project.namespace, @project, service)
+  rescue => e
+    flash[:alert] = e.message
+    redirect_to new_namespace_project_mattermost_path(@project.namespace, @project)
+  end
+
+  private
+
+  def configure_params
+    params.require(:mattermost).permit(:trigger, :team_id).merge(
+      url: service_trigger_url(@service),
+      icon_url: asset_url('gitlab_logo.png'))
+  end
+
+  def teams
+    @teams ||= @service.list_teams(current_user)
+  rescue => e
+    flash[:alert] = e.message
+  end
+
+  def service
+    @service ||= @project.find_or_initialize_service('mattermost_slash_commands')
+  end
+end
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 963e72ce96e..b7731ab4be2 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -149,12 +149,11 @@ module ProjectsHelper
   end
 
   def mattermost_teams_options(teams)
-    teams_options = teams.map do |team|
-      return nil unless team['display_name'] && team['id']
-      [team['display_name'], team['id']]
+    teams_options = teams.map do |id, options|
+      return nil unless id && options['display_name']
+      [options['display_name'], id]
     end.compact
-    teams_options.unshift(['Select team...', '0']) unless teams_options.one?
-    teams_options
+    teams_options.unshift(['Select team...', '0'])
   end
 
   private
diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb
index 92e2ae637fb..51de80f38de 100644
--- a/app/models/project_services/mattermost_slash_commands_service.rb
+++ b/app/models/project_services/mattermost_slash_commands_service.rb
@@ -25,28 +25,17 @@ class MattermostSlashCommandsService < ChatService
     ]
   end
 
-  def configure(current_user, params)
-    result = Mattermost::Session.new(current_user).with_session do |session|
-      Mattermost::Command.create(session, params[:team_id], command)
+  def configure!(current_user, params)
+    token = Mattermost::Session.new(current_user).with_session do |session|
+      Mattermost::Command.create(session, command(params))
     end
 
-    if result.has_key?('message')
-      result['message']
-    else
-      update!(token: result['token'], active: true)
-    end
+    update!(active: true, token: token)
   end
 
-  def list_teams(current_user)
-    begin
-      response = Mattermost::Session.new(current_user).with_session do |session|
-        Mattermost::Team.all(session)
-      end
-
-      # We ignore the error message as we can't display it
-      response.has_key?('message') ? [] : response
-    rescue Mattermost::NoSessionError
-      []
+  def list_teams(user)
+    Mattermost::Session.new(user).with_session do |session|
+      Mattermost::Team.all(session)
     end
   end
 
@@ -64,21 +53,17 @@ class MattermostSlashCommandsService < ChatService
 
   private
 
-  def command(trigger:, url:, icon_url:)
+  def command(params)
     pretty_project_name = project.name_with_namespace
 
-    {
+    params.merge(
       auto_complete: true,
       auto_complete_desc: "Perform common operations on: #{pretty_project_name}",
       auto_complete_hint: '[help]',
       description: "Perform common operations on: #{pretty_project_name}",
       display_name: "GitLab  / #{pretty_project_name}",
       method: 'P',
-      user_name: 'GitLab',
-      trigger: trigger,
-      url: url,
-      icon_url: icon_url
-    }
+      user_name: 'GitLab')
   end
 
   def find_chat_user(params)
diff --git a/app/views/projects/mattermost/_no_teams.html.haml b/app/views/projects/mattermost/_no_teams.html.haml
deleted file mode 100644
index 605c7f61dee..00000000000
--- a/app/views/projects/mattermost/_no_teams.html.haml
+++ /dev/null
@@ -1,12 +0,0 @@
-%p
-  You aren’t a member of any team on the Mattermost instance at
-  %strong= Gitlab.config.mattermost.host
-%p
-  To install this service,
-  = link_to "#{Gitlab.config.mattermost.host}/select_team", target: '__blank' do
-    join a team
-    = icon('external-link')
-  and try again.
-%hr
-.clearfix
-  = link_to 'Go back', edit_namespace_project_service_path(@project.namespace, @project, @service), class: 'btn btn-lg pull-right'
diff --git a/app/views/projects/mattermost/_team_selection.html.haml b/app/views/projects/mattermost/_team_selection.html.haml
deleted file mode 100644
index e0ab63dbc5d..00000000000
--- a/app/views/projects/mattermost/_team_selection.html.haml
+++ /dev/null
@@ -1,41 +0,0 @@
-%p
-  This service will be installed on the Mattermost instance at
-  %strong= Gitlab.config.mattermost.host
-%hr
-= form_for(:create, method: :post, url: configure_namespace_project_mattermost_index_path(@project.namespace, @project)) do |f|
-  %h4 Team
-  %p Select or create the team where the slash commands will be used in
-  - options = mattermost_teams_options(@teams)
-  = f.select(:team_id, options, {}, { class: 'form-control', selected: "#{options.first[1] if options.count.one?}", disabled: options.count.one? })
-  .help-block
-    - if options.count.one?
-      This is the only team where you are an administrator.
-    - else
-      The list shows teams where you are administrator
-    To create a team, ask your Mattermost system administrator.
-    To create a team,
-    = link_to "#{Gitlab.config.mattermost.host}/create_team" do
-      use Mattermost's interface
-      = icon('external-link')
-  %hr
-  %h4 Command trigger word
-  %p Choose the word that will trigger commands
-  = f.text_field(:trigger, value: @project.path, class: 'form-control')
-  .help-block
-    %p Trigger word must be unique, and cannot begin with a slash or contain any spaces. Use the word that works best for your team.
-    %p Fill in the word that works best for your team.
-    %p
-      Suggestions:
-      %code= 'gitlab'
-      %code= @project.path # Path contains no spaces, but dashes
-      %code= @project.path_with_namespace
-    %p
-      Reserved:
-      = link_to 'https://docs.mattermost.com/help/messaging/executing-commands.html#built-in-commands', target: '__blank' do
-        see list of built-in slash commands
-        = icon('external-link')
-  %hr
-  .clearfix
-    .pull-right
-      = link_to 'Cancel', edit_namespace_project_service_path(@project.namespace, @project, @service), class: 'btn btn-lg'
-      = f.submit 'Install', class: 'btn btn-save btn-lg'
diff --git a/app/views/projects/mattermost/new.html.haml b/app/views/projects/mattermost/new.html.haml
deleted file mode 100644
index 96b1d2aee61..00000000000
--- a/app/views/projects/mattermost/new.html.haml
+++ /dev/null
@@ -1,8 +0,0 @@
-.service-installation
-  .inline.pull-right
-    = custom_icon('mattermost_logo', size: 48)
-  %h3 Install Mattermost Command
-  - if @teams.empty?
-    = render 'no_teams'
-  - else
-    = render 'team_selection'
diff --git a/app/views/projects/mattermosts/_no_teams.html.haml b/app/views/projects/mattermosts/_no_teams.html.haml
new file mode 100644
index 00000000000..605c7f61dee
--- /dev/null
+++ b/app/views/projects/mattermosts/_no_teams.html.haml
@@ -0,0 +1,12 @@
+%p
+  You aren’t a member of any team on the Mattermost instance at
+  %strong= Gitlab.config.mattermost.host
+%p
+  To install this service,
+  = link_to "#{Gitlab.config.mattermost.host}/select_team", target: '__blank' do
+    join a team
+    = icon('external-link')
+  and try again.
+%hr
+.clearfix
+  = link_to 'Go back', edit_namespace_project_service_path(@project.namespace, @project, @service), class: 'btn btn-lg pull-right'
diff --git a/app/views/projects/mattermosts/_team_selection.html.haml b/app/views/projects/mattermosts/_team_selection.html.haml
new file mode 100644
index 00000000000..376592e66c9
--- /dev/null
+++ b/app/views/projects/mattermosts/_team_selection.html.haml
@@ -0,0 +1,41 @@
+%p
+  This service will be installed on the Mattermost instance at
+  %strong= link_to Gitlab.config.mattermost.host, Gitlab.config.mattermost.host
+%hr
+= form_for(:mattermost, method: :post, url: namespace_project_mattermost_path(@project.namespace, @project)) do |f|
+  %h4 Team
+  %p Select or create the team where the slash commands will be used in
+  - selected_id = @teams.keys.first if @teams.one?
+  = f.select(:team_id, mattermost_teams_options(@teams), {}, { class: 'form-control', selected: "#{selected_id}", disabled: @teams.one? })
+  .help-block
+    - if @teams.one?
+      This is the only team where you are an administrator.
+    - else
+      The list shows teams where you are administrator
+    To create a team, ask your Mattermost system administrator.
+    To create a team,
+    = link_to "#{Gitlab.config.mattermost.host}/create_team" do
+      use Mattermost's interface
+      = icon('external-link')
+  %hr
+  %h4 Command trigger word
+  %p Choose the word that will trigger commands
+  = f.text_field(:trigger, value: @project.path, class: 'form-control')
+  .help-block
+    %p Trigger word must be unique, and cannot begin with a slash or contain any spaces. Use the word that works best for your team.
+    %p Fill in the word that works best for your team.
+    %p
+      Suggestions:
+      %code= 'gitlab'
+      %code= @project.path # Path contains no spaces, but dashes
+      %code= @project.path_with_namespace
+    %p
+      Reserved:
+      = link_to 'https://docs.mattermost.com/help/messaging/executing-commands.html#built-in-commands', target: '__blank' do
+        see list of built-in slash commands
+        = icon('external-link')
+  %hr
+  .clearfix
+    .pull-right
+      = link_to 'Cancel', edit_namespace_project_service_path(@project.namespace, @project, @service), class: 'btn btn-lg'
+      = f.submit 'Install', class: 'btn btn-save btn-lg'
diff --git a/app/views/projects/mattermosts/new.html.haml b/app/views/projects/mattermosts/new.html.haml
new file mode 100644
index 00000000000..96b1d2aee61
--- /dev/null
+++ b/app/views/projects/mattermosts/new.html.haml
@@ -0,0 +1,8 @@
+.service-installation
+  .inline.pull-right
+    = custom_icon('mattermost_logo', size: 48)
+  %h3 Install Mattermost Command
+  - if @teams.empty?
+    = render 'no_teams'
+  - else
+    = render 'team_selection'
diff --git a/app/views/projects/services/mattermost_slash_commands/_installation_info.html.haml b/app/views/projects/services/mattermost_slash_commands/_installation_info.html.haml
index abc68e955e7..e6fcb09e054 100644
--- a/app/views/projects/services/mattermost_slash_commands/_installation_info.html.haml
+++ b/app/views/projects/services/mattermost_slash_commands/_installation_info.html.haml
@@ -5,18 +5,15 @@
   .row
     %strong.col-sm-3.text-right Mattermost
     = link_to pretty_url(Gitlab.config.mattermost.host), Gitlab.config.mattermost.host, class: 'col-sm-9', target: '__blank'
-  .row
-    %strong.col-sm-3.text-right Installation
-    .col-sm-9
-      - if @service.activated?
-        To edit or uninstall this service, press
-        %strong Edit in Mattermost
-      - else
-        To install this service, press
-        %strong Add to Mattermost
-        and follow the instructions
-  .row
-    .col-sm-9.col-sm-offset-3
-      = link_to new_namespace_project_mattermost_path(@project.namespace, @project), class: 'btn btn-lg' do
-        = custom_icon('mattermost_logo', size: 15)
-        = @service.activated? ? 'Edit in Mattermost' : 'Add to Mattermost'
+  - unless @service.activated?
+    .row
+      %strong.col-sm-3.text-right Installation
+      .col-sm-9
+          To install this service, press
+          %strong Add to Mattermost
+          and follow the instructions
+    .row
+      .col-sm-9.col-sm-offset-3
+        = link_to new_namespace_project_mattermost_path(@project.namespace, @project), class: 'btn btn-lg' do
+          = custom_icon('mattermost_logo', size: 15)
+          = 'Add to Mattermost'
diff --git a/config/routes/project.rb b/config/routes/project.rb
index b42c5e5211c..1d0caac3080 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -76,7 +76,7 @@ constraints(ProjectUrlConstrainer.new) do
         end
       end
 
-      resources :mattermost, only: [:new, :create]
+      resource :mattermost, only: [:new, :create]
 
       resources :deploy_keys, constraints: { id: /\d+/ }, only: [:index, :new, :create] do
         member do
diff --git a/lib/mattermost/command.rb b/lib/mattermost/command.rb
index afbf2ce3349..5c6f5861a7f 100644
--- a/lib/mattermost/command.rb
+++ b/lib/mattermost/command.rb
@@ -1,12 +1,15 @@
 module Mattermost
   class Command
-    def self.create(session, team_id, command)
-      response = session.post("/api/v3/teams/#{team_id}/commands/create", body: command.to_json).parsed_response
+    def self.create(session, params)
+      response = session.post("/api/v3/teams/#{params[:team_id]}/commands/create",
+        body: params.to_json)
 
-      if response.has_key?('message')
-        response
+      if response.success?
+        response.parsed_response['token']
+      elsif response.parsed_response.try(:has_key?, 'message')
+        raise response.parsed_response['message']
       else
-        response['token']
+        raise 'Failed to create a new command'
       end
     end
   end
diff --git a/lib/mattermost/session.rb b/lib/mattermost/session.rb
index 670f83bb6bc..f0ce51d6a71 100644
--- a/lib/mattermost/session.rb
+++ b/lib/mattermost/session.rb
@@ -1,5 +1,10 @@
 module Mattermost
-  class NoSessionError < StandardError; end
+  class NoSessionError < StandardError
+    def message
+      'No session could be set up, is Mattermost configured with Single Sign on?'
+    end
+  end
+
   # This class' prime objective is to obtain a session token on a Mattermost
   # instance with SSO configured where this GitLab instance is the provider.
   #
diff --git a/lib/mattermost/team.rb b/lib/mattermost/team.rb
index 5ee77aa9adf..2de01eced0b 100644
--- a/lib/mattermost/team.rb
+++ b/lib/mattermost/team.rb
@@ -1,7 +1,15 @@
 module Mattermost
   class Team
     def self.all(session)
-      session.get('/api/v3/teams/all').parsed_response
+      response = session.get('/api/v3/teams/all')
+
+      if response.success?
+        response.parsed_response
+      elsif response.parsed_response.try(:has_key?, 'message')
+        raise response.parsed_response['message']
+      else
+        raise 'Failed to list teams'
+      end
     end
   end
 end
-- 
cgit v1.2.1