Commit 52ca0d2c authored by Z.J. van de Weg's avatar Z.J. van de Weg

Incorporate feedback

parent e9d16386
---
title: Reformat messages ChatOps
merge_request: 8528
author:
...@@ -35,7 +35,7 @@ module Gitlab ...@@ -35,7 +35,7 @@ module Gitlab
end end
def presenter(issue) def presenter(issue)
Gitlab::ChatCommands::Presenters::ShowIssue.new(issue) Gitlab::ChatCommands::Presenters::NewIssue.new(issue)
end end
end end
end end
......
module Gitlab::ChatCommands::Presenters module Gitlab
class Access < Gitlab::ChatCommands::Presenters::Base module ChatCommands
def access_denied module Presenters
ephemeral_response(text: "Whoops! This action is not allowed. This incident will be [reported](https://xkcd.com/838/).") class Access < Presenters::Base
end def access_denied
ephemeral_response(text: "Whoops! This action is not allowed. This incident will be [reported](https://xkcd.com/838/).")
def not_found end
ephemeral_response(text: "404 not found! GitLab couldn't find what you were looking for! :boom:")
end
def authorize def not_found
message = ephemeral_response(text: "404 not found! GitLab couldn't find what you were looking for! :boom:")
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 end
ephemeral_response(text: message) 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 end
end end
end end
module Gitlab::ChatCommands::Presenters module Gitlab
class Base module ChatCommands
include Gitlab::Routing.url_helpers module Presenters
class Base
include Gitlab::Routing.url_helpers
def initialize(resource = nil)
@resource = resource
end
def initialize(resource = nil) def display_errors
@resource = resource message = header_with_list("The action was not successful, because:", @resource.errors.full_messages)
end
def display_errors ephemeral_response(text: message)
message = header_with_list("The action was not successful, because:", @resource.errors.full_messages) end
ephemeral_response(text: message) private
end
private def header_with_list(header, items)
message = [header]
def header_with_list(header, items) items.each do |item|
message = [header] message << "- #{item}"
end
items.each do |item| message.join("\n")
message << "- #{item}" end
end
message.join("\n") def ephemeral_response(message)
end response = {
response_type: :ephemeral,
status: 200
}.merge(message)
def ephemeral_response(message) format_response(response)
response = { end
response_type: :ephemeral,
status: 200
}.merge(message)
format_response(response) def in_channel_response(message)
end response = {
response_type: :in_channel,
status: 200
}.merge(message)
def in_channel_response(message) format_response(response)
response = { end
response_type: :in_channel,
status: 200
}.merge(message)
format_response(response) def format_response(response)
end response[:text] = format(response[:text]) if response.has_key?(:text)
def format_response(response) if response.has_key?(:attachments)
response[:text] = format(response[:text]) if response.has_key?(:text) response[:attachments].each do |attachment|
attachment[:pretext] = format(attachment[:pretext]) if attachment[:pretext]
attachment[:text] = format(attachment[:text]) if attachment[:text]
end
end
if response.has_key?(:attachments) response
response[:attachments].each do |attachment|
attachment[:pretext] = format(attachment[:pretext]) if attachment[:pretext]
attachment[:text] = format(attachment[:text]) if attachment[:text]
end end
end
response
end
# Convert Markdown to slacks format # Convert Markdown to slacks format
def format(string) def format(string)
Slack::Notifier::LinkFormatter.format(string) Slack::Notifier::LinkFormatter.format(string)
end end
def resource_url def resource_url
url_for( url_for(
[ [
@resource.project.namespace.becomes(Namespace), @resource.project.namespace.becomes(Namespace),
@resource.project, @resource.project,
@resource @resource
] ]
) )
end
end
end end
end end
end end
module Gitlab::ChatCommands::Presenters module Gitlab
class Deploy < Gitlab::ChatCommands::Presenters::Base module ChatCommands
def present(from, to) module Presenters
message = "Deployment started from #{from} to #{to}. [Follow its progress](#{resource_url})." class Deploy < Presenters::Base
in_channel_response(text: message) def present(from, to)
end message = "Deployment started from #{from} to #{to}. [Follow its progress](#{resource_url})."
def no_actions in_channel_response(text: message)
ephemeral_response(text: "No action found to be executed") end
end
def too_many_actions def no_actions
ephemeral_response(text: "Too many actions defined") ephemeral_response(text: "No action found to be executed")
end end
def too_many_actions
ephemeral_response(text: "Too many actions defined")
end
private private
def resource_url def resource_url
polymorphic_url( polymorphic_url(
[ @resource.project.namespace.becomes(Namespace), @resource.project, @resource] [ @resource.project.namespace.becomes(Namespace), @resource.project, @resource]
) )
end
end
end end
end end
end end
module Gitlab::ChatCommands::Presenters module Gitlab
class Help < Gitlab::ChatCommands::Presenters::Base module ChatCommands
def present(trigger) module Presenters
message = class Help < Presenters::Base
if @resource.none? def present(trigger)
"No commands available :thinking_face:" ephemeral_response(text: help_message(trigger))
else
header_with_list("Available commands", full_commands(trigger))
end end
ephemeral_response(text: message) private
end
private def help_message(trigger)
if @resource.none?
"No commands available :thinking_face:"
else
header_with_list("Available commands", full_commands(trigger))
end
end
def full_commands(trigger) def full_commands(trigger)
@resource.map { |command| "#{trigger} #{command.help_message}" } @resource.map { |command| "#{trigger} #{command.help_message}" }
end
end
end end
end end
end end
module Gitlab::ChatCommands::Presenters module Gitlab
class Issuable < Gitlab::ChatCommands::Presenters::Base module ChatCommands
private module Presenters
class Issuable < Presenters::Base
private
def project def color(issuable)
@resource.project issuable.open? ? '#38ae67' : '#d22852'
end end
def author def status_text(issuable)
@resource.author issuable.open? ? 'Open' : 'Closed'
end end
def project
@resource.project
end
def author
@resource.author
end
def fields def fields
[ [
{ {
title: "Assignee", title: "Assignee",
value: @resource.assignee ? @resource.assignee.name : "_None_", value: @resource.assignee ? @resource.assignee.name : "_None_",
short: true short: true
}, },
{ {
title: "Milestone", title: "Milestone",
value: @resource.milestone ? @resource.milestone.title : "_None_", value: @resource.milestone ? @resource.milestone.title : "_None_",
short: true short: true
}, },
{ {
title: "Labels", title: "Labels",
value: @resource.labels.any? ? @resource.label_names : "_None_", value: @resource.labels.any? ? @resource.label_names : "_None_",
short: true short: true
} }
] ]
end
end
end end
end end
end end
module Gitlab::ChatCommands::Presenters module Gitlab
class ListIssues < Gitlab::ChatCommands::Presenters::Base module ChatCommands
def present module Presenters
ephemeral_response(text: "Here are the issues I found:", attachments: attachments) class ListIssues < Presenters::Issuable
end def present
text = if @resource.count >= 5
"Here are the first 5 issues I found:"
else
"Here are the #{@resource.count} issues I found:"
end
private ephemeral_response(text: text, attachments: attachments)
end
def attachments private
@resource.map do |issue|
state = issue.open? ? "Open" : "Closed"
{ def attachments
fallback: "Issue #{issue.to_reference}: #{issue.title}", @resource.map do |issue|
color: "#d22852", url = "[#{issue.to_reference}](#{url_for([namespace, project, issue])})"
text: "[#{issue.to_reference}](#{url_for([namespace, project, issue])}) · #{issue.title} (#{state})",
mrkdwn_in: [
"text"
]
}
end
end
def project {
@project ||= @resource.first.project color: color(issue),
end fallback: "#{issue.to_reference} #{issue.title}",
text: "#{url} · #{issue.title} (#{status_text(issue)})",
def namespace mrkdwn_in: [
@namespace ||= project.namespace.becomes(Namespace) "text"
]
}
end
end
def project
@project ||= @resource.first.project
end
def namespace
@namespace ||= project.namespace.becomes(Namespace)
end
end
end end
end end
end end
module Gitlab
module ChatCommands
module Presenters
class NewIssue < Presenters::Issuable
def present
in_channel_response(show_issue)
end
private
def show_issue
{
attachments: [
{
title: "#{@resource.title} · #{@resource.to_reference}",
title_link: resource_url,
author_name: author.name,
author_icon: author.avatar_url,
fallback: "New issue #{@resource.to_reference}: #{@resource.title}",
pretext: pretext,
color: color(@resource),
fields: fields,
mrkdwn_in: [
:title,
:text
]
}
]
}
end
def pretext
"I opened an issue on behalf on #{author_profile_link}: *#{@resource.to_reference}* from #{project.name_with_namespace}"
end
def author_profile_link
"[#{author.to_reference}](#{url_for(author)})"
end
end
end
end
end
module Gitlab::ChatCommands::Presenters module Gitlab
class ShowIssue < Gitlab::ChatCommands::Presenters::Issuable module ChatCommands
def present module Presenters
in_channel_response(show_issue) class ShowIssue < Presenters::Issuable
end def present
in_channel_response(show_issue)
end
private private
def show_issue def show_issue
{
attachments: [
{ {
title: @resource.title, attachments: [
title_link: resource_url, {
author_name: author.name, title: "#{@resource.title} · #{@resource.to_reference}",
author_icon: author.avatar_url, title_link: resource_url,
fallback: "#{@resource.to_reference}: #{@resource.title}", author_name: author.name,
text: text, author_icon: author.avatar_url,
fields: fields, fallback: "New issue #{@resource.to_reference}: #{@resource.title}",
mrkdwn_in: [ pretext: pretext,
:title, text: text,
:text color: color(@resource),
fields: fields,
mrkdwn_in: [
:pretext,
:text
]
}
] ]
} }
] end
}
end def text
message = "**#{status_text(@resource)}**"
if @resource.upvotes.zero? && @resource.downvotes.zero? && @resource.user_notes_count.zero?
return message
end
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?
def text message
message = "" end
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 def pretext
"Issue *#{@resource.to_reference} from #{project.name_with_namespace}"
end
end
end end
end end
end end
...@@ -26,7 +26,7 @@ describe Gitlab::ChatCommands::IssueSearch, service: true do ...@@ -26,7 +26,7 @@ describe Gitlab::ChatCommands::IssueSearch, service: true do
it 'returns all results' do it 'returns all results' do
expect(subject).to have_key(:attachments) expect(subject).to have_key(:attachments)
expect(subject[:text]).to match("Here are the issues I found:") expect(subject[:text]).to eq("Here are the 2 issues I found:")
end end
end end
......
...@@ -20,7 +20,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do ...@@ -20,7 +20,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do
it 'returns the issue' do it 'returns the issue' do
expect(subject[:response_type]).to be(:in_channel) expect(subject[:response_type]).to be(:in_channel)
expect(title).to eq(issue.title) expect(title).to start_with(issue.title)
end end
context 'when its reference is given' do context 'when its reference is given' do
...@@ -28,7 +28,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do ...@@ -28,7 +28,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do
it 'shows the issue' do it 'shows the issue' do
expect(subject[:response_type]).to be(:in_channel) expect(subject[:response_type]).to be(:in_channel)
expect(title).to eq(issue.title) expect(title).to start_with(issue.title)
end end
end end
end end
......
...@@ -32,7 +32,7 @@ describe Gitlab::ChatCommands::Presenters::Deploy do ...@@ -32,7 +32,7 @@ describe Gitlab::ChatCommands::Presenters::Deploy do
end end
describe '#too_many_actions' do describe '#too_many_actions' do
subject { described_class.new(nil).too_many_actions } subject { described_class.new([]).too_many_actions }
it { is_expected.to have_key(:text) } it { is_expected.to have_key(:text) }
it { is_expected.to have_key(:response_type) } it { is_expected.to have_key(:response_type) }
......
...@@ -3,13 +3,12 @@ require 'spec_helper' ...@@ -3,13 +3,12 @@ require 'spec_helper'
describe Gitlab::ChatCommands::Presenters::ListIssues do describe Gitlab::ChatCommands::Presenters::ListIssues do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:message) { subject[:text] } let(:message) { subject[:text] }
let(:issue) { project.issues.first }
before { create_list(:issue, 2, project: project) } before { create_list(:issue, 2, project: project) }
subject { described_class.new(project.issues).present } subject { described_class.new(project.issues).present }
it do it 'formats the message correct' do
is_expected.to have_key(:text) is_expected.to have_key(:text)
is_expected.to have_key(:status) is_expected.to have_key(:status)
is_expected.to have_key(:response_type) is_expected.to have_key(:response_type)
...@@ -19,6 +18,6 @@ describe Gitlab::ChatCommands::Presenters::ListIssues do ...@@ -19,6 +18,6 @@ describe Gitlab::ChatCommands::Presenters::ListIssues do
it 'shows a list of results' do it 'shows a list of results' do
expect(subject[:response_type]).to be(:ephemeral) expect(subject[:response_type]).to be(:ephemeral)
expect(message).to start_with("Here are the issues I found") expect(message).to start_with("Here are the 2 issues I found")
end end
end end
...@@ -12,7 +12,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do ...@@ -12,7 +12,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do
it 'shows the issue' do it 'shows the issue' do
expect(subject[:response_type]).to be(:in_channel) expect(subject[:response_type]).to be(:in_channel)
expect(subject).to have_key(:attachments) expect(subject).to have_key(:attachments)
expect(attachment[:title]).to eq(issue.title) expect(attachment[:title]).to start_with(issue.title)
end end
context 'with upvotes' do context 'with upvotes' do
...@@ -21,7 +21,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do ...@@ -21,7 +21,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do
end end
it 'shows the upvote count' do it 'shows the upvote count' do
expect(attachment[:text]).to start_with(":+1: 1") expect(attachment[:text]).to start_with("**Open** · :+1: 1")
end end
end end
end end
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
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
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment