summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2019-05-16 11:59:02 +0000
committerRémy Coutable <remy@rymai.me>2019-05-16 11:59:02 +0000
commit64fc7daf12423fb91c6d41f1883605f9b5df786e (patch)
treeb9fe7fa6acc5a18d0aa3a87a39481521028557d0
parent2d0f349baf83bee47c062896ac71a678e3b5f77b (diff)
parent411f545ce6a88db8370e639989d9c8f6d3621cbe (diff)
downloadgitlab-ce-64fc7daf12423fb91c6d41f1883605f9b5df786e.tar.gz
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
-rw-r--r--app/helpers/emails_helper.rb22
-rw-r--r--app/mailers/emails/issues.rb7
-rw-r--r--app/mailers/emails/merge_requests.rb4
-rw-r--r--app/services/issues/close_service.rb13
-rw-r--r--app/services/notification_service.rb8
-rw-r--r--app/views/notify/closed_issue_email.html.haml2
-rw-r--r--app/views/notify/closed_issue_email.text.haml2
-rw-r--r--app/workers/process_commit_worker.rb2
-rw-r--r--changelogs/unreleased/19569-include-information-if-issue-was-closed-via-mr.yml5
-rw-r--r--spec/helpers/emails_helper_spec.rb39
-rw-r--r--spec/services/issues/close_service_spec.rb44
11 files changed, 126 insertions, 22 deletions
diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb
index 96471d15aac..dc0e5511fcf 100644
--- a/app/helpers/emails_helper.rb
+++ b/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
diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb
index d2e334fb856..2b046d17122 100644
--- a/app/mailers/emails/issues.rb
+++ b/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
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 63148831a24..fc6ed695675 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/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))
diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
index e5cc12e6082..2a19e57a94f 100644
--- a/app/services/issues/close_service.rb
+++ b/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)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 8d3b569498f..f797c0f11c6 100644
--- a/app/services/notification_service.rb
+++ b/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
diff --git a/app/views/notify/closed_issue_email.html.haml b/app/views/notify/closed_issue_email.html.haml
index eb148d72da1..f21cf1ad34b 100644
--- a/app/views/notify/closed_issue_email.html.haml
+++ b/app/views/notify/closed_issue_email.html.haml
@@ -1,2 +1,2 @@
%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)}.
diff --git a/app/views/notify/closed_issue_email.text.haml b/app/views/notify/closed_issue_email.text.haml
index b1f0a3f37ec..5567adc9165 100644
--- a/app/views/notify/closed_issue_email.text.haml
+++ b/app/views/notify/closed_issue_email.text.haml
@@ -1,3 +1,3 @@
-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)}
diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb
index 29a7f8e691a..3efb5343a96 100644
--- a/app/workers/process_commit_worker.rb
+++ b/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
diff --git a/changelogs/unreleased/19569-include-information-if-issue-was-closed-via-mr.yml b/changelogs/unreleased/19569-include-information-if-issue-was-closed-via-mr.yml
new file mode 100644
index 00000000000..bb2fc9af2a1
--- /dev/null
+++ b/changelogs/unreleased/19569-include-information-if-issue-was-closed-via-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Include information if issue was clossed via merge request or commit
+merge_request: 15610
+author: Michał Zając
+type: changed
diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb
index 03b4c19ec22..0434af25866 100644
--- a/spec/helpers/emails_helper_spec.rb
+++ b/spec/helpers/emails_helper_spec.rb
@@ -1,6 +1,45 @@
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
diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
index fce9eed8b08..6874a8a0929 100644
--- a/spec/services/issues/close_service_spec.rb
+++ b/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