Improve sources

- Add proper error handling,
- Use flash[:alert] and flash[:notice],
- Use `resource` instead of `resources`,

12 files changed, 96 insertions, 97 deletions

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/mattermosts/_no_teams.html.haml
index 605c7f61dee..605c7f61dee 100644
--- a/app/views/projects/mattermost/_no_teams.html.haml
+++ b/app/views/projects/mattermosts/_no_teams.html.haml
diff --git a/app/views/projects/mattermost/_team_selection.html.haml b/app/views/projects/mattermosts/_team_selection.html.haml
index e0ab63dbc5d..376592e66c9 100644
--- a/app/views/projects/mattermost/_team_selection.html.haml
+++ b/app/views/projects/mattermosts/_team_selection.html.haml
@@ -1,14 +1,14 @@
 %p
   This service will be installed on the Mattermost instance at
-  %strong= Gitlab.config.mattermost.host
+  %strong= link_to Gitlab.config.mattermost.host, Gitlab.config.mattermost.host
 %hr
-= form_for(:create, method: :post, url: configure_namespace_project_mattermost_index_path(@project.namespace, @project)) do |f|
+= 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
-  - 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? })
+  - 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 options.count.one?
+    - if @teams.one?
       This is the only team where you are an administrator.
     - else
       The list shows teams where you are administrator
diff --git a/app/views/projects/mattermost/new.html.haml b/app/views/projects/mattermosts/new.html.haml
index 96b1d2aee61..96b1d2aee61 100644
--- a/app/views/projects/mattermost/new.html.haml
+++ b/app/views/projects/mattermosts/new.html.haml
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