Merge branch '42-merge-approvals-don-t-appear-to-trigger-notification-emails' into 'master'

Resolve "Merge approvals don't appear to trigger notification emails"

This adds:
* An 'approval required' todo type (I'll need to create an MR to CE with this commit after this MR is done).
* A todo for approvers on MR creation. (They already get an email, as they are participants in the MR.)
* A todo and an email for new approvers when an MR is edited.

Closes #42.

See merge request !547

CHANGELOG-EE

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.10.0 (unreleased)
   - Rename Git Hooks to Push Rules
   - Fix EE keys fingerprint add index migration if came from CE
+  - Add todos for MR approvers !547
   - Isolate EE LDAP library code in EE module (Part 1) !511
 
 v 8.9.5

app/finders/todos_finder.rb

@@ -37,7 +37,7 @@ class TodosFinder
   private
 
   def action_id?
-    action_id.present? && [Todo::ASSIGNED, Todo::MENTIONED, Todo::BUILD_FAILED, Todo::MARKED].include?(action_id.to_i)
+    action_id.present? && Todo::ACTION_NAMES.has_key?(action_id.to_i)
   end
 
   def action_id

app/helpers/todos_helper.rb

@@ -13,6 +13,7 @@ module TodosHelper
     when Todo::MENTIONED then 'mentioned you on'
     when Todo::BUILD_FAILED then 'The build failed for your'
     when Todo::MARKED then 'added a todo for'
+    when Todo::APPROVAL_REQUIRED then 'set you as an approver for'
     end
   end
 

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/models/todo.rb

@@ -3,12 +3,14 @@ class Todo < ActiveRecord::Base
   MENTIONED         = 2
   BUILD_FAILED      = 3
   MARKED            = 4
+  APPROVAL_REQUIRED = 5 # This is an EE-only feature
 
   ACTION_NAMES = {
     ASSIGNED => :assigned,
     MENTIONED => :mentioned,
     BUILD_FAILED => :build_failed,
-    MARKED => :marked
+    MARKED => :marked,
+    APPROVAL_REQUIRED => :approval_required
   }
 
   belongs_to :author, class_name: "User"

app/services/merge_requests/update_service.rb

@@ -12,8 +12,18 @@ module MergeRequests
       params.except!(:source_branch)
 
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
+      old_approvers = merge_request.overall_approvers.to_a
 
       update(merge_request)
+
+      new_approvers = merge_request.overall_approvers.to_a - old_approvers
+
+      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
 
     def handle_changes(merge_request, old_labels: [])

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/services/todo_service.rb

@@ -88,6 +88,14 @@ class TodoService
     create_build_failed_todo(merge_request)
   end
 
+  # When new approvers are added for a merge request:
+  #
+  #  * create a todo for those users to approve the MR
+  #
+  def add_merge_request_approvers(merge_request, approvers)
+    create_approval_required_todos(merge_request, approvers, merge_request.author)
+  end
+
   # When a new commit is pushed to a merge request we should:
   #
   #  * mark all pending todos related to the merge request for that user as done
@@ -167,6 +175,11 @@ class TodoService
 
   def new_issuable(issuable, author)
     create_assignment_todo(issuable, author)
+
+    if issuable.is_a?(MergeRequest)
+      create_approval_required_todos(issuable, issuable.overall_approvers, author)
+    end
+
     create_mention_todos(issuable.project, issuable, author)
   end
 
@@ -206,6 +219,11 @@ class TodoService
     create_todos(mentioned_users, attributes)
   end
 
+  def create_approval_required_todos(merge_request, approvers, author)
+    attributes = attributes_for_todo(merge_request.project, merge_request, author, Todo::APPROVAL_REQUIRED)
+    create_todos(approvers.map(&:user), attributes)
+  end
+
   def create_build_failed_todo(merge_request)
     author = merge_request.author
     attributes = attributes_for_todo(merge_request.project, merge_request, author, Todo::BUILD_FAILED)

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 %>

doc/api/todos.md

@@ -15,7 +15,7 @@ Parameters:
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `action` | string | no | The action to be filtered. Can be `assigned`, `mentioned`, `build_failed`, or `marked`. |
+| `action` | string | no | The action to be filtered. Can be `assigned`, `mentioned`, `build_failed`, `marked`, or `approval_required`. |
 | `author_id` | integer | no | The ID of an author |
 | `project_id` | integer | no | The ID of a project |
 | `state` | string | no | The state of the todo. Can be either `pending` or `done` |

spec/factories/todos.rb

@@ -23,6 +23,10 @@ FactoryGirl.define do
       action { Todo::BUILD_FAILED }
     end
 
+    trait :approval_required do
+      action { Todo::APPROVAL_REQUIRED }
+    end
+
     trait :done do
       state :done
     end

spec/services/merge_requests/update_service_spec.rb

@@ -184,7 +184,7 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
-    context 'when the issue is relabeled' do
+    context 'when the merge request is relabeled' do
       let!(:non_subscriber) { create(:user) }
       let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
@@ -199,7 +199,7 @@ describe MergeRequests::UpdateService, services: true do
         should_not_email(non_subscriber)
       end
 
-      context 'when issue has the `label` label' do
+      context 'when the merge request has the `label` label' do
         before { merge_request.labels << label }
 
         it 'does not send notifications for existing labels' do
@@ -226,6 +226,58 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    context 'when the approvers change' do
+      let(:existing_approver) { create(:user) }
+      let(:removed_approver) { create(:user) }
+      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 do
+          perform_enqueued_jobs do
+            update_merge_request(approver_ids: [new_approver, existing_approver].map(&:id).join(','))
+          end
+        end
+
+        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 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 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
+
+      context 'when the approvers are set to the same values' do
+        it 'does not create any todos' do
+          expect 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
+
     context 'when MergeRequest has tasks' do
       before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
 

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

spec/services/todo_service_spec.rb

@@ -316,6 +316,36 @@ describe TodoService, services: true do
         should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
         should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
       end
+
+      context 'when the merge request has approvers' do
+        let(:approver_1) { create(:user) }
+        let(:approver_2) { create(:user) }
+        let(:approver_3) { create(:user) }
+        let(:approver_mentions) { [john_doe, approver_1].map(&:to_reference).join(' ') }
+        let(:mr_approvers) { create(:merge_request, source_project: project, author: author, description: approver_mentions) }
+
+        before do
+          project.team << [approver_1, :developer]
+          project.team << [approver_2, :developer]
+          project.team << [approver_3, :developer]
+
+          create(:approver, user: approver_1, target: mr_approvers)
+          create(:approver, user: approver_2, target: mr_approvers)
+
+          service.new_merge_request(mr_approvers, author)
+        end
+
+        it 'creates a todo for each approver' do
+          should_create_todo(user: approver_1, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
+          should_create_todo(user: approver_2, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
+          should_not_create_todo(user: approver_3, target: mr_approvers, action: Todo::APPROVAL_REQUIRED)
+        end
+
+        it 'creates a todo for each valid mentioned user' do
+          should_create_todo(user: john_doe, target: mr_approvers, action: Todo::MENTIONED)
+          should_not_create_todo(user: approver_1, target: mr_approvers, action: Todo::MENTIONED)
+        end
+      end
     end
 
     describe '#update_merge_request' do