Merge branch 'add-link-to-mrs-references' into 'master'

Add link to Merge request references in emails

See merge request gitlab-org/gitlab!33316

app/helpers/notify_helper.rb

+# frozen_string_literal: true
+
+module NotifyHelper
+  def merge_request_reference_link(entity, *args)
+    link_to(entity.to_reference, merge_request_url(entity, *args))
+  end
+
+  def issue_reference_link(entity, *args)
+    link_to(entity.to_reference, issue_url(entity, *args))
+  end
+end

app/views/notify/closed_merge_request_email.html.haml

 %p
-  Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)}
+  Merge Request #{merge_request_reference_link(@merge_request)}
+  was closed by #{sanitize_name(@updated_by.name)}

app/views/notify/closed_merge_request_email.text.haml

 Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)}
 
-Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 

app/views/notify/merge_request_status_email.html.haml

 %p
-  Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)}
+  Merge Request #{merge_request_reference_link(@merge_request)}
+  was #{@mr_status} by #{sanitize_name(@updated_by.name)}

app/views/notify/merge_request_status_email.text.haml

 Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)}
 
-Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 

app/views/notify/merge_request_unmergeable_email.html.haml

 %p
-  Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} can no longer be merged due to conflict.
+  Merge Request #{merge_request_reference_link(@merge_request)} can no longer be merged due to conflict.

app/views/notify/merge_request_unmergeable_email.text.haml

 Merge Request #{@merge_request.to_reference} can no longer be merged due to conflict.
 
-Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 

app/views/notify/merged_merge_request_email.html.haml

 %p
-  Merge Request #{link_to @merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)} was merged
+  Merge Request #{merge_request_reference_link(@merge_request)} was merged

app/views/notify/new_issue_email.html.haml

 %p.details
-  #{link_to @issue.author_name, user_url(@issue.author)} created an issue #{link_to @issue.to_reference(full: false), issue_url(@issue)}:
+  #{link_to @issue.author_name, user_url(@issue.author)} created an issue #{issue_reference_link(@issue)}:
 
 - if @issue.assignees.any?
   %p

app/views/notify/new_mention_in_merge_request_email.html.haml

 %p
-  You have been mentioned in Merge Request #{@merge_request.to_reference}
+  You have been mentioned in Merge Request #{merge_request_reference_link(@merge_request)}
 
 = render template: 'notify/new_merge_request_email'

app/views/notify/push_to_merge_request_email.html.haml

 %h3
   = sanitize_name(@updated_by_user.name)
   pushed new commits to merge request
-  = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request))
+  = merge_request_reference_link(@merge_request)
 
 - if @existing_commits.any?
   - count = @existing_commits.size

app/views/notify/push_to_merge_request_email.text.haml

-#{sanitize_name(@updated_by_user.name)} pushed new commits to merge request #{@merge_request.to_reference}
-\
-#{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))}
+#{sanitize_name(@updated_by_user.name)} pushed new commits to merge request #{merge_request_reference_link(@merge_request)}
 \
 - if @existing_commits.any?
   - count = @existing_commits.size

app/views/notify/resolved_all_discussions_email.html.haml

 %p
-  All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{sanitize_name(@resolved_by.name)}
+  All discussions on Merge Request #{merge_request_reference_link(@merge_request)}
+  were resolved by #{sanitize_name(@resolved_by.name)}

changelogs/unreleased/add-link-to-mrs-references.yml

+---
+title: Render Merge request reference as link in email templates
+merge_request: 33316
+author:
+type: changed

ee/app/views/notify/approved_merge_request_email.text.haml

 Merge Request #{@merge_request.to_reference} was approved by #{sanitize_name(@approved_by.name)}
 
-Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 

ee/app/views/notify/unapproved_merge_request_email.text.haml

 Merge Request #{@merge_request.to_reference} was unapproved by #{@unapproved_by.name}
 
-Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
+Merge Request URL: #{project_merge_request_url(@merge_request.target_project, @merge_request)}
 
 = merge_path_description(@merge_request, 'to')
 

spec/helpers/notify_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NotifyHelper do
+  include ActionView::Helpers::UrlHelper
+
+  describe 'merge_request_reference_link' do
+    let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+
+    it 'returns link to merge request with the text reference' do
+      url = "http://test.host/#{project.full_path}/-/merge_requests/#{merge_request.iid}"
+
+      expect(merge_request_reference_link(merge_request)).to eq(reference_link(merge_request, url))
+    end
+  end
+
+  describe 'issue_reference_link' do
+    let(:project) { create(:project) }
+    let(:issue) { create(:issue, project: project) }
+
+    it 'returns link to issue with the text reference' do
+      url = "http://test.host/#{project.full_path}/-/issues/#{issue.iid}"
+
+      expect(issue_reference_link(issue)).to eq(reference_link(issue, url))
+    end
+  end
+
+  def reference_link(entity, url)
+    "<a href=\"#{url}\">#{entity.to_reference}</a>"
+  end
+end

spec/mailers/notify_spec.rb

@@ -105,6 +105,7 @@ RSpec.describe Notify do
         it 'contains a link to issue author' do
           is_expected.to have_body_text(issue.author_name)
           is_expected.to have_body_text 'created an issue'
+          is_expected.to have_link(issue.to_reference, href: project_issue_url(issue.project, issue))
         end
 
         it 'contains a link to the issue' do
@@ -467,6 +468,7 @@ RSpec.describe Notify do
             is_expected.to have_body_text(status)
             is_expected.to have_body_text(current_user_sanitized)
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
+            is_expected.to have_link(merge_request.to_reference, href: project_merge_request_url(merge_request.target_project, merge_request))
           end
         end
       end
@@ -497,6 +499,7 @@ RSpec.describe Notify do
             is_expected.to have_referable_subject(merge_request, reply: true)
             is_expected.to have_body_text('merged')
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
+            is_expected.to have_link(merge_request.to_reference, href: project_merge_request_url(merge_request.target_project, merge_request))
           end
         end
       end
@@ -534,6 +537,7 @@ RSpec.describe Notify do
             is_expected.to have_referable_subject(merge_request, reply: true)
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
             is_expected.to have_body_text('due to conflict.')
+            is_expected.to have_link(merge_request.to_reference, href: project_merge_request_url(merge_request.target_project, merge_request))
           end
         end
       end
@@ -567,6 +571,7 @@ RSpec.describe Notify do
             is_expected.to have_referable_subject(merge_request, reply: true)
             is_expected.to have_body_text("#{push_user.name} pushed new commits")
             is_expected.to have_body_text(project_merge_request_path(project, merge_request))
+            is_expected.to have_link(merge_request.to_reference, href: project_merge_request_url(merge_request.target_project, merge_request))
           end
         end
       end