Create todos for new MR approvers on update

app/services/merge_requests/update_service.rb

@@ -12,8 +12,14 @@ 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
+      todo_service.add_merge_request_approvers(merge_request, new_approvers) if new_approvers.any?
+
+      merge_request
     end
 
     def handle_changes(merge_request, old_labels: [])

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,7 +175,11 @@ class TodoService
 
   def new_issuable(issuable, author)
     create_assignment_todo(issuable, author)
-    create_approval_required_todos(issuable, author) if issuable.is_a?(MergeRequest)
+
+    if issuable.is_a?(MergeRequest)
+      create_approval_required_todos(issuable, issuable.overall_approvers, author)
+    end
+
     create_mention_todos(issuable.project, issuable, author)
   end
 
@@ -207,10 +219,9 @@ class TodoService
     create_todos(mentioned_users, attributes)
   end
 
-  def create_approval_required_todos(merge_request, author)
-    approvers = merge_request.overall_approvers.map(&:user)
+  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, attributes)
+    create_todos(approvers.map(&:user), attributes)
   end
 
   def create_build_failed_todo(merge_request)

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,41 @@ 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
+        update_merge_request(approver_ids: [existing_approver, removed_approver].map(&:id).join(','))
+        Todo.where(action: Todo::APPROVAL_REQUIRED).destroy_all
+      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(',')) }
+
+        it 'adds todos for the new approvers' do
+          expect(Todo.where(user: new_approver, action: Todo::APPROVAL_REQUIRED)).not_to be_empty
+        end
+
+        it 'does not add todos for the existing approvers' do
+          expect(Todo.where(user: existing_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+        end
+
+        it 'does not add todos for the removed approvers' do
+          expect(Todo.where(user: removed_approver, action: Todo::APPROVAL_REQUIRED)).to be_empty
+        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
+      end
+    end
+
     context 'when MergeRequest has tasks' do
       before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
 

spec/services/todo_service_spec.rb

@@ -9,9 +9,6 @@ describe TodoService, services: true do
   let(:admin) { create(:admin) }
   let(:john_doe) { create(:user) }
   let(:project) { create(:project) }
-  let(:approver_1) { create(:user) }
-  let(:approver_2) { create(:user) }
-  let(:approver_3) { create(:user) }
   let(:mentions) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') }
   let(:service) { described_class.new }
 
@@ -20,9 +17,6 @@ describe TodoService, services: true do
     project.team << [author, :developer]
     project.team << [member, :developer]
     project.team << [john_doe, :developer]
-    project.team << [approver_1, :developer]
-    project.team << [approver_2, :developer]
-    project.team << [approver_3, :developer]
   end
 
   describe 'Issues' do
@@ -324,10 +318,17 @@ describe TodoService, services: true do
       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)