From 045c6715330d25adff95304a3de908037c63a163 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 18 Oct 2016 20:02:35 +0800
Subject: [PATCH] Use bcc for pipeline emails because:
We use bcc here because we don't want to generate this emails for a
thousand times. This could be potentially expensive in a loop, and
recipients would contain all project watchers so it could be a lot.
---
app/mailers/emails/pipelines.rb | 17 +++++++++++------
app/services/notification_service.rb | 4 +---
spec/models/ci/pipeline_spec.rb | 2 +-
spec/support/email_helpers.rb | 8 ++++----
.../pipeline_notification_worker_spec.rb | 14 ++++++--------
5 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/app/mailers/emails/pipelines.rb b/app/mailers/emails/pipelines.rb
index 601c8b5cd62..9460a6cd2be 100644
--- a/app/mailers/emails/pipelines.rb
+++ b/app/mailers/emails/pipelines.rb
@@ -1,22 +1,27 @@
module Emails
module Pipelines
- def pipeline_success_email(pipeline, to)
- pipeline_mail(pipeline, to, 'succeeded')
+ def pipeline_success_email(pipeline, recipients)
+ pipeline_mail(pipeline, recipients, 'succeeded')
end
- def pipeline_failed_email(pipeline, to)
- pipeline_mail(pipeline, to, 'failed')
+ def pipeline_failed_email(pipeline, recipients)
+ pipeline_mail(pipeline, recipients, 'failed')
end
private
- def pipeline_mail(pipeline, to, status)
+ def pipeline_mail(pipeline, recipients, status)
@project = pipeline.project
@pipeline = pipeline
@merge_request = pipeline.merge_requests.first
add_headers
- mail(to: to, subject: pipeline_subject(status), skip_premailer: true) do |format|
+ # We use bcc here because we don't want to generate this emails for a
+ # thousand times. This could be potentially expensive in a loop, and
+ # recipients would contain all project watchers so it could be a lot.
+ mail(bcc: recipients,
+ subject: pipeline_subject(status),
+ skip_premailer: true) do |format|
format.html { render layout: false }
format.text
end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index d4976aa8362..d5d69248af7 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -323,9 +323,7 @@ class NotificationService
nil, # The acting user, who won't be added to recipients
action: pipeline.status).map(&:email)
- recipients.each do |to|
- mailer.public_send(email_template, pipeline, to).deliver_later
- end
+ mailer.public_send(email_template, pipeline, recipients).deliver_later
end
protected
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index caf191e11ae..4032029b80b 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -536,7 +536,7 @@ describe Ci::Pipeline, models: true do
shared_examples 'sending a notification' do
it 'sends an email' do
- should_only_email(pipeline.user)
+ should_only_email(pipeline.user, kind: :bcc)
end
end
diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb
index 36f5a2d5975..a946f10f71a 100644
--- a/spec/support/email_helpers.rb
+++ b/spec/support/email_helpers.rb
@@ -7,8 +7,8 @@ module EmailHelpers
ActionMailer::Base.deliveries.clear
end
- def should_only_email(*users)
- recipients = email_recipients
+ def should_only_email(*users, kind: :to)
+ recipients = email_recipients(kind: kind)
users.each { |user| should_email(user, recipients) }
@@ -27,7 +27,7 @@ module EmailHelpers
expect(ActionMailer::Base.deliveries).to be_empty
end
- def email_recipients
- ActionMailer::Base.deliveries.flat_map(&:to)
+ def email_recipients(kind: :to)
+ ActionMailer::Base.deliveries.flat_map(&kind)
end
end
diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb
index c641d7a7801..c334b2057a6 100644
--- a/spec/workers/pipeline_notification_worker_spec.rb
+++ b/spec/workers/pipeline_notification_worker_spec.rb
@@ -26,15 +26,13 @@ describe PipelineNotificationWorker do
subject.perform(pipeline.id)
end
- expected_receivers = [pusher, watcher].uniq.sort_by(&:email)
- actual = ActionMailer::Base.deliveries.sort_by(&:to)
+ emails = ActionMailer::Base.deliveries
+ actual = emails.flat_map(&:bcc).sort
+ expected_receivers = [pusher, watcher].map(&:email).uniq.sort
- expect(expected_receivers.size).to eq(actual.size)
-
- actual.zip(expected_receivers).each do |(email, receiver)|
- expect(email.subject).to include(email_subject)
- expect(email.to).to eq([receiver.email])
- end
+ expect(actual).to eq(expected_receivers)
+ expect(emails.size).to eq(1)
+ expect(emails.last.subject).to include(email_subject)
end
end
--
GitLab