diff --git a/app/models/project_services/chat_message/base_message.rb b/app/models/project_services/chat_message/base_message.rb index a03605d01fb..86d271a3f69 100644 --- a/app/models/project_services/chat_message/base_message.rb +++ b/app/models/project_services/chat_message/base_message.rb @@ -30,5 +30,9 @@ module ChatMessage def attachment_color '#345' end + + def link(text, url) + "[#{text}](#{url})" + end end end diff --git a/app/models/project_services/chat_message/build_message.rb b/app/models/project_services/chat_message/build_message.rb index 53e35cb21bf..c776e0a20c4 100644 --- a/app/models/project_services/chat_message/build_message.rb +++ b/app/models/project_services/chat_message/build_message.rb @@ -7,7 +7,11 @@ module ChatMessage attr_reader :project_name attr_reader :project_url attr_reader :user_name + attr_reader :user_url attr_reader :duration + attr_reader :stage + attr_reader :build_id + attr_reader :build_name def initialize(params) @sha = params[:sha] @@ -17,7 +21,11 @@ module ChatMessage @project_url = params[:project_url] @status = params[:commit][:status] @user_name = params[:commit][:author_name] + @user_url = params[:commit][:author_url] @duration = params[:commit][:duration] + @stage = params[:build_stage] + @build_name = params[:build_name] + @build_id = params[:build_id] end def pretext @@ -35,7 +43,19 @@ module ChatMessage private def message - "#{project_link}: Commit #{commit_link} of #{branch_link} #{ref_type} by #{user_name} #{humanized_status} in #{duration} #{'second'.pluralize(duration)}" + "#{project_link}: Commit #{commit_link} of #{branch_link} #{ref_type} by #{user_link} #{humanized_status} on build #{build_link} of stage #{stage} in #{duration} #{'second'.pluralize(duration)}" + end + + def build_url + "#{project_url}/builds/#{build_id}" + end + + def build_link + link(build_name, build_url) + end + + def user_link + link(user_name, user_url) end def format(string) @@ -64,11 +84,11 @@ module ChatMessage end def branch_link - "[#{ref}](#{branch_url})" + link(ref, branch_url) end def project_link - "[#{project_name}](#{project_url})" + link(project_name, project_url) end def commit_url @@ -76,7 +96,7 @@ module ChatMessage end def commit_link - "[#{Commit.truncate_sha(sha)}](#{commit_url})" + link(Commit.truncate_sha(sha), commit_url) end end end diff --git a/app/models/project_services/chat_message/issue_message.rb b/app/models/project_services/chat_message/issue_message.rb index 14fd64e5332..b96aca47e65 100644 --- a/app/models/project_services/chat_message/issue_message.rb +++ b/app/models/project_services/chat_message/issue_message.rb @@ -55,11 +55,11 @@ module ChatMessage end def project_link - "[#{project_name}](#{project_url})" + link(project_name, project_url) end def issue_link - "[#{issue_title}](#{issue_url})" + link(issue_title, issue_url) end def issue_title diff --git a/app/models/project_services/chat_message/merge_message.rb b/app/models/project_services/chat_message/merge_message.rb index ab5e8b24167..5e5efca7bec 100644 --- a/app/models/project_services/chat_message/merge_message.rb +++ b/app/models/project_services/chat_message/merge_message.rb @@ -42,7 +42,7 @@ module ChatMessage end def project_link - "[#{project_name}](#{project_url})" + link(project_name, project_url) end def merge_request_message @@ -50,7 +50,7 @@ module ChatMessage end def merge_request_link - "[merge request !#{merge_request_id}](#{merge_request_url})" + link("merge request !#{merge_request_id}", merge_request_url) end def merge_request_url diff --git a/app/models/project_services/chat_message/note_message.rb b/app/models/project_services/chat_message/note_message.rb index ca1d7207034..552113bac29 100644 --- a/app/models/project_services/chat_message/note_message.rb +++ b/app/models/project_services/chat_message/note_message.rb @@ -3,10 +3,9 @@ module ChatMessage attr_reader :message attr_reader :user_name attr_reader :project_name - attr_reader :project_link + attr_reader :project_url attr_reader :note attr_reader :note_url - attr_reader :title def initialize(params) params = HashWithIndifferentAccess.new(params) @@ -69,15 +68,15 @@ module ChatMessage end def description_message - [{ text: format(@note), color: attachment_color }] + [{ text: format(note), color: attachment_color }] end def project_link - "[#{@project_name}](#{@project_url})" + link(project_name, project_url) end def commented_on_message(target, title) - @message = "#{@user_name} [commented on #{target}](#{@note_url}) in #{project_link}: *#{title}*" + @message = "#{user_name} #{link('commented on ' + target, note_url)} in #{project_link}: *#{title}*" end end end diff --git a/changelogs/unreleased/26500-informative-slack-notifications.yml b/changelogs/unreleased/26500-informative-slack-notifications.yml new file mode 100644 index 00000000000..342235424f4 --- /dev/null +++ b/changelogs/unreleased/26500-informative-slack-notifications.yml @@ -0,0 +1,4 @@ +--- +title: Add user & build links in Slack Notifications +merge_request: 8641 +author: Poornima M diff --git a/lib/gitlab/data_builder/build.rb b/lib/gitlab/data_builder/build.rb index 6548e6475c6..f78106f5b10 100644 --- a/lib/gitlab/data_builder/build.rb +++ b/lib/gitlab/data_builder/build.rb @@ -8,6 +8,8 @@ module Gitlab commit = build.pipeline user = build.user + author_url = build_author_url(build.commit, commit) + data = { object_kind: 'build', @@ -43,6 +45,7 @@ module Gitlab message: commit.git_commit_message, author_name: commit.git_author_name, author_email: commit.git_author_email, + author_url: author_url, status: commit.status, duration: commit.duration, started_at: commit.started_at, @@ -62,6 +65,13 @@ module Gitlab data end + + private + + def build_author_url(commit, pipeline) + author = commit.try(:author) + author ? Gitlab::Routing.url_helpers.user_url(author) : "mailto:#{pipeline.git_author_email}" + end end end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 0397d5d4001..e4cac0e1058 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -128,5 +128,17 @@ FactoryGirl.define do build.save! end end + + trait :with_commit do + after(:build) do |build| + allow(build).to receive(:commit).and_return build(:commit, :without_author) + end + end + + trait :with_commit_and_author do + after(:build) do |build| + allow(build).to receive(:commit).and_return build(:commit) + end + end end end diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb index ac6eb0a7897..89e260cf65b 100644 --- a/spec/factories/commits.rb +++ b/spec/factories/commits.rb @@ -8,5 +8,15 @@ FactoryGirl.define do initialize_with do new(git_commit, project) end + + after(:build) do |commit| + allow(commit).to receive(:author).and_return build(:author) + end + + trait :without_author do + after(:build) do |commit| + allow(commit).to receive(:author).and_return nil + end + end end end diff --git a/spec/lib/gitlab/data_builder/build_spec.rb b/spec/lib/gitlab/data_builder/build_spec.rb index 6c71e98066b..91c43f2bdc0 100644 --- a/spec/lib/gitlab/data_builder/build_spec.rb +++ b/spec/lib/gitlab/data_builder/build_spec.rb @@ -17,5 +17,31 @@ describe Gitlab::DataBuilder::Build do it { expect(data[:build_allow_failure]).to eq(false) } it { expect(data[:project_id]).to eq(build.project.id) } it { expect(data[:project_name]).to eq(build.project.name_with_namespace) } + + context 'commit author_url' do + context 'when no commit present' do + let(:build) { create(:ci_build) } + + it 'sets to mailing address of git_author_email' do + expect(data[:commit][:author_url]).to eq("mailto:#{build.pipeline.git_author_email}") + end + end + + context 'when commit present but has no author' do + let(:build) { create(:ci_build, :with_commit) } + + it 'sets to mailing address of git_author_email' do + expect(data[:commit][:author_url]).to eq("mailto:#{build.pipeline.git_author_email}") + end + end + + context 'when commit and author are present' do + let(:build) { create(:ci_build, :with_commit_and_author) } + + it 'sets to GitLab user url' do + expect(data[:commit][:author_url]).to eq(Gitlab::Routing.url_helpers.user_url(username: build.commit.author.username)) + end + end + end end end diff --git a/spec/models/project_services/chat_message/build_message_spec.rb b/spec/models/project_services/chat_message/build_message_spec.rb index 50ad5013df9..3bd7ec18ae0 100644 --- a/spec/models/project_services/chat_message/build_message_spec.rb +++ b/spec/models/project_services/chat_message/build_message_spec.rb @@ -11,21 +11,28 @@ describe ChatMessage::BuildMessage do project_name: 'project_name', project_url: 'http://example.gitlab.com', + build_id: 1, + build_name: build_name, + build_stage: stage, commit: { status: status, author_name: 'hacker', + author_url: 'http://example.gitlab.com/hacker', duration: duration, }, } end let(:message) { build_message } + let(:stage) { 'test' } + let(:status) { 'success' } + let(:build_name) { 'rspec' } + let(:duration) { 10 } context 'build succeeded' do let(:status) { 'success' } let(:color) { 'good' } - let(:duration) { 10 } let(:message) { build_message('passed') } it 'returns a message with information about succeeded build' do @@ -38,7 +45,6 @@ describe ChatMessage::BuildMessage do context 'build failed' do let(:status) { 'failed' } let(:color) { 'danger' } - let(:duration) { 10 } it 'returns a message with information about failed build' do expect(subject.pretext).to be_empty @@ -47,11 +53,25 @@ describe ChatMessage::BuildMessage do end end - def build_message(status_text = status) + it 'returns a message with information on build' do + expect(subject.fallback).to include("on build ") + end + + it 'returns a message with stage name' do + expect(subject.fallback).to include("of stage #{stage}") + end + + it 'returns a message with link to author' do + expect(subject.fallback).to include("by ") + end + + def build_message(status_text = status, stage_text = stage, build_text = build_name) ":" \ " Commit " \ " of branch" \ - " by hacker #{status_text} in #{duration} #{'second'.pluralize(duration)}" + " by #{status_text}" \ + " on build " \ + " of stage #{stage_text} in #{duration} #{'second'.pluralize(duration)}" end end