Merge branch '17270-only-generate-email-on-push-once-for-all-recipients' into 'master'

Only generate repository push email once

The repository push email can be very expensive to generate, especially
with syntax-highlighted diffs. Instead of generating the email for each
recipient, generate one email object and reset the Message-Id and To
headers for each recipient. (Cloning would also be expensive in the case
of large emails, although probably not as bad as generating from
scratch.)

Closes #17270.

See merge request !4070

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

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

app/workers/emails_on_push_worker.rb

 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

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)

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

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

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