Send email to new approvers of MRs

When an approver is added to an MR, create a todo for them and send them
an email.

app/mailers/emails/merge_requests.rb

@@ -42,6 +42,13 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
     end
 
+    def add_merge_request_approver_email(recipient_id, merge_request_id, updated_by_user_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
+      @updated_by = User.find(updated_by_user_id)
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+    end
+
     def approved_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
       setup_merge_request_mail(merge_request_id, recipient_id)
 

app/services/merge_requests/update_service.rb

@@ -17,7 +17,11 @@ module MergeRequests
       update(merge_request)
 
       new_approvers = merge_request.overall_approvers.to_a - old_approvers
-      todo_service.add_merge_request_approvers(merge_request, new_approvers) if new_approvers.any?
+
+      if new_approvers.any?
+        todo_service.add_merge_request_approvers(merge_request, new_approvers)
+        notification_service.add_merge_request_approvers(merge_request, new_approvers, current_user)
+      end
 
       merge_request
     end

app/services/notification_service.rb

@@ -70,6 +70,7 @@ class NotificationService
   #  * project team members with notification level higher then Participating
   #  * watchers of the mr's labels
   #  * users with custom level checked with "new merge request"
+  #  * approvers of the merge request
   #
   def new_merge_request(merge_request, current_user)
     new_resource_email(merge_request, merge_request.target_project, :new_merge_request_email)
@@ -101,6 +102,14 @@ class NotificationService
     reopen_resource_email(issue, issue.project, current_user, :issue_status_changed_email, 'reopened')
   end
 
+  # When we add approvers to a merge request we should send an email to:
+  #
+  #  * the new approvers
+  #
+  def add_merge_request_approvers(merge_request, new_approvers, current_user)
+    add_mr_approvers_email(merge_request, new_approvers, current_user)
+  end
+
   def merge_mr(merge_request, current_user)
     close_resource_email(
       merge_request,
@@ -526,6 +535,14 @@ class NotificationService
     end
   end
 
+  def add_mr_approvers_email(merge_request, approvers, current_user)
+    approvers.each do |approver|
+      recipient = approver.user
+
+      mailer.add_merge_request_approver_email(recipient.id, merge_request.id, current_user.id).deliver_later
+    end
+  end
+
   def build_recipients(target, project, current_user, action: nil, previous_assignee: nil)
     custom_action = build_custom_key(action, target)
 

app/views/notify/add_merge_request_approver_email.html.haml

+- if current_application_settings.email_author_in_body
+  %div
+    #{link_to @updated_by.name, user_url(@updated_by)} added you as an approver for:
+%p.details
+  != merge_path_description(@merge_request, '→')
+
+- if @merge_request.assignee_id.present?
+  %p
+    Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
+
+- if @merge_request.approvers.any?
+  %p
+    Approvers: #{render_items_list(@merge_request.approvers_left.map(&:name))}
+
+-if @merge_request.description
+  = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)

app/views/notify/add_merge_request_approver_email.text.erb

+<%= @updated_by.name %> added you as an approver for <%= @merge_request.to_reference %>
+
+<%= url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request)) %>
+
+<%= merge_path_description(@merge_request, 'to') %>
+Author:    <%= @merge_request.author_name %>
+Assignee:  <%= @merge_request.assignee_name %>
+<% if @merge_request.approvers.any? %>
+Approvers: <%= render_items_list(@merge_request.approvers_left.map(&:name)) %>
+<% end %>

spec/services/merge_requests/update_service_spec.rb

@@ -232,23 +232,34 @@ describe MergeRequests::UpdateService, services: true do
       let(:new_approver) { create(:user) }
 
       before do
+        perform_enqueued_jobs do
           update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+        end
+
         Todo.where(action: Todo::APPROVAL_REQUIRED).destroy_all
+        ActionMailer::Base.deliveries.clear
       end
 
       context 'when an approver is added and an approver is removed' do
-        before { update_merge_request(approver_ids: [new_approver, existing_approver].map(&:id).join(',')) }
+        before do
+          perform_enqueued_jobs do
+            update_merge_request(approver_ids: [new_approver, existing_approver].map(&:id).join(','))
+          end
+        end
 
-        it 'adds todos for the new approvers' do
+        it 'adds todos for and sends emails to the new approvers' do
           expect(Todo.where(user: new_approver, action: Todo::APPROVAL_REQUIRED)).not_to be_empty
+          should_email(new_approver)
         end
 
-        it 'does not add todos for the existing approvers' do
+        it 'does not add todos for or send emails to the existing approvers' do
           expect(Todo.where(user: existing_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+          should_not_email(existing_approver)
         end
 
-        it 'does not add todos for the removed approvers' do
+        it 'does not add todos for or send emails to the removed approvers' do
           expect(Todo.where(user: removed_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+          should_not_email(removed_approver)
         end
       end
 
@@ -258,6 +269,12 @@ describe MergeRequests::UpdateService, services: true do
             update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
           end.not_to change { Todo.count }
         end
+
+        it 'does not send any emails' do
+          expect do
+            update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+          end.not_to change { ActionMailer::Base.deliveries.count }
+        end
       end
     end
 

spec/services/notification_service_spec.rb

@@ -709,6 +709,41 @@ describe NotificationService, services: true do
         should_email(subscriber)
       end
 
+      context 'when the target project has approvers set' do
+        let(:project_approvers) { create_list(:user, 3) }
+
+        before do
+          merge_request.target_project.update_attributes(approvals_before_merge: 1)
+          project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
+        end
+
+        it 'emails the approvers' do
+          notification.new_merge_request(merge_request, @u_disabled)
+
+          project_approvers.each { |approver| should_email(approver) }
+        end
+
+        context 'when the merge request has approvers set' do
+          let(:mr_approvers) { create_list(:user, 3) }
+
+          before do
+            mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
+          end
+
+          it 'emails the MR approvers' do
+            notification.new_merge_request(merge_request, @u_disabled)
+
+            mr_approvers.each { |approver| should_email(approver) }
+          end
+
+          it 'does not email approvers set on the project who are not approvers of this MR' do
+            notification.new_merge_request(merge_request, @u_disabled)
+
+            project_approvers.each { |approver| should_not_email(approver) }
+          end
+        end
+      end
+
       context 'participating' do
         context 'by assignee' do
           before do