From 75000979703a58297f76278c4b630ff026b9c50d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 6 Oct 2017 13:00:50 -0700 Subject: [PATCH] Include GitLab full name with username in Slack messages We used to include the first and last name of the user, but !6624 modified this to include only the username. Let's compromise and add both in the form of `First Last (username)`. Closes #38865 --- .../chat_message/base_message.rb | 10 +++++++++ .../chat_message/issue_message.rb | 6 ++--- .../chat_message/merge_message.rb | 4 ++-- .../chat_message/note_message.rb | 4 ++-- .../chat_message/pipeline_message.rb | 6 ++--- .../chat_message/push_message.rb | 8 +++---- .../chat_message/wiki_page_message.rb | 4 ++-- .../unreleased/sh-show-all-slack-names.yml | 5 +++++ .../chat_message/issue_message_spec.rb | 12 +++++----- .../chat_message/merge_message_spec.rb | 12 +++++----- .../chat_message/note_message_spec.rb | 22 +++++++++---------- .../chat_message/pipeline_message_spec.rb | 15 +++++++------ .../chat_message/wiki_page_message_spec.rb | 12 +++++----- 13 files changed, 68 insertions(+), 52 deletions(-) create mode 100644 changelogs/unreleased/sh-show-all-slack-names.yml diff --git a/app/models/project_services/chat_message/base_message.rb b/app/models/project_services/chat_message/base_message.rb index e2ad586aea7..22a65b5145e 100644 --- a/app/models/project_services/chat_message/base_message.rb +++ b/app/models/project_services/chat_message/base_message.rb @@ -3,6 +3,7 @@ require 'slack-notifier' module ChatMessage class BaseMessage attr_reader :markdown + attr_reader :user_full_name attr_reader :user_name attr_reader :user_avatar attr_reader :project_name @@ -12,10 +13,19 @@ module ChatMessage @markdown = params[:markdown] || false @project_name = params.dig(:project, :path_with_namespace) || params[:project_name] @project_url = params.dig(:project, :web_url) || params[:project_url] + @user_full_name = params.dig(:user, :name) || params[:user_full_name] @user_name = params.dig(:user, :username) || params[:user_name] @user_avatar = params.dig(:user, :avatar_url) || params[:user_avatar] end + def user_combined_name + if user_full_name.present? + "#{user_full_name} (#{user_name})" + else + user_name + end + end + def pretext return message if markdown diff --git a/app/models/project_services/chat_message/issue_message.rb b/app/models/project_services/chat_message/issue_message.rb index 4b9a2b1e1f3..1327b075858 100644 --- a/app/models/project_services/chat_message/issue_message.rb +++ b/app/models/project_services/chat_message/issue_message.rb @@ -29,7 +29,7 @@ module ChatMessage def activity { - title: "Issue #{state} by #{user_name}", + title: "Issue #{state} by #{user_combined_name}", subtitle: "in #{project_link}", text: issue_link, image: user_avatar @@ -40,9 +40,9 @@ module ChatMessage def message if state == 'opened' - "[#{project_link}] Issue #{state} by #{user_name}" + "[#{project_link}] Issue #{state} by #{user_combined_name}" else - "[#{project_link}] Issue #{issue_link} #{state} by #{user_name}" + "[#{project_link}] Issue #{issue_link} #{state} by #{user_combined_name}" end end diff --git a/app/models/project_services/chat_message/merge_message.rb b/app/models/project_services/chat_message/merge_message.rb index 7d0de81cdf0..f412b6833d9 100644 --- a/app/models/project_services/chat_message/merge_message.rb +++ b/app/models/project_services/chat_message/merge_message.rb @@ -24,7 +24,7 @@ module ChatMessage def activity { - title: "Merge Request #{state} by #{user_name}", + title: "Merge Request #{state} by #{user_combined_name}", subtitle: "in #{project_link}", text: merge_request_link, image: user_avatar @@ -46,7 +46,7 @@ module ChatMessage end def merge_request_message - "#{user_name} #{state} #{merge_request_link} in #{project_link}: #{title}" + "#{user_combined_name} #{state} #{merge_request_link} in #{project_link}: #{title}" end def merge_request_link diff --git a/app/models/project_services/chat_message/note_message.rb b/app/models/project_services/chat_message/note_message.rb index 2da4c244229..7f9486132e6 100644 --- a/app/models/project_services/chat_message/note_message.rb +++ b/app/models/project_services/chat_message/note_message.rb @@ -32,7 +32,7 @@ module ChatMessage def activity { - title: "#{user_name} #{link('commented on ' + target, note_url)}", + title: "#{user_combined_name} #{link('commented on ' + target, note_url)}", subtitle: "in #{project_link}", text: formatted_title, image: user_avatar @@ -42,7 +42,7 @@ module ChatMessage private def message - "#{user_name} #{link('commented on ' + target, note_url)} in #{project_link}: *#{formatted_title}*" + "#{user_combined_name} #{link('commented on ' + target, note_url)} in #{project_link}: *#{formatted_title}*" end def format_title(title) diff --git a/app/models/project_services/chat_message/pipeline_message.rb b/app/models/project_services/chat_message/pipeline_message.rb index d63d4ec2b12..2135122278a 100644 --- a/app/models/project_services/chat_message/pipeline_message.rb +++ b/app/models/project_services/chat_message/pipeline_message.rb @@ -9,7 +9,7 @@ module ChatMessage def initialize(data) super - @user_name = data.dig(:user, :name) || 'API' + @user_name = data.dig(:user, :username) || 'API' pipeline_attributes = data[:object_attributes] @ref_type = pipeline_attributes[:tag] ? 'tag' : 'branch' @@ -35,7 +35,7 @@ module ChatMessage def activity { - title: "Pipeline #{pipeline_link} of #{ref_type} #{branch_link} by #{user_name} #{humanized_status}", + title: "Pipeline #{pipeline_link} of #{ref_type} #{branch_link} by #{user_combined_name} #{humanized_status}", subtitle: "in #{project_link}", text: "in #{pretty_duration(duration)}", image: user_avatar || '' @@ -45,7 +45,7 @@ module ChatMessage private def message - "#{project_link}: Pipeline #{pipeline_link} of #{ref_type} #{branch_link} by #{user_name} #{humanized_status} in #{pretty_duration(duration)}" + "#{project_link}: Pipeline #{pipeline_link} of #{ref_type} #{branch_link} by #{user_combined_name} #{humanized_status} in #{pretty_duration(duration)}" end def humanized_status diff --git a/app/models/project_services/chat_message/push_message.rb b/app/models/project_services/chat_message/push_message.rb index c52dd6ef8ef..8d599c5f116 100644 --- a/app/models/project_services/chat_message/push_message.rb +++ b/app/models/project_services/chat_message/push_message.rb @@ -33,7 +33,7 @@ module ChatMessage end { - title: "#{user_name} #{action} #{ref_type}", + title: "#{user_combined_name} #{action} #{ref_type}", subtitle: "in #{project_link}", text: compare_link, image: user_avatar @@ -57,15 +57,15 @@ module ChatMessage end def new_branch_message - "#{user_name} pushed new #{ref_type} #{branch_link} to #{project_link}" + "#{user_combined_name} pushed new #{ref_type} #{branch_link} to #{project_link}" end def removed_branch_message - "#{user_name} removed #{ref_type} #{ref} from #{project_link}" + "#{user_combined_name} removed #{ref_type} #{ref} from #{project_link}" end def push_message - "#{user_name} pushed to #{ref_type} #{branch_link} of #{project_link} (#{compare_link})" + "#{user_combined_name} pushed to #{ref_type} #{branch_link} of #{project_link} (#{compare_link})" end def commit_messages diff --git a/app/models/project_services/chat_message/wiki_page_message.rb b/app/models/project_services/chat_message/wiki_page_message.rb index a139a8ee727..d84b80f2de2 100644 --- a/app/models/project_services/chat_message/wiki_page_message.rb +++ b/app/models/project_services/chat_message/wiki_page_message.rb @@ -31,7 +31,7 @@ module ChatMessage def activity { - title: "#{user_name} #{action} #{wiki_page_link}", + title: "#{user_combined_name} #{action} #{wiki_page_link}", subtitle: "in #{project_link}", text: title, image: user_avatar @@ -41,7 +41,7 @@ module ChatMessage private def message - "#{user_name} #{action} #{wiki_page_link} in #{project_link}: *#{title}*" + "#{user_combined_name} #{action} #{wiki_page_link} in #{project_link}: *#{title}*" end def description_message diff --git a/changelogs/unreleased/sh-show-all-slack-names.yml b/changelogs/unreleased/sh-show-all-slack-names.yml new file mode 100644 index 00000000000..f970cd0fb15 --- /dev/null +++ b/changelogs/unreleased/sh-show-all-slack-names.yml @@ -0,0 +1,5 @@ +--- +title: Include GitLab full name in Slack messages +merge_request: +author: +type: changed diff --git a/spec/models/project_services/chat_message/issue_message_spec.rb b/spec/models/project_services/chat_message/issue_message_spec.rb index 4bb1db684e6..d37726dc3f1 100644 --- a/spec/models/project_services/chat_message/issue_message_spec.rb +++ b/spec/models/project_services/chat_message/issue_message_spec.rb @@ -42,7 +42,7 @@ describe ChatMessage::IssueMessage do context 'open' do it 'returns a message regarding opening of issues' do expect(subject.pretext).to eq( - '[] Issue opened by test.user') + '[] Issue opened by Test User (test.user)') expect(subject.attachments).to eq([ { title: "#100 Issue title", @@ -62,7 +62,7 @@ describe ChatMessage::IssueMessage do it 'returns a message regarding closing of issues' do expect(subject.pretext). to eq( - '[] Issue closed by test.user') + '[] Issue closed by Test User (test.user)') expect(subject.attachments).to be_empty end end @@ -76,10 +76,10 @@ describe ChatMessage::IssueMessage do context 'open' do it 'returns a message regarding opening of issues' do expect(subject.pretext).to eq( - '[[project_name](http://somewhere.com)] Issue opened by test.user') + '[[project_name](http://somewhere.com)] Issue opened by Test User (test.user)') expect(subject.attachments).to eq('issue description') expect(subject.activity).to eq({ - title: 'Issue opened by test.user', + title: 'Issue opened by Test User (test.user)', subtitle: 'in [project_name](http://somewhere.com)', text: '[#100 Issue title](http://url.com)', image: 'http://someavatar.com' @@ -95,10 +95,10 @@ describe ChatMessage::IssueMessage do it 'returns a message regarding closing of issues' do expect(subject.pretext). to eq( - '[[project_name](http://somewhere.com)] Issue [#100 Issue title](http://url.com) closed by test.user') + '[[project_name](http://somewhere.com)] Issue [#100 Issue title](http://url.com) closed by Test User (test.user)') expect(subject.attachments).to be_empty expect(subject.activity).to eq({ - title: 'Issue closed by test.user', + title: 'Issue closed by Test User (test.user)', subtitle: 'in [project_name](http://somewhere.com)', text: '[#100 Issue title](http://url.com)', image: 'http://someavatar.com' diff --git a/spec/models/project_services/chat_message/merge_message_spec.rb b/spec/models/project_services/chat_message/merge_message_spec.rb index b600a36f578..184a07ae0f9 100644 --- a/spec/models/project_services/chat_message/merge_message_spec.rb +++ b/spec/models/project_services/chat_message/merge_message_spec.rb @@ -33,7 +33,7 @@ describe ChatMessage::MergeMessage do context 'open' do it 'returns a message regarding opening of merge requests' do expect(subject.pretext).to eq( - 'test.user opened in : *Merge Request title*') + 'Test User (test.user) opened in : *Merge Request title*') expect(subject.attachments).to be_empty end end @@ -44,7 +44,7 @@ describe ChatMessage::MergeMessage do end it 'returns a message regarding closing of merge requests' do expect(subject.pretext).to eq( - 'test.user closed in : *Merge Request title*') + 'Test User (test.user) closed in : *Merge Request title*') expect(subject.attachments).to be_empty end end @@ -58,10 +58,10 @@ describe ChatMessage::MergeMessage do context 'open' do it 'returns a message regarding opening of merge requests' do expect(subject.pretext).to eq( - 'test.user opened [!100 *Merge Request title*](http://somewhere.com/merge_requests/100) in [project_name](http://somewhere.com): *Merge Request title*') + 'Test User (test.user) opened [!100 *Merge Request title*](http://somewhere.com/merge_requests/100) in [project_name](http://somewhere.com): *Merge Request title*') expect(subject.attachments).to be_empty expect(subject.activity).to eq({ - title: 'Merge Request opened by test.user', + title: 'Merge Request opened by Test User (test.user)', subtitle: 'in [project_name](http://somewhere.com)', text: '[!100 *Merge Request title*](http://somewhere.com/merge_requests/100)', image: 'http://someavatar.com' @@ -76,10 +76,10 @@ describe ChatMessage::MergeMessage do it 'returns a message regarding closing of merge requests' do expect(subject.pretext).to eq( - 'test.user closed [!100 *Merge Request title*](http://somewhere.com/merge_requests/100) in [project_name](http://somewhere.com): *Merge Request title*') + 'Test User (test.user) closed [!100 *Merge Request title*](http://somewhere.com/merge_requests/100) in [project_name](http://somewhere.com): *Merge Request title*') expect(subject.attachments).to be_empty expect(subject.activity).to eq({ - title: 'Merge Request closed by test.user', + title: 'Merge Request closed by Test User (test.user)', subtitle: 'in [project_name](http://somewhere.com)', text: '[!100 *Merge Request title*](http://somewhere.com/merge_requests/100)', image: 'http://someavatar.com' diff --git a/spec/models/project_services/chat_message/note_message_spec.rb b/spec/models/project_services/chat_message/note_message_spec.rb index a09c2f9935c..5abbd7bec18 100644 --- a/spec/models/project_services/chat_message/note_message_spec.rb +++ b/spec/models/project_services/chat_message/note_message_spec.rb @@ -38,7 +38,7 @@ describe ChatMessage::NoteMessage do context 'without markdown' do it 'returns a message regarding notes on commits' do - expect(subject.pretext).to eq("test.user in : " \ "*Added a commit message*") expect(subject.attachments).to eq([{ @@ -55,11 +55,11 @@ describe ChatMessage::NoteMessage do it 'returns a message regarding notes on commits' do expect(subject.pretext).to eq( - 'test.user [commented on commit 5f163b2b](http://url.com) in [project_name](http://somewhere.com): *Added a commit message*' + 'Test User (test.user) [commented on commit 5f163b2b](http://url.com) in [project_name](http://somewhere.com): *Added a commit message*' ) expect(subject.attachments).to eq('comment on a commit') expect(subject.activity).to eq({ - title: 'test.user [commented on commit 5f163b2b](http://url.com)', + title: 'Test User (test.user) [commented on commit 5f163b2b](http://url.com)', subtitle: 'in [project_name](http://somewhere.com)', text: 'Added a commit message', image: 'http://fakeavatar' @@ -81,7 +81,7 @@ describe ChatMessage::NoteMessage do context 'without markdown' do it 'returns a message regarding notes on a merge request' do - expect(subject.pretext).to eq("test.user in : " \ "*merge request title*") expect(subject.attachments).to eq([{ @@ -98,10 +98,10 @@ describe ChatMessage::NoteMessage do it 'returns a message regarding notes on a merge request' do expect(subject.pretext).to eq( - 'test.user [commented on merge request !30](http://url.com) in [project_name](http://somewhere.com): *merge request title*') + 'Test User (test.user) [commented on merge request !30](http://url.com) in [project_name](http://somewhere.com): *merge request title*') expect(subject.attachments).to eq('comment on a merge request') expect(subject.activity).to eq({ - title: 'test.user [commented on merge request !30](http://url.com)', + title: 'Test User (test.user) [commented on merge request !30](http://url.com)', subtitle: 'in [project_name](http://somewhere.com)', text: 'merge request title', image: 'http://fakeavatar' @@ -124,7 +124,7 @@ describe ChatMessage::NoteMessage do context 'without markdown' do it 'returns a message regarding notes on an issue' do expect(subject.pretext).to eq( - "test.user in : " \ "*issue title*") expect(subject.attachments).to eq([{ @@ -141,10 +141,10 @@ describe ChatMessage::NoteMessage do it 'returns a message regarding notes on an issue' do expect(subject.pretext).to eq( - 'test.user [commented on issue #20](http://url.com) in [project_name](http://somewhere.com): *issue title*') + 'Test User (test.user) [commented on issue #20](http://url.com) in [project_name](http://somewhere.com): *issue title*') expect(subject.attachments).to eq('comment on an issue') expect(subject.activity).to eq({ - title: 'test.user [commented on issue #20](http://url.com)', + title: 'Test User (test.user) [commented on issue #20](http://url.com)', subtitle: 'in [project_name](http://somewhere.com)', text: 'issue title', image: 'http://fakeavatar' @@ -165,7 +165,7 @@ describe ChatMessage::NoteMessage do context 'without markdown' do it 'returns a message regarding notes on a project snippet' do - expect(subject.pretext).to eq("test.user in : " \ "*snippet title*") expect(subject.attachments).to eq([{ @@ -182,7 +182,7 @@ describe ChatMessage::NoteMessage do it 'returns a message regarding notes on a project snippet' do expect(subject.pretext).to eq( - 'test.user [commented on snippet $5](http://url.com) in [project_name](http://somewhere.com): *snippet title*') + 'Test User (test.user) [commented on snippet $5](http://url.com) in [project_name](http://somewhere.com): *snippet title*') expect(subject.attachments).to eq('comment on a snippet') end end diff --git a/spec/models/project_services/chat_message/pipeline_message_spec.rb b/spec/models/project_services/chat_message/pipeline_message_spec.rb index 43b02568cb9..0ff20400999 100644 --- a/spec/models/project_services/chat_message/pipeline_message_spec.rb +++ b/spec/models/project_services/chat_message/pipeline_message_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe ChatMessage::PipelineMessage do subject { described_class.new(args) } - let(:user) { { name: 'hacker' } } + let(:user) { { name: "The Hacker", username: 'hacker' } } let(:duration) { 7210 } let(:args) do { @@ -22,12 +22,13 @@ describe ChatMessage::PipelineMessage do user: user } end + let(:combined_name) { "The Hacker (hacker)" } context 'without markdown' do context 'pipeline succeeded' do let(:status) { 'success' } let(:color) { 'good' } - let(:message) { build_message('passed') } + let(:message) { build_message('passed', combined_name) } it 'returns a message with information about succeeded build' do expect(subject.pretext).to be_empty @@ -39,7 +40,7 @@ describe ChatMessage::PipelineMessage do context 'pipeline failed' do let(:status) { 'failed' } let(:color) { 'danger' } - let(:message) { build_message } + let(:message) { build_message(status, combined_name) } it 'returns a message with information about failed build' do expect(subject.pretext).to be_empty @@ -75,13 +76,13 @@ describe ChatMessage::PipelineMessage do context 'pipeline succeeded' do let(:status) { 'success' } let(:color) { 'good' } - let(:message) { build_markdown_message('passed') } + let(:message) { build_markdown_message('passed', combined_name) } it 'returns a message with information about succeeded build' do expect(subject.pretext).to be_empty expect(subject.attachments).to eq(message) expect(subject.activity).to eq({ - title: 'Pipeline [#123](http://example.gitlab.com/pipelines/123) of branch [develop](http://example.gitlab.com/commits/develop) by hacker passed', + title: 'Pipeline [#123](http://example.gitlab.com/pipelines/123) of branch [develop](http://example.gitlab.com/commits/develop) by The Hacker (hacker) passed', subtitle: 'in [project_name](http://example.gitlab.com)', text: 'in 02:00:10', image: '' @@ -92,13 +93,13 @@ describe ChatMessage::PipelineMessage do context 'pipeline failed' do let(:status) { 'failed' } let(:color) { 'danger' } - let(:message) { build_markdown_message } + let(:message) { build_markdown_message(status, combined_name) } it 'returns a message with information about failed build' do expect(subject.pretext).to be_empty expect(subject.attachments).to eq(message) expect(subject.activity).to eq({ - title: 'Pipeline [#123](http://example.gitlab.com/pipelines/123) of branch [develop](http://example.gitlab.com/commits/develop) by hacker failed', + title: 'Pipeline [#123](http://example.gitlab.com/pipelines/123) of branch [develop](http://example.gitlab.com/commits/develop) by The Hacker (hacker) failed', subtitle: 'in [project_name](http://example.gitlab.com)', text: 'in 02:00:10', image: '' diff --git a/spec/models/project_services/chat_message/wiki_page_message_spec.rb b/spec/models/project_services/chat_message/wiki_page_message_spec.rb index c4adee4f489..7efcba9bcfd 100644 --- a/spec/models/project_services/chat_message/wiki_page_message_spec.rb +++ b/spec/models/project_services/chat_message/wiki_page_message_spec.rb @@ -29,7 +29,7 @@ describe ChatMessage::WikiPageMessage do it 'returns a message that a new wiki page was created' do expect(subject.pretext).to eq( - 'test.user created in : '\ + 'Test User (test.user) created in : '\ '*Wiki page title*') end end @@ -41,7 +41,7 @@ describe ChatMessage::WikiPageMessage do it 'returns a message that a wiki page was updated' do expect(subject.pretext).to eq( - 'test.user edited in : '\ + 'Test User (test.user) edited in : '\ '*Wiki page title*') end end @@ -95,7 +95,7 @@ describe ChatMessage::WikiPageMessage do it 'returns a message that a new wiki page was created' do expect(subject.pretext).to eq( - 'test.user created [wiki page](http://url.com) in [project_name](http://somewhere.com): *Wiki page title*') + 'Test User (test.user) created [wiki page](http://url.com) in [project_name](http://somewhere.com): *Wiki page title*') end end @@ -106,7 +106,7 @@ describe ChatMessage::WikiPageMessage do it 'returns a message that a wiki page was updated' do expect(subject.pretext).to eq( - 'test.user edited [wiki page](http://url.com) in [project_name](http://somewhere.com): *Wiki page title*') + 'Test User (test.user) edited [wiki page](http://url.com) in [project_name](http://somewhere.com): *Wiki page title*') end end end @@ -141,7 +141,7 @@ describe ChatMessage::WikiPageMessage do it 'returns the attachment for a new wiki page' do expect(subject.activity).to eq({ - title: 'test.user created [wiki page](http://url.com)', + title: 'Test User (test.user) created [wiki page](http://url.com)', subtitle: 'in [project_name](http://somewhere.com)', text: 'Wiki page title', image: 'http://someavatar.com' @@ -156,7 +156,7 @@ describe ChatMessage::WikiPageMessage do it 'returns the attachment for an updated wiki page' do expect(subject.activity).to eq({ - title: 'test.user edited [wiki page](http://url.com)', + title: 'Test User (test.user) edited [wiki page](http://url.com)', subtitle: 'in [project_name](http://somewhere.com)', text: 'Wiki page title', image: 'http://someavatar.com'