Assign new code owner as approver for code push

Set @push early for EE

In EE, there is need to call `merge_requests_for_source_branch`
before calling CE's `refresh_merge_requests,
in order to obtain the old diffs.

However that requires `@push`,
initialized inside CE's refresh_merge_requests.

However it can't be set early in EE, or static analysis would fail
because use of instance variables in module is discouraged.
So @push is set in execute instead.

app/services/merge_requests/refresh_service.rb

@@ -2,18 +2,18 @@
 
 module MergeRequests
   class RefreshService < MergeRequests::BaseService
+    attr_reader :push
+
     def execute(oldrev, newrev, ref)
-      push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
-      return true unless push.branch_push?
+      @push = Gitlab::Git::Push.new(@project, oldrev, newrev, ref)
+      return true unless @push.branch_push?
 
-      refresh_merge_requests!(push)
+      refresh_merge_requests!
     end
 
     private
 
-    def refresh_merge_requests!(push)
-      @push = push
-
+    def refresh_merge_requests!
       Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge

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

@@ -6,8 +6,10 @@ module EE
       private
 
       override :refresh_merge_requests!
-      def refresh_merge_requests!(push)
-        super && reset_approvals_for_merge_requests(push.ref, push.newrev)
+      def refresh_merge_requests!
+        update_approvers do
+          super && reset_approvals_for_merge_requests(push.ref, push.newrev)
+        end
       end
 
       # Note: Closed merge requests also need approvals reset.
@@ -25,6 +27,53 @@ module EE
           end
         end
       end
+
+      # @return [Hash<Integer, MergeRequestDiff>] Diffs prior to code push, mapped from merge request id
+      def fetch_latest_merge_request_diffs
+        merge_requests = merge_requests_for_source_branch
+        ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord
+        merge_requests.map(&:latest_merge_request_diff)
+      end
+
+      def update_approvers
+        return yield unless project.feature_available?(:code_owners)
+
+        previous_diffs = fetch_latest_merge_request_diffs
+
+        results = yield
+
+        merge_requests = merge_requests_for_source_branch
+        ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
+
+        merge_requests.each do |merge_request|
+          previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
+          previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
+          new_code_owners = merge_request.code_owners - previous_code_owners
+
+          create_approvers(merge_request, new_code_owners)
+        end
+
+        results
+      end
+
+      def create_approvers(merge_request, users)
+        return if users.empty?
+
+        if merge_request.approvers_overwritten?
+          rows = users.map do |user|
+            {
+              target_id: merge_request.id,
+              target_type: merge_request.class.name,
+              user_id: user.id
+            }
+          end
+
+          ::Gitlab::Database.bulk_insert(Approver.table_name, rows)
+        end
+
+        todo_service.add_merge_request_approvers(merge_request, users)
+        notification_service.add_merge_request_approvers(merge_request, users, current_user)
+      end
     end
   end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::RefreshService do
+  include ProjectForksHelper
+
+  let(:group) { create(:group) }
+  let(:project) { create(:project, :repository, namespace: group, approvals_before_merge: 1, reset_approvals_on_push: true) }
+  let(:forked_project) { fork_project(project, fork_user, repository: true) }
+
+  let(:fork_user) { create(:user) }
+
+  let(:source_branch) { 'between-create-delete-modify-move' }
+  let(:merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: source_branch,
+      target_branch: 'master',
+      target_project: project)
+  end
+  let(:another_merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: source_branch,
+      target_branch: 'test',
+      target_project: project)
+  end
+  let(:forked_merge_request) do
+    create(:merge_request,
+      source_project: forked_project,
+      source_branch: source_branch,
+      target_branch: 'master',
+      target_project: project)
+  end
+  let(:oldrev) { TestEnv::BRANCH_SHA[source_branch] }
+  let(:newrev) { TestEnv::BRANCH_SHA['after-create-delete-modify-move'] } # Pretend source_branch is now updated
+
+  subject { service.execute(oldrev, newrev, "refs/heads/#{source_branch}") }
+
+  describe '#execute' do
+    context '#update_approvers' do
+      let(:owner) { create(:user) }
+      let(:current_user) { merge_request.author }
+      let(:service) { described_class.new(project, current_user) }
+      let(:enable_code_owner) { true }
+      let(:todo_service) { double(:todo_service) }
+      let(:notification_service) { double(:notification_service) }
+
+      before do
+        stub_licensed_features(code_owners: enable_code_owner)
+
+        allow(service).to receive(:mark_pending_todos_done)
+        allow(service).to receive(:notify_about_push)
+        allow(service).to receive(:execute_hooks)
+        allow(service).to receive(:todo_service).and_return(todo_service)
+        allow(service).to receive(:notification_service).and_return(notification_service)
+
+        group.add_master(fork_user)
+
+        merge_request
+        another_merge_request
+        forked_merge_request
+      end
+
+      context 'when code owners disabled' do
+        let(:enable_code_owner) { false }
+
+        it 'does nothing' do
+          expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request)
+
+          subject
+        end
+      end
+
+      context 'when code owners enabled' do
+        let(:old_owners) { [] }
+        let(:new_owners) { [] }
+        let(:relevant_merge_requests) { [merge_request, another_merge_request] }
+
+        before do
+          relevant_merge_requests.each do |merge_request|
+            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
+            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request, merge_request_diff: anything).and_wrap_original do |m, *args|
+              expect(args.last[:merge_request_diff]).to eq(merge_request.merge_request_diffs.order(id: :desc).offset(1).first)
+
+              old_owners
+            end
+          end
+
+          [forked_merge_request].each do |merge_request|
+            expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request, anything)
+          end
+        end
+
+        shared_examples 'notification and todo' do
+          it 'does nothing if owners do not change' do
+            expect(service.todo_service).not_to receive(:add_merge_request_approvers)
+            expect(service.notification_service).not_to receive(:add_merge_request_approvers)
+
+            subject
+          end
+
+          context 'when new owners are being added' do
+            let(:new_owners) { [owner] }
+
+            it 'notifies new owner' do
+              relevant_merge_requests.each do |merge_request|
+                expect(todo_service).to receive(:add_merge_request_approvers).with(merge_request, [owner])
+                expect(notification_service).to receive(:add_merge_request_approvers).with(merge_request, [owner], current_user)
+              end
+
+              subject
+            end
+          end
+
+          context 'when old owners are being removed' do
+            let(:old_owners) { [owner] }
+
+            it 'does nothing' do
+              expect(service.todo_service).not_to receive(:add_merge_request_approvers)
+              expect(service.notification_service).not_to receive(:add_merge_request_approvers)
+
+              subject
+            end
+          end
+        end
+
+        context 'merge request has overwritten approvers' do
+          include_examples 'notification and todo'
+        end
+
+        context 'merge request has default approvers' do
+          let(:existing_approver) { create(:user) }
+
+          before do
+            create(:approver, target: merge_request, user: existing_approver)
+          end
+
+          include_examples 'notification and todo'
+
+          context 'when new owners are being added' do
+            let(:new_owners) { [owner] }
+
+            it 'creates Approver' do
+              allow(service.todo_service).to receive(:add_merge_request_approvers)
+              allow(service.notification_service).to receive(:add_merge_request_approvers)
+
+              subject
+
+              expect(merge_request.approvers.first.user).to eq(existing_approver)
+              expect(merge_request.approvers.last.user).to eq(owner)
+            end
+          end
+        end
+      end
+    end
+  end
+end