First steps on refactoring Mattermost Slash commands

Now, each subcommand has its own service, plus I've introduced
presenters to be able to delegate the generation of the views.

14 files changed, 234 insertions, 260 deletions

diff --git a/app/models/project_services/mattermost_chat_service.rb b/app/models/project_services/mattermost_chat_service.rb
index 01ff6b0f8b4..5e9f4d255b6 100644
--- a/app/models/project_services/mattermost_chat_service.rb
+++ b/app/models/project_services/mattermost_chat_service.rb
@@ -33,31 +33,10 @@ class MattermostChatService < ChatService
   end
 
   def trigger(params)
-    return nil unless valid_token?(params[:token])
-
-    user = find_chat_user(params)
-    return authorize_chat_name(params) unless user
+    user = ChatNames::FindUserService.new(chat_names, params).execute
+    return Mattermost::Presenter.authorize_chat_name(params) unless user
 
     Mattermost::CommandService.new(project, user, params.slice(:command, :text)).
       execute
   end
-
-  private
-
-  def find_chat_user(params)
-    params = params.slice(:team_id, :user_id)
-    ChatNames::FindUserService.
-      new(chat_names, params).
-      execute
-  end
-
-  def authorize_chat_name(params)
-    params = params.slice(:team_id, :team_domain, :user_id, :user_name)
-    url = ChatNames::AuthorizeUserService.new(self, params).execute
-
-    {
-      response_type: :ephemeral,
-      message: "You are not authorized. Click this [link](#{url}) to authorize."
-    }
-  end
 end
diff --git a/app/services/mattermost/command_service.rb b/app/services/mattermost/command_service.rb
deleted file mode 100644
index 2a104174d97..00000000000
--- a/app/services/mattermost/command_service.rb
+++ /dev/null
@@ -1,74 +0,0 @@
-module Mattermost
-  class CommandService < BaseService
-    SERVICES = [
-      Mattermost::Commands::IssueService,
-      Mattermost::Commands::MergeRequestService
-    ]
-
-    def execute
-      return unknown_user unless current_user
-      return not_found_404 unless can?(current_user, :read_project, project)
-
-      triggered_command = command
-      service = SERVICES.find do |service|
-        service.triggered_by?(triggered_command) && service.available?(project)
-      end
-
-      if service
-        present service.new(project, current_user, params).execute
-      else
-        help_message
-      end
-    end
-
-    private
-
-    def command
-      params[:text].split.first
-    end
-
-    def present(result)
-      return not_found_404 unless result
-
-      if result.respond_to?(:count)
-        if count > 1
-          # TODO
-          return resource_list(result)
-        else
-          result = result.first
-        end
-      end
-
-      message = "### [#{result.to_reference} #{result.title}](link(result))"
-      message << "\n\n#{result.description}" if result.description
-
-      {
-        response_type: :in_channel,
-        text: message
-      }
-    end
-
-    def unknown_user
-      {
-        response_type: :ephemeral,
-        text: 'Hi there! I have not yet had the pleasure to get acquainted!' # TODO allow user to authenticate and authorize
-      }
-    end
-
-    def not_found_404
-      {
-        response_type: :ephemeral,
-        text: "404 not found! GitLab couldn't find what your were looking for! :boom:",
-      }
-    end
-
-    def help_message
-      command_help_messages = SERVICES.map { |service| service.help_message(project) }
-
-      {
-        response_type: :ephemeral,
-        text: "Sadly, the used command does not exist, lets take a look at your options here:\n\n#{command_help_messages.join("\n")}"
-      }
-    end
-  end
-end
diff --git a/app/services/mattermost/commands/base_service.rb b/app/services/mattermost/commands/base_service.rb
index 54d8fa088b8..16bb92b9d0d 100644
--- a/app/services/mattermost/commands/base_service.rb
+++ b/app/services/mattermost/commands/base_service.rb
@@ -1,59 +1,42 @@
 module Mattermost
   module Commands
     class BaseService < ::BaseService
-      class << self
-        def triggered_by?(_)
-          raise NotImplementedError
-        end
-
-        def available?(_)
-          raise NotImplementedError
-        end
-
-        def help_message(_)
-          NotImplementedError
-        end
-      end
-
       QUERY_LIMIT = 5
 
       def execute
-        subcommand, args = parse_command
+        raise NotImplementedError
+      end
 
-        if subcommands.include?(subcommand)
-          send(subcommand, args)
-        else
-          nil
-        end
+      def available?
+        raise NotImplementedError
+      end
+
+      def collection
+        raise NotImplementedError
       end
 
       private
 
-      # This method can only be used by a resource that has an iid. Also, the
-      # class should implement #collection itself. Probably project.resource
-      # would suffice
-      def show(args)
-        iid = args.first
-
-        result = collection.find_by(iid: iid)
-        if readable?(result)
-          result
-        else
-          nil
-        end
-      end
+      def find_by_iid(iid)
+        resource = collection.find_by(iid: iid)
 
-      # Child class should implement #collection
-      def search(args)
-        query = args.join(' ')
+        readable?(resource) ? resource : nil
+      end
 
-        collection.search(query).limit(QUERY_LIMIT).select do |issuable|
-          readable?(issuable)
+      def search
+        collection.search(query).limit(QUERY_LIMIT).select do |resource|
+          readable?(resource)
         end
       end
 
-      def command
-        params[:text]
+      # params[:text] = issue search <search query>
+      def query
+        params[:text].split[2..-1].join(' ')
+      end
+
+      # params[:text] = 'mergerequest show 123'
+      def iid
+        params[:text].split[2]
       end
     end
   end
diff --git a/app/services/mattermost/commands/issue_create_service.rb b/app/services/mattermost/commands/issue_create_service.rb
new file mode 100644
index 00000000000..e52dad6f532
--- /dev/null
+++ b/app/services/mattermost/commands/issue_create_service.rb
@@ -0,0 +1,13 @@
+module Mattermost
+  module Commands
+    class IssueShowService < Mattermost::Commands::BaseService
+      def execute
+        return Mattermost::Messages::Issues.not_available unless available?
+
+        issue = find_by_iid(iid)
+
+        present issue
+      end
+    end
+  end
+end
diff --git a/app/services/mattermost/commands/issue_search_service.rb b/app/services/mattermost/commands/issue_search_service.rb
new file mode 100644
index 00000000000..358a891ae60
--- /dev/null
+++ b/app/services/mattermost/commands/issue_search_service.rb
@@ -0,0 +1,11 @@
+module Mattermost
+  module Commands
+    class IssueShowService < IssueService
+      def execute
+        return Mattermost::Messages::Issues.not_available unless available?
+
+        present search_results
+      end
+    end
+  end
+end
diff --git a/app/services/mattermost/commands/issue_service.rb b/app/services/mattermost/commands/issue_service.rb
index 17407355547..9ce2fac455b 100644
--- a/app/services/mattermost/commands/issue_service.rb
+++ b/app/services/mattermost/commands/issue_service.rb
@@ -1,39 +1,8 @@
 module Mattermost
   module Commands
     class IssueService < Mattermost::Commands::BaseService
-      class << self
-        def triggered_by?(command)
-          command == 'issue'
-        end
-
-        def available?(project)
-          project.issues_enabled? && project.default_issues_tracker?
-        end
-
-        def help_message(project)
-          return nil unless available?(project)
-
-          message = "issue show <issue_id>"
-          message << "issue search <query>"
-        end
-      end
-
-      private
-
-      def create(_)
-        return nil unless can?(current_user, :create_issue, project)
-
-        # We parse again as the previous split splits on continues whitespace
-        # per the ruby spec, but we loose information on where the new lines were
-        match = command.match(/\Aissue create (?<title>.*)\n*/)
-        title = match[:title]
-        description = match.post_match
-
-        Issues::CreateService.new(project, current_user, title: title, description: description).execute
-      end
-
-      def subcommands
-        %w[create search show]
+      def available?
+        project.issues_enabled? && project.default_issues_tracker?
       end
 
       def collection
@@ -44,12 +13,8 @@ module Mattermost
         can?(current_user, :read_issue, issue)
       end
 
-      def parse_command
-        split = command.split
-        subcommand = split[1]
-        args = split[2..-1]
-
-        [subcommand, args]
+      def present
+        Mattermost::Presenter.issue
       end
     end
   end
diff --git a/app/services/mattermost/commands/issue_show_service.rb b/app/services/mattermost/commands/issue_show_service.rb
new file mode 100644
index 00000000000..f8ca41b2992
--- /dev/null
+++ b/app/services/mattermost/commands/issue_show_service.rb
@@ -0,0 +1,12 @@
+module Mattermost
+  module Commands
+    class IssueShowService < IssueService
+      def execute
+        return Mattermost::Messages.not_available unless available?
+
+        issue = find_by_iid(iid)
+        present issue
+      end
+    end
+  end
+end
diff --git a/app/services/mattermost/commands/merge_request_search_service.rb b/app/services/mattermost/commands/merge_request_search_service.rb
new file mode 100644
index 00000000000..842719685e4
--- /dev/null
+++ b/app/services/mattermost/commands/merge_request_search_service.rb
@@ -0,0 +1,11 @@
+module Mattermost
+  module Commands
+    class MergeRequestService < Mattermost::Commands::BaseService
+      def execute
+        return Mattermost::Messages::MergeRequests.not_available unless available?
+
+        Mattermost::Messages::IssuePresenter.present search_results
+      end
+    end
+  end
+end
diff --git a/app/services/mattermost/commands/merge_request_service.rb b/app/services/mattermost/commands/merge_request_service.rb
index a27b3861b23..985ce649cac 100644
--- a/app/services/mattermost/commands/merge_request_service.rb
+++ b/app/services/mattermost/commands/merge_request_service.rb
@@ -1,27 +1,8 @@
 module Mattermost
   module Commands
     class MergeRequestService < Mattermost::Commands::BaseService
-      class << self
-        def triggered_by?(command)
-          command == 'mergerequest'
-        end
-
-        def available?(project)
-          project.merge_requests_enabled?
-        end
-
-        def help_message(project)
-          return nil unless available?(project)
-
-          message = "mergerequest show <merge request id>\n"
-          message << "mergerequest search <query>"
-        end
-      end
-
-      private
-
-      def subcommands
-        %w[show search]
+      def available?
+        project.issues_enabled? && project.default_issues_tracker?
       end
 
       def collection
@@ -32,14 +13,8 @@ module Mattermost
         can?(current_user, :read_merge_request, project)
       end
 
-      # 'mergerequest show 123' => 'show', ['123']
-      # 'mergerequest search my query' => 'search',['my', 'query']
-      def parse_command
-        split = command.split
-        subcommand = split[1]
-        args = split[2..-1]
-
-        [subcommand, args]
+      def present
+        Mattermost::Presenter.merge_request
       end
     end
   end
diff --git a/app/services/mattermost/commands/merge_request_show_service.rb b/app/services/mattermost/commands/merge_request_show_service.rb
new file mode 100644
index 00000000000..9d9cb0f50f6
--- /dev/null
+++ b/app/services/mattermost/commands/merge_request_show_service.rb
@@ -0,0 +1,12 @@
+module Mattermost
+  module Commands
+    class MergeRequestShowService < Mattermost::Commands::BaseService
+      def execute
+        return Mattermost::Messages.not_available unless available?
+
+        merge_request = find_by_iid(iid)
+        present merge_request
+      end
+    end
+  end
+end
diff --git a/app/services/mattermost/slash_command_service.rb b/app/services/mattermost/slash_command_service.rb
new file mode 100644
index 00000000000..fc028d78537
--- /dev/null
+++ b/app/services/mattermost/slash_command_service.rb
@@ -0,0 +1,28 @@
+module Mattermost
+  class SlashCommandService < BaseService
+    def self.command(command, sub_command, klass, help_message)
+      registry[command][sub_command] = { klass: klass, help_message: help_message }
+    end
+
+    command 'issue', 'show',    Mattermost::Commands::IssueShowService,   'issue show <id>'
+    command 'issue', 'search',  Mattermost::Commands::IssueSearchService, 'issue search <query>'
+    command 'issue', 'create',  Mattermost::Commands::IssueCreateService, 'issue create my title'
+
+    command 'mergerequest', 'show',   Mattermost::Commands::MergeRequestShowService,    'mergerequest show <id>'
+    command 'mergerequest', 'search', Mattermost::Commands::MergeRequestSearchService,  'mergerequest search <query>'
+
+    def execute
+      service = registry[command][subcommand]
+
+      return help_messages(registry) unless service.try(:available?)
+
+      service.new(project, current_user, params).execute
+    end
+
+    private
+
+    def self.registry
+      @registry ||= Hash.new({})
+    end
+  end
+end
diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb
new file mode 100644
index 00000000000..d8e3b3805f9
--- /dev/null
+++ b/lib/mattermost/presenter.rb
@@ -0,0 +1,84 @@
+module Mattermost
+  class Presenter
+    class << self
+      COMMAND_PREFIX = '/gitlab'.freeze
+
+      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."
+        }
+      end
+
+      # TODO figure out how I know which are available or not
+      def help_message(commands)
+        messages = ["Available commands:"]
+
+        commands.each do |sub_command, attrs|
+          messages << "\t#{COMMAND_PREFIX} #{attrs[:help_message]}"
+        end
+
+        {
+          response_type: :ephemeral,
+          text: messages.join("\n")
+        }
+      end
+
+      def not_found
+        {
+          response_type: :ephemeral,
+          text: "404 not found! GitLab couldn't find what your were looking for! :boom:",
+        }
+      end
+    end
+
+    attr_reader :result
+
+    def initialize(result)
+      @result = result
+    end
+
+    def present
+      if result.respond_to?(:count)
+        if result.count > 1
+          return respond_collection(result)
+        elsif result.count == 0
+          return not_found
+        else
+          result = result.first
+        end
+      end
+
+      single_resource
+    end
+
+    private
+
+    def single_resource
+      message = title(resource)
+      message << "\n\n#{resource.description}" if resource.description
+
+      {
+        response_type: :in_channel,
+        text: message
+      }
+    end
+
+    def multiple_resources(resources)
+      message = "Multiple results were found:\n"
+      message << resource.map { |resource| "  #{title(resource)}" }.join("\n")
+
+      {
+        response_type: :ephemeral,
+        text: message
+      }
+    end
+
+    def title(resource)
+      url = url_for([resource.project.namespace.becomes(Namespace), resource.project, resource])
+      "### [#{resource.to_reference} #{resource.title}](#{url})"
+    end
+  end
+end
diff --git a/spec/services/mattermost/command_service_spec.rb b/spec/services/mattermost/command_service_spec.rb
deleted file mode 100644
index 35051c09f8d..00000000000
--- a/spec/services/mattermost/command_service_spec.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-require 'spec_helper'
-
-describe Mattermost::CommandService, service: true do
-  let(:project)         { build(:project) }
-  let(:user)            { build(:user) }
-  let(:params)          { { text: 'issue show 1' } }
-
-  subject { described_class.new(project, user, params).execute }
-
-  describe '#execute' do
-    context 'no user could be found' do
-      let(:user) { nil }
-
-      it 'asks the user to introduce him/herself' do
-        expect(subject[:response_type]).to be :ephemeral
-        expect(subject[:text]).to start_with 'Hi there!'
-      end
-    end
-
-    context 'no project could be found' do
-      it 'shows a 404 not found message' do
-        expect(subject[:response_type]).to be :ephemeral
-        expect(subject[:text]).to start_with '404 not found!'
-      end
-    end
-
-    context 'the user has access to the project' do
-      let(:project) { create(:project) }
-      let(:user)    { create(:user) }
-
-      before do
-        project.team << [user, :master]
-      end
-
-      context 'no command service is triggered' do
-        let(:params) { { text: 'unknown_command' } }
-
-        it 'shows the help messages' do
-          expect(subject[:response_type]).to be :ephemeral
-          expect(subject[:text]).to start_with 'Sadly, the used command'
-        end
-      end
-
-      context 'a valid command is executed' do
-        let(:issue)   { create(:issue, project: project) }
-        let(:params)  { { text: "issue show #{issue.iid}" } }
-
-        it 'a resource is presented to the user' do
-          expect(subject[:response_type]).to be :in_channel
-          expect(subject[:text]).to match issue.title
-        end
-      end
-    end
-  end
-end
diff --git a/spec/services/mattermost/slash_command_service_spec.rb b/spec/services/mattermost/slash_command_service_spec.rb
new file mode 100644
index 00000000000..e1bfd073ef4
--- /dev/null
+++ b/spec/services/mattermost/slash_command_service_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe Mattermost::SlashCommandService, service: true do
+  let(:project)         { build(:project) }
+  let(:user)            { build(:user) }
+  let(:params)          { { text: 'issue show 1' } }
+
+  subject { described_class.new(project, user, params).execute }
+
+  describe '#execute' do
+    context 'no command service is triggered' do
+      let(:params) { { text: 'unknown_command' } }
+
+      it 'shows the help messages' do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with 'Sadly, the used command'
+      end
+    end
+
+    context 'a valid command is executed' do
+      let(:issue)   { create(:issue, project: project) }
+      let(:params)  { { text: "issue show #{issue.iid}" } }
+
+      it 'a resource is presented to the user' do
+        expect(subject[:response_type]).to be :in_channel
+        expect(subject[:text]).to match issue.title
+      end
+    end
+  end
+end