From 8c8bc07d32f1103bb7996b499ead6ad6eb5bd337 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" <git@zjvandeweg.nl> Date: Tue, 15 Nov 2016 21:50:27 +0100 Subject: [PATCH] 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) --- .../mattermost/commands/base_service.rb | 8 +- .../commands/issue_create_service.rb | 16 +++- .../commands/issue_search_service.rb | 4 +- .../mattermost/commands/issue_service.rb | 6 +- .../mattermost/commands/issue_show_service.rb | 5 +- .../commands/merge_request_search_service.rb | 6 +- .../commands/merge_request_service.rb | 8 +- .../commands/merge_request_show_service.rb | 7 +- .../mattermost/slash_command_service.rb | 19 ++++- lib/mattermost/presenter.rb | 80 +++++++++--------- .../commands/issue_create_service_spec.rb | 35 ++++++++ .../commands/issue_search_service_spec.rb | 39 +++++++++ .../mattermost/commands/issue_service_spec.rb | 83 ------------------- .../commands/issue_show_service_spec.rb | 31 +++++++ .../merge_request_search_service_spec.rb | 30 +++++++ .../commands/merge_request_service_spec.rb | 68 --------------- .../merge_request_show_service_spec.rb | 30 +++++++ .../mattermost/slash_command_service_spec.rb | 21 ++--- 18 files changed, 255 insertions(+), 241 deletions(-) create mode 100644 spec/services/mattermost/commands/issue_create_service_spec.rb create mode 100644 spec/services/mattermost/commands/issue_search_service_spec.rb delete mode 100644 spec/services/mattermost/commands/issue_service_spec.rb create mode 100644 spec/services/mattermost/commands/issue_show_service_spec.rb create mode 100644 spec/services/mattermost/commands/merge_request_search_service_spec.rb delete mode 100644 spec/services/mattermost/commands/merge_request_service_spec.rb create mode 100644 spec/services/mattermost/commands/merge_request_show_service_spec.rb diff --git a/app/services/mattermost/commands/base_service.rb b/app/services/mattermost/commands/base_service.rb index 16bb92b9d0..c6bfd7c9ab 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 e52dad6f53..db3f868fc0 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 358a891ae6..072cb8e259 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 9ce2fac455..d27dcda21c 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 f8ca41b299..100b2780d0 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 842719685e..f675320dda 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 985ce649ca..8b81c9bbfd 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 9d9cb0f50f..c5b2850169 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 fc028d7853..0e74c929b9 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 d8e3b3805f..e1502e3f9b 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 0000000000..5ed86606b8 --- /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 0000000000..21934dfcdd --- /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 5ef363274a..0000000000 --- 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 0000000000..7578cd3a22 --- /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 0000000000..5212dc206a --- /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 39381520a9..0000000000 --- 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 0000000000..688737df0f --- /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 e1bfd073ef..14c2d9b7c3 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 -- 2.30.9