diff --git a/app/mailers/emails/pipelines.rb b/app/mailers/emails/pipelines.rb index 601c8b5cd621843f44867888dd04b6d262910ba2..9460a6cd2be9bc2edf15e180d2316313380303b8 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 d4976aa8362ba8cc1e2801320dded853ed350941..d5d69248af7fc7a058ba9cb1f0921935285b583e 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 caf191e11ae3dcfe0f60a693da83a9f000e23076..4032029b80b1511193115ff6da6b08a2cd0d70b5 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 36f5a2d5975bfc81851cf4faa35309686e1f8795..a946f10f71aee8668d75ac1962a4a54eca09b9d5 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 c641d7a7801655a79d14312cf79b7c02a4b0f821..c334b2057a6ee90795fa73fed9390ea7ba6192a1 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