diff options
author | Z.J. van de Weg <git@zjvandeweg.nl> | 2016-11-17 14:06:45 +0300 |
---|---|---|
committer | Z.J. van de Weg <git@zjvandeweg.nl> | 2016-11-17 23:34:24 +0300 |
commit | d4def9cbcd664b7067e7f9f4ea8be54463bd1d50 (patch) | |
tree | 10c5fe56054be36687146877b4000286334e76e7 | |
parent | 1b4fdb9893af28606b7594ee656438c7ef21e9d8 (diff) |
Incorporate feedback, improve presenter class
[ci skip]
-rw-r--r-- | app/models/project_services/chat_service.rb | 4 | ||||
-rw-r--r-- | app/models/project_services/mattermost_command_service.rb (renamed from app/models/project_services/mattermost_chat_service.rb) | 28 | ||||
-rw-r--r-- | app/models/service.rb | 4 | ||||
-rw-r--r-- | config/environments/development.rb | 3 | ||||
-rw-r--r-- | lib/api/services.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/base_command.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/command.rb | 22 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/issue_create.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/issue_search.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/merge_request_command.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/chat_commands/merge_request_search.rb | 2 | ||||
-rw-r--r-- | lib/mattermost/presenter.rb | 81 | ||||
-rw-r--r-- | spec/lib/gitlab/chat_commands/issue_create_spec.rb | 32 | ||||
-rw-r--r-- | spec/lib/gitlab/chat_commands/issue_show_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/chat_commands/merge_request_search_spec.rb | 16 |
15 files changed, 151 insertions, 91 deletions
diff --git a/app/models/project_services/chat_service.rb b/app/models/project_services/chat_service.rb index db387ef3026..d36beff5fa6 100644 --- a/app/models/project_services/chat_service.rb +++ b/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 diff --git a/app/models/project_services/mattermost_chat_service.rb b/app/models/project_services/mattermost_command_service.rb index 5e9f4d255b6..262dd3076f7 100644 --- a/app/models/project_services/mattermost_chat_service.rb +++ b/app/models/project_services/mattermost_command_service.rb @@ -1,4 +1,4 @@ -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 diff --git a/app/models/service.rb b/app/models/service.rb index 8eda1930400..7ca44ced20e 100644 --- a/app/models/service.rb +++ b/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 diff --git a/config/environments/development.rb b/config/environments/development.rb index 6780dbef8e3..45a8c1add3e 100644 --- a/config/environments/development.rb +++ b/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 diff --git a/lib/api/services.rb b/lib/api/services.rb index b4b3bb6e41a..094fca49c28 100644 --- a/lib/api/services.rb +++ b/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 diff --git a/lib/gitlab/chat_commands/base_command.rb b/lib/gitlab/chat_commands/base_command.rb index 13dc2a0f3e8..b5d58af3588 100644 --- a/lib/gitlab/chat_commands/base_command.rb +++ b/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) diff --git a/lib/gitlab/chat_commands/command.rb b/lib/gitlab/chat_commands/command.rb index 06d09ab0e24..f1490c045c3 100644 --- a/lib/gitlab/chat_commands/command.rb +++ b/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 diff --git a/lib/gitlab/chat_commands/issue_create.rb b/lib/gitlab/chat_commands/issue_create.rb index c424e845402..b5cf85b58f1 100644 --- a/lib/gitlab/chat_commands/issue_create.rb +++ b/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] diff --git a/lib/gitlab/chat_commands/issue_search.rb b/lib/gitlab/chat_commands/issue_search.rb index 4169e2a7a88..f64f3ad2680 100644 --- a/lib/gitlab/chat_commands/issue_search.rb +++ b/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 diff --git a/lib/gitlab/chat_commands/merge_request_command.rb b/lib/gitlab/chat_commands/merge_request_command.rb index e0f69a49afd..ad485483b8a 100644 --- a/lib/gitlab/chat_commands/merge_request_command.rb +++ b/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 diff --git a/lib/gitlab/chat_commands/merge_request_search.rb b/lib/gitlab/chat_commands/merge_request_search.rb index caecb1a788e..19a29546736 100644 --- a/lib/gitlab/chat_commands/merge_request_search.rb +++ b/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 diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb index 3db55d6bd51..0f2beb2cd6b 100644 --- a/lib/mattermost/presenter.rb +++ b/lib/mattermost/presenter.rb @@ -1,35 +1,24 @@ 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 diff --git a/spec/lib/gitlab/chat_commands/issue_create_spec.rb b/spec/lib/gitlab/chat_commands/issue_create_spec.rb index 184c09708a4..5f5cc706c96 100644 --- a/spec/lib/gitlab/chat_commands/issue_create_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/chat_commands/issue_show_spec.rb b/spec/lib/gitlab/chat_commands/issue_show_spec.rb index ddf7fc87c36..d7824dd6bf5 100644 --- a/spec/lib/gitlab/chat_commands/issue_show_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/chat_commands/merge_request_search_spec.rb b/spec/lib/gitlab/chat_commands/merge_request_search_spec.rb index 4cb4563e589..4033358ab2e 100644 --- a/spec/lib/gitlab/chat_commands/merge_request_search_spec.rb +++ b/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 |