More refactoring, push present to base command

6 files changed, 67 insertions, 46 deletions

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