Commit 411f545c authored by Michał Zając's avatar Michał Zając Committed by Rémy Coutable

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

parent 2d0f349b
...@@ -91,6 +91,28 @@ module EmailsHelper ...@@ -91,6 +91,28 @@ module EmailsHelper
].join(';') ].join(';')
end 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}" # "You are receiving this email because #{reason}"
def notification_reason_text(reason) def notification_reason_text(reason)
string = case reason string = case reason
......
...@@ -30,8 +30,8 @@ module Emails ...@@ -30,8 +30,8 @@ module Emails
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil) def closed_issue_email(recipient_id, issue_id, updated_by_user_id, reason: nil, closed_via: nil)
setup_issue_mail(issue_id, recipient_id) setup_issue_mail(issue_id, recipient_id, closed_via: closed_via)
@updated_by = User.find(updated_by_user_id) @updated_by = User.find(updated_by_user_id)
mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason))
...@@ -91,10 +91,11 @@ module Emails ...@@ -91,10 +91,11 @@ module Emails
private 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) @issue = Issue.find(issue_id)
@project = @issue.project @project = @issue.project
@target_url = project_issue_url(@project, @issue) @target_url = project_issue_url(@project, @issue)
@closed_via = closed_via
@sent_notification = SentNotification.record(@issue, recipient_id, reply_key) @sent_notification = SentNotification.record(@issue, recipient_id, reply_key)
end end
......
...@@ -58,14 +58,14 @@ module Emails ...@@ -58,14 +58,14 @@ module Emails
})) }))
end 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) setup_merge_request_mail(merge_request_id, recipient_id)
@updated_by = User.find(updated_by_user_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)) mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
end 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) 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)) mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
......
...@@ -7,7 +7,7 @@ module Issues ...@@ -7,7 +7,7 @@ module Issues
return issue unless can?(current_user, :update_issue, issue) return issue unless can?(current_user, :update_issue, issue)
close_issue(issue, close_issue(issue,
commit: commit, closed_via: commit,
notifications: notifications, notifications: notifications,
system_note: system_note) system_note: system_note)
end end
...@@ -17,9 +17,9 @@ module Issues ...@@ -17,9 +17,9 @@ module Issues
# #
# The code calling this method is responsible for ensuring that a user is # The code calling this method is responsible for ensuring that a user is
# allowed to close the given issue. # 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) 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) todo_service.close_issue(issue, current_user)
return issue return issue
end end
...@@ -27,8 +27,11 @@ module Issues ...@@ -27,8 +27,11 @@ module Issues
if project.issues_enabled? && issue.close if project.issues_enabled? && issue.close
issue.update(closed_by: current_user) issue.update(closed_by: current_user)
event_service.close_issue(issue, current_user) event_service.close_issue(issue, current_user)
create_note(issue, commit) if system_note create_note(issue, closed_via) if system_note
notification_service.async.close_issue(issue, current_user) if notifications
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) todo_service.close_issue(issue, current_user)
execute_hooks(issue, 'close') execute_hooks(issue, 'close')
invalidate_cache_counts(issue, users: issue.assignees) invalidate_cache_counts(issue, users: issue.assignees)
......
...@@ -89,8 +89,8 @@ class NotificationService ...@@ -89,8 +89,8 @@ class NotificationService
# * project team members with notification level higher then Participating # * project team members with notification level higher then Participating
# * users with custom level checked with "close issue" # * users with custom level checked with "close issue"
# #
def close_issue(issue, current_user) def close_issue(issue, current_user, closed_via: nil)
close_resource_email(issue, current_user, :closed_issue_email) close_resource_email(issue, current_user, :closed_issue_email, closed_via: closed_via)
end end
# When we reassign an issue we should send an email to: # When we reassign an issue we should send an email to:
...@@ -504,7 +504,7 @@ class NotificationService ...@@ -504,7 +504,7 @@ class NotificationService
end end
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" action = method == :merged_merge_request_email ? "merge" : "close"
recipients = NotificationRecipientService.build_recipients( recipients = NotificationRecipientService.build_recipients(
...@@ -515,7 +515,7 @@ class NotificationService ...@@ -515,7 +515,7 @@ class NotificationService
) )
recipients.each do |recipient| 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
end end
......
%p %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)}.
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)} Issue ##{@issue.iid}: #{project_issue_url(@issue.project, @issue)}
...@@ -48,7 +48,7 @@ class ProcessCommitWorker ...@@ -48,7 +48,7 @@ class ProcessCommitWorker
# Issues::CloseService#execute. # Issues::CloseService#execute.
IssueCollection.new(issues).updatable_by_user(user).each do |issue| IssueCollection.new(issues).updatable_by_user(user).each do |issue|
Issues::CloseService.new(project, author) Issues::CloseService.new(project, author)
.close_issue(issue, commit: commit) .close_issue(issue, closed_via: commit)
end end
end end
......
---
title: Include information if issue was clossed via merge request or commit
merge_request: 15610
author: Michał Zając
type: changed
require 'spec_helper' require 'spec_helper'
describe EmailsHelper do 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 describe 'sanitize_name' do
context 'when name contains a valid URL string' 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 it 'returns name with `.` replaced with `_` to prevent mail clients from auto-linking URLs' do
......
...@@ -3,11 +3,13 @@ ...@@ -3,11 +3,13 @@
require 'spec_helper' require 'spec_helper'
describe Issues::CloseService do describe Issues::CloseService do
let(:user) { create(:user) } let(:project) { create(:project, :repository) }
let(:user2) { create(:user) } let(:user) { create(:user, email: "user@example.com") }
let(:user2) { create(:user, email: "user2@example.com") }
let(:guest) { create(:user) } let(:guest) { create(:user) }
let(:issue) { create(:issue, assignees: [user2], author: create(:user)) } let(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: create(:user)) }
let(:project) { issue.project } 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) } let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
before do before do
...@@ -39,7 +41,7 @@ describe Issues::CloseService do ...@@ -39,7 +41,7 @@ describe Issues::CloseService do
.and_return(true) .and_return(true)
expect(service).to receive(:close_issue) 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) service.execute(issue)
end end
...@@ -57,6 +59,38 @@ describe Issues::CloseService do ...@@ -57,6 +59,38 @@ describe Issues::CloseService do
end end
describe '#close_issue' do 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 context "valid params" do
before do before do
perform_enqueued_jobs do perform_enqueued_jobs do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment