Incorporate feedback, improve presenter class

[ci skip]

app/models/project_services/chat_service.rb

@@ -18,8 +18,4 @@ class ChatService < Service
   def trigger(params)
     raise NotImplementedError
   end
-
-  def chat_user_params(params)
-    params.permit()
-  end
 end

app/models/project_services/mattermost_chat_service.rb → app/models/project_services/mattermost_command_service.rb

-class MattermostChatService < ChatService
+class MattermostCommandService < ChatService
   include TriggersHelper
 
   prop_accessor :token
@@ -8,7 +8,7 @@ class MattermostChatService < ChatService
   end
 
   def title
-    'Mattermost'
+    'Mattermost Command'
   end
 
   def description
@@ -16,7 +16,7 @@ class MattermostChatService < ChatService
   end
 
   def to_param
-    'mattermost'
+    'mattermost_command'
   end
 
   def help
@@ -33,10 +33,24 @@ class MattermostChatService < ChatService
   end
 
   def trigger(params)
-    user = ChatNames::FindUserService.new(chat_names, params).execute
-    return Mattermost::Presenter.authorize_chat_name(params) unless user
+    return nil unless valid_token?(params[:token])
 
-    Mattermost::CommandService.new(project, user, params.slice(:command, :text)).
-      execute
+    user = find_chat_user(params)
+    unless user
+      url = authorize_chat_name_url(params)
+      return Mattermost::Presenter.authorize_user(url)
+    end
+
+    Mattermost::CommandService.new(project, user, params).execute
+  end
+
+  private
+
+  def find_chat_user(params)
+    ChatNames::FindUserService.new(chat_names, params).execute
+  end
+
+  def authorize_chat_name_url(params)
+    ChatNames::RequestService.new(self, params).execute
   end
 end

app/models/service.rb

@@ -202,7 +202,6 @@ class Service < ActiveRecord::Base
       bamboo
       buildkite
       builds_email
-      pipelines_email
       bugzilla
       campfire
       custom_issue_tracker
@@ -214,7 +213,8 @@ class Service < ActiveRecord::Base
       hipchat
       irker
       jira
-      mattermost_chat
+      mattermost_command
+      pipelines_email
       pivotaltracker
       pushover
       redmine

config/environments/development.rb

@@ -45,7 +45,4 @@ Rails.application.configure do
 
   # Do not log asset requests
   config.assets.quiet = true
-
-  # Make hot reloading to work with Grape API
-  ActiveSupport::Dependencies.explicitly_unloadable_constants << "API"
 end

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

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)

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

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]
 

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

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

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

lib/mattermost/presenter.rb

 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

spec/lib/gitlab/chat_commands/issue_create_spec.rb

@@ -6,9 +6,13 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do
     let(:user)     { create(:user) }
     let(:regex_match) { described_class.match("issue create bird is the word") }
 
-    before { project.team << [user, :master] }
+    before do
+      project.team << [user, :master]
+    end
 
-    subject { described_class.new(project, user).execute(regex_match) }
+    subject do
+      described_class.new(project, user).execute(regex_match)
+    end
 
     context 'without description' do
       it 'creates the issue' do
@@ -17,7 +21,7 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do
         end.to change { project.issues.count }.by(1)
 
         expect(subject[:response_type]).to be :in_channel
-        expect(subject[:text]).to match 'bird is the word'
+        expect(subject[:text]).to match('bird is the word')
       end
     end
 
@@ -25,11 +29,29 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do
       let(:description) { "Surfin bird" }
       let(:regex_match) { described_class.match("issue create bird is the word\n#{description}") }
 
-      before { subject }
+      before do
+        subject
+      end
 
       it 'creates the issue with description' do
-        expect(Issue.last.description).to eq description
+        expect(Issue.last.description).to eq(description)
       end
     end
   end
+
+  describe 'self.match' do
+    it 'matches the title without description' do
+      match = described_class.match("issue create my title")
+
+      expect(match[:title]).to eq('my title')
+      expect(match[:description]).to eq("")
+    end
+
+    it 'matches the title with description' do
+      match = described_class.match("issue create my title\n\ndescription")
+
+      expect(match[:title]).to eq('my title')
+      expect(match[:description]).to eq('description')
+    end
+  end
 end

spec/lib/gitlab/chat_commands/issue_show_spec.rb

@@ -2,19 +2,23 @@ require 'spec_helper'
 
 describe Gitlab::ChatCommands::IssueShow, service: true do
   describe '#execute' do
-    let(:issue)    { create(:issue) }
-    let(:project)  { issue.project }
-    let(:user)     { issue.author }
+    let(:issue) { create(:issue) }
+    let(:project) { issue.project }
+    let(:user) { issue.author }
     let(:regex_match) { described_class.match("issue show #{issue.iid}") }
 
-    before { project.team << [user, :master] }
+    before do
+      project.team << [user, :master]
+    end
 
-    subject { described_class.new(project, user).execute(regex_match) }
+    subject do
+      described_class.new(project, user).execute(regex_match)
+    end
 
     context 'the issue exists' do
       it 'returns the issue' do
-        expect(subject[:response_type]).to be :in_channel
-        expect(subject[:text]).to match issue.title
+        expect(subject[:response_type]).to be(:in_channel)
+        expect(subject[:text]).to match(issue.title)
       end
     end
 
@@ -22,9 +26,17 @@ describe Gitlab::ChatCommands::IssueShow, service: true do
       let(:regex_match) { described_class.match("issue show 1234") }
 
       it "returns nil" do
-        expect(subject[:response_type]).to be :ephemeral
-        expect(subject[:text]).to start_with '404 not found!'
+        expect(subject[:response_type]).to be(:ephemeral)
+        expect(subject[:text]).to start_with('404 not found!')
       end
     end
   end
+
+  describe 'self.match' do
+    it 'matches the iid' do
+      match = described_class.match("issue show 123")
+
+      expect(match[:iid]).to eq("123")
+    end
+  end
 end

spec/lib/gitlab/chat_commands/merge_request_search_spec.rb

@@ -7,14 +7,18 @@ describe Gitlab::ChatCommands::MergeRequestSearch, service: true do
     let(:user)           { merge_request.author }
     let(:regex_match)    { described_class.match("mergerequest search #{merge_request.title}") }
 
-    before { project.team << [user, :master] }
+    before do
+      project.team << [user, :master]
+    end
 
-    subject { described_class.new(project, user, {}).execute(regex_match) }
+    subject do
+      described_class.new(project, user).execute(regex_match)
+    end
 
     context 'the merge request exists' do
       it 'returns the merge request' do
-        expect(subject[:response_type]).to be :in_channel
-        expect(subject[:text]).to match merge_request.title
+        expect(subject[:response_type]).to be(:in_channel)
+        expect(subject[:text]).to match(merge_request.title)
       end
     end
 
@@ -22,8 +26,8 @@ describe Gitlab::ChatCommands::MergeRequestSearch, service: true do
       let(:regex_match) { described_class.match("mergerequest search 12334") }
 
       it "returns a 404 message" do
-        expect(subject[:response_type]).to be :ephemeral
-        expect(subject[:text]).to start_with '404 not found!'
+        expect(subject[:response_type]).to be(:ephemeral)
+        expect(subject[:text]).to start_with('404 not found!')
       end
     end
   end