Create approval todos on update

Ensures that all approvers are set a new todo when the MR
is updated, which will have cleared old approvals.

changelogs/unreleased/create-approval-todos-on-update.yml

+---
+title: Create approval todos on update
+merge_request: 26077
+author:
+type: fixed

ee/app/services/ee/merge_requests/base_service.rb

@@ -35,6 +35,28 @@ module EE
 
         super
       end
+
+      def reset_approvals?(merge_request, _newrev)
+        merge_request.target_project.reset_approvals_on_push
+      end
+
+      def reset_approvals(merge_request, newrev = nil)
+        return unless reset_approvals?(merge_request, newrev)
+
+        merge_request.approvals.delete_all
+        create_new_approval_todos_for_all_approvers(merge_request)
+      end
+
+      def all_approvers(merge_request)
+        merge_request.overall_approvers(exclude_code_owners: true)
+      end
+
+      def create_new_approval_todos_for_all_approvers(merge_request)
+        return unless ::Feature.enabled?(:create_approval_todos_on_mr_update, merge_request.project, default_enabled: true)
+        return if merge_request.closed?
+
+        todo_service.add_merge_request_approvers(merge_request, all_approvers(merge_request))
+      end
     end
   end
 end

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -22,13 +22,7 @@ module EE
         merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
 
         merge_requests.each do |merge_request|
-          target_project = merge_request.target_project
-
-          if target_project.reset_approvals_on_push &&
-              merge_request.rebase_commit_sha != newrev
-
-            merge_request.approvals.delete_all
-          end
+          reset_approvals(merge_request, newrev)
         end
       end
 
@@ -54,6 +48,10 @@ module EE
           .execute(project, @push.branch_name, @push.newrev)
       end
       # rubocop:enable Gitlab/ModuleWithInstanceVariables
+
+      def reset_approvals?(merge_request, newrev)
+        super && merge_request.rebase_commit_sha != newrev
+      end
     end
   end
 end

ee/app/services/ee/merge_requests/update_service.rb

@@ -24,7 +24,7 @@ module EE
 
         return merge_request if update_task_event?
 
-        new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
+        new_approvers = all_approvers(merge_request) - old_approvers
 
         if new_approvers.any?
           todo_service.add_merge_request_approvers(merge_request, new_approvers)
@@ -46,12 +46,6 @@ module EE
 
         reset_approvals(merge_request)
       end
-
-      def reset_approvals(merge_request)
-        target_project = merge_request.target_project
-
-        merge_request.approvals.delete_all if target_project.reset_approvals_on_push
-      end
     end
   end
 end

ee/app/services/ee/todo_service.rb

@@ -16,12 +16,6 @@ module EE
     def new_issuable(issuable, author)
       if issuable.is_a?(MergeRequest)
         approvers = issuable.overall_approvers(exclude_code_owners: true)
-        issuable.project.team.max_member_access_for_user_ids(approvers.map(&:id))
-
-        approvers = approvers.select do |approver|
-          approver.can?(:approve_merge_request, issuable)
-        end
-
         create_approval_required_todos(issuable, approvers, author)
       end
 
@@ -51,6 +45,14 @@ module EE
 
     def create_approval_required_todos(merge_request, approvers, author)
       attributes = attributes_for_todo(merge_request.project, merge_request, author, ::Todo::APPROVAL_REQUIRED)
+
+      # Preload project_authorizations to prevent n+1 queries
+      merge_request.project.team.max_member_access_for_user_ids(approvers.map(&:id))
+
+      approvers = approvers.select do |approver|
+        approver.can?(:approve_merge_request, merge_request)
+      end
+
       create_todos(approvers, attributes)
     end
   end

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -66,7 +66,7 @@ describe MergeRequests::RefreshService do
       let(:service) { described_class.new(project, current_user) }
       let(:enable_code_owner) { true }
       let(:enable_report_approver_rules) { true }
-      let(:todo_service) { double(:todo_service) }
+      let(:todo_service) { double(:todo_service, add_merge_request_approvers: true) }
       let(:notification_service) { double(:notification_service) }
 
       before do

ee/spec/services/merge_requests/refresh_service_spec.rb

@@ -37,6 +37,18 @@ describe MergeRequests::RefreshService do
 
       @oldrev = @commits.last.id
       @newrev = @commits.first.id
+
+      @approver = create(:user)
+      @project.add_developer(@approver)
+
+      perform_enqueued_jobs do
+        @merge_request.update(approver_ids: [@approver].map(&:id).join(','))
+        @fork_merge_request.update(approver_ids: [@approver].map(&:id).join(','))
+      end
+    end
+
+    def approval_todos(merge_request)
+      Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
     end
 
     context 'push to origin repo source branch' do
@@ -54,6 +66,8 @@ describe MergeRequests::RefreshService do
 
         expect(@merge_request.approvals).to be_empty
         expect(@fork_merge_request.approvals).not_to be_empty
+        expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
+        expect(approval_todos(@fork_merge_request)).to be_empty
       end
     end
 
@@ -67,6 +81,8 @@ describe MergeRequests::RefreshService do
         it 'does not reset approvals' do
           expect(@merge_request.approvals).not_to be_empty
           expect(@fork_merge_request.approvals).not_to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
+          expect(approval_todos(@fork_merge_request)).to be_empty
         end
       end
     end
@@ -86,6 +102,8 @@ describe MergeRequests::RefreshService do
 
           expect(@merge_request.approvals).not_to be_empty
           expect(@fork_merge_request.approvals).to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
+          expect(approval_todos(@fork_merge_request).map(&:user)).to contain_exactly(@approver)
         end
       end
 
@@ -99,6 +117,8 @@ describe MergeRequests::RefreshService do
 
           expect(@merge_request.approvals).not_to be_empty
           expect(@fork_merge_request.approvals).to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
+          expect(approval_todos(@fork_merge_request)).to be_empty
         end
       end
     end
@@ -113,6 +133,8 @@ describe MergeRequests::RefreshService do
         it 'does not reset approvals', :sidekiq_might_not_need_inline do
           expect(@merge_request.approvals).not_to be_empty
           expect(@fork_merge_request.approvals).not_to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
+          expect(approval_todos(@fork_merge_request)).to be_empty
         end
       end
     end
@@ -127,6 +149,8 @@ describe MergeRequests::RefreshService do
       it 'does not reset approvals' do
         expect(@merge_request.approvals).not_to be_empty
         expect(@fork_merge_request.approvals).not_to be_empty
+        expect(approval_todos(@merge_request)).to be_empty
+        expect(approval_todos(@fork_merge_request)).to be_empty
       end
     end
 
@@ -142,6 +166,7 @@ describe MergeRequests::RefreshService do
 
         it 'resets approvals' do
           expect(@merge_request.approvals).to be_empty
+          expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
         end
       end
 
@@ -156,6 +181,7 @@ describe MergeRequests::RefreshService do
 
         it 'does not reset approvals' do
           expect(@merge_request.approvals).not_to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
         end
       end
 
@@ -170,6 +196,7 @@ describe MergeRequests::RefreshService do
 
         it 'does not reset approvals' do
           expect(@merge_request.approvals).not_to be_empty
+          expect(approval_todos(@merge_request)).to be_empty
         end
       end
 
@@ -185,6 +212,7 @@ describe MergeRequests::RefreshService do
 
           it 'resets the approvals' do
             expect(@merge_request.approvals).to be_empty
+            expect(approval_todos(@merge_request)).to be_empty
           end
         end
 
@@ -198,6 +226,7 @@ describe MergeRequests::RefreshService do
 
           it 'resets the approvals' do
             expect(@merge_request.approvals).to be_empty
+            expect(approval_todos(@merge_request).map(&:user)).to contain_exactly(@approver)
           end
         end
       end

ee/spec/services/merge_requests/update_service_spec.rb

@@ -172,14 +172,30 @@ describe MergeRequests::UpdateService, :mailer do
     end
 
     context 'updating target_branch' do
-      it 'resets approvals when target_branch is changed' do
+      let(:existing_approver) { create(:user) }
+      let(:new_approver) { create(:user) }
+
+      before do
+        project.add_developer(existing_approver)
+        project.add_developer(new_approver)
+
+        perform_enqueued_jobs do
+          update_merge_request(approver_ids: "#{existing_approver.id},#{new_approver.id}")
+        end
+
         merge_request.target_project.update(reset_approvals_on_push: true)
-        merge_request.approvals.create(user_id: user2.id)
+        merge_request.approvals.create(user_id: existing_approver.id)
+      end
 
+      it 'resets approvals when target_branch is changed' do
         update_merge_request(target_branch: 'video')
 
         expect(merge_request.reload.approvals).to be_empty
       end
+
+      it 'creates new todos for the approvers' do
+        expect(Todo.where(action: Todo::APPROVAL_REQUIRED).map(&:user)).to contain_exactly(new_approver, existing_approver)
+      end
     end
 
     context 'updating blocking merge requests' do