Merge branch...

Merge branch '351293-large-pushes-to-merge-requests-can-cause-emails-not-to-be-delivered' into 'master'

Limit the number of commits in push merge request emails

See merge request gitlab-org/gitlab!82801

app/mailers/emails/merge_requests.rb

@@ -14,10 +14,17 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, reason))
     end
 
-    def push_to_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil, new_commits: [], existing_commits: [])
+    # existing_commits - an array containing the first and last commits
+    def push_to_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil, new_commits: [], total_new_commits_count: nil, existing_commits: [], total_existing_commits_count: nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
+
       @new_commits = new_commits
+      @total_new_commits_count = total_new_commits_count || @new_commits.length
+      @total_stripped_new_commits_count = @total_new_commits_count - @new_commits.length
+
       @existing_commits = existing_commits
+      @total_existing_commits_count = total_existing_commits_count || @existing_commits.length
+
       @updated_by_user = User.find(updated_by_user_id)
 
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, reason))

app/services/notification_service.rb

@@ -208,13 +208,30 @@ class NotificationService
     new_resource_email(merge_request, current_user, :new_merge_request_email)
   end
 
+  NEW_COMMIT_EMAIL_DISPLAY_LIMIT = 20
   def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: [])
-    new_commits = new_commits.map { |c| { short_id: c.short_id, title: c.title } }
-    existing_commits = existing_commits.map { |c| { short_id: c.short_id, title: c.title } }
+    total_new_commits_count = new_commits.count
+    truncated_new_commits = new_commits.first(NEW_COMMIT_EMAIL_DISPLAY_LIMIT).map do |commit|
+      { short_id: commit.short_id, title: commit.title }
+    end
+
+    # We don't need the list of all existing commits. We need the first, the
+    # last, and the total number of existing commits only.
+    total_existing_commits_count = existing_commits.count
+    existing_commits = [existing_commits.first, existing_commits.last] if total_existing_commits_count > 2
+    existing_commits = existing_commits.map do |commit|
+      { short_id: commit.short_id, title: commit.title }
+    end
+
     recipients = NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: "push_to")
 
     recipients.each do |recipient|
-      mailer.send(:push_to_merge_request_email, recipient.user.id, merge_request.id, current_user.id, recipient.reason, new_commits: new_commits, existing_commits: existing_commits).deliver_later
+      mailer.send(
+        :push_to_merge_request_email,
+        recipient.user.id, merge_request.id, current_user.id, recipient.reason,
+        new_commits: truncated_new_commits, total_new_commits_count: total_new_commits_count,
+        existing_commits: existing_commits, total_existing_commits_count: total_existing_commits_count
+      ).deliver_later
     end
   end
 

app/views/notify/push_to_merge_request_email.html.haml

@@ -3,24 +3,25 @@
   pushed new commits to merge request
   = merge_request_reference_link(@merge_request)
 
-- if @existing_commits.any?
-  - count = @existing_commits.size
+- if @total_existing_commits_count > 0
   %ul
     %li
-      - if count == 1
+      - if @total_existing_commits_count == 1
         - commit_id = @existing_commits.first[:short_id]
         = link_to(commit_id, project_commit_url(@merge_request.target_project, commit_id))
       - else
         = link_to(project_compare_url(@merge_request.target_project, from: @existing_commits.first[:short_id], to: @existing_commits.last[:short_id])) do
           #{@existing_commits.first[:short_id]}...#{@existing_commits.last[:short_id]}
       = precede ' - ' do
-        - commits_text = "#{count} commit".pluralize(count)
+        - commits_text = "#{@total_existing_commits_count} commit".pluralize(@total_existing_commits_count)
         #{commits_text} from branch `#{@merge_request.target_branch}`
 
-- if @new_commits.any?
+- if @total_new_commits_count > 0
   %ul
     - @new_commits.each do |commit|
       %li
         = link_to(commit[:short_id], project_commit_url(@merge_request.target_project, commit[:short_id]))
         = precede ' - ' do
           #{commit[:title]}
+    - if @total_stripped_new_commits_count > 0
+      %li And #{@total_stripped_new_commits_count} more

spec/mailers/notify_spec.rb

@@ -317,6 +317,9 @@ RSpec.describe Notify do
     end
 
     context 'for merge requests' do
+      let(:push_user) { create(:user) }
+      let(:commit_limit) { NotificationService::NEW_COMMIT_EMAIL_DISPLAY_LIMIT }
+
       describe 'that are new' do
         subject { described_class.new_merge_request_email(merge_request.assignee_ids.first, merge_request.id) }
 
@@ -457,12 +460,6 @@ RSpec.describe Notify do
       end
 
       shared_examples 'a push to an existing merge request' do
-        let(:push_user) { create(:user) }
-
-        subject do
-          described_class.push_to_merge_request_email(recipient.id, merge_request.id, push_user.id, new_commits: merge_request.commits, existing_commits: existing_commits)
-        end
-
         it_behaves_like 'a multiple recipients email'
         it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
           let(:model) { merge_request }
@@ -487,16 +484,136 @@ RSpec.describe Notify do
         end
       end
 
-      describe 'that have new commits' do
-        let(:existing_commits) { [] }
+      shared_examples 'shows the compare url between first and last commits' do |count|
+        it 'shows the compare url between first and last commits' do
+          commit_id_1 = existing_commits.first[:short_id]
+          commit_id_2 = existing_commits.last[:short_id]
+
+          is_expected.to have_link("#{commit_id_1}...#{commit_id_2}", href: project_compare_url(project, from: commit_id_1, to: commit_id_2))
+          is_expected.to have_body_text("#{count} commits from branch `#{merge_request.target_branch}`")
+        end
+      end
+
+      shared_examples 'shows new commit urls' do |count|
+        it 'shows new commit urls' do
+          displayed_new_commits.each do |commit|
+            is_expected.to have_link(commit[:short_id], href: project_commit_url(project, commit[:short_id]))
+            is_expected.to have_body_text(commit[:title])
+          end
+        end
+
+        it 'does not show hidden new commit urls' do
+          hidden_new_commits.each do |commit|
+            is_expected.not_to have_link(commit[:short_id], href: project_commit_url(project, commit[:short_id]))
+            is_expected.not_to have_body_text(commit[:title])
+          end
+        end
+      end
+
+      describe 'that have no new commits' do
+        subject do
+          described_class.push_to_merge_request_email(recipient.id, merge_request.id, push_user.id, new_commits: [], total_new_commits_count: 0, existing_commits: [], total_existing_commits_count: 0)
+        end
+
+        it_behaves_like 'a push to an existing merge request'
+      end
+
+      describe 'that have fewer than the commit truncation limit' do
+        let(:new_commits) { merge_request.commits }
+        let(:displayed_new_commits) { new_commits }
+        let(:hidden_new_commits) { [] }
+
+        subject do
+          described_class.push_to_merge_request_email(
+            recipient.id, merge_request.id, push_user.id,
+            new_commits: new_commits, total_new_commits_count: new_commits.length,
+            existing_commits: [], total_existing_commits_count: 0
+          )
+        end
+
+        it_behaves_like 'a push to an existing merge request'
+        it_behaves_like 'shows new commit urls'
+      end
+
+      describe 'that have more than the commit truncation limit' do
+        let(:new_commits) do
+          Array.new(commit_limit + 10) do |i|
+            {
+              short_id: SecureRandom.hex(4),
+              title: "This is commit #{i}"
+            }
+          end
+        end
+
+        let(:displayed_new_commits) { new_commits.first(commit_limit) }
+        let(:hidden_new_commits) { new_commits.last(10) }
+
+        subject do
+          described_class.push_to_merge_request_email(
+            recipient.id, merge_request.id, push_user.id,
+            new_commits: displayed_new_commits, total_new_commits_count: commit_limit + 10,
+            existing_commits: [], total_existing_commits_count: 0
+          )
+        end
 
         it_behaves_like 'a push to an existing merge request'
+        it_behaves_like 'shows new commit urls'
+
+        it 'shows "and more" message' do
+          is_expected.to have_body_text("And 10 more")
+        end
       end
 
       describe 'that have new commits on top of an existing one' do
         let(:existing_commits) { [merge_request.commits.first] }
 
+        subject do
+          described_class.push_to_merge_request_email(
+            recipient.id, merge_request.id, push_user.id,
+            new_commits: merge_request.commits, total_new_commits_count: merge_request.commits.length,
+            existing_commits: existing_commits, total_existing_commits_count: existing_commits.length
+          )
+        end
+
+        it_behaves_like 'a push to an existing merge request'
+
+        it 'shows the existing commit' do
+          commit_id = existing_commits.first.short_id
+          is_expected.to have_link(commit_id, href: project_commit_url(project, commit_id))
+          is_expected.to have_body_text("1 commit from branch `#{merge_request.target_branch}`")
+        end
+      end
+
+      describe 'that have new commits on top of two existing ones' do
+        let(:existing_commits) { [merge_request.commits.first, merge_request.commits.second] }
+
+        subject do
+          described_class.push_to_merge_request_email(
+            recipient.id, merge_request.id, push_user.id,
+            new_commits: merge_request.commits, total_new_commits_count: merge_request.commits.length,
+            existing_commits: existing_commits, total_existing_commits_count: existing_commits.length
+          )
+        end
+
+        it_behaves_like 'a push to an existing merge request'
+        it_behaves_like 'shows the compare url between first and last commits', 2
+      end
+
+      describe 'that have new commits on top of more than two existing ones' do
+        let(:existing_commits) do
+          [merge_request.commits.first] + [double(:commit)] * 3 + [merge_request.commits.second]
+        end
+
+        subject do
+          described_class.push_to_merge_request_email(
+            recipient.id, merge_request.id, push_user.id,
+            new_commits: merge_request.commits, total_new_commits_count: merge_request.commits.length,
+            existing_commits: existing_commits, total_existing_commits_count: existing_commits.length
+          )
+        end
+
         it_behaves_like 'a push to an existing merge request'
+        it_behaves_like 'shows the compare url between first and last commits', 5
       end
     end
 

spec/services/notification_service_spec.rb

@@ -2101,6 +2101,70 @@ RSpec.describe NotificationService, :mailer do
         should_not_email(@u_lazy_participant)
       end
 
+      describe 'triggers push_to_merge_request_email with corresponding email' do
+        let_it_be(:merge_request) { create(:merge_request, author: author, source_project: project) }
+
+        def mock_commits(length)
+          Array.new(length) { |i| double(:commit, short_id: SecureRandom.hex(4), title: "This is commit #{i}") }
+        end
+
+        def commit_to_hash(commit)
+          { short_id: commit.short_id, title: commit.title }
+        end
+
+        let(:existing_commits) { mock_commits(50) }
+        let(:expected_existing_commits) { [commit_to_hash(existing_commits.first), commit_to_hash(existing_commits.last)] }
+
+        before do
+          allow(::Notify).to receive(:push_to_merge_request_email).and_call_original
+        end
+
+        where(:number_of_new_commits, :number_of_new_commits_displayed) do
+          limit = described_class::NEW_COMMIT_EMAIL_DISPLAY_LIMIT
+          [
+            [0, 0],
+            [limit - 2, limit - 2],
+            [limit - 1, limit - 1],
+            [limit, limit],
+            [limit + 1, limit],
+            [limit + 2, limit]
+          ]
+        end
+
+        with_them do
+          let(:new_commits) { mock_commits(number_of_new_commits) }
+          let(:expected_new_commits) { new_commits.first(number_of_new_commits_displayed).map(&method(:commit_to_hash)) }
+
+          it 'triggers the corresponding mailer method with list of stripped commits' do
+            notification.push_to_merge_request(
+              merge_request, merge_request.author,
+              new_commits: new_commits, existing_commits: existing_commits
+            )
+
+            expect(Notify).to have_received(:push_to_merge_request_email).at_least(:once).with(
+              @subscriber.id, merge_request.id, merge_request.author.id, "subscribed",
+              new_commits: expected_new_commits, total_new_commits_count: number_of_new_commits,
+              existing_commits: expected_existing_commits, total_existing_commits_count: 50
+            )
+          end
+        end
+
+        context 'there is only one existing commit' do
+          let(:new_commits) { mock_commits(10) }
+          let(:expected_new_commits) { new_commits.map(&method(:commit_to_hash)) }
+
+          it 'triggers corresponding mailer method with only one existing commit' do
+            notification.push_to_merge_request(merge_request, merge_request.author, new_commits: new_commits, existing_commits: existing_commits.first(1))
+
+            expect(Notify).to have_received(:push_to_merge_request_email).at_least(:once).with(
+              @subscriber.id, merge_request.id, merge_request.author.id, "subscribed",
+              new_commits: expected_new_commits, total_new_commits_count: 10,
+              existing_commits: expected_existing_commits.first(1), total_existing_commits_count: 1
+            )
+          end
+        end
+      end
+
       it_behaves_like 'participating notifications' do
         let(:participant) { create(:user, username: 'user-participant') }
         let(:issuable) { merge_request }