Incorporate feedback, improve presenter class

[ci skip]

8 files changed, 70 insertions, 55 deletions

diff --git a/lib/api/services.rb b/lib/api/services.rb
index b4b3bb6e41a..094fca49c28 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -61,6 +61,10 @@ module API
     end
 
     resource :projects do
+
+      desc 'Trigger a slash command' do
+        detail 'Added in GitLab 8.13'
+      end
       post ':id/services/:service_slug/trigger' do
         project = Project.find_with_namespace(params[:id]) || Project.find_by(id: params[:id])
 
@@ -71,9 +75,7 @@ module API
 
         service = project.public_send(service_method)
 
-        result = if service.try(:active?) && service.respond_to?(:trigger)
-          service.trigger(params)
-        end
+        result = service.try(:active?) && service.try(:trigger, params)
 
         if result
           present result, status: result[:status] || 200
diff --git a/lib/gitlab/chat_commands/base_command.rb b/lib/gitlab/chat_commands/base_command.rb
index 13dc2a0f3e8..b5d58af3588 100644
--- a/lib/gitlab/chat_commands/base_command.rb
+++ b/lib/gitlab/chat_commands/base_command.rb
@@ -38,6 +38,10 @@ module Gitlab
       def present(resource)
         Mattermost::Presenter.present(resource)
       end
+      
+      def help(messages)
+        Mattermost::Presenter.help(messages)
+      end
 
       def find_by_iid(iid)
         resource = collection.find_by(iid: iid)
diff --git a/lib/gitlab/chat_commands/command.rb b/lib/gitlab/chat_commands/command.rb
index 06d09ab0e24..f1490c045c3 100644
--- a/lib/gitlab/chat_commands/command.rb
+++ b/lib/gitlab/chat_commands/command.rb
@@ -13,7 +13,7 @@ module Gitlab
       def execute
         klass, match = fetch_klass
 
-        return help(help_messages) unless klass.try(:available?, project)
+        return help(help_messages, params[:command]) unless klass.try(:available?, project)
 
         klass.new(project, current_user, params).execute(match)
       end
@@ -22,23 +22,23 @@ module Gitlab
 
       def fetch_klass
         match = nil
-        service = COMMANDS.find do |klass|
-          if klass.available?(project)
-            false
-          else
-            match = klass.match(command)
-          end
+        service = available_commands.find do |klass|
+          match = klass.match(command)
         end
 
         [service, match]
       end
 
       def help_messages
-        COMMANDS.map do |klass|
-          next unless klass.available?(project)
-
+        available_commands.map do |klass|
           klass.help_message
-        end.compact
+        end
+      end
+
+      def available_commands
+        COMMANDS.select do |klass|
+          klass.available?(project)
+        end
       end
 
       def command
diff --git a/lib/gitlab/chat_commands/issue_create.rb b/lib/gitlab/chat_commands/issue_create.rb
index c424e845402..b5cf85b58f1 100644
--- a/lib/gitlab/chat_commands/issue_create.rb
+++ b/lib/gitlab/chat_commands/issue_create.rb
@@ -6,6 +6,8 @@ module Gitlab
       end
 
       def execute(match)
+        present nil unless can?(current_user, :create_issue, project)
+
         title = match[:title]
         description = match[:description]
 
diff --git a/lib/gitlab/chat_commands/issue_search.rb b/lib/gitlab/chat_commands/issue_search.rb
index 4169e2a7a88..f64f3ad2680 100644
--- a/lib/gitlab/chat_commands/issue_search.rb
+++ b/lib/gitlab/chat_commands/issue_search.rb
@@ -2,7 +2,7 @@ module Gitlab
   module ChatCommands
     class IssueSearch < IssueCommand
       def self.match(text)
-        /\Aissue\s+search\s+(?<query>.*)/.match(text)
+        /\Aissue\s+search\s+(?<query>.*)\s*/.match(text)
       end
 
       def self.help_message
diff --git a/lib/gitlab/chat_commands/merge_request_command.rb b/lib/gitlab/chat_commands/merge_request_command.rb
index e0f69a49afd..ad485483b8a 100644
--- a/lib/gitlab/chat_commands/merge_request_command.rb
+++ b/lib/gitlab/chat_commands/merge_request_command.rb
@@ -9,8 +9,8 @@ module Gitlab
         project.merge_requests
       end
 
-      def readable?(_)
-        can?(current_user, :read_merge_request, project)
+      def readable?(merge_request)
+        can?(current_user, :read_merge_request, merge_request)
       end
     end
   end
diff --git a/lib/gitlab/chat_commands/merge_request_search.rb b/lib/gitlab/chat_commands/merge_request_search.rb
index caecb1a788e..19a29546736 100644
--- a/lib/gitlab/chat_commands/merge_request_search.rb
+++ b/lib/gitlab/chat_commands/merge_request_search.rb
@@ -2,7 +2,7 @@ module Gitlab
   module ChatCommands
     class MergeRequestSearch < MergeRequestCommand
       def self.match(text)
-        /\Amergerequest\s+search\s+(?<query>.*)/.match(text)
+        /\Amergerequest\s+search\s+(?<query>.*)\s*/.match(text)
       end
 
       def self.help_message
diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb
index 3db55d6bd51..0f2beb2cd6b 100644
--- a/lib/mattermost/presenter.rb
+++ b/lib/mattermost/presenter.rb
@@ -1,35 +1,24 @@
 module Mattermost
   class Presenter
     class << self
-      COMMAND_PREFIX = '/gitlab'.freeze
+      def authorize_chat_name(url)
+        message = "Hi there! We've yet to get acquainted! Please [introduce yourself](#{url})!"
 
-      def authorize_chat_name(params)
-        url = ChatNames::RequestService.new(service, params).execute
-
-        {
-          response_type: :ephemeral,
-          message: "You are not authorized. Click this [link](#{url}) to authorize."
-        }
+        ephemeral_response(message)
       end
 
-      def help(messages)
-        messages = ["Available commands:"]
+      def help(messages, command)
+        message = ["Available commands:"]
 
         messages.each do |messsage|
-          messages << "- #{message}"
+          message << "- #{command} #{message}"
         end
 
-        {
-          response_type: :ephemeral,
-          text: messages.join("\n")
-        }
+        ephemeral_response(messages.join("\n"))
       end
 
       def not_found
-        {
-          response_type: :ephemeral,
-          text: "404 not found! GitLab couldn't find what your were looking for! :boom:",
-        }
+        ephemeral_response("404 not found! GitLab couldn't find what your were looking for! :boom:")
       end
 
       def present(resource)
@@ -51,38 +40,56 @@ module Mattermost
       private
 
       def single_resource(resource)
-        message = title(resource)
+        return error(resource) if resource.errors.any?
+
+        message = "### #{title(resource)}"
         message << "\n\n#{resource.description}" if resource.description
 
-        {
-          response_type: :in_channel,
-          text: message
-        }
+        in_channel_response(message)
       end
 
       def multiple_resources(resources)
         message = "Multiple results were found:\n"
-        message << resources.map { |resource| "  #{title(resource)}" }.join("\n")
+        message << resources.map { |resource| "- #{title(resource)}" }.join("\n")
 
-        {
-          response_type: :ephemeral,
-          text: message
-        }
+        ephemeral_response(message)
+      end
+
+      def error(resource)
+        message = "The action was not succesfull because:\n"
+        message << resource.errors.messages.map { |message| "- #{message}" }.join("\n")
+
+        ephemeral_response(resource.errors.messages.join("\n")
       end
 
       def title(resource)
-        "### [#{resource.to_reference} #{resource.title}](#{url(resource)})"
+        "[#{resource.to_reference} #{resource.title}](#{url(resource)})"
       end
 
       def url(resource)
-        helper = Rails.application.routes.url_helpers
+        polymorphic_url(
+          [
+            resource.project.namespace.becomes(Namespace),
+            resource.project,
+           resource 
+          ],
+          id: resource_id,
+          routing_type: :url
+        )
+      end
 
-        case resource
-        when Issue
-          helper.namespace_project_issue_url(resource.project.namespace.becomes(Namespace), resource.project, resource)
-        when MergeRequest
-          helper.namespace_project_merge_request_url(resource.project.namespace.becomes(Namespace), resource.project, resource)
-        end
+      def ephemeral_response(message)
+        {
+          response_type: :ephemeral,
+          text: message
+        }
+      end
+
+      def in_channel_response(message)
+        {
+          response_type: :in_channel,
+          text: message
+        }
       end
     end
   end