From c4e8ba44c91f5b694f67d07b37b83c1a116489d6 Mon Sep 17 00:00:00 2001
From: Gary Holtz <gholtz@gitlab.com>
Date: Tue, 26 Apr 2022 13:43:08 -0500
Subject: [PATCH] Moving approval notifications from EE to FOSS
Changelog: added
---
app/mailers/emails/merge_requests.rb | 14 +++++
.../merge_requests/approval_service.rb | 2 +
.../merge_requests/remove_approval_service.rb | 1 +
app/services/notification_service.rb | 24 ++++++++
.../approved_merge_request_email.html.haml | 13 +++--
.../approved_merge_request_email.text.haml | 0
.../unapproved_merge_request_email.html.haml | 12 ++--
.../unapproved_merge_request_email.text.haml | 0
ee/app/mailers/ee/emails/merge_requests.rb | 14 -----
ee/app/services/ee/notification_service.rb | 24 --------
.../mailers/ee/emails/merge_requests_spec.rb | 30 ----------
.../remove_approval_service_spec.rb | 16 ------
spec/mailers/emails/merge_requests_spec.rb | 30 ++++++++++
.../merge_requests/approval_service_spec.rb | 13 +++++
.../remove_approval_service_spec.rb | 8 ++-
spec/services/notification_service_spec.rb | 55 +++++++++++++++++++
16 files changed, 162 insertions(+), 94 deletions(-)
rename {ee/app => app}/views/notify/approved_merge_request_email.html.haml (96%)
rename {ee/app => app}/views/notify/approved_merge_request_email.text.haml (100%)
rename {ee/app => app}/views/notify/unapproved_merge_request_email.html.haml (96%)
rename {ee/app => app}/views/notify/unapproved_merge_request_email.text.haml (100%)
diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb
index 341accaea32cd..5cbc3c9ef9c85 100644
--- a/app/mailers/emails/merge_requests.rb
+++ b/app/mailers/emails/merge_requests.rb
@@ -155,6 +155,20 @@ def merge_requests_csv_email(user, project, csv_data, export_status)
end
end
+ def approved_merge_request_email(recipient_id, merge_request_id, approved_by_user_id, reason = nil)
+ setup_merge_request_mail(merge_request_id, recipient_id)
+
+ @approved_by = User.find(approved_by_user_id)
+ mail_answer_thread(@merge_request, merge_request_thread_options(approved_by_user_id, reason))
+ end
+
+ def unapproved_merge_request_email(recipient_id, merge_request_id, unapproved_by_user_id, reason = nil)
+ setup_merge_request_mail(merge_request_id, recipient_id)
+
+ @unapproved_by = User.find(unapproved_by_user_id)
+ mail_answer_thread(@merge_request, merge_request_thread_options(unapproved_by_user_id, reason))
+ end
+
private
def setup_merge_request_mail(merge_request_id, recipient_id, present: false)
diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb
index 37c2676e51c58..e3f0758699b52 100644
--- a/app/services/merge_requests/approval_service.rb
+++ b/app/services/merge_requests/approval_service.rb
@@ -33,6 +33,8 @@ def reset_approvals_cache(merge_request)
def execute_approval_hooks(merge_request, current_user)
# Only one approval is required for a merge request to be approved
+ notification_service.async.approve_mr(merge_request, current_user)
+
execute_hooks(merge_request, 'approved')
end
diff --git a/app/services/merge_requests/remove_approval_service.rb b/app/services/merge_requests/remove_approval_service.rb
index c7bc35322646a..d9bb17a7b1b01 100644
--- a/app/services/merge_requests/remove_approval_service.rb
+++ b/app/services/merge_requests/remove_approval_service.rb
@@ -33,6 +33,7 @@ def reset_approvals_cache(merge_request)
def trigger_approval_hooks(merge_request)
yield
+ notification_service.async.unapprove_mr(merge_request, current_user)
execute_hooks(merge_request, 'unapproved')
end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index a3f250bb235db..062841e3aa5da 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -761,6 +761,14 @@ def in_product_marketing(user_id, group_id, track, series)
mailer.in_product_marketing_email(user_id, group_id, track, series).deliver_later
end
+ def approve_mr(merge_request, current_user)
+ approve_mr_email(merge_request, merge_request.target_project, current_user)
+ end
+
+ def unapprove_mr(merge_request, current_user)
+ unapprove_mr_email(merge_request, merge_request.target_project, current_user)
+ end
+
protected
def new_resource_email(target, current_user, method)
@@ -866,6 +874,22 @@ def mailer
private
+ def approve_mr_email(merge_request, project, current_user)
+ recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'approve')
+
+ recipients.each do |recipient|
+ mailer.approved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
+ end
+ end
+
+ def unapprove_mr_email(merge_request, project, current_user)
+ recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'unapprove')
+
+ recipients.each do |recipient|
+ mailer.unapproved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
+ end
+ end
+
def pipeline_notification_status(ref_status, pipeline)
if Ci::Ref.failing_state?(ref_status)
'failed'
diff --git a/ee/app/views/notify/approved_merge_request_email.html.haml b/app/views/notify/approved_merge_request_email.html.haml
similarity index 96%
rename from ee/app/views/notify/approved_merge_request_email.html.haml
rename to app/views/notify/approved_merge_request_email.html.haml
index 6db30b5afa277..4393186a8ad2f 100644
--- a/ee/app/views/notify/approved_merge_request_email.html.haml
+++ b/app/views/notify/approved_merge_request_email.html.haml
@@ -78,7 +78,10 @@
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;padding-right:5px;" }
%img{ alt: "✓", height: "13", src: image_url('mailers/ci_pipeline_notif_v1/icon-check-green-inverted.gif'), style: "display:block;", width: "13" }/
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;" }
- %span Merge request was approved (#{@merge_request.approvals.count}/#{@merge_request.approvals_required})
+ - if @merge_request.respond_to? :approvals_required
+ %span Merge request was approved (#{@merge_request.approvals.count}/#{@merge_request.approvals_required})
+ - else
+ %span Merge request was approved
%tr.spacer
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;height:18px;font-size:18px;line-height:18px;" }
@@ -140,10 +143,10 @@
- if @merge_request.reviewers.any?
= render 'users_list', users: @merge_request.reviewers, user_label: reviewers_label(@merge_request, include_value: false)
-
- -# EE-specific start
- = render 'layouts/mailer/additional_text'
- -# EE-specific end
+ - if Gitlab.ee?
+ -# EE-specific start
+ = render 'layouts/mailer/additional_text'
+ -# EE-specific end
%tr.footer
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:25px 0;font-size:13px;line-height:1.6;color:#5c5c5c;" }
diff --git a/ee/app/views/notify/approved_merge_request_email.text.haml b/app/views/notify/approved_merge_request_email.text.haml
similarity index 100%
rename from ee/app/views/notify/approved_merge_request_email.text.haml
rename to app/views/notify/approved_merge_request_email.text.haml
diff --git a/ee/app/views/notify/unapproved_merge_request_email.html.haml b/app/views/notify/unapproved_merge_request_email.html.haml
similarity index 96%
rename from ee/app/views/notify/unapproved_merge_request_email.html.haml
rename to app/views/notify/unapproved_merge_request_email.html.haml
index c43d37bc1aa8a..8a4138b751592 100644
--- a/ee/app/views/notify/unapproved_merge_request_email.html.haml
+++ b/app/views/notify/unapproved_merge_request_email.html.haml
@@ -78,7 +78,10 @@
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;padding-right:5px;" }
%img{ alt: "✗", height: "13", src: image_url('mailers/approval/icon-x-orange-inverted.gif'), style: "display:block;", width: "13" }/
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;vertical-align:middle;color:#ffffff;text-align:center;" }
- %span Merge request was unapproved (#{@merge_request.approvals.count}/#{@merge_request.approvals_required})
+ - if @merge_request.respond_to? :approvals_required
+ %span Merge request was unapproved (#{@merge_request.approvals.count}/#{@merge_request.approvals_required})
+ - else
+ %span Merge request was unapproved
%tr.spacer
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;height:18px;font-size:18px;line-height:18px;" }
@@ -139,9 +142,10 @@
- if @merge_request.reviewers.any?
= render 'users_list', users: @merge_request.reviewers, user_label: reviewers_label(@merge_request, include_value: false)
- -# EE-specific start
- = render 'layouts/mailer/additional_text'
- -# EE-specific end
+ - if Gitlab.ee?
+ -# EE-specific start
+ = render 'layouts/mailer/additional_text'
+ -# EE-specific end
%tr.footer
%td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;padding:25px 0;font-size:13px;line-height:1.6;color:#5c5c5c;" }
diff --git a/ee/app/views/notify/unapproved_merge_request_email.text.haml b/app/views/notify/unapproved_merge_request_email.text.haml
similarity index 100%
rename from ee/app/views/notify/unapproved_merge_request_email.text.haml
rename to app/views/notify/unapproved_merge_request_email.text.haml
diff --git a/ee/app/mailers/ee/emails/merge_requests.rb b/ee/app/mailers/ee/emails/merge_requests.rb
index 9ecfa21759b6c..704e149667775 100644
--- a/ee/app/mailers/ee/emails/merge_requests.rb
+++ b/ee/app/mailers/ee/emails/merge_requests.rb
@@ -9,20 +9,6 @@ def add_merge_request_approver_email(recipient_id, merge_request_id, updated_by_
@updated_by = ::User.find(updated_by_user_id)
mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, reason))
end
-
- def approved_merge_request_email(recipient_id, merge_request_id, approved_by_user_id, reason = nil)
- setup_merge_request_mail(merge_request_id, recipient_id)
-
- @approved_by = ::User.find(approved_by_user_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(approved_by_user_id, reason))
- end
-
- def unapproved_merge_request_email(recipient_id, merge_request_id, unapproved_by_user_id, reason = nil)
- setup_merge_request_mail(merge_request_id, recipient_id)
-
- @unapproved_by = ::User.find(unapproved_by_user_id)
- mail_answer_thread(@merge_request, merge_request_thread_options(unapproved_by_user_id, reason))
- end
end
end
end
diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb
index c40e0692dc5d5..6ad77f1b66200 100644
--- a/ee/app/services/ee/notification_service.rb
+++ b/ee/app/services/ee/notification_service.rb
@@ -15,14 +15,6 @@ def add_merge_request_approvers(merge_request, new_approvers, current_user)
add_mr_approvers_email(merge_request, new_approvers, current_user)
end
- def approve_mr(merge_request, current_user)
- approve_mr_email(merge_request, merge_request.target_project, current_user)
- end
-
- def unapprove_mr(merge_request, current_user)
- unapprove_mr_email(merge_request, merge_request.target_project, current_user)
- end
-
def mirror_was_hard_failed(project)
return if project.emails_disabled?
@@ -136,22 +128,6 @@ def add_mr_approvers_email(merge_request, approvers, current_user)
end
end
- def approve_mr_email(merge_request, project, current_user)
- recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'approve')
-
- recipients.each do |recipient|
- mailer.approved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
- end
- end
-
- def unapprove_mr_email(merge_request, project, current_user)
- recipients = ::NotificationRecipients::BuildService.build_recipients(merge_request, current_user, action: 'unapprove')
-
- recipients.each do |recipient|
- mailer.unapproved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
- end
- end
-
def removed_iteration_resource_email(target, current_user)
recipients = ::NotificationRecipients::BuildService.build_recipients(
target,
diff --git a/ee/spec/mailers/ee/emails/merge_requests_spec.rb b/ee/spec/mailers/ee/emails/merge_requests_spec.rb
index 4f55f2a425780..0ef39f99a4e77 100644
--- a/ee/spec/mailers/ee/emails/merge_requests_spec.rb
+++ b/ee/spec/mailers/ee/emails/merge_requests_spec.rb
@@ -46,34 +46,4 @@
end
end
end
-
- describe '#approved_merge_request_email' do
- subject { Notify.approved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
-
- it 'has the correct body' do
- aggregate_failures do
- is_expected.to have_body_text('was approved by')
- is_expected.to have_body_text(current_user.name)
- is_expected.to have_text_part_content(assignee.name)
- is_expected.to have_html_part_content(assignee.name)
- is_expected.to have_text_part_content(reviewer.name)
- is_expected.to have_html_part_content(reviewer.name)
- end
- end
- end
-
- describe '#unapproved_merge_request_email' do
- subject { Notify.unapproved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
-
- it 'has the correct body' do
- aggregate_failures do
- is_expected.to have_body_text('was unapproved by')
- is_expected.to have_body_text(current_user.name)
- is_expected.to have_text_part_content(assignee.name)
- is_expected.to have_html_part_content(assignee.name)
- is_expected.to have_text_part_content(reviewer.name)
- is_expected.to have_html_part_content(reviewer.name)
- end
- end
- end
end
diff --git a/ee/spec/services/merge_requests/remove_approval_service_spec.rb b/ee/spec/services/merge_requests/remove_approval_service_spec.rb
index 3ea83280115be..40d9dbd947459 100644
--- a/ee/spec/services/merge_requests/remove_approval_service_spec.rb
+++ b/ee/spec/services/merge_requests/remove_approval_service_spec.rb
@@ -49,21 +49,5 @@ def execute!
execute!
end
end
-
- context 'with an approved merge request' do
- let(:notification_service) { NotificationService.new }
-
- before do
- merge_request.approvals.create!(user: user)
- allow(service).to receive(:notification_service).and_return(notification_service)
- end
-
- it 'fires an unapproved webhook and sends a notification' do
- expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user)
- expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved')
-
- execute!
- end
- end
end
end
diff --git a/spec/mailers/emails/merge_requests_spec.rb b/spec/mailers/emails/merge_requests_spec.rb
index dea54f7315d9b..d4dc7a7651638 100644
--- a/spec/mailers/emails/merge_requests_spec.rb
+++ b/spec/mailers/emails/merge_requests_spec.rb
@@ -182,6 +182,36 @@
end
end
+ describe '#approved_merge_request_email' do
+ subject { Notify.approved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
+
+ it 'has the correct body' do
+ aggregate_failures do
+ is_expected.to have_body_text('was approved by')
+ is_expected.to have_body_text(current_user.name)
+ is_expected.to have_text_part_content(assignee.name)
+ is_expected.to have_html_part_content(assignee.name)
+ is_expected.to have_text_part_content(reviewer.name)
+ is_expected.to have_html_part_content(reviewer.name)
+ end
+ end
+ end
+
+ describe '#unapproved_merge_request_email' do
+ subject { Notify.unapproved_merge_request_email(recipient.id, merge_request.id, current_user.id) }
+
+ it 'has the correct body' do
+ aggregate_failures do
+ is_expected.to have_body_text('was unapproved by')
+ is_expected.to have_body_text(current_user.name)
+ is_expected.to have_text_part_content(assignee.name)
+ is_expected.to have_html_part_content(assignee.name)
+ is_expected.to have_text_part_content(reviewer.name)
+ is_expected.to have_html_part_content(reviewer.name)
+ end
+ end
+ end
+
describe "#resolved_all_discussions_email" do
subject { Notify.resolved_all_discussions_email(recipient.id, merge_request.id, current_user.id) }
diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb
index 4d20d62b86440..e500102a00c64 100644
--- a/spec/services/merge_requests/approval_service_spec.rb
+++ b/spec/services/merge_requests/approval_service_spec.rb
@@ -41,6 +41,12 @@
end
context 'with valid approval' do
+ let(:notification_service) { NotificationService.new }
+
+ before do
+ allow(service).to receive(:notification_service).and_return(notification_service)
+ end
+
it 'creates an approval note and marks pending todos as done' do
expect(SystemNoteService).to receive(:approve_mr).with(merge_request, user)
expect(merge_request.approvals).to receive(:reset)
@@ -59,6 +65,13 @@
service.execute(merge_request)
end
+ it 'sends a notification when approving' do
+ expect(notification_service).to receive_message_chain(:async, :approve_mr)
+ .with(merge_request, user)
+
+ service.execute(merge_request)
+ end
+
it 'removes attention requested state' do
expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new)
.with(project: project, current_user: user, merge_request: merge_request, user: user)
diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb
index ef6a0ec69bddf..5a319e90a682b 100644
--- a/spec/services/merge_requests/remove_approval_service_spec.rb
+++ b/spec/services/merge_requests/remove_approval_service_spec.rb
@@ -21,14 +21,20 @@ def execute!
context 'with a user who has approved' do
let!(:approval) { create(:approval, user: user, merge_request: merge_request) }
+ let(:notification_service) { NotificationService.new }
+
+ before do
+ allow(service).to receive(:notification_service).and_return(notification_service)
+ end
it 'removes the approval' do
expect { execute! }.to change { merge_request.approvals.size }.from(2).to(1)
end
- it 'creates an unapproval note and triggers web hook' do
+ it 'creates an unapproval note, triggers a web hook, and sends a notification' do
expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved')
expect(SystemNoteService).to receive(:unapprove_mr)
+ expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user)
execute!
end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index d2d55c5ab794b..956a95eafb7aa 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1869,6 +1869,61 @@
let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) }
end
+ describe 'Approvals' do
+ let(:notification_target) { merge_request }
+ let(:maintainer) { create(:user) }
+
+ describe '#approve_mr' do
+ it 'will notify the author, subscribers, and assigned users' do
+ notification.approve_mr(merge_request, maintainer)
+
+ merge_request.assignees.each { |assignee| should_email(assignee) }
+ should_email(merge_request.author)
+ should_email(@u_watcher)
+ should_email(@u_participant_mentioned)
+ should_email(@subscribed_participant)
+ should_email(@subscriber)
+ should_email(@watcher_and_subscriber)
+ should_email(@u_guest_watcher)
+
+ should_not_email(@unsubscriber)
+ should_not_email(@u_participating)
+ should_not_email(@u_disabled)
+ should_not_email(@u_lazy_participant)
+
+ expect(email_recipients.size).to eq(8)
+ # assignee, author, @u_watcher,
+ # @u_participant_mentioned, @subscribed_participant,
+ # @subscriber, @watcher_and_subscriber, @u_guest_watcher
+ end
+ end
+
+ describe '#unapprove_mr' do
+ it 'will notify the author, subscribers, and assigned users' do
+ notification.unapprove_mr(merge_request, maintainer)
+
+ merge_request.assignees.each { |assignee| should_email(assignee) }
+ should_email(merge_request.author)
+ should_email(@u_watcher)
+ should_email(@u_participant_mentioned)
+ should_email(@subscribed_participant)
+ should_email(@subscriber)
+ should_email(@watcher_and_subscriber)
+ should_email(@u_guest_watcher)
+
+ should_not_email(@unsubscriber)
+ should_not_email(@u_participating)
+ should_not_email(@u_disabled)
+ should_not_email(@u_lazy_participant)
+
+ expect(email_recipients.size).to eq(8)
+ # assignee, author, @u_watcher,
+ # @u_participant_mentioned, @subscribed_participant,
+ # @subscriber, @watcher_and_subscriber, @u_guest_watcher
+ end
+ end
+ end
+
context 'participating' do
it_behaves_like 'participating by assignee notification' do
let(:participant) { create(:user, username: 'user-participant')}
--
GitLab