More refactoring, push present to base command

12 files changed, 85 insertions, 54 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index a118761d93d..c5acae99c4c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -91,7 +91,7 @@ class Project < ActiveRecord::Base
   has_one :assembla_service, dependent: :destroy
   has_one :asana_service, dependent: :destroy
   has_one :gemnasium_service, dependent: :destroy
-  has_one :mattermost_chat_service, dependent: :destroy
+  has_one :mattermost_command_service, dependent: :destroy
   has_one :slack_service, dependent: :destroy
   has_one :buildkite_service, dependent: :destroy
   has_one :bamboo_service, dependent: :destroy
diff --git a/app/models/project_services/mattermost_command_service.rb b/app/models/project_services/mattermost_command_service.rb
index 262dd3076f7..3c9b14c66b4 100644
--- a/app/models/project_services/mattermost_command_service.rb
+++ b/app/models/project_services/mattermost_command_service.rb
@@ -38,19 +38,19 @@ class MattermostCommandService < ChatService
     user = find_chat_user(params)
     unless user
       url = authorize_chat_name_url(params)
-      return Mattermost::Presenter.authorize_user(url)
+      return Mattermost::Presenter.authorize_chat_name(url)
     end
 
-    Mattermost::CommandService.new(project, user, params).execute
+    Gitlab::ChatCommands::Command.new(project, user, params).execute
   end
 
   private
 
   def find_chat_user(params)
-    ChatNames::FindUserService.new(chat_names, params).execute
+    ChatNames::FindUserService.new(self, params).execute
   end
 
   def authorize_chat_name_url(params)
-    ChatNames::RequestService.new(self, params).execute
+    ChatNames::AuthorizeUserService.new(self, params).execute
   end
 end
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 094fca49c28..b0a94508d10 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -61,7 +61,6 @@ module API
     end
 
     resource :projects do
-
       desc 'Trigger a slash command' do
         detail 'Added in GitLab 8.13'
       end
@@ -78,7 +77,8 @@ module API
         result = service.try(:active?) && service.try(:trigger, params)
 
         if result
-          present result, status: result[:status] || 200
+          status result[:status] || 200
+          present result
         else
           not_found!('Service')
         end
diff --git a/lib/gitlab/chat_commands/base_command.rb b/lib/gitlab/chat_commands/base_command.rb
index 81b15bd1f7a..f84aca5365d 100644
--- a/lib/gitlab/chat_commands/base_command.rb
+++ b/lib/gitlab/chat_commands/base_command.rb
@@ -15,6 +15,14 @@ module Gitlab
         raise NotImplementedError
       end
 
+      def self.allowed?(_, _)
+        true
+      end
+
+      def self.can?(object, action, subject)
+        Ability.allowed?(object, action, subject)
+      end
+
       def execute(_)
         raise NotImplementedError
       end
@@ -31,21 +39,11 @@ module Gitlab
 
       private
 
-      def can?(object, action, subject)
-        Ability.allowed?(object, action, subject)
-      end
-
       def find_by_iid(iid)
         resource = collection.find_by(iid: iid)
 
         readable?(resource) ? resource : nil
       end
-
-      def search_results(query)
-        collection.search(query).limit(QUERY_LIMIT).select do |resource|
-          readable?(resource)
-        end
-      end
     end
   end
 end
diff --git a/lib/gitlab/chat_commands/command.rb b/lib/gitlab/chat_commands/command.rb
index 43144975901..5f131703d40 100644
--- a/lib/gitlab/chat_commands/command.rb
+++ b/lib/gitlab/chat_commands/command.rb
@@ -7,10 +7,14 @@ module Gitlab
       ].freeze
 
       def execute
-        klass, match = fetch_klass
-
-        if klass
-          present klass.new(project, current_user, params).execute(match)
+        command, match = match_command
+
+        if command
+          if command.allowed?(project, current_user)
+            present command.new(project, current_user, params).execute(match)
+          else
+            access_denied
+          end
         else
           help(help_messages)
         end
@@ -18,7 +22,7 @@ module Gitlab
 
       private
 
-      def fetch_klass
+      def match_command
         match = nil
         service = available_commands.find do |klass|
           match = klass.match(command)
@@ -28,9 +32,7 @@ module Gitlab
       end
 
       def help_messages
-        available_commands.map do |klass|
-          klass.help_message
-        end
+        available_commands.map(&:help_message)
       end
 
       def available_commands
@@ -43,13 +45,17 @@ module Gitlab
         params[:text]
       end
 
-      def present(resource)
-        Mattermost::Presenter.present(resource)
-      end
-
       def help(messages)
         Mattermost::Presenter.help(messages, params[:command])
       end
+
+      def access_denied
+        Mattermost::Presenter.access_denied
+      end
+
+      def present(resource)
+        Mattermost::Presenter.present(resource)
+      end
     end
   end
 end
diff --git a/lib/gitlab/chat_commands/issue_command.rb b/lib/gitlab/chat_commands/issue_command.rb
index 2426b3714b7..f1bc36239d5 100644
--- a/lib/gitlab/chat_commands/issue_command.rb
+++ b/lib/gitlab/chat_commands/issue_command.rb
@@ -10,7 +10,7 @@ module Gitlab
       end
 
       def readable?(issue)
-        can?(current_user, :read_issue, issue)
+        self.class.can?(current_user, :read_issue, issue)
       end
     end
   end
diff --git a/lib/gitlab/chat_commands/issue_create.rb b/lib/gitlab/chat_commands/issue_create.rb
index 1e311e09771..98338ebfa27 100644
--- a/lib/gitlab/chat_commands/issue_create.rb
+++ b/lib/gitlab/chat_commands/issue_create.rb
@@ -9,9 +9,11 @@ module Gitlab
         'issue create <title>\n<description>'
       end
 
-      def execute(match)
-        return nil unless can?(current_user, :create_issue, project)
+      def self.allowed?(project, user)
+        can?(user, :create_issue, project)
+      end
 
+      def execute(match)
         title = match[:title]
         description = match[:description]
 
diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb
index 84b7b8edd9e..7722022c658 100644
--- a/lib/mattermost/presenter.rb
+++ b/lib/mattermost/presenter.rb
@@ -4,24 +4,19 @@ module Mattermost
       include Rails.application.routes.url_helpers
 
       def authorize_chat_name(url)
-        message = "Hi there! We've yet to get acquainted! Please introduce yourself by [connection your GitLab profile](#{url})!"
+        message = ":wave: Hi there! Before I do anything for you, please [connect your GitLab account](#{url})."
 
         ephemeral_response(message)
       end
 
-      def help(messages, command)
-        return ephemeral_response("No commands configured") unless messages.count > 1
-        message = ["Available commands:"]
+      def help(commands, trigger)
+        if commands.count == 0
+          ephemeral_response("No commands configured") unless messages.count > 1
+        else
+          message = header_with_list("Available commands", commands)
 
-        messages.each do |messsage|
-          message << "- #{command} #{message}"
+          ephemeral_response(message)
         end
-
-        ephemeral_response(message.join("\n"))
-      end
-
-      def not_found
-        ephemeral_response("404 not found! GitLab couldn't find what your were looking for! :boom:")
       end
 
       def present(resource)
@@ -40,8 +35,16 @@ module Mattermost
         single_resource(resource)
       end
 
+      def access_denied
+        ephemeral_response("Whoops! That action is not allowed. This incident will be [reported](https://xkcd.com/838/).")
+      end
+
       private
 
+      def not_found
+        ephemeral_response("404 not found! GitLab couldn't find what your were looking for! :boom:")
+      end
+
       def single_resource(resource)
         return error(resource) if resource.errors.any?
 
@@ -52,23 +55,33 @@ module Mattermost
       end
 
       def multiple_resources(resources)
-        message = "Multiple results were found:\n"
-        message << resources.map { |resource| "- #{title(resource)}" }.join("\n")
+        resources.map! { |resource| title(resource) }
+
+        message = header_with_list("Multiple results were found:", resources)
 
         ephemeral_response(message)
       end
 
       def error(resource)
-        message = "The action was not succesfull because:\n"
-        message << resource.errors.messages.map { |message| "- #{message}" }.join("\n")
+        message = header_with_list("The action was not succesful, because:", resource.errors.messages)
 
-        ephemeral_response(resource.errors.messages.join("\n"))
+        ephemeral_response(message)
       end
 
       def title(resource)
         "[#{resource.to_reference} #{resource.title}](#{url(resource)})"
       end
 
+      def header_with_list(header, items)
+        message = [header]
+
+        items.each do |item|
+          message << "- #{item}"
+        end
+
+        message.join("\n")
+      end
+
       def url(resource)
         url_for(
           [
@@ -82,14 +95,16 @@ module Mattermost
       def ephemeral_response(message)
         {
           response_type: :ephemeral,
-          text: message
+          text: message,
+          status: 200
         }
       end
 
       def in_channel_response(message)
         {
           response_type: :in_channel,
-          text: message
+          text: message,
+          status: 200
         }
       end
     end
diff --git a/spec/lib/gitlab/chat_commands/command_spec.rb b/spec/lib/gitlab/chat_commands/command_spec.rb
index 328187b5048..528e690d234 100644
--- a/spec/lib/gitlab/chat_commands/command_spec.rb
+++ b/spec/lib/gitlab/chat_commands/command_spec.rb
@@ -26,6 +26,15 @@ describe Gitlab::ChatCommands::Command, service: true do
       end
     end
 
+    context 'the user can not create an issue' do
+      let(:params) { { text: "issue create my new issue" } }
+
+      it 'rejects the actions' do
+        expect(subject[:response_type]).to be(:ephemeral)
+        expect(subject[:text]).to start_with('Whoops! That action is not allowed')
+      end
+    end
+
     context 'issue is succesfully created' do
       let(:params) { { text: "issue create my new issue" } }
 
diff --git a/spec/lib/gitlab/chat_name_token_spec.rb b/spec/lib/gitlab/chat_name_token_spec.rb
index 8d7e7a99059..10153682973 100644
--- a/spec/lib/gitlab/chat_name_token_spec.rb
+++ b/spec/lib/gitlab/chat_name_token_spec.rb
@@ -12,9 +12,9 @@ describe Gitlab::ChatNameToken, lib: true do
   end
 
   context 'when storing data' do
-    let(:data) {
+    let(:data) do
       { key: 'value' }
-    }
+    end
 
     subject { described_class.new(@token) }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c74d9c282cf..d89a83cfc71 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -35,6 +35,7 @@ describe Project, models: true do
     it { is_expected.to have_one(:hipchat_service).dependent(:destroy) }
     it { is_expected.to have_one(:flowdock_service).dependent(:destroy) }
     it { is_expected.to have_one(:assembla_service).dependent(:destroy) }
+    it { is_expected.to have_one(:mattermost_command_service).dependent(:destroy) }
     it { is_expected.to have_one(:gemnasium_service).dependent(:destroy) }
     it { is_expected.to have_one(:buildkite_service).dependent(:destroy) }
     it { is_expected.to have_one(:bamboo_service).dependent(:destroy) }
diff --git a/spec/services/chat_names/authorize_user_service_spec.rb b/spec/services/chat_names/authorize_user_service_spec.rb
index f8c26e51bfc..d5178176526 100644
--- a/spec/services/chat_names/authorize_user_service_spec.rb
+++ b/spec/services/chat_names/authorize_user_service_spec.rb
@@ -17,7 +17,7 @@ describe ChatNames::AuthorizeUserService, services: true do
     end
 
     context 'when there are missing parameters' do
-      let(:params) { { } }
+      let(:params) { {} }
 
       it 'does not request a new token' do
         is_expected.to be_nil