From 106b1e39c0d00f25e34dda0eba962c8cf1d43f1b Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 15 Nov 2016 09:55:56 +0100 Subject: 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. --- .../project_services/mattermost_chat_service.rb | 25 +------ app/services/mattermost/command_service.rb | 74 ------------------- app/services/mattermost/commands/base_service.rb | 63 ++++++---------- .../mattermost/commands/issue_create_service.rb | 13 ++++ .../mattermost/commands/issue_search_service.rb | 11 +++ app/services/mattermost/commands/issue_service.rb | 43 ++--------- .../mattermost/commands/issue_show_service.rb | 12 ++++ .../commands/merge_request_search_service.rb | 11 +++ .../mattermost/commands/merge_request_service.rb | 33 ++------- .../commands/merge_request_show_service.rb | 12 ++++ app/services/mattermost/slash_command_service.rb | 28 ++++++++ lib/mattermost/presenter.rb | 84 ++++++++++++++++++++++ spec/services/mattermost/command_service_spec.rb | 55 -------------- .../mattermost/slash_command_service_spec.rb | 30 ++++++++ 14 files changed, 234 insertions(+), 260 deletions(-) delete mode 100644 app/services/mattermost/command_service.rb create mode 100644 app/services/mattermost/commands/issue_create_service.rb create mode 100644 app/services/mattermost/commands/issue_search_service.rb create mode 100644 app/services/mattermost/commands/issue_show_service.rb create mode 100644 app/services/mattermost/commands/merge_request_search_service.rb create mode 100644 app/services/mattermost/commands/merge_request_show_service.rb create mode 100644 app/services/mattermost/slash_command_service.rb create mode 100644 lib/mattermost/presenter.rb delete mode 100644 spec/services/mattermost/command_service_spec.rb create mode 100644 spec/services/mattermost/slash_command_service_spec.rb 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 + 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 " - message << "issue search " - 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 (?.*)\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 -- cgit v1.2.1