Merge branch '19569-include-information-if-issue-was-closed-via-mr' into 'master'

Include MR information if possible when emailing notification of closing an issue

Closes #19569

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

app/helpers/emails_helper.rb

@@ -91,6 +91,28 @@ module EmailsHelper
     ].join(';')
   end
 
+  def closure_reason_text(closed_via, format: nil)
+    case closed_via
+    when MergeRequest
+      merge_request = MergeRequest.find(closed_via[:id]).present
+
+      case format
+      when :html
+        " via merge request #{link_to(merge_request.to_reference, merge_request.web_url)}"
+      else
+        # If it's not HTML nor text then assume it's text to be safe
+        " via merge request #{merge_request.to_reference} (#{merge_request.web_url})"
+      end
+    when String
+      # Technically speaking this should be Commit but per
+      # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15610#note_163812339
+      # we can't deserialize Commit without custom serializer for ActiveJob
+      " via #{closed_via}"
+    else
+      ""
+    end
+  end
+
   # "You are receiving this email because #{reason}"
   def notification_reason_text(reason)
     string = case reason

app/mailers/emails/issues.rb

@@ -30,8 +30,8 @@ module Emails
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil)
-      setup_issue_mail(issue_id, recipient_id)
+    def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason: nil, closed_via: nil)
+      setup_issue_mail(issue_id, recipient_id, closed_via: closed_via)
 
       @updated_by = User.find(updated_by_user_id)
       mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
@@ -91,10 +91,11 @@ module Emails
 
     private
 
-    def setup_issue_mail(issue_id, recipient_id)
+    def setup_issue_mail(issue_id, recipient_id, closed_via: nil)
       @issue = Issue.find(issue_id)
       @project = @issue.project
       @target_url = project_issue_url(@project, @issue)
+      @closed_via = closed_via
 
       @sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
     end

app/mailers/emails/merge_requests.rb

@@ -58,14 +58,14 @@ module Emails
       }))
     end
 
-    def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
+    def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason: nil, closed_via: nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
       @updated_by = User.find(updated_by_user_id)
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
 
-    def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
+    def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason: nil, closed_via: nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
 
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))

app/services/issues/close_service.rb

@@ -7,7 +7,7 @@ module Issues
       return issue unless can?(current_user, :update_issue, issue)
 
       close_issue(issue,
-                  commit: commit,
+                  closed_via: commit,
                   notifications: notifications,
                   system_note: system_note)
     end
@@ -17,9 +17,9 @@ module Issues
     #
     # The code calling this method is responsible for ensuring that a user is
     # allowed to close the given issue.
-    def close_issue(issue, commit: nil, notifications: true, system_note: true)
+    def close_issue(issue, closed_via: nil, notifications: true, system_note: true)
       if project.jira_tracker? && project.jira_service.active && issue.is_a?(ExternalIssue)
-        project.jira_service.close_issue(commit, issue)
+        project.jira_service.close_issue(closed_via, issue)
         todo_service.close_issue(issue, current_user)
         return issue
       end
@@ -27,8 +27,11 @@ module Issues
       if project.issues_enabled? && issue.close
         issue.update(closed_by: current_user)
         event_service.close_issue(issue, current_user)
-        create_note(issue, commit) if system_note
-        notification_service.async.close_issue(issue, current_user) if notifications
+        create_note(issue, closed_via) if system_note
+
+        closed_via = "commit #{closed_via.id}" if closed_via.is_a?(Commit)
+
+        notification_service.async.close_issue(issue, current_user, closed_via: closed_via) if notifications
         todo_service.close_issue(issue, current_user)
         execute_hooks(issue, 'close')
         invalidate_cache_counts(issue, users: issue.assignees)

app/services/notification_service.rb

@@ -89,8 +89,8 @@ class NotificationService
   #  * project team members with notification level higher then Participating
   #  * users with custom level checked with "close issue"
   #
-  def close_issue(issue, current_user)
-    close_resource_email(issue, current_user, :closed_issue_email)
+  def close_issue(issue, current_user, closed_via: nil)
+    close_resource_email(issue, current_user, :closed_issue_email, closed_via: closed_via)
   end
 
   # When we reassign an issue we should send an email to:
@@ -504,7 +504,7 @@ class NotificationService
     end
   end
 
-  def close_resource_email(target, current_user, method, skip_current_user: true)
+  def close_resource_email(target, current_user, method, skip_current_user: true, closed_via: nil)
     action = method == :merged_merge_request_email ? "merge" : "close"
 
     recipients = NotificationRecipientService.build_recipients(
@@ -515,7 +515,7 @@ class NotificationService
     )
 
     recipients.each do |recipient|
-      mailer.send(method, recipient.user.id, target.id, current_user.id, recipient.reason).deliver_later
+      mailer.send(method, recipient.user.id, target.id, current_user.id, reason: recipient.reason, closed_via: closed_via).deliver_later
     end
   end
 

app/views/notify/closed_issue_email.html.haml

 %p
-  Issue was closed by #{sanitize_name(@updated_by.name)}
+  Issue was closed by #{sanitize_name(@updated_by.name)} #{closure_reason_text(@closed_via, format: formats.first)}.

app/views/notify/closed_issue_email.text.haml

-Issue was closed by #{sanitize_name(@updated_by.name)}
+Issue was closed by #{sanitize_name(@updated_by.name)} #{closure_reason_text(@closed_via, format: formats.first)}.
 
 Issue ##{@issue.iid}: #{project_issue_url(@issue.project, @issue)}

app/workers/process_commit_worker.rb

@@ -48,7 +48,7 @@ class ProcessCommitWorker
     # Issues::CloseService#execute.
     IssueCollection.new(issues).updatable_by_user(user).each do |issue|
       Issues::CloseService.new(project, author)
-        .close_issue(issue, commit: commit)
+        .close_issue(issue, closed_via: commit)
     end
   end
 

changelogs/unreleased/19569-include-information-if-issue-was-closed-via-mr.yml

+---
+title: Include information if issue was clossed via merge request or commit
+merge_request: 15610
+author: Michał Zając
+type: changed

spec/helpers/emails_helper_spec.rb

 require 'spec_helper'
 
 describe EmailsHelper do
+  describe 'closure_reason_text' do
+    context 'when given a MergeRequest' do
+      let(:merge_request) { create(:merge_request) }
+      let(:merge_request_presenter) { merge_request.present }
+
+      context "and format is text" do
+        it "returns plain text" do
+          expect(closure_reason_text(merge_request, format: :text)).to eq(" via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+        end
+      end
+
+      context "and format is HTML" do
+        it "returns HTML" do
+          expect(closure_reason_text(merge_request, format: :html)).to eq(" via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}")
+        end
+      end
+
+      context "and format is unknown" do
+        it "returns plain text" do
+          expect(closure_reason_text(merge_request, format: :text)).to eq(" via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})")
+        end
+      end
+    end
+
+    context 'when given a String' do
+      let(:closed_via) { "5a0eb6fd7e0f133044378c662fcbbc0d0c16dbfa" }
+
+      it "returns plain text" do
+        expect(closure_reason_text(closed_via)).to eq(" via #{closed_via}")
+      end
+    end
+
+    context 'when not given anything' do
+      it "returns empty string" do
+        expect(closure_reason_text(nil)).to eq("")
+      end
+    end
+  end
+
   describe 'sanitize_name' do
     context 'when name contains a valid URL string' do
       it 'returns name with `.` replaced with `_` to prevent mail clients from auto-linking URLs' do

spec/services/issues/close_service_spec.rb

@@ -3,11 +3,13 @@
 require 'spec_helper'
 
 describe Issues::CloseService do
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user, email: "user@example.com") }
+  let(:user2) { create(:user, email: "user2@example.com") }
   let(:guest) { create(:user) }
-  let(:issue) { create(:issue, assignees: [user2], author: create(:user)) }
-  let(:project) { issue.project }
+  let(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: create(:user)) }
+  let(:closing_merge_request) { create(:merge_request, source_project: project) }
+  let(:closing_commit) { create(:commit, project: project) }
   let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
 
   before do
@@ -39,7 +41,7 @@ describe Issues::CloseService do
         .and_return(true)
 
       expect(service).to receive(:close_issue)
-        .with(issue, commit: nil, notifications: true, system_note: true)
+        .with(issue, closed_via: nil, notifications: true, system_note: true)
 
       service.execute(issue)
     end
@@ -57,6 +59,38 @@ describe Issues::CloseService do
   end
 
   describe '#close_issue' do
+    context "closed by a merge request" do
+      before do
+        perform_enqueued_jobs do
+          described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request)
+        end
+      end
+
+      it 'mentions closure via a merge request' do
+        email = ActionMailer::Base.deliveries.last
+
+        expect(email.to.first).to eq(user2.email)
+        expect(email.subject).to include(issue.title)
+        expect(email.body.parts.map(&:body)).to all(include(closing_merge_request.to_reference))
+      end
+    end
+
+    context "closed by a commit" do
+      before do
+        perform_enqueued_jobs do
+          described_class.new(project, user).close_issue(issue, closed_via: closing_commit)
+        end
+      end
+
+      it 'mentions closure via a commit' do
+        email = ActionMailer::Base.deliveries.last
+
+        expect(email.to.first).to eq(user2.email)
+        expect(email.subject).to include(issue.title)
+        expect(email.body.parts.map(&:body)).to all(include(closing_commit.id))
+      end
+    end
+
     context "valid params" do
       before do
         perform_enqueued_jobs do