Merge branch '42-approval-notifications' into 'master'

Send notification email when MR is approved

Closes #42.

See merge request !484

CHANGELOG-EE

@@ -7,6 +7,7 @@ v 8.9.0 (unreleased)
   - Forbid MR authors from approving their own MRs
   - Instrument instance methods of Gitlab::InsecureKeyFingerprint class
   - Add API endpoint for Merge Request Approvals !449
+  - Send notification email when merge request is approved
   - Distribute RepositoryUpdateMirror jobs in time and add exclusive lease on them by project_id
   - [Elastic] Move ES settings to application settings
   - Disable mirror flag for projects without import_url

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 approved_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
+      @approved_by_users = @merge_request.approved_by_users.map(&:name)
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
+    end
+
     private
 
     def setup_merge_request_mail(merge_request_id, recipient_id)

app/services/merge_requests/approval_service.rb

@@ -8,6 +8,7 @@ module MergeRequests
         mark_pending_todos_as_done(merge_request)
 
         if merge_request.approvals_left.zero?
+          notification_service.approve_mr(merge_request, current_user)
           execute_hooks(merge_request, 'approved')
         end
       end

app/services/notification_service.rb

@@ -115,6 +115,10 @@ class NotificationService
     )
   end
 
+  def approve_mr(merge_request, current_user)
+    approve_mr_email(merge_request, merge_request.target_project, current_user)
+  end
+
   # Notify new user with email after creation
   def new_user(user, token = nil)
     # Don't email omniauth created users
@@ -477,6 +481,14 @@ class NotificationService
     end
   end
 
+  def approve_mr_email(merge_request, project, current_user)
+    recipients = build_recipients(merge_request, project, current_user)
+
+    recipients.each do |recipient|
+      mailer.approved_merge_request_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)
     recipients = target.participants(current_user)
     recipients = add_project_watchers(recipients, project)

app/views/notify/approved_merge_request_email.html.haml

+%p
+  = "Merge Request #{@merge_request.to_reference} was approved by #{@approved_by_users.to_sentence}"

app/views/notify/approved_merge_request_email.text.haml

+= "Merge Request #{@merge_request.to_reference} was approved by #{@approved_by_users.to_sentence}"
+
+Merge Request url: #{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}

spec/mailers/notify_spec.rb

@@ -366,6 +366,46 @@ describe Notify do
           end
         end
 
+        describe 'that are approved' do
+          let(:last_approver) { create(:user) }
+          subject { Notify.approved_merge_request_email(recipient.id, merge_request.id, last_approver.id) }
+
+          before do
+            merge_request.approvals.create(user: merge_request.assignee)
+            merge_request.approvals.create(user: last_approver)
+          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 }
+          end
+          it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like 'an unsubscribeable thread'
+
+          it 'is sent as the last approver' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(last_approver.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{merge_request.title} \(#{merge_request.to_reference}\)/
+          end
+
+          it 'contains the new status' do
+            is_expected.to have_body_text /approved/i
+          end
+
+          it 'contains a link to the merge request' do
+            is_expected.to have_body_text /#{namespace_project_merge_request_path project.namespace, project, merge_request}/
+          end
+
+          it 'contains the names of all of the approvers' do
+            is_expected.to have_body_text /#{merge_request.assignee.name}/
+            is_expected.to have_body_text /#{last_approver.name}/
+          end
+        end
+
         describe 'that are merged' do
           subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
 

spec/services/merge_requests/approval_service_spec.rb

@@ -47,15 +47,36 @@ describe MergeRequests::ApprovalService, services: true do
 
           service.execute(merge_request)
         end
+
+        it 'does not send an email' do
+          expect(merge_request).to receive(:approvals_left).and_return(5)
+          expect(service).not_to receive(:notification_service)
+
+          service.execute(merge_request)
+        end
       end
 
       context 'with required approvals' do
-        it 'fires a webhook' do
+        let(:notification_service) { NotificationService.new }
+
+        before do
           expect(merge_request).to receive(:approvals_left).and_return(0)
+          allow(service).to receive(:notification_service).and_return(notification_service)
+          allow(service).to receive(:execute_hooks)
+          allow(notification_service).to receive(:approve_mr)
+        end
+
+        it 'fires a webhook' do
           expect(service).to receive(:execute_hooks).with(merge_request, 'approved')
 
           service.execute(merge_request)
         end
+
+        it 'sends an email' do
+          expect(notification_service).to receive(:approve_mr).with(merge_request, user)
+
+          service.execute(merge_request)
+        end
       end
     end
   end