Commit e177daa9 authored by Matthias Käppler's avatar Matthias Käppler

Merge branch '16507-use-to-when-sending-pipeline-emails' into 'master'

Use the To header when sending pipeline emails

See merge request gitlab-org/gitlab!73363
parents aaeffccc 3b5330e9
......@@ -2,21 +2,23 @@
module Emails
module Pipelines
def pipeline_success_email(pipeline, recipients)
pipeline_mail(pipeline, recipients, 'Successful')
def pipeline_success_email(pipeline, recipient)
pipeline_mail(pipeline, recipient, 'Successful')
end
def pipeline_failed_email(pipeline, recipients)
pipeline_mail(pipeline, recipients, 'Failed')
def pipeline_failed_email(pipeline, recipient)
pipeline_mail(pipeline, recipient, 'Failed')
end
def pipeline_fixed_email(pipeline, recipients)
pipeline_mail(pipeline, recipients, 'Fixed')
def pipeline_fixed_email(pipeline, recipient)
pipeline_mail(pipeline, recipient, 'Fixed')
end
private
def pipeline_mail(pipeline, recipients, status)
def pipeline_mail(pipeline, recipient, status)
raise ArgumentError if recipient.is_a?(Array)
@project = pipeline.project
@pipeline = pipeline
......@@ -28,10 +30,7 @@ module Emails
add_headers
# We use bcc here because we don't want to generate these 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,
mail(to: recipient,
subject: subject(pipeline_subject(status))) do |format|
format.html { render layout: 'mailer' }
format.text { render layout: 'mailer' }
......
......@@ -97,7 +97,7 @@ module Integrations
return if recipients.blank?
if self.class.valid_recipients(recipients).size > RECIPIENTS_LIMIT
errors.add(:recipients, s_("EmailsOnPushService|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
errors.add(:recipients, s_("Integrations|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
end
end
end
......
......@@ -4,9 +4,12 @@ module Integrations
class PipelinesEmail < Integration
include NotificationBranchSelection
RECIPIENTS_LIMIT = 30
prop_accessor :recipients, :branches_to_be_notified
boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch
validates :recipients, presence: true, if: :validate_recipients?
validate :number_of_recipients_within_limit, if: :validate_recipients?
def initialize_properties
if properties.nil?
......@@ -49,7 +52,7 @@ module Integrations
return unless supported_events.include?(data[:object_kind])
return unless force || should_pipeline_be_notified?(data)
all_recipients = retrieve_recipients(data)
all_recipients = retrieve_recipients
return unless all_recipients.any?
......@@ -99,8 +102,18 @@ module Integrations
end
end
def retrieve_recipients(data)
def retrieve_recipients
recipients.to_s.split(/[,\r\n ]+/).reject(&:empty?)
end
private
def number_of_recipients_within_limit
return if recipients.blank?
if retrieve_recipients.size > RECIPIENTS_LIMIT
errors.add(:recipients, s_("Integrations|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
end
end
end
end
......@@ -598,8 +598,8 @@ class NotificationService
user.notification_email_for(pipeline.project.group)
end
if recipients.any?
mailer.public_send(email_template, pipeline, recipients).deliver_later
recipients.each do |recipient|
mailer.public_send(email_template, pipeline, recipient).deliver_later
end
end
......
......@@ -12658,9 +12658,6 @@ msgstr ""
msgid "EmailsOnPushService|Send notifications from the committer's email address if the domain matches the domain used by your GitLab instance (such as %{domains})."
msgstr ""
msgid "EmailsOnPushService|can't exceed %{recipients_limit}"
msgstr ""
msgid "EmailsOnPushService|tanuki@example.com gitlab@example.com"
msgstr ""
......@@ -18623,6 +18620,9 @@ msgstr ""
msgid "Integrations|ZenTao issues display here when you create issues in your project in ZenTao."
msgstr ""
msgid "Integrations|can't exceed %{recipients_limit}"
msgstr ""
msgid "Interactive mode"
msgstr ""
......
......@@ -71,10 +71,19 @@ RSpec.describe Emails::Pipelines do
end
end
shared_examples_for 'only accepts a single recipient' do
let(:recipient) { ['test@gitlab.com', 'test2@gitlab.com'] }
it 'raises an ArgumentError' do
expect { subject.deliver_now }.to raise_error(ArgumentError)
end
end
describe '#pipeline_success_email' do
subject { Notify.pipeline_success_email(pipeline, pipeline.user.try(:email)) }
subject { Notify.pipeline_success_email(pipeline, recipient) }
let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
let(:recipient) { pipeline.user.try(:email) }
let(:ref) { 'master' }
let(:sha) { project.commit(ref).sha }
......@@ -93,12 +102,15 @@ RSpec.describe Emails::Pipelines do
stub_config_setting(email_subject_suffix: email_subject_suffix)
end
end
it_behaves_like 'only accepts a single recipient'
end
describe '#pipeline_failed_email' do
subject { Notify.pipeline_failed_email(pipeline, pipeline.user.try(:email)) }
subject { Notify.pipeline_failed_email(pipeline, recipient) }
let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
let(:recipient) { pipeline.user.try(:email) }
let(:ref) { 'master' }
let(:sha) { project.commit(ref).sha }
......@@ -106,12 +118,15 @@ RSpec.describe Emails::Pipelines do
let(:status) { 'Failed' }
let(:status_text) { "Pipeline ##{pipeline.id} has failed!" }
end
it_behaves_like 'only accepts a single recipient'
end
describe '#pipeline_fixed_email' do
subject { Notify.pipeline_fixed_email(pipeline, pipeline.user.try(:email)) }
let(:pipeline) { create(:ci_pipeline, project: project, ref: ref, sha: sha) }
let(:recipient) { pipeline.user.try(:email) }
let(:ref) { 'master' }
let(:sha) { project.commit(ref).sha }
......@@ -119,5 +134,7 @@ RSpec.describe Emails::Pipelines do
let(:status) { 'Fixed' }
let(:status_text) { "Pipeline has been fixed and ##{pipeline.id} has passed!" }
end
it_behaves_like 'only accepts a single recipient'
end
end
......@@ -3361,7 +3361,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
shared_examples 'sending a notification' do
it 'sends an email', :sidekiq_might_not_need_inline do
should_only_email(pipeline.user, kind: :bcc)
should_only_email(pipeline.user)
end
end
......
......@@ -35,6 +35,42 @@ RSpec.describe Integrations::PipelinesEmail, :mailer do
it { is_expected.not_to validate_presence_of(:recipients) }
end
describe 'validates number of recipients' do
before do
stub_const("#{described_class}::RECIPIENTS_LIMIT", 2)
end
subject(:integration) { described_class.new(project: project, recipients: recipients, active: true) }
context 'valid number of recipients' do
let(:recipients) { 'foo@bar.com, , ' }
it 'does not count empty emails' do
is_expected.to be_valid
end
end
context 'invalid number of recipients' do
let(:recipients) { 'foo@bar.com bar@foo.com bob@gitlab.com' }
it { is_expected.not_to be_valid }
it 'adds an error message' do
integration.valid?
expect(integration.errors).to contain_exactly('Recipients can\'t exceed 2')
end
context 'when integration is not active' do
before do
integration.active = false
end
it { is_expected.to be_valid }
end
end
end
end
shared_examples 'sending email' do |branches_to_be_notified: nil|
......@@ -50,7 +86,7 @@ RSpec.describe Integrations::PipelinesEmail, :mailer do
it 'sends email' do
emails = receivers.map { |r| double(notification_email_or_default: r) }
should_only_email(*emails, kind: :bcc)
should_only_email(*emails)
end
end
......
......@@ -3040,7 +3040,7 @@ RSpec.describe NotificationService, :mailer do
it 'emails only the creator' do
notification.pipeline_finished(pipeline)
should_only_email(u_custom_notification_enabled, kind: :bcc)
should_only_email(u_custom_notification_enabled)
end
it_behaves_like 'project emails are disabled' do
......@@ -3063,7 +3063,7 @@ RSpec.describe NotificationService, :mailer do
it 'sends to group notification email' do
notification.pipeline_finished(pipeline)
expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
expect(email_recipients.first).to eq(group_notification_email)
end
end
end
......@@ -3076,7 +3076,7 @@ RSpec.describe NotificationService, :mailer do
it 'emails only the creator' do
notification.pipeline_finished(pipeline)
should_only_email(u_member, kind: :bcc)
should_only_email(u_member)
end
it_behaves_like 'project emails are disabled' do
......@@ -3098,7 +3098,7 @@ RSpec.describe NotificationService, :mailer do
it 'sends to group notification email' do
notification.pipeline_finished(pipeline)
expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
expect(email_recipients.first).to eq(group_notification_email)
end
end
end
......@@ -3110,7 +3110,7 @@ RSpec.describe NotificationService, :mailer do
end
it 'emails only the creator' do
should_only_email(u_watcher, kind: :bcc)
should_only_email(u_watcher)
end
end
......@@ -3121,7 +3121,7 @@ RSpec.describe NotificationService, :mailer do
end
it 'emails only the creator' do
should_only_email(u_custom_notification_unset, kind: :bcc)
should_only_email(u_custom_notification_unset)
end
end
......@@ -3143,7 +3143,7 @@ RSpec.describe NotificationService, :mailer do
end
it 'emails only the creator' do
should_only_email(u_custom_notification_enabled, kind: :bcc)
should_only_email(u_custom_notification_enabled)
end
end
......@@ -3170,7 +3170,7 @@ RSpec.describe NotificationService, :mailer do
it 'emails only the creator' do
notification.pipeline_finished(pipeline, ref_status: ref_status)
should_only_email(u_member, kind: :bcc)
should_only_email(u_member)
end
it_behaves_like 'project emails are disabled' do
......@@ -3192,7 +3192,7 @@ RSpec.describe NotificationService, :mailer do
it 'sends to group notification email' do
notification.pipeline_finished(pipeline, ref_status: ref_status)
expect(email_recipients(kind: :bcc).first).to eq(group_notification_email)
expect(email_recipients.first).to eq(group_notification_email)
end
end
end
......@@ -3204,7 +3204,7 @@ RSpec.describe NotificationService, :mailer do
end
it 'emails only the creator' do
should_only_email(u_watcher, kind: :bcc)
should_only_email(u_watcher)
end
end
......@@ -3215,7 +3215,7 @@ RSpec.describe NotificationService, :mailer do
end
it 'emails only the creator' do
should_only_email(u_custom_notification_unset, kind: :bcc)
should_only_email(u_custom_notification_unset)
end
end
......@@ -3236,7 +3236,7 @@ RSpec.describe NotificationService, :mailer do
notification.pipeline_finished(pipeline, ref_status: ref_status)
should_only_email(u_custom_notification_enabled, kind: :bcc)
should_only_email(u_custom_notification_enabled)
end
end
end
......
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