Merge branch '51959-branch-and-tag-name-links' into 'master'

Add support for surfacing a link to the branch or tag name in push message…

Closes #51959

See merge request gitlab-org/gitlab-ce!22651

3 files changed, 77 insertions, 40 deletions

diff --git a/app/models/project_services/chat_message/push_message.rb b/app/models/project_services/chat_message/push_message.rb
index 82be33a12a1..5dd0414b7e6 100644
--- a/app/models/project_services/chat_message/push_message.rb
+++ b/app/models/project_services/chat_message/push_message.rb
@@ -26,16 +26,8 @@ module ChatMessage
     end
 
     def activity
-      action = if new_branch?
-                 "created"
-               elsif removed_branch?
-                 "removed"
-               else
-                 "pushed to"
-               end
-
       {
-        title: "#{user_combined_name} #{action} #{ref_type}",
+        title: humanized_action(short: true),
         subtitle: "in #{project_link}",
         text: compare_link,
         image: user_avatar
@@ -44,32 +36,21 @@ module ChatMessage
 
     private
 
+    def humanized_action(short: false)
+      action, ref_link, target_link = compose_action_details
+      text = [user_combined_name, action, ref_type, ref_link]
+      text << target_link unless short
+      text.join(' ')
+    end
+
     def message
-      if new_branch?
-        new_branch_message
-      elsif removed_branch?
-        removed_branch_message
-      else
-        push_message
-      end
+      humanized_action
     end
 
     def format(string)
       Slack::Notifier::LinkFormatter.format(string)
     end
 
-    def new_branch_message
-      "#{user_combined_name} pushed new #{ref_type} #{branch_link} to #{project_link}"
-    end
-
-    def removed_branch_message
-      "#{user_combined_name} removed #{ref_type} #{ref} from #{project_link}"
-    end
-
-    def push_message
-      "#{user_combined_name} pushed to #{ref_type} #{branch_link} of #{project_link} (#{compare_link})"
-    end
-
     def commit_messages
       commits.map { |commit| compose_commit_message(commit) }.join("\n\n")
     end
@@ -115,6 +96,16 @@ module ChatMessage
       "[Compare changes](#{compare_url})"
     end
 
+    def compose_action_details
+      if new_branch?
+        ['pushed new', branch_link, "to #{project_link}"]
+      elsif removed_branch?
+        ['removed', ref, "from #{project_link}"]
+      else
+        ['pushed to', branch_link, "of #{project_link} (#{compare_link})"]
+      end
+    end
+
     def attachment_color
       '#345'
     end
diff --git a/changelogs/unreleased/51959-branch-and-tag-name-links.yml b/changelogs/unreleased/51959-branch-and-tag-name-links.yml
new file mode 100644
index 00000000000..64f1522c70d
--- /dev/null
+++ b/changelogs/unreleased/51959-branch-and-tag-name-links.yml
@@ -0,0 +1,5 @@
+---
+title: Chat message push notifications now include links back to GitLab branches
+merge_request: 22651
+author: Tony Castrogiovanni
+type: added
diff --git a/spec/models/project_services/chat_message/push_message_spec.rb b/spec/models/project_services/chat_message/push_message_spec.rb
index 19c2862264f..973d6bdb2a0 100644
--- a/spec/models/project_services/chat_message/push_message_spec.rb
+++ b/spec/models/project_services/chat_message/push_message_spec.rb
@@ -48,12 +48,12 @@ describe ChatMessage::PushMessage do
           'test.user pushed to branch [master](http://url.com/commits/master) of [project_name](http://url.com) ([Compare changes](http://url.com/compare/before...after))')
         expect(subject.attachments).to eq(
           "[abcdefgh](http://url1.com): message1 - author1\n\n[12345678](http://url2.com): message2 - author2")
-        expect(subject.activity).to eq({
-          title: 'test.user pushed to branch',
+        expect(subject.activity).to eq(
+          title: 'test.user pushed to branch [master](http://url.com/commits/master)',
           subtitle: 'in [project_name](http://url.com)',
           text: '[Compare changes](http://url.com/compare/before...after)',
           image: 'http://someavatar.com'
-        })
+        )
       end
     end
   end
@@ -89,12 +89,53 @@ describe ChatMessage::PushMessage do
         expect(subject.pretext).to eq(
           'test.user pushed new tag [new_tag](http://url.com/commits/new_tag) to [project_name](http://url.com)')
         expect(subject.attachments).to be_empty
-        expect(subject.activity).to eq({
-          title: 'test.user created tag',
+        expect(subject.activity).to eq(
+          title: 'test.user pushed new tag [new_tag](http://url.com/commits/new_tag)',
           subtitle: 'in [project_name](http://url.com)',
           text: '[Compare changes](http://url.com/compare/0000000000000000000000000000000000000000...after)',
           image: 'http://someavatar.com'
-        })
+        )
+      end
+    end
+  end
+
+  context 'removed tag' do
+    let(:args) do
+      {
+        after: Gitlab::Git::BLANK_SHA,
+        before: 'before',
+        project_name: 'project_name',
+        ref: 'refs/tags/new_tag',
+        user_name: 'test.user',
+        user_avatar: 'http://someavatar.com',
+        project_url: 'http://url.com'
+      }
+    end
+
+    context 'without markdown' do
+      it 'returns a message regarding removal of tags' do
+        expect(subject.pretext).to eq('test.user removed tag ' \
+          'new_tag from ' \
+          '<http://url.com|project_name>')
+        expect(subject.attachments).to be_empty
+      end
+    end
+
+    context 'with markdown' do
+      before do
+        args[:markdown] = true
+      end
+
+      it 'returns a message regarding removal of tags' do
+        expect(subject.pretext).to eq(
+          'test.user removed tag new_tag from [project_name](http://url.com)')
+        expect(subject.attachments).to be_empty
+        expect(subject.activity).to eq(
+          title: 'test.user removed tag new_tag',
+          subtitle: 'in [project_name](http://url.com)',
+          text: '[Compare changes](http://url.com/compare/before...0000000000000000000000000000000000000000)',
+          image: 'http://someavatar.com'
+        )
       end
     end
   end
@@ -122,12 +163,12 @@ describe ChatMessage::PushMessage do
         expect(subject.pretext).to eq(
           'test.user pushed new branch [master](http://url.com/commits/master) to [project_name](http://url.com)')
         expect(subject.attachments).to be_empty
-        expect(subject.activity).to eq({
-          title: 'test.user created branch',
+        expect(subject.activity).to eq(
+          title: 'test.user pushed new branch [master](http://url.com/commits/master)',
           subtitle: 'in [project_name](http://url.com)',
           text: '[Compare changes](http://url.com/compare/0000000000000000000000000000000000000000...after)',
           image: 'http://someavatar.com'
-        })
+        )
       end
     end
   end
@@ -154,12 +195,12 @@ describe ChatMessage::PushMessage do
         expect(subject.pretext).to eq(
           'test.user removed branch master from [project_name](http://url.com)')
         expect(subject.attachments).to be_empty
-        expect(subject.activity).to eq({
-          title: 'test.user removed branch',
+        expect(subject.activity).to eq(
+          title: 'test.user removed branch master',
           subtitle: 'in [project_name](http://url.com)',
           text: '[Compare changes](http://url.com/compare/before...0000000000000000000000000000000000000000)',
           image: 'http://someavatar.com'
-        })
+        )
       end
     end
   end