From 931bad569d7535beea367e71160929fae4c3ca08 Mon Sep 17 00:00:00 2001 From: Quang-Minh Nguyen Date: Mon, 14 Mar 2022 16:35:29 +0700 Subject: [PATCH] Limit the number of commits in push merge request emails Changelog: changed Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/351293 MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82801 --- app/mailers/emails/merge_requests.rb | 9 +- app/services/notification_service.rb | 23 ++- .../push_to_merge_request_email.html.haml | 11 +- spec/mailers/notify_spec.rb | 133 ++++++++++++++++-- spec/services/notification_service_spec.rb | 64 +++++++++ 5 files changed, 223 insertions(+), 17 deletions(-) diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index d2e710cc329a00..341accaea32cd9 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -14,10 +14,17 @@ def new_mention_in_merge_request_email(recipient_id, merge_request_id, updated_b 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)) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index aecf7cf99b91e1..a3f250bb235db8 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -208,13 +208,30 @@ def new_merge_request(merge_request, current_user) 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 diff --git a/app/views/notify/push_to_merge_request_email.html.haml b/app/views/notify/push_to_merge_request_email.html.haml index 3e9f9b442e0e8a..5197a1bdd086bd 100644 --- a/app/views/notify/push_to_merge_request_email.html.haml +++ b/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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 978118ed1b17af..4b7f0057559772 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -317,6 +317,9 @@ 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 @@ 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 @@ 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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 826a0cb46db17c..d2d55c5ab794b3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2101,6 +2101,70 @@ 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 } -- GitLab