From dc6921bdbbabd08be4426345140cb507b286eac7 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 10 Jan 2017 19:43:58 +0100 Subject: [PATCH] Chat Commands have presenters This improves the styling and readability of the code. This is supported by both Mattermost and Slack. --- .../chat_slash_commands_service.rb | 22 +-- lib/gitlab/chat_commands/base_command.rb | 4 - lib/gitlab/chat_commands/command.rb | 22 +-- lib/gitlab/chat_commands/deploy.rb | 24 +-- lib/gitlab/chat_commands/issue_create.rb | 18 +- lib/gitlab/chat_commands/issue_search.rb | 10 +- lib/gitlab/chat_commands/issue_show.rb | 8 +- lib/gitlab/chat_commands/presenter.rb | 131 -------------- lib/gitlab/chat_commands/presenters/access.rb | 22 +++ lib/gitlab/chat_commands/presenters/base.rb | 73 ++++++++ lib/gitlab/chat_commands/presenters/deploy.rb | 24 +++ .../chat_commands/presenters/issuable.rb | 33 ++++ .../chat_commands/presenters/list_issues.rb | 32 ++++ .../chat_commands/presenters/show_issue.rb | 38 +++++ lib/mattermost/error.rb | 3 - lib/mattermost/session.rb | 160 ------------------ spec/lib/gitlab/chat_commands/command_spec.rb | 50 +----- spec/lib/gitlab/chat_commands/deploy_spec.rb | 24 +-- .../gitlab/chat_commands/issue_create_spec.rb | 12 +- .../gitlab/chat_commands/issue_search_spec.rb | 12 +- .../gitlab/chat_commands/issue_show_spec.rb | 25 ++- .../chat_commands/presenters/access_spec.rb | 49 ++++++ .../chat_commands/presenters/deploy_spec.rb | 47 +++++ .../presenters/list_issues_spec.rb | 24 +++ .../presenters/show_issue_spec.rb | 27 +++ spec/lib/mattermost/client_spec.rb | 24 --- spec/lib/mattermost/command_spec.rb | 61 ------- spec/lib/mattermost/session_spec.rb | 123 -------------- spec/lib/mattermost/team_spec.rb | 66 -------- 29 files changed, 480 insertions(+), 688 deletions(-) delete mode 100644 lib/gitlab/chat_commands/presenter.rb create mode 100644 lib/gitlab/chat_commands/presenters/access.rb create mode 100644 lib/gitlab/chat_commands/presenters/base.rb create mode 100644 lib/gitlab/chat_commands/presenters/deploy.rb create mode 100644 lib/gitlab/chat_commands/presenters/issuable.rb create mode 100644 lib/gitlab/chat_commands/presenters/list_issues.rb create mode 100644 lib/gitlab/chat_commands/presenters/show_issue.rb delete mode 100644 lib/mattermost/error.rb delete mode 100644 lib/mattermost/session.rb create mode 100644 spec/lib/gitlab/chat_commands/presenters/access_spec.rb create mode 100644 spec/lib/gitlab/chat_commands/presenters/deploy_spec.rb create mode 100644 spec/lib/gitlab/chat_commands/presenters/list_issues_spec.rb create mode 100644 spec/lib/gitlab/chat_commands/presenters/show_issue_spec.rb delete mode 100644 spec/lib/mattermost/client_spec.rb delete mode 100644 spec/lib/mattermost/command_spec.rb delete mode 100644 spec/lib/mattermost/session_spec.rb delete mode 100644 spec/lib/mattermost/team_spec.rb diff --git a/app/models/project_services/chat_slash_commands_service.rb b/app/models/project_services/chat_slash_commands_service.rb index 2bcff541cc0..608754f3035 100644 --- a/app/models/project_services/chat_slash_commands_service.rb +++ b/app/models/project_services/chat_slash_commands_service.rb @@ -28,20 +28,24 @@ class ChatSlashCommandsService < Service end def trigger(params) - return unless valid_token?(params[:token]) + return access_presenter unless valid_token?(params[:token]) user = find_chat_user(params) - unless user - url = authorize_chat_name_url(params) - return presenter.authorize_chat_name(url) - end - Gitlab::ChatCommands::Command.new(project, user, - params).execute + if user + Gitlab::ChatCommands::Command.new(project, user, params).execute + else + url = authorize_chat_name_url(params) + access_presenter(url).authorize + end end private + def access_presenter(url = nil) + Gitlab::ChatCommands::Presenters::Access.new(url) + end + def find_chat_user(params) ChatNames::FindUserService.new(self, params).execute end @@ -49,8 +53,4 @@ class ChatSlashCommandsService < Service def authorize_chat_name_url(params) ChatNames::AuthorizeUserService.new(self, params).execute end - - def presenter - Gitlab::ChatCommands::Presenter.new - end end diff --git a/lib/gitlab/chat_commands/base_command.rb b/lib/gitlab/chat_commands/base_command.rb index 4fe53ce93a9..25da8474e95 100644 --- a/lib/gitlab/chat_commands/base_command.rb +++ b/lib/gitlab/chat_commands/base_command.rb @@ -42,10 +42,6 @@ module Gitlab def find_by_iid(iid) collection.find_by(iid: iid) end - - def presenter - Gitlab::ChatCommands::Presenter.new - end end end end diff --git a/lib/gitlab/chat_commands/command.rb b/lib/gitlab/chat_commands/command.rb index 145086755e4..ac7ee868402 100644 --- a/lib/gitlab/chat_commands/command.rb +++ b/lib/gitlab/chat_commands/command.rb @@ -13,9 +13,9 @@ module Gitlab if command if command.allowed?(project, current_user) - present command.new(project, current_user, params).execute(match) + command.new(project, current_user, params).execute(match) else - access_denied + Gitlab::ChatCommands::Presenters::Access.new.access_denied end else help(help_messages) @@ -25,7 +25,7 @@ module Gitlab def match_command match = nil service = available_commands.find do |klass| - match = klass.match(command) + match = klass.match(params[:text]) end [service, match] @@ -42,22 +42,6 @@ module Gitlab klass.available?(project) end end - - def command - params[:text] - end - - def help(messages) - presenter.help(messages, params[:command]) - end - - def access_denied - presenter.access_denied - end - - def present(resource) - presenter.present(resource) - end end end end diff --git a/lib/gitlab/chat_commands/deploy.rb b/lib/gitlab/chat_commands/deploy.rb index 7127d2f6d04..458d90f84e8 100644 --- a/lib/gitlab/chat_commands/deploy.rb +++ b/lib/gitlab/chat_commands/deploy.rb @@ -1,8 +1,6 @@ module Gitlab module ChatCommands class Deploy < BaseCommand - include Gitlab::Routing.url_helpers - def self.match(text) /\Adeploy\s+(?\S+.*)\s+to+\s+(?\S+.*)\z/.match(text) end @@ -24,35 +22,29 @@ module Gitlab to = match[:to] actions = find_actions(from, to) - return unless actions.present? - if actions.one? - play!(from, to, actions.first) + if actions.none? + Gitlab::ChatCommands::Presenters::Deploy.new(nil).no_actions + elsif actions.one? + action = play!(from, to, actions.first) + Gitlab::ChatCommands::Presenters::Deploy.new(action).present(from, to) else - Result.new(:error, 'Too many actions defined') + Gitlab::ChatCommands::Presenters::Deploy.new(actions).too_many_actions end end private def play!(from, to, action) - new_action = action.play(current_user) - - Result.new(:success, "Deployment from #{from} to #{to} started. Follow the progress: #{url(new_action)}.") + action.play(current_user) end def find_actions(from, to) environment = project.environments.find_by(name: from) - return unless environment + return [] unless environment environment.actions_for(to).select(&:starts_environment?) end - - def url(subject) - polymorphic_url( - [subject.project.namespace.becomes(Namespace), subject.project, subject] - ) - end end end end diff --git a/lib/gitlab/chat_commands/issue_create.rb b/lib/gitlab/chat_commands/issue_create.rb index cefb6775db8..a06f13b0f72 100644 --- a/lib/gitlab/chat_commands/issue_create.rb +++ b/lib/gitlab/chat_commands/issue_create.rb @@ -2,7 +2,7 @@ module Gitlab module ChatCommands class IssueCreate < IssueCommand def self.match(text) - # we can not match \n with the dot by passing the m modifier as than + # we can not match \n with the dot by passing the m modifier as than # the title and description are not seperated /\Aissue\s+(new|create)\s+(?[^\n]*)\n*(?<description>(.|\n)*)/.match(text) end @@ -19,8 +19,24 @@ module Gitlab title = match[:title] description = match[:description].to_s.rstrip + issue = create_issue(title: title, description: description) + + if issue.errors.any? + presenter(issue).display_errors + else + presenter(issue).present + end + end + + private + + def create_issue(title:, description:) Issues::CreateService.new(project, current_user, title: title, description: description).execute end + + def presenter(issue) + Gitlab::ChatCommands::Presenters::ShowIssue.new(issue) + end end end end diff --git a/lib/gitlab/chat_commands/issue_search.rb b/lib/gitlab/chat_commands/issue_search.rb index 51bf80c800b..e2d3a0f466a 100644 --- a/lib/gitlab/chat_commands/issue_search.rb +++ b/lib/gitlab/chat_commands/issue_search.rb @@ -10,7 +10,15 @@ module Gitlab end def execute(match) - collection.search(match[:query]).limit(QUERY_LIMIT) + issues = collection.search(match[:query]).limit(QUERY_LIMIT) + + if issues.none? + Presenters::Access.new(issues).not_found + elsif issues.one? + Presenters::ShowIssue.new(issues.first).present + else + Presenters::ListIssues.new(issues).present + end end end end diff --git a/lib/gitlab/chat_commands/issue_show.rb b/lib/gitlab/chat_commands/issue_show.rb index 2a45d49cf6b..9f3e1b9a64b 100644 --- a/lib/gitlab/chat_commands/issue_show.rb +++ b/lib/gitlab/chat_commands/issue_show.rb @@ -10,7 +10,13 @@ module Gitlab end def execute(match) - find_by_iid(match[:iid]) + issue = find_by_iid(match[:iid]) + + if issue + Gitlab::ChatCommands::Presenters::ShowIssue.new(issue).present + else + Gitlab::ChatCommands::Presenters::Access.new.not_found + end end end end diff --git a/lib/gitlab/chat_commands/presenter.rb b/lib/gitlab/chat_commands/presenter.rb deleted file mode 100644 index 8930a21f406..00000000000 --- a/lib/gitlab/chat_commands/presenter.rb +++ /dev/null @@ -1,131 +0,0 @@ -module Gitlab - module ChatCommands - class Presenter - include Gitlab::Routing - - def authorize_chat_name(url) - message = if url - ":wave: Hi there! Before I do anything for you, please [connect your GitLab account](#{url})." - else - ":sweat_smile: Couldn't identify you, nor can I autorize you!" - end - - ephemeral_response(message) - end - - def help(commands, trigger) - if commands.none? - ephemeral_response("No commands configured") - else - commands.map! { |command| "#{trigger} #{command}" } - message = header_with_list("Available commands", commands) - - ephemeral_response(message) - end - end - - def present(subject) - return not_found unless subject - - if subject.is_a?(Gitlab::ChatCommands::Result) - show_result(subject) - elsif subject.respond_to?(:count) - if subject.none? - not_found - elsif subject.one? - single_resource(subject.first) - else - multiple_resources(subject) - end - else - single_resource(subject) - end - end - - def access_denied - ephemeral_response("Whoops! That action is not allowed. This incident will be [reported](https://xkcd.com/838/).") - end - - private - - def show_result(result) - case result.type - when :success - in_channel_response(result.message) - else - ephemeral_response(result.message) - end - end - - def not_found - ephemeral_response("404 not found! GitLab couldn't find what you were looking for! :boom:") - end - - def single_resource(resource) - return error(resource) if resource.errors.any? || !resource.persisted? - - message = "#{title(resource)}:" - message << "\n\n#{resource.description}" if resource.try(:description) - - in_channel_response(message) - end - - def multiple_resources(resources) - titles = resources.map { |resource| title(resource) } - - message = header_with_list("Multiple results were found:", titles) - - ephemeral_response(message) - end - - def error(resource) - message = header_with_list("The action was not successful, because:", resource.errors.messages) - - ephemeral_response(message) - end - - def title(resource) - reference = resource.try(:to_reference) || resource.try(:id) - title = resource.try(:title) || resource.try(:name) - - "[#{reference} #{title}](#{url(resource)})" - end - - def header_with_list(header, items) - message = [header] - - items.each do |item| - message << "- #{item}" - end - - message.join("\n") - end - - def url(resource) - url_for( - [ - resource.project.namespace.becomes(Namespace), - resource.project, - resource - ] - ) - end - - def ephemeral_response(message) - { - response_type: :ephemeral, - text: message, - status: 200 - } - end - - def in_channel_response(message) - { - response_type: :in_channel, - text: message, - status: 200 - } - end - end - end -end diff --git a/lib/gitlab/chat_commands/presenters/access.rb b/lib/gitlab/chat_commands/presenters/access.rb new file mode 100644 index 00000000000..6d18d745608 --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/access.rb @@ -0,0 +1,22 @@ +module Gitlab::ChatCommands::Presenters + class Access < Gitlab::ChatCommands::Presenters::Base + def access_denied + ephemeral_response(text: "Whoops! This action is not allowed. This incident will be [reported](https://xkcd.com/838/).") + end + + def not_found + ephemeral_response(text: "404 not found! GitLab couldn't find what you were looking for! :boom:") + end + + def authorize + message = + if @resource + ":wave: Hi there! Before I do anything for you, please [connect your GitLab account](#{@resource})." + else + ":sweat_smile: Couldn't identify you, nor can I autorize you!" + end + + ephemeral_response(text: message) + end + end +end diff --git a/lib/gitlab/chat_commands/presenters/base.rb b/lib/gitlab/chat_commands/presenters/base.rb new file mode 100644 index 00000000000..0897025d85f --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/base.rb @@ -0,0 +1,73 @@ +module Gitlab::ChatCommands::Presenters + class Base + include Gitlab::Routing.url_helpers + + def initialize(resource = nil) + @resource = resource + end + + def display_errors + message = header_with_list("The action was not successful, because:", @resource.errors.full_messages) + + ephemeral_response(text: message) + end + + private + + def header_with_list(header, items) + message = [header] + + items.each do |item| + message << "- #{item}" + end + + message.join("\n") + end + + def ephemeral_response(message) + response = { + response_type: :ephemeral, + status: 200 + }.merge(message) + + format_response(response) + end + + def in_channel_response(message) + response = { + response_type: :in_channel, + status: 200 + }.merge(message) + + format_response(response) + end + + def format_response(response) + response[:text] = format(response[:text]) if response.has_key?(:text) + + if response.has_key?(:attachments) + response[:attachments].each do |attachment| + attachment[:pretext] = format(attachment[:pretext]) if attachment[:pretext] + attachment[:text] = format(attachment[:text]) if attachment[:text] + end + end + + response + end + + # Convert Markdown to slacks format + def format(string) + Slack::Notifier::LinkFormatter.format(string) + end + + def resource_url + url_for( + [ + @resource.project.namespace.becomes(Namespace), + @resource.project, + @resource + ] + ) + end + end +end diff --git a/lib/gitlab/chat_commands/presenters/deploy.rb b/lib/gitlab/chat_commands/presenters/deploy.rb new file mode 100644 index 00000000000..4f6333812ff --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/deploy.rb @@ -0,0 +1,24 @@ +module Gitlab::ChatCommands::Presenters + class Deploy < Gitlab::ChatCommands::Presenters::Base + def present(from, to) + message = "Deployment started from #{from} to #{to}. [Follow its progress](#{resource_url})." + in_channel_response(text: message) + end + + def no_actions + ephemeral_response(text: "No action found to be executed") + end + + def too_many_actions + ephemeral_response(text: "Too many actions defined") + end + + private + + def resource_url + polymorphic_url( + [ @resource.project.namespace.becomes(Namespace), @resource.project, @resource] + ) + end + end +end diff --git a/lib/gitlab/chat_commands/presenters/issuable.rb b/lib/gitlab/chat_commands/presenters/issuable.rb new file mode 100644 index 00000000000..9623387f188 --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/issuable.rb @@ -0,0 +1,33 @@ +module Gitlab::ChatCommands::Presenters + class Issuable < Gitlab::ChatCommands::Presenters::Base + private + + def project + @resource.project + end + + def author + @resource.author + end + + def fields + [ + { + title: "Assignee", + value: @resource.assignee ? @resource.assignee.name : "_None_", + short: true + }, + { + title: "Milestone", + value: @resource.milestone ? @resource.milestone.title : "_None_", + short: true + }, + { + title: "Labels", + value: @resource.labels.any? ? @resource.label_names : "_None_", + short: true + } + ] + end + end +end diff --git a/lib/gitlab/chat_commands/presenters/list_issues.rb b/lib/gitlab/chat_commands/presenters/list_issues.rb new file mode 100644 index 00000000000..5a7b3fca5c2 --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/list_issues.rb @@ -0,0 +1,32 @@ +module Gitlab::ChatCommands::Presenters + class ListIssues < Gitlab::ChatCommands::Presenters::Base + def present + ephemeral_response(text: "Here are the issues I found:", attachments: attachments) + end + + private + + def attachments + @resource.map do |issue| + state = issue.open? ? "Open" : "Closed" + + { + fallback: "Issue #{issue.to_reference}: #{issue.title}", + color: "#d22852", + text: "[#{issue.to_reference}](#{url_for([namespace, project, issue])}) ยท #{issue.title} (#{state})", + mrkdwn_in: [ + "text" + ] + } + end + end + + def project + @project ||= @resource.first.project + end + + def namespace + @namespace ||= project.namespace.becomes(Namespace) + end + end +end diff --git a/lib/gitlab/chat_commands/presenters/show_issue.rb b/lib/gitlab/chat_commands/presenters/show_issue.rb new file mode 100644 index 00000000000..2a89c30b972 --- /dev/null +++ b/lib/gitlab/chat_commands/presenters/show_issue.rb @@ -0,0 +1,38 @@ +module Gitlab::ChatCommands::Presenters + class ShowIssue < Gitlab::ChatCommands::Presenters::Issuable + def present + in_channel_response(show_issue) + end + + private + + def show_issue + { + attachments: [ + { + title: @resource.title, + title_link: resource_url, + author_name: author.name, + author_icon: author.avatar_url, + fallback: "#{@resource.to_reference}: #{@resource.title}", + text: text, + fields: fields, + mrkdwn_in: [ + :title, + :text + ] + } + ] + } + end + + def text + message = "" + message << ":+1: #{@resource.upvotes} " unless @resource.upvotes.zero? + message << ":-1: #{@resource.downvotes} " unless @resource.downvotes.zero? + message << ":speech_balloon: #{@resource.user_notes_count}" unless @resource.user_notes_count.zero? + + message + end + end +end diff --git a/lib/mattermost/error.rb b/lib/mattermost/error.rb deleted file mode 100644 index 014df175be0..00000000000 --- a/lib/mattermost/error.rb +++ /dev/null @@ -1,3 +0,0 @@ -module Mattermost - class Error < StandardError; end -end diff --git a/lib/mattermost/session.rb b/lib/mattermost/session.rb deleted file mode 100644 index 377cb7b1021..00000000000 --- a/lib/mattermost/session.rb +++ /dev/null @@ -1,160 +0,0 @@ -module Mattermost - class NoSessionError < Mattermost::Error - def message - 'No session could be set up, is Mattermost configured with Single Sign On?' - end - end - - class ConnectionError < Mattermost::Error; end - - # This class' prime objective is to obtain a session token on a Mattermost - # instance with SSO configured where this GitLab instance is the provider. - # - # The process depends on OAuth, but skips a step in the authentication cycle. - # For example, usually a user would click the 'login in GitLab' button on - # Mattermost, which would yield a 302 status code and redirects you to GitLab - # to approve the use of your account on Mattermost. Which would trigger a - # callback so Mattermost knows this request is approved and gets the required - # data to create the user account etc. - # - # This class however skips the button click, and also the approval phase to - # speed up the process and keep it without manual action and get a session - # going. - class Session - include Doorkeeper::Helpers::Controller - include HTTParty - - LEASE_TIMEOUT = 60 - - base_uri Settings.mattermost.host - - attr_accessor :current_resource_owner, :token - - def initialize(current_user) - @current_resource_owner = current_user - end - - def with_session - with_lease do - raise Mattermost::NoSessionError unless create - - begin - yield self - rescue Errno::ECONNREFUSED - raise Mattermost::NoSessionError - ensure - destroy - end - end - end - - # Next methods are needed for Doorkeeper - def pre_auth - @pre_auth ||= Doorkeeper::OAuth::PreAuthorization.new( - Doorkeeper.configuration, server.client_via_uid, params) - end - - def authorization - @authorization ||= strategy.request - end - - def strategy - @strategy ||= server.authorization_request(pre_auth.response_type) - end - - def request - @request ||= OpenStruct.new(parameters: params) - end - - def params - Rack::Utils.parse_query(oauth_uri.query).symbolize_keys - end - - def get(path, options = {}) - handle_exceptions do - self.class.get(path, options.merge(headers: @headers)) - end - end - - def post(path, options = {}) - handle_exceptions do - self.class.post(path, options.merge(headers: @headers)) - end - end - - private - - def create - return unless oauth_uri - return unless token_uri - - @token = request_token - @headers = { - Authorization: "Bearer #{@token}" - } - - @token - end - - def destroy - post('/api/v3/users/logout') - end - - def oauth_uri - return @oauth_uri if defined?(@oauth_uri) - - @oauth_uri = nil - - response = get("/api/v3/oauth/gitlab/login", follow_redirects: false) - return unless 300 <= response.code && response.code < 400 - - redirect_uri = response.headers['location'] - return unless redirect_uri - - @oauth_uri = URI.parse(redirect_uri) - end - - def token_uri - @token_uri ||= - if oauth_uri - authorization.authorize.redirect_uri if pre_auth.authorizable? - end - end - - def request_token - response = get(token_uri, follow_redirects: false) - - if 200 <= response.code && response.code < 400 - response.headers['token'] - end - end - - def with_lease - lease_uuid = lease_try_obtain - raise NoSessionError unless lease_uuid - - begin - yield - ensure - Gitlab::ExclusiveLease.cancel(lease_key, lease_uuid) - end - end - - def lease_key - "mattermost:session" - end - - def lease_try_obtain - lease = ::Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT) - lease.try_obtain - end - - def handle_exceptions - yield - rescue HTTParty::Error => e - raise Mattermost::ConnectionError.new(e.message) - rescue Errno::ECONNREFUSED - raise Mattermost::ConnectionError.new(e.message) - end - end -end diff --git a/spec/lib/gitlab/chat_commands/command_spec.rb b/spec/lib/gitlab/chat_commands/command_spec.rb index 1e81eaef18c..d8b2303555c 100644 --- a/spec/lib/gitlab/chat_commands/command_spec.rb +++ b/spec/lib/gitlab/chat_commands/command_spec.rb @@ -5,6 +5,7 @@ describe Gitlab::ChatCommands::Command, service: true do let(:user) { create(:user) } describe '#execute' do +<<<<<<< HEAD subject do described_class.new(project, user, params).execute end @@ -18,6 +19,9 @@ describe Gitlab::ChatCommands::Command, service: true do expect(subject[:text]).to start_with('404 not found') end end +======= + subject { described_class.new(project, user, params).execute } +>>>>>>> Chat Commands have presenters context 'when an unknown command is triggered' do let(:params) { { command: '/gitlab', text: "unknown command 123" } } @@ -34,47 +38,7 @@ describe Gitlab::ChatCommands::Command, service: true do it 'rejects the actions' do expect(subject[:response_type]).to be(:ephemeral) - expect(subject[:text]).to start_with('Whoops! That action is not allowed') - end - end - - context 'issue is successfully created' do - let(:params) { { text: "issue create my new issue" } } - - before do - project.team << [user, :master] - end - - it 'presents the issue' do - expect(subject[:text]).to match("my new issue") - end - - it 'shows a link to the new issue' do - expect(subject[:text]).to match(/\/issues\/\d+/) - end - end - - context 'searching for an issue' do - let(:params) { { text: 'issue search find me' } } - let!(:issue) { create(:issue, project: project, title: 'find me') } - - before do - project.team << [user, :master] - end - - context 'a single issue is found' do - it 'presents the issue' do - expect(subject[:text]).to match(issue.title) - end - end - - context 'multiple issues found' do - let!(:issue2) { create(:issue, project: project, title: "someone find me") } - - it 'shows a link to the new issue' do - expect(subject[:text]).to match(issue.title) - expect(subject[:text]).to match(issue2.title) - end + expect(subject[:text]).to start_with('Whoops! This action is not allowed') end end @@ -90,7 +54,7 @@ describe Gitlab::ChatCommands::Command, service: true do context 'and user can not create deployment' do it 'returns action' do expect(subject[:response_type]).to be(:ephemeral) - expect(subject[:text]).to start_with('Whoops! That action is not allowed') + expect(subject[:text]).to start_with('Whoops! This action is not allowed') end end @@ -100,7 +64,7 @@ describe Gitlab::ChatCommands::Command, service: true do end it 'returns action' do - expect(subject[:text]).to include('Deployment from staging to production started.') + expect(subject[:text]).to include('Deployment started from staging to production') expect(subject[:response_type]).to be(:in_channel) end diff --git a/spec/lib/gitlab/chat_commands/deploy_spec.rb b/spec/lib/gitlab/chat_commands/deploy_spec.rb index bd8099c92da..b3358a32161 100644 --- a/spec/lib/gitlab/chat_commands/deploy_spec.rb +++ b/spec/lib/gitlab/chat_commands/deploy_spec.rb @@ -15,8 +15,9 @@ describe Gitlab::ChatCommands::Deploy, service: true do end context 'if no environment is defined' do - it 'returns nil' do - expect(subject).to be_nil + it 'does not execute an action' do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to eq("No action found to be executed") end end @@ -26,8 +27,9 @@ describe Gitlab::ChatCommands::Deploy, service: true do let!(:deployment) { create(:deployment, environment: staging, deployable: build) } context 'without actions' do - it 'returns nil' do - expect(subject).to be_nil + it 'does not execute an action' do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to eq("No action found to be executed") end end @@ -37,8 +39,8 @@ describe Gitlab::ChatCommands::Deploy, service: true do end it 'returns success result' do - expect(subject.type).to eq(:success) - expect(subject.message).to include('Deployment from staging to production started') + expect(subject[:response_type]).to be(:in_channel) + expect(subject[:text]).to start_with('Deployment started from staging to production') end context 'when duplicate action exists' do @@ -47,8 +49,8 @@ describe Gitlab::ChatCommands::Deploy, service: true do end it 'returns error' do - expect(subject.type).to eq(:error) - expect(subject.message).to include('Too many actions defined') + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to eq('Too many actions defined') end end @@ -59,9 +61,9 @@ describe Gitlab::ChatCommands::Deploy, service: true do name: 'teardown', environment: 'production') end - it 'returns success result' do - expect(subject.type).to eq(:success) - expect(subject.message).to include('Deployment from staging to production started') + it 'returns the success message' do + expect(subject[:response_type]).to be(:in_channel) + expect(subject[:text]).to start_with('Deployment started from staging to production') 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 6c71e79ff6d..0f84b19a5a4 100644 --- a/spec/lib/gitlab/chat_commands/issue_create_spec.rb +++ b/spec/lib/gitlab/chat_commands/issue_create_spec.rb @@ -18,7 +18,7 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do it 'creates the issue' do expect { subject }.to change { project.issues.count }.by(1) - expect(subject.title).to eq('bird is the word') + expect(subject[:response_type]).to be(:in_channel) end end @@ -41,6 +41,16 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do expect { subject }.to change { project.issues.count }.by(1) end end + + context 'issue cannot be created' do + let!(:issue) { create(:issue, project: project, title: 'bird is the word') } + let(:regex_match) { described_class.match("issue create #{'a' * 512}}") } + + it 'displays the errors' do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to match("- Title is too long") + end + end end describe '.match' do diff --git a/spec/lib/gitlab/chat_commands/issue_search_spec.rb b/spec/lib/gitlab/chat_commands/issue_search_spec.rb index 24c06a967fa..04d10ad52a1 100644 --- a/spec/lib/gitlab/chat_commands/issue_search_spec.rb +++ b/spec/lib/gitlab/chat_commands/issue_search_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' describe Gitlab::ChatCommands::IssueSearch, service: true do describe '#execute' do - let!(:issue) { create(:issue, title: 'find me') } + let!(:issue) { create(:issue, project: project, title: 'find me') } let!(:confidential) { create(:issue, :confidential, project: project, title: 'mepmep find') } - let(:project) { issue.project } + let(:project) { create(:empty_project) } let(:user) { issue.author } let(:regex_match) { described_class.match("issue search find") } @@ -14,7 +14,8 @@ describe Gitlab::ChatCommands::IssueSearch, service: true do context 'when the user has no access' do it 'only returns the open issues' do - expect(subject).not_to include(confidential) + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to match("not found") end end @@ -24,13 +25,14 @@ describe Gitlab::ChatCommands::IssueSearch, service: true do end it 'returns all results' do - expect(subject).to include(confidential, issue) + expect(subject).to have_key(:attachments) + expect(subject[:text]).to match("Here are the issues I found:") end end context 'without hits on the query' do it 'returns an empty collection' do - expect(subject).to be_empty + expect(subject[:text]).to match("not found") 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 2eab73e49e5..89932c395c6 100644 --- a/spec/lib/gitlab/chat_commands/issue_show_spec.rb +++ b/spec/lib/gitlab/chat_commands/issue_show_spec.rb @@ -2,8 +2,8 @@ require 'spec_helper' describe Gitlab::ChatCommands::IssueShow, service: true do describe '#execute' do - let(:issue) { create(:issue) } - let(:project) { issue.project } + let(:issue) { create(:issue, project: project) } + let(:project) { create(:empty_project) } let(:user) { issue.author } let(:regex_match) { described_class.match("issue show #{issue.iid}") } @@ -16,15 +16,19 @@ describe Gitlab::ChatCommands::IssueShow, service: true do end context 'the issue exists' do + let(:title) { subject[:attachments].first[:title] } + it 'returns the issue' do - expect(subject.iid).to be issue.iid + expect(subject[:response_type]).to be(:in_channel) + expect(title).to eq(issue.title) end context 'when its reference is given' do let(:regex_match) { described_class.match("issue show #{issue.to_reference}") } it 'shows the issue' do - expect(subject.iid).to be issue.iid + expect(subject[:response_type]).to be(:in_channel) + expect(title).to eq(issue.title) end end end @@ -32,17 +36,24 @@ describe Gitlab::ChatCommands::IssueShow, service: true do context 'the issue does not exist' do let(:regex_match) { described_class.match("issue show 2343242") } - it "returns nil" do - expect(subject).to be_nil + it "returns not found" do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to match("not found") end end end - describe 'self.match' do + describe '.match' do it 'matches the iid' do match = described_class.match("issue show 123") expect(match[:iid]).to eq("123") end + + it 'accepts a reference' do + match = described_class.match("issue show #{Issue.reference_prefix}123") + + expect(match[:iid]).to eq("123") + end end end diff --git a/spec/lib/gitlab/chat_commands/presenters/access_spec.rb b/spec/lib/gitlab/chat_commands/presenters/access_spec.rb new file mode 100644 index 00000000000..ae41d75ab0c --- /dev/null +++ b/spec/lib/gitlab/chat_commands/presenters/access_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Gitlab::ChatCommands::Presenters::Access do + describe '#access_denied' do + subject { described_class.new.access_denied } + + it { is_expected.to be_a(Hash) } + + it 'displays an error message' do + expect(subject[:text]).to match("is not allowed") + expect(subject[:response_type]).to be(:ephemeral) + end + end + + describe '#not_found' do + subject { described_class.new.not_found } + + it { is_expected.to be_a(Hash) } + + it 'tells the user the resource was not found' do + expect(subject[:text]).to match("not found!") + expect(subject[:response_type]).to be(:ephemeral) + end + end + + describe '#authorize' do + context 'with an authorization URL' do + subject { described_class.new('http://authorize.me').authorize } + + it { is_expected.to be_a(Hash) } + + it 'tells the user to authorize' do + expect(subject[:text]).to match("connect your GitLab account") + expect(subject[:response_type]).to be(:ephemeral) + end + end + + context 'without authorization url' do + subject { described_class.new.authorize } + + it { is_expected.to be_a(Hash) } + + it 'tells the user to authorize' do + expect(subject[:text]).to match("Couldn't identify you") + expect(subject[:response_type]).to be(:ephemeral) + end + end + end +end diff --git a/spec/lib/gitlab/chat_commands/presenters/deploy_spec.rb b/spec/lib/gitlab/chat_commands/presenters/deploy_spec.rb new file mode 100644 index 00000000000..1c48c727e30 --- /dev/null +++ b/spec/lib/gitlab/chat_commands/presenters/deploy_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe Gitlab::ChatCommands::Presenters::Deploy do + let(:build) { create(:ci_build) } + + describe '#present' do + subject { described_class.new(build).present('staging', 'prod') } + + it { is_expected.to have_key(:text) } + it { is_expected.to have_key(:response_type) } + it { is_expected.to have_key(:status) } + it { is_expected.not_to have_key(:attachments) } + + it 'messages the channel of the deploy' do + expect(subject[:response_type]).to be(:in_channel) + expect(subject[:text]).to start_with("Deployment started from staging to prod") + end + end + + describe '#no_actions' do + subject { described_class.new(nil).no_actions } + + it { is_expected.to have_key(:text) } + it { is_expected.to have_key(:response_type) } + it { is_expected.to have_key(:status) } + it { is_expected.not_to have_key(:attachments) } + + it 'tells the user there is no action' do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to eq("No action found to be executed") + end + end + + describe '#too_many_actions' do + subject { described_class.new(nil).too_many_actions } + + it { is_expected.to have_key(:text) } + it { is_expected.to have_key(:response_type) } + it { is_expected.to have_key(:status) } + it { is_expected.not_to have_key(:attachments) } + + it 'tells the user there is no action' do + expect(subject[:response_type]).to be(:ephemeral) + expect(subject[:text]).to eq("Too many actions defined") + end + end +end diff --git a/spec/lib/gitlab/chat_commands/presenters/list_issues_spec.rb b/spec/lib/gitlab/chat_commands/presenters/list_issues_spec.rb new file mode 100644 index 00000000000..1852395fc97 --- /dev/null +++ b/spec/lib/gitlab/chat_commands/presenters/list_issues_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe Gitlab::ChatCommands::Presenters::ListIssues do + let(:project) { create(:empty_project) } + let(:message) { subject[:text] } + let(:issue) { project.issues.first } + + before { create_list(:issue, 2, project: project) } + + subject { described_class.new(project.issues).present } + + it do + is_expected.to have_key(:text) + is_expected.to have_key(:status) + is_expected.to have_key(:response_type) + is_expected.to have_key(:attachments) + end + + it 'shows a list of results' do + expect(subject[:response_type]).to be(:ephemeral) + + expect(message).to start_with("Here are the issues I found") + end +end diff --git a/spec/lib/gitlab/chat_commands/presenters/show_issue_spec.rb b/spec/lib/gitlab/chat_commands/presenters/show_issue_spec.rb new file mode 100644 index 00000000000..13a318fe680 --- /dev/null +++ b/spec/lib/gitlab/chat_commands/presenters/show_issue_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe Gitlab::ChatCommands::Presenters::ShowIssue do + let(:project) { create(:empty_project) } + let(:issue) { create(:issue, project: project) } + let(:attachment) { subject[:attachments].first } + + subject { described_class.new(issue).present } + + it { is_expected.to be_a(Hash) } + + it 'shows the issue' do + expect(subject[:response_type]).to be(:in_channel) + expect(subject).to have_key(:attachments) + expect(attachment[:title]).to eq(issue.title) + end + + context 'with upvotes' do + before do + create(:award_emoji, :upvote, awardable: issue) + end + + it 'shows the upvote count' do + expect(attachment[:text]).to start_with(":+1: 1") + end + end +end diff --git a/spec/lib/mattermost/client_spec.rb b/spec/lib/mattermost/client_spec.rb deleted file mode 100644 index dc11a414717..00000000000 --- a/spec/lib/mattermost/client_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -require 'spec_helper' - -describe Mattermost::Client do - let(:user) { build(:user) } - - subject { described_class.new(user) } - - context 'JSON parse error' do - before do - Struct.new("Request", :body, :success?) - end - - it 'yields an error on malformed JSON' do - bad_json = Struct::Request.new("I'm not json", true) - expect { subject.send(:json_response, bad_json) }.to raise_error(Mattermost::ClientError) - end - - it 'shows a client error if the request was unsuccessful' do - bad_request = Struct::Request.new("true", false) - - expect { subject.send(:json_response, bad_request) }.to raise_error(Mattermost::ClientError) - end - end -end diff --git a/spec/lib/mattermost/command_spec.rb b/spec/lib/mattermost/command_spec.rb deleted file mode 100644 index 5ccf1100898..00000000000 --- a/spec/lib/mattermost/command_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -require 'spec_helper' - -describe Mattermost::Command do - let(:params) { { 'token' => 'token', team_id: 'abc' } } - - before do - Mattermost::Session.base_uri('http://mattermost.example.com') - - allow_any_instance_of(Mattermost::Client).to receive(:with_session). - and_yield(Mattermost::Session.new(nil)) - end - - describe '#create' do - let(:params) do - { team_id: 'abc', - trigger: 'gitlab' - } - end - - subject { described_class.new(nil).create(params) } - - context 'for valid trigger word' do - before do - stub_request(:post, 'http://mattermost.example.com/api/v3/teams/abc/commands/create'). - with(body: { - team_id: 'abc', - trigger: 'gitlab' }.to_json). - to_return( - status: 200, - headers: { 'Content-Type' => 'application/json' }, - body: { token: 'token' }.to_json - ) - end - - it 'returns a token' do - is_expected.to eq('token') - end - end - - context 'for error message' do - before do - stub_request(:post, 'http://mattermost.example.com/api/v3/teams/abc/commands/create'). - to_return( - status: 500, - headers: { 'Content-Type' => 'application/json' }, - body: { - id: 'api.command.duplicate_trigger.app_error', - message: 'This trigger word is already in use. Please choose another word.', - detailed_error: '', - request_id: 'obc374man7bx5r3dbc1q5qhf3r', - status_code: 500 - }.to_json - ) - end - - it 'raises an error with message' do - expect { subject }.to raise_error(Mattermost::Error, 'This trigger word is already in use. Please choose another word.') - end - end - end -end diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb deleted file mode 100644 index 74d12e37181..00000000000 --- a/spec/lib/mattermost/session_spec.rb +++ /dev/null @@ -1,123 +0,0 @@ -require 'spec_helper' - -describe Mattermost::Session, type: :request do - let(:user) { create(:user) } - - let(:gitlab_url) { "http://gitlab.com" } - let(:mattermost_url) { "http://mattermost.com" } - - subject { described_class.new(user) } - - # Needed for doorkeeper to function - it { is_expected.to respond_to(:current_resource_owner) } - it { is_expected.to respond_to(:request) } - it { is_expected.to respond_to(:authorization) } - it { is_expected.to respond_to(:strategy) } - - before do - described_class.base_uri(mattermost_url) - end - - describe '#with session' do - let(:location) { 'http://location.tld' } - let!(:stub) do - WebMock.stub_request(:get, "#{mattermost_url}/api/v3/oauth/gitlab/login"). - to_return(headers: { 'location' => location }, status: 307) - end - - context 'without oauth uri' do - it 'makes a request to the oauth uri' do - expect { subject.with_session }.to raise_error(Mattermost::NoSessionError) - end - end - - context 'with oauth_uri' do - let!(:doorkeeper) do - Doorkeeper::Application.create( - name: "GitLab Mattermost", - redirect_uri: "#{mattermost_url}/signup/gitlab/complete\n#{mattermost_url}/login/gitlab/complete", - scopes: "") - end - - context 'without token_uri' do - it 'can not create a session' do - expect do - subject.with_session - end.to raise_error(Mattermost::NoSessionError) - end - end - - context 'with token_uri' do - let(:state) { "state" } - let(:params) do - { response_type: "code", - client_id: doorkeeper.uid, - redirect_uri: "#{mattermost_url}/signup/gitlab/complete", - state: state } - end - let(:location) do - "#{gitlab_url}/oauth/authorize?#{URI.encode_www_form(params)}" - end - - before do - WebMock.stub_request(:get, "#{mattermost_url}/signup/gitlab/complete"). - with(query: hash_including({ 'state' => state })). - to_return do |request| - post "/oauth/token", - client_id: doorkeeper.uid, - client_secret: doorkeeper.secret, - redirect_uri: params[:redirect_uri], - grant_type: 'authorization_code', - code: request.uri.query_values['code'] - - if response.status == 200 - { headers: { 'token' => 'thisworksnow' }, status: 202 } - end - end - - WebMock.stub_request(:post, "#{mattermost_url}/api/v3/users/logout"). - to_return(headers: { Authorization: 'token thisworksnow' }, status: 200) - end - - it 'can setup a session' do - subject.with_session do |session| - end - - expect(subject.token).not_to be_nil - end - - it 'returns the value of the block' do - result = subject.with_session do |session| - "value" - end - - expect(result).to eq("value") - end - end - end - - context 'with lease' do - before do - allow(subject).to receive(:lease_try_obtain).and_return('aldkfjsldfk') - end - - it 'tries to obtain a lease' do - expect(subject).to receive(:lease_try_obtain) - expect(Gitlab::ExclusiveLease).to receive(:cancel) - - # Cannot setup a session, but we should still cancel the lease - expect { subject.with_session }.to raise_error(Mattermost::NoSessionError) - end - end - - context 'without lease' do - before do - allow(subject).to receive(:lease_try_obtain).and_return(nil) - end - - it 'returns a NoSessionError error' do - expect { subject.with_session }.to raise_error(Mattermost::NoSessionError) - end - end - end -end diff --git a/spec/lib/mattermost/team_spec.rb b/spec/lib/mattermost/team_spec.rb deleted file mode 100644 index 2d14be6bcc2..00000000000 --- a/spec/lib/mattermost/team_spec.rb +++ /dev/null @@ -1,66 +0,0 @@ -require 'spec_helper' - -describe Mattermost::Team do - before do - Mattermost::Session.base_uri('http://mattermost.example.com') - - allow_any_instance_of(Mattermost::Client).to receive(:with_session). - and_yield(Mattermost::Session.new(nil)) - end - - describe '#all' do - subject { described_class.new(nil).all } - - context 'for valid request' do - let(:response) do - [{ - "id" => "xiyro8huptfhdndadpz8r3wnbo", - "create_at" => 1482174222155, - "update_at" => 1482174222155, - "delete_at" => 0, - "display_name" => "chatops", - "name" => "chatops", - "email" => "admin@example.com", - "type" => "O", - "company_name" => "", - "allowed_domains" => "", - "invite_id" => "o4utakb9jtb7imctdfzbf9r5ro", - "allow_open_invite" => false }] - end - - before do - stub_request(:get, 'http://mattermost.example.com/api/v3/teams/all'). - to_return( - status: 200, - headers: { 'Content-Type' => 'application/json' }, - body: response.to_json - ) - end - - it 'returns a token' do - is_expected.to eq(response) - end - end - - context 'for error message' do - before do - stub_request(:get, 'http://mattermost.example.com/api/v3/teams/all'). - to_return( - status: 500, - headers: { 'Content-Type' => 'application/json' }, - body: { - id: 'api.team.list.app_error', - message: 'Cannot list teams.', - detailed_error: '', - request_id: 'obc374man7bx5r3dbc1q5qhf3r', - status_code: 500 - }.to_json - ) - end - - it 'raises an error with message' do - expect { subject }.to raise_error(Mattermost::Error, 'Cannot list teams.') - end - end - end -end