summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG1
-rw-r--r--app/mailers/emails/projects.rb5
-rw-r--r--app/workers/emails_on_push_worker.rb25
-rw-r--r--lib/gitlab/email/message/repository_push.rb4
-rw-r--r--spec/lib/gitlab/email/message/repository_push_spec.rb2
-rw-r--r--spec/mailers/notify_spec.rb36
-rw-r--r--spec/workers/emails_on_push_worker_spec.rb65
7 files changed, 83 insertions, 55 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 1c21ad36b69..96643fc88a3 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -21,6 +21,7 @@ v 8.8.0 (unreleased)
- Fix error when visiting commit builds page before build was updated
- Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project
- Update SVG sanitizer to conform to SVG 1.1
+ - Speed up push emails with multiple recipients by only generating the email once
- Updated search UI
- Display informative message when new milestone is created
- Sanitize milestones and labels titles
diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb
index 377c2999d6c..5489283432b 100644
--- a/app/mailers/emails/projects.rb
+++ b/app/mailers/emails/projects.rb
@@ -59,9 +59,9 @@ module Emails
subject: subject("Project was moved"))
end
- def repository_push_email(project_id, recipient, opts = {})
+ def repository_push_email(project_id, opts = {})
@message =
- Gitlab::Email::Message::RepositoryPush.new(self, project_id, recipient, opts)
+ Gitlab::Email::Message::RepositoryPush.new(self, project_id, opts)
# used in notify layout
@target_url = @message.target_url
@@ -72,7 +72,6 @@ module Emails
mail(from: sender(@message.author_id, @message.send_from_committer_email?),
reply_to: @message.reply_to,
- to: @message.recipient,
subject: @message.subject)
end
end
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb
index c4d8595d45d..6ebcba5f39b 100644
--- a/app/workers/emails_on_push_worker.rb
+++ b/app/workers/emails_on_push_worker.rb
@@ -1,6 +1,8 @@
class EmailsOnPushWorker
include Sidekiq::Worker
+ attr_reader :email, :skip_premailer
+
def perform(project_id, recipients, push_data, options = {})
options.symbolize_keys!
options.reverse_merge!(
@@ -41,11 +43,11 @@ class EmailsOnPushWorker
end
end
- recipients.split(" ").each do |recipient|
+ recipients.split.each do |recipient|
begin
- Notify.repository_push_email(
- project_id,
+ send_email(
recipient,
+ project_id,
author_id: author_id,
ref: ref,
action: action,
@@ -53,14 +55,29 @@ class EmailsOnPushWorker
reverse_compare: reverse_compare,
send_from_committer_email: send_from_committer_email,
disable_diffs: disable_diffs
- ).deliver_now
+ )
+
# These are input errors and won't be corrected even if Sidekiq retries
rescue Net::SMTPFatalError, Net::SMTPSyntaxError => e
logger.info("Failed to send e-mail for project '#{project.name_with_namespace}' to #{recipient}: #{e}")
end
end
ensure
+ @email = nil
compare = nil
GC.start
end
+
+ private
+
+ def send_email(recipient, project_id, options)
+ # Generating the body of this email can be expensive, so only do it once
+ @skip_premailer ||= email.present?
+ @email ||= Notify.repository_push_email(project_id, options)
+
+ email.to = recipient
+ email.add_message_id
+ email.header[:skip_premailer] = true if skip_premailer
+ email.deliver_now
+ end
end
diff --git a/lib/gitlab/email/message/repository_push.rb b/lib/gitlab/email/message/repository_push.rb
index 8f9be6cd9a3..2c91a0487c3 100644
--- a/lib/gitlab/email/message/repository_push.rb
+++ b/lib/gitlab/email/message/repository_push.rb
@@ -2,7 +2,6 @@ module Gitlab
module Email
module Message
class RepositoryPush
- attr_accessor :recipient
attr_reader :author_id, :ref, :action
include Gitlab::Routing.url_helpers
@@ -11,13 +10,12 @@ module Gitlab
delegate :name, to: :author, prefix: :author
delegate :username, to: :author, prefix: :author
- def initialize(notify, project_id, recipient, opts = {})
+ def initialize(notify, project_id, opts = {})
raise ArgumentError, 'Missing options: author_id, ref, action' unless
opts[:author_id] && opts[:ref] && opts[:action]
@notify = notify
@project_id = project_id
- @recipient = recipient
@opts = opts.dup
@author_id = @opts.delete(:author_id)
diff --git a/spec/lib/gitlab/email/message/repository_push_spec.rb b/spec/lib/gitlab/email/message/repository_push_spec.rb
index b2d7a799810..7d6cce6daec 100644
--- a/spec/lib/gitlab/email/message/repository_push_spec.rb
+++ b/spec/lib/gitlab/email/message/repository_push_spec.rb
@@ -8,7 +8,7 @@ describe Gitlab::Email::Message::RepositoryPush do
let!(:author) { create(:author, name: 'Author') }
let(:message) do
- described_class.new(Notify, project.id, 'recipient@example.com', opts)
+ described_class.new(Notify, project.id, opts)
end
context 'new commits have been pushed to repository' do
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 495c5cbac00..5f7e4a526e6 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -593,7 +593,7 @@ describe Notify do
let(:user) { create(:user) }
let(:tree_path) { namespace_project_tree_path(project.namespace, project, "master") }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :create) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :create) }
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -606,10 +606,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /Pushed new branch master/
end
@@ -624,7 +620,7 @@ describe Notify do
let(:user) { create(:user) }
let(:tree_path) { namespace_project_tree_path(project.namespace, project, "v1.0") }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :create) }
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -637,10 +633,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /Pushed new tag v1\.0/
end
@@ -654,7 +646,7 @@ describe Notify do
let(:example_site_path) { root_path }
let(:user) { create(:user) }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :delete) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :delete) }
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -667,10 +659,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /Deleted branch master/
end
@@ -680,7 +668,7 @@ describe Notify do
let(:example_site_path) { root_path }
let(:user) { create(:user) }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) }
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -693,10 +681,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /Deleted tag v1\.0/
end
@@ -710,7 +694,7 @@ describe Notify do
let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
let(:send_from_committer_email) { false }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -723,10 +707,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /\[#{project.path_with_namespace}\]\[master\] #{commits.length} commits:/
end
@@ -818,7 +798,7 @@ describe Notify do
let(:commits) { Commit.decorate(compare.commits, nil) }
let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
- subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
+ subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
it_behaves_like 'it should show Gmail Actions View Commit link'
it_behaves_like "a user cannot unsubscribe through footer link"
@@ -831,10 +811,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender)
end
- it 'is sent to recipient' do
- is_expected.to deliver_to 'devs@company.name'
- end
-
it 'has the correct subject' do
is_expected.to have_subject /#{commits.first.title}/
end
diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb
index 3600c771075..439da765c2c 100644
--- a/spec/workers/emails_on_push_worker_spec.rb
+++ b/spec/workers/emails_on_push_worker_spec.rb
@@ -6,29 +6,66 @@ describe EmailsOnPushWorker do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) }
+ let(:recipients) { user.email }
+ let(:perform) { subject.perform(project.id, recipients, data.stringify_keys) }
subject { EmailsOnPushWorker.new }
- before do
- allow(Project).to receive(:find).and_return(project)
- end
-
describe "#perform" do
- it "sends mail" do
- subject.perform(project.id, user.email, data.stringify_keys)
+ context "when there are no errors in sending" do
+ let(:email) { ActionMailer::Base.deliveries.last }
+
+ before { perform }
- email = ActionMailer::Base.deliveries.last
- expect(email.subject).to include('Change some files')
- expect(email.to).to eq([user.email])
+ it "sends a mail with the correct subject" do
+ expect(email.subject).to include('Change some files')
+ end
+
+ it "sends the mail to the correct recipient" do
+ expect(email.to).to eq([user.email])
+ end
end
- it "gracefully handles an input SMTP error" do
- ActionMailer::Base.deliveries.clear
- allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError)
+ context "when there is an SMTP error" do
+ before do
+ ActionMailer::Base.deliveries.clear
+ allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError)
+ perform
+ end
+
+ it "gracefully handles an input SMTP error" do
+ expect(ActionMailer::Base.deliveries.count).to eq(0)
+ end
+ end
+
+ context "when there are multiple recipients" do
+ let(:recipients) do
+ 1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n")
+ end
+
+ before do
+ # This is a hack because we modify the mail object before sending, for efficency,
+ # but the TestMailer adapter just appends the objects to an array. To clone a mail
+ # object, create a new one!
+ # https://github.com/mikel/mail/issues/314#issuecomment-12750108
+ allow_any_instance_of(Mail::TestMailer).to receive(:deliver!).and_wrap_original do |original, mail|
+ original.call(Mail.new(mail.encoded))
+ end
+
+ ActionMailer::Base.deliveries.clear
+ end
- subject.perform(project.id, user.email, data.stringify_keys)
+ it "sends the mail to each of the recipients" do
+ perform
+ expect(ActionMailer::Base.deliveries.count).to eq(5)
+ expect(ActionMailer::Base.deliveries.map(&:to).flatten).to contain_exactly(*recipients.split)
+ end
- expect(ActionMailer::Base.deliveries.count).to eq(0)
+ it "only generates the mail once" do
+ expect(Notify).to receive(:repository_push_email).once.and_call_original
+ expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original
+ perform
+ end
end
end
end
View Lorry Controller Status