Hook Approver/ApproverGroup to migrate service

Changes to Approver/ApproverGroup are reflected to rule members.

Skip syncing code rules for approver hooks
This avoids duplicated effort as it is sync by other hooks

ee/app/models/approver.rb

@@ -4,6 +4,8 @@ class Approver < ActiveRecord::Base
   belongs_to :target, polymorphic: true  # rubocop:disable Cop/PolymorphicAssociations
   belongs_to :user
 
+  include ApproverMigrateHook
+
   validates :user, presence: true
 
   def find_by_user_id(user_id)

ee/app/models/approver_group.rb

@@ -4,6 +4,8 @@ class ApproverGroup < ActiveRecord::Base
   belongs_to :target, polymorphic: true  # rubocop:disable Cop/PolymorphicAssociations
   belongs_to :group
 
+  include ApproverMigrateHook
+
   validates :group, presence: true
 
   delegate :users, to: :group

ee/app/models/concerns/approver_migrate_hook.rb

+# frozen_string_literal: true
+#
+# Sync up old models (Approver and ApproverGroup)
+# to new models (ApprovalMergeRequestRule and ApprovalProjectRule)
+#
+# TODO: remove once #1979 is closed
+
+module ApproverMigrateHook
+  extend ActiveSupport::Concern
+
+  included do
+    after_commit :schedule_approval_migration, on: [:create, :destroy]
+  end
+
+  def schedule_approval_migration
+    # After merge, approval information is frozen
+    return if target.is_a?(MergeRequest) && target.merged?
+
+    Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform(target.class.name, target.id, sync_code_owner_rule: false)
+  end
+end

ee/lib/api/merge_request_approvals.rb

@@ -77,6 +77,8 @@ module API
           requires :approver_group_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of Group IDs to set as approvers.'
         end
         put 'approvers' do
+          Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ee/issues/8883')
+
           merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
 
           merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)

ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -94,9 +94,10 @@ module Gitlab
 
       # @param target_type [String] class of target, either 'MergeRequest' or 'Project'
       # @param target_id [Integer] id of target
-      def perform(target_type, target_id)
+      def perform(target_type, target_id, sync_code_owner_rule: true)
         @target_type = target_type
         @target_id = target_id
+        @sync_code_owner_rule = sync_code_owner_rule
 
         raise "Incorrect target_type #{target_type}" unless ALLOWED_TARGET_TYPES.include?(@target_type)
 
@@ -117,7 +118,7 @@ module Gitlab
           rule.approval_project_rule = target.target_project.approval_rules.regular.first
         end
 
-        target.sync_code_owners_with_approvers
+        target.sync_code_owners_with_approvers if @sync_code_owner_rule
       end
 
       def handle_project

ee/spec/models/concerns/approver_migrate_hook_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApproverMigrateHook do
+  def members(rule)
+    rule.users.reload + rule.groups.reload
+  end
+
+  context 'create rule and member mapping' do
+    shared_examples 'migrating approver' do
+      context 'when approver is created' do
+        it 'creates rule and member mapping' do
+          expect do
+            approver
+          end.to change { target.approval_rules.count }.by(1)
+
+          rule = target.approval_rules.regular.last
+
+          expect(rule.approvals_required).to eq(0)
+          expect(rule.name).to eq('Default')
+          expect(members(rule)).to contain_exactly(approver.member)
+        end
+
+        context 'when rule already exists' do
+          let!(:approval_rule) { target.approval_rules.create(name: 'foo') }
+
+          it 'reuses rule' do
+            expect do
+              approver
+            end.not_to change { target.approval_rules.regular.count }
+
+            rule = target.approval_rules.regular.last
+
+            expect(rule).to eq(approval_rule)
+            expect(members(rule)).to contain_exactly(approver.member)
+          end
+
+          context 'when member mapping already exists' do
+            before do
+              approval_rule.add_member approver.member
+            end
+
+            it 'does nothing' do
+              approver
+
+              expect(members(approval_rule)).to contain_exactly(approver.member)
+            end
+          end
+        end
+      end
+
+      context 'when approver is destroyed' do
+        it 'destroys rule member' do
+          approver
+
+          rule = target.approval_rules.regular.first
+
+          expect do
+            approver.destroy!
+          end.to change { members(rule).count }.by(-1)
+        end
+      end
+    end
+
+    context 'User' do
+      let(:approver) { create(:approver, target: target) }
+
+      context 'merge request' do
+        let(:target) { create(:merge_request) }
+
+        it_behaves_like 'migrating approver'
+      end
+
+      context 'project' do
+        let(:target) { create(:project) }
+
+        it_behaves_like 'migrating approver'
+      end
+    end
+
+    context 'Group' do
+      let(:approver) { create(:approver_group, target: target) }
+
+      context 'merge request' do
+        let(:target) { create(:merge_request) }
+
+        it_behaves_like 'migrating approver'
+      end
+
+      context 'project' do
+        let(:target) { create(:project) }
+
+        it_behaves_like 'migrating approver'
+      end
+    end
+  end
+end