Refactor and test Slash commands

This is the structure Kamil proposed, which leaves us with a bunch of
smaller classes. This commits deletes outdated files and tests
everything from the SlashCommandService and down (child classes and
subcommands)

18 files changed, 255 insertions, 241 deletions

diff --git a/app/services/mattermost/commands/base_service.rb b/app/services/mattermost/commands/base_service.rb
index 16bb92b9d0d..c6bfd7c9ab4 100644
--- a/app/services/mattermost/commands/base_service.rb
+++ b/app/services/mattermost/commands/base_service.rb
@@ -17,13 +17,17 @@ module Mattermost
 
       private
 
-      def find_by_iid(iid)
+      def present(resource)
+        Mattermost::Presenter.present(resource)
+      end
+
+      def find_by_iid
         resource = collection.find_by(iid: iid)
 
         readable?(resource) ? resource : nil
       end
 
-      def search
+      def search_results
         collection.search(query).limit(QUERY_LIMIT).select do |resource|
           readable?(resource)
         end
diff --git a/app/services/mattermost/commands/issue_create_service.rb b/app/services/mattermost/commands/issue_create_service.rb
index e52dad6f532..db3f868fc09 100644
--- a/app/services/mattermost/commands/issue_create_service.rb
+++ b/app/services/mattermost/commands/issue_create_service.rb
@@ -1,12 +1,20 @@
 module Mattermost
   module Commands
-    class IssueShowService < Mattermost::Commands::BaseService
+    class IssueCreateService < IssueService
       def execute
-        return Mattermost::Messages::Issues.not_available unless available?
+        title, description = parse_command
 
-        issue = find_by_iid(iid)
+        present Issues::CreateService.new(project, current_user, title: title, description: description).execute
+      end
+
+      private
+
+      def parse_command
+        match = params[:text].match(/\Aissue create (?<title>.*)\n*/)
+        title = match[:title]
+        description = match.post_match
 
-        present issue
+        [title, description]
       end
     end
   end
diff --git a/app/services/mattermost/commands/issue_search_service.rb b/app/services/mattermost/commands/issue_search_service.rb
index 358a891ae60..072cb8e2590 100644
--- a/app/services/mattermost/commands/issue_search_service.rb
+++ b/app/services/mattermost/commands/issue_search_service.rb
@@ -1,9 +1,7 @@
 module Mattermost
   module Commands
-    class IssueShowService < IssueService
+    class IssueSearchService < IssueService
       def execute
-        return Mattermost::Messages::Issues.not_available unless available?
-
         present search_results
       end
     end
diff --git a/app/services/mattermost/commands/issue_service.rb b/app/services/mattermost/commands/issue_service.rb
index 9ce2fac455b..d27dcda21c4 100644
--- a/app/services/mattermost/commands/issue_service.rb
+++ b/app/services/mattermost/commands/issue_service.rb
@@ -1,7 +1,7 @@
 module Mattermost
   module Commands
     class IssueService < Mattermost::Commands::BaseService
-      def available?
+      def self.available?(project)
         project.issues_enabled? && project.default_issues_tracker?
       end
 
@@ -12,10 +12,6 @@ module Mattermost
       def readable?(issue)
         can?(current_user, :read_issue, issue)
       end
-
-      def present
-        Mattermost::Presenter.issue
-      end
     end
   end
 end
diff --git a/app/services/mattermost/commands/issue_show_service.rb b/app/services/mattermost/commands/issue_show_service.rb
index f8ca41b2992..100b2780d0e 100644
--- a/app/services/mattermost/commands/issue_show_service.rb
+++ b/app/services/mattermost/commands/issue_show_service.rb
@@ -2,10 +2,7 @@ module Mattermost
   module Commands
     class IssueShowService < IssueService
       def execute
-        return Mattermost::Messages.not_available unless available?
-
-        issue = find_by_iid(iid)
-        present issue
+        present find_by_iid
       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
index 842719685e4..f675320ddae 100644
--- a/app/services/mattermost/commands/merge_request_search_service.rb
+++ b/app/services/mattermost/commands/merge_request_search_service.rb
@@ -1,10 +1,8 @@
 module Mattermost
   module Commands
-    class MergeRequestService < Mattermost::Commands::BaseService
+    class MergeRequestSearchService < MergeRequestService
       def execute
-        return Mattermost::Messages::MergeRequests.not_available unless available?
-
-        Mattermost::Messages::IssuePresenter.present search_results
+        present search_results
       end
     end
   end
diff --git a/app/services/mattermost/commands/merge_request_service.rb b/app/services/mattermost/commands/merge_request_service.rb
index 985ce649cac..8b81c9bbfd9 100644
--- a/app/services/mattermost/commands/merge_request_service.rb
+++ b/app/services/mattermost/commands/merge_request_service.rb
@@ -1,8 +1,8 @@
 module Mattermost
   module Commands
     class MergeRequestService < Mattermost::Commands::BaseService
-      def available?
-        project.issues_enabled? && project.default_issues_tracker?
+      def self.available?(project)
+        project.merge_requests_enabled?
       end
 
       def collection
@@ -12,10 +12,6 @@ module Mattermost
       def readable?(_)
         can?(current_user, :read_merge_request, project)
       end
-
-      def present
-        Mattermost::Presenter.merge_request
-      end
     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
index 9d9cb0f50f6..c5b2850169a 100644
--- a/app/services/mattermost/commands/merge_request_show_service.rb
+++ b/app/services/mattermost/commands/merge_request_show_service.rb
@@ -1,11 +1,8 @@
 module Mattermost
   module Commands
-    class MergeRequestShowService < Mattermost::Commands::BaseService
+    class MergeRequestShowService < MergeRequestService
       def execute
-        return Mattermost::Messages.not_available unless available?
-
-        merge_request = find_by_iid(iid)
-        present merge_request
+        present find_by_iid
       end
     end
   end
diff --git a/app/services/mattermost/slash_command_service.rb b/app/services/mattermost/slash_command_service.rb
index fc028d78537..0e74c929b9b 100644
--- a/app/services/mattermost/slash_command_service.rb
+++ b/app/services/mattermost/slash_command_service.rb
@@ -1,5 +1,9 @@
 module Mattermost
   class SlashCommandService < BaseService
+    def self.registry
+      @registry ||= Hash.new({})
+    end
+
     def self.command(command, sub_command, klass, help_message)
       registry[command][sub_command] = { klass: klass, help_message: help_message }
     end
@@ -12,17 +16,24 @@ module Mattermost
     command 'mergerequest', 'search', Mattermost::Commands::MergeRequestSearchService,  'mergerequest search <query>'
 
     def execute
-      service = registry[command][subcommand]
+      command, subcommand = parse_command
+
+      #TODO think how to do this to support ruby 2.1
+      service = registry.dig(command, subcommand, :klass)
 
-      return help_messages(registry) unless service.try(:available?)
+      return help_messages(registry) unless service.try(:available?, project)
 
       service.new(project, current_user, params).execute
     end
 
     private
 
-    def self.registry
-      @registry ||= Hash.new({})
+    def parse_command
+      params[:text].split.first(2)
+    end
+
+    def registry
+      self.class.registry
     end
   end
 end
diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb
index d8e3b3805f9..e1502e3f9ba 100644
--- a/lib/mattermost/presenter.rb
+++ b/lib/mattermost/presenter.rb
@@ -32,53 +32,59 @@ module Mattermost
           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
+      def present(resource)
+        return not_found unless resource
+
+        if resource.respond_to?(:count)
+          if resource.count > 1
+            return multiple_resources(resource)
+          elsif resource.count == 0
+            return not_found
+          else
+            resource = resource.first
+          end
         end
+
+        single_resource(resource)
       end
 
-      single_resource
-    end
+      private
 
-    private
+      def single_resource(resource)
+        message = title(resource)
+        message << "\n\n#{resource.description}" if resource.description
 
-    def single_resource
-      message = title(resource)
-      message << "\n\n#{resource.description}" if resource.description
+        {
+          response_type: :in_channel,
+          text: message
+        }
+      end
 
-      {
-        response_type: :in_channel,
-        text: message
-      }
-    end
+      def multiple_resources(resources)
+        message = "Multiple results were found:\n"
+        message << resources.map { |resource| "  #{title(resource)}" }.join("\n")
 
-    def multiple_resources(resources)
-      message = "Multiple results were found:\n"
-      message << resource.map { |resource| "  #{title(resource)}" }.join("\n")
+        {
+          response_type: :ephemeral,
+          text: message
+        }
+      end
 
-      {
-        response_type: :ephemeral,
-        text: message
-      }
-    end
+      def title(resource)
+        "### [#{resource.to_reference} #{resource.title}](#{url(resource)})"
+      end
+
+      def url(resource)
+        helper = Rails.application.routes.url_helpers
 
-    def title(resource)
-      url = url_for([resource.project.namespace.becomes(Namespace), resource.project, resource])
-      "### [#{resource.to_reference} #{resource.title}](#{url})"
+        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
+      end
     end
   end
 end
diff --git a/spec/services/mattermost/commands/issue_create_service_spec.rb b/spec/services/mattermost/commands/issue_create_service_spec.rb
new file mode 100644
index 00000000000..5ed86606b8f
--- /dev/null
+++ b/spec/services/mattermost/commands/issue_create_service_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Mattermost::Commands::IssueCreateService, service: true do
+  describe '#execute' do
+    let(:project)  { create(:empty_project) }
+    let(:user)     { create(:user) }
+    let(:params)   { { text: "issue create bird is the word" } }
+
+    before { project.team << [user, :master] }
+
+    subject { described_class.new(project, user, params).execute }
+
+    context 'without description' do
+      it 'creates the issue' do
+        expect do
+          subject # this trigger the execution
+        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'
+      end
+    end
+
+    context 'with description' do
+      let(:description) { "Surfin bird" }
+      let(:params) { { text: "issue create The bird is the word\n#{description}" } }
+
+      before { subject }
+
+      it 'creates the issue with description' do
+        expect(Issue.last.description).to eq description
+      end
+    end
+  end
+end
diff --git a/spec/services/mattermost/commands/issue_search_service_spec.rb b/spec/services/mattermost/commands/issue_search_service_spec.rb
new file mode 100644
index 00000000000..21934dfcdd0
--- /dev/null
+++ b/spec/services/mattermost/commands/issue_search_service_spec.rb
@@ -0,0 +1,39 @@
+require 'spec_helper'
+
+describe Mattermost::Commands::IssueSearchService, service: true do
+  describe '#execute' do
+    let!(:issue)    { create(:issue, title: 'The bird is the word') }
+    let(:project)  { issue.project }
+    let(:user)     { issue.author }
+    let(:params)   { { text: "issue search bird is the" } }
+
+    before { project.team << [user, :master] }
+
+    subject { described_class.new(project, user, params).execute }
+
+    context 'without results' do
+      let(:params) { { text: "issue search no results for this one" } }
+
+      it "returns nil" do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with '404 not found!'
+      end
+    end
+
+    context 'with 1 result' do
+      it 'returns the issue' do
+        expect(subject[:response_type]).to be :in_channel
+        expect(subject[:text]).to match issue.title
+      end
+    end
+
+    context 'with 2 or more results' do
+      let!(:issue2) { create(:issue, project: project, title: 'bird is the word!') }
+
+      it 'returns multiple resources' do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with 'Multiple results were found'
+      end
+    end
+  end
+end
diff --git a/spec/services/mattermost/commands/issue_service_spec.rb b/spec/services/mattermost/commands/issue_service_spec.rb
deleted file mode 100644
index 5ef363274ad..00000000000
--- a/spec/services/mattermost/commands/issue_service_spec.rb
+++ /dev/null
@@ -1,83 +0,0 @@
-require 'spec_helper'
-
-describe Mattermost::Commands::IssueService do
-  let(:project) { create(:project) }
-  let(:issue )  { create(:issue, :confidential, title: 'Bird is the word', project: project) }
-  let(:user)    { issue.author }
-
-  subject { described_class.new(project, user, params).execute }
-
-  before do
-    project.team << [user, :developer]
-  end
-
-  describe '#execute' do
-    context 'show as subcommand' do
-      context 'issue can be found' do
-        let(:params) { { text: "issue show #{issue.iid}" } }
-
-        it 'returns the merge request' do
-          expect(subject).to eq issue
-        end
-
-        context 'the user has no access' do
-          let(:non_member) { create(:user) }
-          subject { described_class.new(project, non_member, params).execute }
-
-          it 'returns nil' do
-            expect(subject).to eq nil
-          end
-        end
-      end
-
-      context 'issue can not be found' do
-        let(:params) { { text: 'issue show 12345' } }
-
-        it 'returns nil' do
-          expect(subject).to eq nil
-        end
-      end
-    end
-
-    context 'search as a subcommand' do
-      context 'with results' do
-        let(:params) { { text: "issue search is the word" } }
-
-        it 'returns the issue' do
-          expect(subject).to eq [issue]
-        end
-      end
-
-      context 'without results' do
-        let(:params) { { text: 'issue search mepmep' } }
-
-        it 'returns an empty collection' do
-          expect(subject).to eq []
-        end
-      end
-    end
-
-    context 'create as subcommand' do
-      let(:title)  { 'my new issue' }
-      let(:params) { { text: "issue create #{title}" } }
-
-      it 'return the new issue' do
-        expect(subject).to be_a Issue
-      end
-
-      it 'creates a new issue' do
-        expect { subject }.to change { Issue.count }.by(1)
-      end
-    end
-  end
-
-  describe 'help_message' do
-    context 'issues are disabled' do
-      it 'returns nil' do
-        allow(described_class).to receive(:available?).and_return false
-
-        expect(described_class.help_message(project)).to eq nil
-      end
-    end
-  end
-end
diff --git a/spec/services/mattermost/commands/issue_show_service_spec.rb b/spec/services/mattermost/commands/issue_show_service_spec.rb
new file mode 100644
index 00000000000..7578cd3a22c
--- /dev/null
+++ b/spec/services/mattermost/commands/issue_show_service_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Mattermost::Commands::IssueShowService, service: true do
+  describe '#execute' do
+    let(:issue)    { create(:issue) }
+    let(:project)  { issue.project }
+    let(:user)     { issue.author }
+    let(:params)   { { text: "issue show #{issue.iid}" } }
+
+    before { project.team << [user, :master] }
+
+    subject { described_class.new(project, user, params).execute }
+
+    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
+      end
+    end
+
+    context 'the issue does not exist' do
+      let(:params) { { text: "issue show 12345" } }
+
+      it "returns nil" do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with '404 not found!'
+      end
+    end
+  end
+end
diff --git a/spec/services/mattermost/commands/merge_request_search_service_spec.rb b/spec/services/mattermost/commands/merge_request_search_service_spec.rb
new file mode 100644
index 00000000000..5212dc206a6
--- /dev/null
+++ b/spec/services/mattermost/commands/merge_request_search_service_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe Mattermost::Commands::MergeRequestSearchService, service: true do
+  describe '#execute' do
+    let!(:merge_request) { create(:merge_request, title: 'The bird is the word') }
+    let(:project)        { merge_request.source_project }
+    let(:user)           { merge_request.author }
+    let(:params)         { { text: "mergerequest search #{merge_request.title}" } }
+
+    before { project.team << [user, :master] }
+
+    subject { described_class.new(project, user, params).execute }
+
+    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
+      end
+    end
+
+    context 'no results can be found' do
+      let(:params) { { text: "mergerequest search 12345" } }
+
+      it "returns a 404 message" do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with '404 not found!'
+      end
+    end
+  end
+end
diff --git a/spec/services/mattermost/commands/merge_request_service_spec.rb b/spec/services/mattermost/commands/merge_request_service_spec.rb
deleted file mode 100644
index 39381520a99..00000000000
--- a/spec/services/mattermost/commands/merge_request_service_spec.rb
+++ /dev/null
@@ -1,68 +0,0 @@
-require 'spec_helper'
-
-describe Mattermost::Commands::MergeRequestService do
-  let(:project)       { create(:project, :private) }
-  let(:merge_request) { create(:merge_request, title: 'Bird is the word', source_project: project) }
-  let(:user)          { merge_request.author }
-
-  subject { described_class.new(project, user, params).execute }
-
-  before do
-    project.team << [user, :developer]
-  end
-
-  context 'show as subcommand' do
-    context 'merge request can be found' do
-      let(:params) { { text: "mergerequest show #{merge_request.iid}" } }
-
-      it 'returns the merge request' do
-        expect(subject).to eq merge_request
-      end
-
-      context 'the user has no access' do
-        let(:non_member) { create(:user) }
-        subject { described_class.new(project, non_member, params).execute }
-
-        it 'returns nil' do
-          expect(subject).to eq nil
-        end
-      end
-    end
-
-    context 'merge request can not be found' do
-      let(:params) { { text: 'mergerequest show 12345' } }
-
-      it 'returns nil' do
-        expect(subject).to eq nil
-      end
-    end
-  end
-
-  context 'search as a subcommand' do
-    context 'with results' do
-      let(:params) { { text: "mergerequest search is the word" } }
-
-      it 'returns the merge_request' do
-        expect(subject).to eq [merge_request]
-      end
-    end
-
-    context 'without results' do
-      let(:params) { { text: 'mergerequest search mepmep' } }
-
-      it 'returns an empty collection' do
-        expect(subject).to eq []
-      end
-    end
-  end
-
-  describe 'help_message' do
-    context 'issues are disabled' do
-      it 'returns nil' do
-        allow(described_class).to receive(:available?).and_return false
-
-        expect(described_class.help_message(project)).to eq nil
-      end
-    end
-  end
-end
diff --git a/spec/services/mattermost/commands/merge_request_show_service_spec.rb b/spec/services/mattermost/commands/merge_request_show_service_spec.rb
new file mode 100644
index 00000000000..688737df0f5
--- /dev/null
+++ b/spec/services/mattermost/commands/merge_request_show_service_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe Mattermost::Commands::MergeRequestShowService, service: true do
+  describe '#execute' do
+    let!(:merge_request)  { create(:merge_request) }
+    let(:project)         { merge_request.source_project }
+    let(:user)            { merge_request.author }
+    let(:params)          { { text: "mergerequest show #{merge_request.iid}" } }
+
+    before { project.team << [user, :master] }
+
+    subject { described_class.new(project, user, params).execute }
+
+    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
+      end
+    end
+
+    context 'the merge request does not exist' do
+      let(:params) { { text: "mergerequest show 12345" } }
+
+      it "returns nil" do
+        expect(subject[:response_type]).to be :ephemeral
+        expect(subject[:text]).to start_with '404 not found!'
+      end
+    end
+  end
+end
diff --git a/spec/services/mattermost/slash_command_service_spec.rb b/spec/services/mattermost/slash_command_service_spec.rb
index e1bfd073ef4..14c2d9b7c37 100644
--- a/spec/services/mattermost/slash_command_service_spec.rb
+++ b/spec/services/mattermost/slash_command_service_spec.rb
@@ -7,23 +7,12 @@ describe Mattermost::SlashCommandService, service: true do
 
   subject { described_class.new(project, user, params).execute }
 
-  describe '#execute' do
-    context 'no command service is triggered' do
-      let(:params) { { text: 'unknown_command' } }
+  xdescribe '#execute' do
+    context 'when issue show is triggered' do
+      it 'calls IssueShowService' do
+        expect_any_instance_of(Mattermost::Commands::IssueShowService).to receive(:new).with(project, user, params)
 
-      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
+        subject
       end
     end
   end