From c89c4d82738d230a879da3bae62305810092aed4 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 18:58:49 +0800
Subject: [PATCH 01/25] Sync approvals_required count setting to rules

When project/MR's approvals_before_merge is changed,
update its rule's approvals_required.
---
 ee/app/services/ee/merge_requests/update_service.rb  | 10 ++++++++++
 ee/app/services/ee/projects/update_service.rb        |  9 +++++++++
 .../services/merge_requests/update_service_spec.rb   | 12 ++++++++++++
 ee/spec/services/projects/update_service_spec.rb     | 10 ++++++++++
 4 files changed, 41 insertions(+)

diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb
index a5e70e52072c00..b32f01f957f625 100644
--- a/ee/app/services/ee/merge_requests/update_service.rb
+++ b/ee/app/services/ee/merge_requests/update_service.rb
@@ -25,6 +25,8 @@ def execute(merge_request)
           cleanup_approvers(merge_request, reload: true)
         end
 
+        sync_approval_rules(merge_request)
+
         merge_request
       end
 
@@ -42,6 +44,14 @@ def reset_approvals(merge_request)
 
         merge_request.approvals.delete_all if target_project.reset_approvals_on_push
       end
+
+      # TODO remove after #1979 is closed
+      def sync_approval_rules(merge_request)
+        return if merge_request.merged?
+        return unless merge_request.previous_changes.include?(:approvals_before_merge)
+
+        merge_request.approval_rules.regular.update_all(approvals_required: merge_request.approvals_before_merge)
+      end
     end
   end
 end
diff --git a/ee/app/services/ee/projects/update_service.rb b/ee/app/services/ee/projects/update_service.rb
index d4666acec9f975..80617e43ef7ec8 100644
--- a/ee/app/services/ee/projects/update_service.rb
+++ b/ee/app/services/ee/projects/update_service.rb
@@ -37,6 +37,8 @@ def execute
 
           sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled?
           project.import_state.force_import_job! if params[:mirror].present? && project.mirror?
+
+          sync_approval_rules
         end
 
         result
@@ -67,6 +69,13 @@ def log_audit_events
       def sync_wiki_on_enable
         ::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
       end
+
+      # TODO remove after #1979 is closed
+      def sync_approval_rules
+        return unless project.previous_changes.include?(:approvals_before_merge)
+
+        project.approval_rules.update_all(approvals_required: project.approvals_before_merge)
+      end
     end
   end
 end
diff --git a/ee/spec/services/merge_requests/update_service_spec.rb b/ee/spec/services/merge_requests/update_service_spec.rb
index 5a1ac265b013aa..7902e1621a8b53 100644
--- a/ee/spec/services/merge_requests/update_service_spec.rb
+++ b/ee/spec/services/merge_requests/update_service_spec.rb
@@ -56,5 +56,17 @@ def update_merge_request(opts)
         end.not_to change { ActionMailer::Base.deliveries.count }
       end
     end
+
+    context 'when approvals_before_merge changes' do
+      it "updates approval_rules' approvals_required" do
+        rule = create(:approval_merge_request_rule, merge_request: merge_request)
+        code_owner_rule = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true)
+
+        update_merge_request(approvals_before_merge: 42)
+
+        expect(rule.reload.approvals_required).to eq(42)
+        expect(code_owner_rule.reload.approvals_required).to eq(0)
+      end
+    end
   end
 end
diff --git a/ee/spec/services/projects/update_service_spec.rb b/ee/spec/services/projects/update_service_spec.rb
index 50754cb8da9448..5c644a54b97cab 100644
--- a/ee/spec/services/projects/update_service_spec.rb
+++ b/ee/spec/services/projects/update_service_spec.rb
@@ -215,6 +215,16 @@
     end
   end
 
+  context 'with approval_rules' do
+    it "updates approval_rules' approvals_required" do
+      rule = create(:approval_project_rule, project: project)
+
+      update_project(project, user, approvals_before_merge: 42)
+
+      expect(rule.reload.approvals_required).to eq(42)
+    end
+  end
+
   def update_project(project, user, opts)
     Projects::UpdateService.new(project, user, opts).execute
   end
-- 
GitLab


From 8cabf62b6b091f214b0cb62e737c14f94e178773 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 18:59:39 +0800
Subject: [PATCH 02/25] Finalize approval state when merged

---
 .../merge_requests/post_merge_service.rb      |  2 ++
 .../ee/merge_requests/post_merge_service.rb   | 15 ++++++++++++
 .../merge_requests/post_merge_service_spec.rb | 23 +++++++++++++++++++
 3 files changed, 40 insertions(+)
 create mode 100644 ee/app/services/ee/merge_requests/post_merge_service.rb
 create mode 100644 ee/spec/services/ee/merge_requests/post_merge_service_spec.rb

diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index f26e3bee06f68c..f247349ecbd83c 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -52,3 +52,5 @@ def create_event(merge_request)
     end
   end
 end
+
+MergeRequests::PostMergeService.prepend(EE::MergeRequests::PostMergeService)
diff --git a/ee/app/services/ee/merge_requests/post_merge_service.rb b/ee/app/services/ee/merge_requests/post_merge_service.rb
new file mode 100644
index 00000000000000..3427932858bbd5
--- /dev/null
+++ b/ee/app/services/ee/merge_requests/post_merge_service.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module PostMergeService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(merge_request)
+        super
+        ApprovalRules::FinalizeService.new(merge_request).execute
+      end
+    end
+  end
+end
diff --git a/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb
new file mode 100644
index 00000000000000..1b47226d8e93ce
--- /dev/null
+++ b/ee/spec/services/ee/merge_requests/post_merge_service_spec.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::PostMergeService do
+  let(:project) { merge_request.target_project }
+  let(:merge_request) { create(:merge_request) }
+  let(:current_user) { merge_request.author }
+  let(:service) { described_class.new(project, current_user) }
+
+  describe '#execute' do
+    context 'finalize approvals' do
+      let(:finalize_service) { double(:finalize_service) }
+
+      it 'executes ApprovalRules::FinalizeService' do
+        expect(ApprovalRules::FinalizeService).to receive(:new).and_return(finalize_service)
+        expect(finalize_service).to receive(:execute)
+
+        service.execute(merge_request)
+      end
+    end
+  end
+end
-- 
GitLab


From 6f6f91d1ffe35045378fc466d852bd64522243a4 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 19:00:30 +0800
Subject: [PATCH 03/25] Sync code owner changes to approval rules

Update code owner rule when MR is updated.
---
 app/services/merge_requests/create_service.rb |  2 +
 ee/app/models/approval_merge_request_rule.rb  |  2 +
 ee/app/models/ee/merge_request.rb             |  9 ++++
 .../ee/merge_requests/create_service.rb       | 16 +++++++
 .../ee/merge_requests/refresh_service.rb      |  2 +
 ee/spec/models/merge_request_spec.rb          | 35 +++++++++++++++
 .../ee/merge_requests/create_service_spec.rb  | 43 +++++++++++++++++++
 .../ee/merge_requests/refresh_service_spec.rb |  6 +++
 8 files changed, 115 insertions(+)
 create mode 100644 ee/app/services/ee/merge_requests/create_service.rb
 create mode 100644 ee/spec/services/ee/merge_requests/create_service_spec.rb

diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 02c2388c05cf64..410351520301ce 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -66,3 +66,5 @@ def set_projects!
     end
   end
 end
+
+MergeRequests::CreateService.include(EE::MergeRequests::CreateService)
diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index ec9e69060dc3d6..6dbc1bb45ce4e7 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -3,6 +3,8 @@
 class ApprovalMergeRequestRule < ApplicationRecord
   include ApprovalRuleLike
 
+  DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
+
   scope :regular, -> { where(code_owner: false) }
   scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes
 
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 18ff8260629d4e..238fea3a96be40 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -81,5 +81,14 @@ def compare_license_management_reports
 
       compare_reports(::Ci::CompareLicenseManagementReportsService)
     end
+
+    def sync_code_owners_with_approvers
+      ActiveRecord::Base.transaction do
+        rule = approval_rules.code_owner.first
+        rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
+
+        rule.users = code_owners
+      end
+    end
   end
 end
diff --git a/ee/app/services/ee/merge_requests/create_service.rb b/ee/app/services/ee/merge_requests/create_service.rb
new file mode 100644
index 00000000000000..bc0ca4b3bd8485
--- /dev/null
+++ b/ee/app/services/ee/merge_requests/create_service.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    module CreateService
+      extend ::Gitlab::Utils::Override
+
+      override :after_create
+      def after_create(issuable)
+        super
+
+        issuable.sync_code_owners_with_approvers
+      end
+    end
+  end
+end
diff --git a/ee/app/services/ee/merge_requests/refresh_service.rb b/ee/app/services/ee/merge_requests/refresh_service.rb
index b647c27330fdc2..25c6af35585cfd 100644
--- a/ee/app/services/ee/merge_requests/refresh_service.rb
+++ b/ee/app/services/ee/merge_requests/refresh_service.rb
@@ -53,6 +53,8 @@ def update_approvers
           new_code_owners = merge_request.code_owners - previous_code_owners
 
           create_approvers(merge_request, new_code_owners)
+
+          merge_request.sync_code_owners_with_approvers
         end
 
         results
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index 309ac585fd0d12..647eec9e24307e 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -70,6 +70,41 @@
     end
   end
 
+  describe '#sync_code_owners_with_approvers' do
+    let(:owners) { [create(:user), create(:user)] }
+
+    before do
+      allow(subject).to receive(:code_owners).and_return(owners)
+    end
+
+    it 'sync code owner to the code owner rule' do
+      expect do
+        subject.sync_code_owners_with_approvers
+      end.to change { subject.approval_rules.count }.by(1)
+
+      expect(subject.approval_rules.code_owner.first.users).to contain_exactly(*owners)
+    end
+
+    context 'when code owner rule already exists' do
+      let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner') }
+
+      before do
+        code_owner_rule.users << create(:user)
+      end
+
+      it 'reuses existing rule' do
+        expect do
+          subject.sync_code_owners_with_approvers
+        end.not_to change { subject.approval_rules.count }
+
+        rule = subject.approval_rules.code_owner.first
+
+        expect(rule).to eq(code_owner_rule)
+        expect(rule.users).to contain_exactly(*owners)
+      end
+    end
+  end
+
   describe '#base_pipeline' do
     let!(:pipeline) { create(:ci_empty_pipeline, project: subject.project, sha: subject.diff_base_sha) }
 
diff --git a/ee/spec/services/ee/merge_requests/create_service_spec.rb b/ee/spec/services/ee/merge_requests/create_service_spec.rb
new file mode 100644
index 00000000000000..132c8beef0f5c5
--- /dev/null
+++ b/ee/spec/services/ee/merge_requests/create_service_spec.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::CreateService do
+  include ProjectForksHelper
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user) }
+  let(:service) { described_class.new(project, user, opts) }
+  let(:opts) do
+    {
+      title: 'Awesome merge_request',
+      description: 'please fix',
+      source_branch: 'feature',
+      target_branch: 'master',
+      force_remove_source_branch: '1'
+    }
+  end
+
+  before do
+    project.add_maintainer(user)
+    allow(service).to receive(:execute_hooks)
+  end
+
+  describe '#execute' do
+    context 'code owners' do
+      let!(:owners) { create_list(:user, 2) }
+
+      before do
+        allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners)
+      end
+
+      it 'syncs code owner to approval rule' do
+        merge_request = service.execute
+
+        rule = merge_request.approval_rules.code_owner.first
+
+        expect(rule.users).to contain_exactly(*owners)
+      end
+    end
+  end
+end
diff --git a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb
index 64da1f747127a6..1b167a94b01835 100644
--- a/ee/spec/services/ee/merge_requests/refresh_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/refresh_service_spec.rb
@@ -102,6 +102,9 @@
 
             it 'does not create Approver' do
               expect { subject }.not_to change { Approver.count }
+
+              rule = merge_request.approval_rules.code_owner.first
+              expect(rule.users).to eq(new_owners)
             end
           end
         end
@@ -125,6 +128,9 @@
               expect(new_approver.user).to eq(owner)
               expect(new_approver.created_at).to be_present
               expect(new_approver.updated_at).to be_present
+
+              rule = merge_request.approval_rules.code_owner.first
+              expect(rule.users).to eq(new_owners)
             end
           end
         end
-- 
GitLab


From 2714d05a208853716fc52f6cef92d63cf3577673 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 19:03:41 +0800
Subject: [PATCH 04/25] Add background task to migrate projects and MR

Projects are migrated first, then its MRs are migrates,
to ensure approver overriding works.
---
 ...0181204040404_migrate_project_approvers.rb |  24 ++
 .../migrate_approver_to_approval_rules.rb     | 222 +++++++++++++++++
 ...ate_approver_to_approval_rules_in_batch.rb |  13 +
 ...migrate_approver_to_approval_rules_spec.rb | 234 ++++++++++++++++++
 .../migrate_project_approvers_spec.rb         |  40 +++
 5 files changed, 533 insertions(+)
 create mode 100644 ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
 create mode 100644 ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
 create mode 100644 ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
 create mode 100644 ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
 create mode 100644 ee/spec/migrations/migrate_project_approvers_spec.rb

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
new file mode 100644
index 00000000000000..3dd18368060a34
--- /dev/null
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class MigrateProjectApprovers < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 1000
+
+  class Project < ActiveRecord::Base
+    include ::EachBatch
+    self.table_name = 'projects'
+  end
+
+  def up
+    jobs = []
+    Project.each_batch(of: BATCH_SIZE) do |scope, _|
+      jobs << ['MigrateApproverToApprovalRulesInBatch', ['Project', scope.pluck(:id)]]
+    end
+    BackgroundMigration.bulk_perform_async(jobs)
+  end
+
+  def down
+  end
+end
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
new file mode 100644
index 00000000000000..4faad150cab39d
--- /dev/null
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -0,0 +1,222 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # A Project/MergeRequest level migration, aiming to convert existing data
+    # (from approvers, approver_groups and approvals tables)
+    # to new rule based schema.
+    class MigrateApproverToApprovalRules
+      include Gitlab::Utils::StrongMemoize
+
+      class Approver < ActiveRecord::Base
+        self.table_name = 'approvers'
+      end
+
+      class ApproverGroup < ActiveRecord::Base
+        self.table_name = 'approver_groups'
+      end
+
+      class ApprovalMergeRequestRule < ApplicationRecord
+        self.table_name = 'approval_merge_request_rules'
+
+        include Gitlab::Utils::StrongMemoize
+
+        belongs_to :merge_request
+        scope :code_owner, -> { where(code_owner: true) }
+        scope :regular, -> { where(code_owner: false) } # Non code owner rule
+
+        has_and_belongs_to_many :users
+        has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_merge_request_rules_groups
+        has_and_belongs_to_many :approvals # This is only populated after merge request is merged
+        has_many :approved_approvers, through: :approvals, source: :user
+
+        def project
+          merge_request.target_project
+        end
+
+        # Users who are eligible to approve, including specified group members.
+        # Excludes the author if 'self-approval' isn't explicitly
+        # enabled on project settings.
+        # @return [Array<User>]
+        def approvers
+          strong_memoize(:approvers) do
+            scope = User.from_union(
+              [
+                users,
+                User.joins(:group_members).where(members: { source_id: groups })
+              ]
+            )
+
+            if merge_request.author && !project.merge_requests_author_approval?
+              scope = scope.where.not(id: merge_request.author)
+            end
+
+            scope
+          end
+        end
+
+        def sync_approvals
+          # Before being merged, approvals are dynamically calculated instead of being persisted in the db.
+          return unless merge_request.merged?
+
+          self.approvals = merge_request.approvals.where(user_id: approvers.map(&:id))
+        end
+      end
+
+      class ApprovalProjectRule < ActiveRecord::Base
+        self.table_name = 'approval_project_rules'
+
+        belongs_to :project
+        has_and_belongs_to_many :users
+        has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_project_rules_groups
+
+        scope :regular, -> { all }
+      end
+
+      class MergeRequest < ActiveRecord::Base
+        self.table_name = 'merge_requests'
+        include ::EachBatch
+
+        belongs_to :target_project, class_name: "Project"
+        belongs_to :author, class_name: "User"
+        has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
+        has_many :approvals
+
+        def approvers
+          Approver.where(target_type: 'MergeRequest', target_id: id)
+        end
+
+        def approver_groups
+          ApproverGroup.where(target_type: 'MergeRequest', target_id: id)
+        end
+
+        def merged?
+          state == 'merged'
+        end
+
+        def finalize_approvals
+          return unless merged?
+
+          copy_project_approval_rules unless approval_rules.regular.exists?
+
+          approval_rules.reload.each(&:sync_approvals)
+        end
+
+        private
+
+        def copy_project_approval_rules
+          target_project.approval_rules.each do |project_rule|
+            rule = approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
+            rule.users = project_rule.users
+            rule.groups = project_rule.groups
+          end
+        end
+      end
+
+      class Project < ActiveRecord::Base
+        self.table_name = 'projects'
+
+        has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project
+        has_many :approval_rules, class_name: 'ApprovalProjectRule'
+
+        def approvers
+          Approver.where(target_type: 'Project', target_id: id)
+        end
+
+        def approver_groups
+          ApproverGroup.where(target_type: 'Project', target_id: id)
+        end
+      end
+
+      class User < ActiveRecord::Base
+        include FromUnion
+
+        self.table_name = 'users'
+
+        has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
+      end
+
+      ALLOWED_TARGET_TYPES = %w{MergeRequest Project}.freeze
+
+      # @param target_type [String] class of target, either 'MergeRequest' or 'Project'
+      # @param target_id [Integer] id of target
+      def perform(target_type, target_id)
+        @target_type = target_type
+        @target_id = target_id
+
+        raise "Incorrect target_type #{target_type}" unless ALLOWED_TARGET_TYPES.include?(@target_type)
+
+        case target
+        when MergeRequest
+          handle_merge_request
+        when Project
+          handle_project
+        end
+      end
+
+      private
+
+      def handle_merge_request
+        ActiveRecord::Base.transaction do
+          sync_rule
+          target.finalize_approvals if target.merged?
+        end
+      end
+
+      def handle_project
+        ActiveRecord::Base.transaction do
+          sync_rule
+        end
+
+        schedule_to_migrate_merge_requests(target)
+      end
+
+      # Sync users and groups on rule
+      def sync_rule
+        unless approvers_exists?
+          target.approval_rules.regular.delete_all
+          return
+        end
+
+        rule = find_or_create_rule
+
+        rule.user_ids = target.approvers.pluck(:user_id)
+        rule.group_ids = target.approver_groups.pluck(:group_id)
+      end
+
+      def schedule_to_migrate_merge_requests(project)
+        jobs = []
+        project.merge_requests.each_batch do |scope, _|
+          jobs << ['MigrateApproverToApprovalRulesInBatch', ['MergeRequest', scope.pluck(:id)]]
+        end
+        BackgroundMigrationWorker.bulk_perform_async(jobs)
+      end
+
+      def target
+        strong_memoize(:target) do
+          case @target_type
+          when 'MergeRequest'
+            MergeRequest.find_by(id: @target_id)
+          when 'Project'
+            Project.find_by(id: @target_id)
+          end
+        end
+      end
+
+      def find_or_create_rule
+        rule = target.approval_rules.find_or_initialize_by(name: 'Default')
+
+        unless rule.persisted?
+          rule.approvals_required = target.approvals_before_merge || 0
+          rule.save!
+        end
+
+        rule
+      end
+
+      def approvers_exists?
+        target.approvers.to_a.any? || target.approver_groups.to_a.any?
+      end
+    end
+  end
+end
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
new file mode 100644
index 00000000000000..cf776af7ec0224
--- /dev/null
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class MigrateApproverToApprovalRulesInBatch
+      def perform(target_type, target_ids)
+        target_ids.each do |target_id|
+          MigrateApproverToApprovalRules.new.perform(target_type, target_id)
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
new file mode 100644
index 00000000000000..29c0aba2fb9bcc
--- /dev/null
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -0,0 +1,234 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
+  def create_skip_sync(*args)
+    build(*args) do |record|
+      allow(record).to receive(:schedule_approval_migration)
+      record.save!
+    end
+  end
+
+  def create_member_in(member, *populate_in)
+    if populate_in.include?(:old_schema)
+      case member
+      when User
+        create_skip_sync(:approver, target_type: target_type, target_id: target.id, user_id: member.id)
+      when Group
+        create_skip_sync(:approver_group, target_type: target_type, target_id: target.id, group_id: member.id)
+      end
+    end
+
+    if populate_in.include?(:new_schema)
+      approval_rule.add_member(member)
+    end
+  end
+
+  context 'sync approval rule and its members' do
+    shared_examples 'sync approval member' do
+      let(:user1) { create(:user) }
+      let(:user2) { create(:user) }
+      let(:group1) { create(:group) }
+      let(:group2) { create(:group) }
+
+      context 'when member in old schema but not in new schema' do
+        before do
+          create_member_in(user1, :old_schema)
+          create_member_in(group1, :old_schema)
+        end
+
+        it 'creates in new schema' do
+          expect do
+            described_class.new.perform(target_type, target.id)
+          end.to change { approval_rule.users.count }.by(1)
+            .and change { approval_rule.groups.count }.by(1)
+
+          approval_rule = target.approval_rules.first
+
+          expect(approval_rule.approvals_required).to eq(0)
+          expect(approval_rule.name).to eq('Default')
+
+          expect(approval_rule.users).to contain_exactly(user1)
+          expect(approval_rule.groups).to contain_exactly(group1)
+        end
+      end
+
+      context 'when member not in old schema but in new schema' do
+        before do
+          create_member_in(user1, :new_schema)
+          create_member_in(user2, :old_schema, :new_schema)
+          create_member_in(group1, :new_schema)
+          create_member_in(group2, :old_schema, :new_schema)
+        end
+
+        it 'removes in new schema' do
+          expect do
+            described_class.new.perform(target_type, target.id)
+          end.to change { approval_rule.users.count }.by(-1)
+            .and change { approval_rule.groups.count }.by(-1)
+
+          approval_rule = target.approval_rules.first
+
+          expect(approval_rule.users).to contain_exactly(user2)
+          expect(approval_rule.groups).to contain_exactly(group2)
+        end
+      end
+    end
+
+    context 'merge request' do
+      let(:target) { create(:merge_request) }
+      let(:target_type) { 'MergeRequest' }
+      let(:approval_rule) { create(:approval_merge_request_rule, merge_request: target) }
+
+      it_behaves_like 'sync approval member'
+
+      context 'when approver is no longer overwritten' do
+        before do
+          create_member_in(create(:user), :new_schema)
+          create_member_in(create(:group), :new_schema)
+        end
+
+        it 'removes rule' do
+          expect do
+            described_class.new.perform(target_type, target.id)
+          end.to change { approval_rule.users.count }.by(-1)
+            .and change { approval_rule.groups.count }.by(-1)
+
+          expect(target.approval_rules.exists?(approval_rule.id)).to eq(false)
+        end
+      end
+    end
+
+    context 'project' do
+      let(:target) { create(:project) }
+      let(:target_type) { 'Project' }
+      let(:approval_rule) { create(:approval_project_rule, project: target) }
+
+      it_behaves_like 'sync approval member'
+
+      context 'when project contains some merge requests' do
+        let!(:merge_request) { create(:merge_request, source_project: target, target_project: target) }
+
+        it 'schedules migrations for all its merge requests' do
+          expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([['MigrateApproverToApprovalRulesInBatch', ["MergeRequest", [merge_request.id]]]])
+
+          described_class.new.perform(target.class.name, target.id)
+        end
+      end
+    end
+  end
+
+  # Copied and modified from merge_request_spec.rb
+  describe '#finalize_approvals' do
+    let(:project) { create(:project, :repository) }
+    subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+    let(:target) { merge_request }
+
+    let!(:member1) { create(:user) }
+    let!(:member2) { create(:user) }
+    let!(:member3) { create(:user) }
+    let!(:group1) { create(:group) }
+    let!(:group2) { create(:group) }
+    let!(:group1_member) { create(:user) }
+    let!(:group2_member) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: subject, user: member1) }
+    let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
+    let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
+    let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
+
+    before do
+      group1.add_guest(group1_member)
+      group2.add_guest(group2_member)
+
+      rule = create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12)
+
+      rule.users = [member1, member2]
+      rule.groups << group1
+    end
+
+    context 'when without MR rules (project rule not overwritten)' do
+      it 'does nothing if unmerged' do
+        expect do
+          described_class.new.perform(target.class.name, target.id)
+        end.not_to change { ApprovalMergeRequestRule.count }
+
+        expect(approval1.approval_rules).to be_empty
+        expect(approval2.approval_rules).to be_empty
+        expect(approval3.approval_rules).to be_empty
+        expect(approval4.approval_rules).to be_empty
+      end
+
+      context 'when merged' do
+        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        it 'copies project rules to MR' do
+          expect do
+            described_class.new.perform(target.class.name, target.id)
+          end.to change { ApprovalMergeRequestRule.count }.by(1)
+
+          rule = subject.approval_rules.first
+
+          expect(rule.name).to eq('foo')
+          expect(rule.approvals_required).to eq(12)
+
+          expect(rule.users).to contain_exactly(member1, member2)
+          expect(rule.groups).to contain_exactly(group1)
+
+          rule = subject.approval_rules.first
+
+          expect(approval1.approval_rules).to contain_exactly(rule)
+          expect(approval2.approval_rules).to be_empty
+          expect(approval3.approval_rules).to contain_exactly(rule)
+          expect(approval4.approval_rules).to be_empty
+        end
+      end
+    end
+
+    context 'when with MR approver exists (project rule overwritten)' do
+      before do
+        create_skip_sync(:approver, target: subject, user: member2)
+        create_skip_sync(:approver, target: subject, user: member3)
+        create_skip_sync(:approver_group, target: subject, group: group2)
+
+        merge_request.update(approvals_before_merge: 32)
+      end
+
+      it 'does not call finalize_approvals if unmerged' do
+        expect do
+          described_class.new.perform(target.class.name, target.id)
+        end.to change { ApprovalMergeRequestRule.count }.by(1)
+
+        expect(approval1.approval_rules).to be_empty
+        expect(approval2.approval_rules).to be_empty
+        expect(approval3.approval_rules).to be_empty
+        expect(approval4.approval_rules).to be_empty
+      end
+
+      context 'when merged' do
+        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        it 'does not copy project rules, and updates approval mapping with MR rules' do
+          expect(subject).not_to receive(:copy_project_approval_rules)
+
+          expect do
+            described_class.new.perform(target.class.name, target.id)
+          end.to change { ApprovalMergeRequestRule.count }.by(1)
+
+          rule = subject.approval_rules.first
+
+          expect(rule.name).to eq('Default')
+          expect(rule.approvals_required).to eq(32)
+
+          expect(rule.users).to contain_exactly(member2, member3)
+          expect(rule.groups).to contain_exactly(group2)
+
+          expect(approval1.approval_rules).to be_empty
+          expect(approval2.approval_rules).to contain_exactly(rule)
+          expect(approval3.approval_rules).to be_empty
+          expect(approval4.approval_rules).to contain_exactly(rule)
+        end
+      end
+    end
+  end
+end
diff --git a/ee/spec/migrations/migrate_project_approvers_spec.rb b/ee/spec/migrations/migrate_project_approvers_spec.rb
new file mode 100644
index 00000000000000..6b7b02154ce815
--- /dev/null
+++ b/ee/spec/migrations/migrate_project_approvers_spec.rb
@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'post_migrate', '20181204040404_migrate_project_approvers.rb')
+
+describe MigrateProjectApprovers, :migration do
+  let(:migration) { described_class.new }
+
+  describe '#up' do
+    let(:projects) { table(:projects) }
+    let(:namespaces) { table(:namespaces) }
+    let(:approvers) { table(:approvers) }
+    let(:approval_project_rules) { table(:approval_project_rules) }
+    let(:approval_project_rules_users) { table(:approval_project_rules_users) }
+    let(:users) { table(:users) }
+
+    before do
+      namespaces.create(id: 1, name: 'gitlab-org', path: 'gitlab-org')
+      projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1', namespace_id: 1)
+      projects.create!(id: 124, name: 'gitlab2', path: 'gitlab2', namespace_id: 1)
+
+      users.create(id: 1, name: 'user1', email: 'user1@example.com', projects_limit: 0)
+      users.create(id: 2, name: 'user2', email: 'user2@example.com', projects_limit: 0)
+
+      approvers.create!(target_id: 123, target_type: 'Project', user_id: 1)
+      approvers.create!(target_id: 124, target_type: 'Project', user_id: 2)
+    end
+
+    it 'creates approval rules and its associations' do
+      migrate!
+
+      expect(approval_project_rules.pluck(:project_id)).to eq([123, 124])
+
+      rule_ids = approval_project_rules.pluck(:id)
+
+      expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.first).pluck(:user_id)).to contain_exactly(1)
+      expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.last).pluck(:user_id)).to contain_exactly(2)
+    end
+  end
+end
-- 
GitLab


From 840cb6442791e5e3a1f950ae868790a491d42594 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Fri, 14 Dec 2018 16:18:27 +0800
Subject: [PATCH 05/25] Add migration progress checker

---
 ...0181204040404_migrate_project_approvers.rb |  5 ++-
 ...prover_to_approval_rules_check_progress.rb | 41 +++++++++++++++++++
 .../migrate_project_approvers_spec.rb         |  2 +
 3 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
index 3dd18368060a34..3daef03d682009 100644
--- a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -16,7 +16,10 @@ def up
     Project.each_batch(of: BATCH_SIZE) do |scope, _|
       jobs << ['MigrateApproverToApprovalRulesInBatch', ['Project', scope.pluck(:id)]]
     end
-    BackgroundMigration.bulk_perform_async(jobs)
+    BackgroundMigrationWorker.bulk_perform_async(jobs)
+
+    check_time = Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress::RESCHEDULE_DELAY
+    BackgroundMigrationWorker.bulk_perform_in(check_time, [['MigrateApproverToApprovalRulesCheckProgress']])
   end
 
   def down
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb
new file mode 100644
index 00000000000000..0d4e6f79cbe6c6
--- /dev/null
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    class MigrateApproverToApprovalRulesCheckProgress
+      RESCHEDULE_DELAY = 1.day
+
+      def perform
+        if remaining('MergeRequest') == 0 && remaining('Project') == 0
+          Feature.enable(:approval_rule)
+        else
+          BackgroundMigrationWorker.perform_in(RESCHEDULE_DELAY, self.class.name)
+        end
+      end
+
+      private
+
+      def remaining(class_name)
+        target_type = ActiveRecord::Base.connection.quote(class_name)
+
+        sql_old_schema = <<-SQL.strip_heredoc
+          SELECT count(*) FROM (
+            SELECT target_id FROM "approvers" WHERE "approvers"."target_type" = #{target_type}
+            UNION
+            SELECT target_id FROM "approver_groups" WHERE "approver_groups"."target_type" = #{target_type}
+          ) AS target_count
+        SQL
+
+        sql_new_schema = <<-SQL.strip_heredoc
+          SELECT count(distinct #{class_name.foreign_key}) from approval_#{class_name.underscore}_rules
+        SQL
+
+        count(sql_old_schema) - count(sql_new_schema)
+      end
+
+      def count(sql)
+        ActiveRecord::Base.connection.exec_query(sql).first['count']
+      end
+    end
+  end
+end
diff --git a/ee/spec/migrations/migrate_project_approvers_spec.rb b/ee/spec/migrations/migrate_project_approvers_spec.rb
index 6b7b02154ce815..d624019dab87e4 100644
--- a/ee/spec/migrations/migrate_project_approvers_spec.rb
+++ b/ee/spec/migrations/migrate_project_approvers_spec.rb
@@ -13,6 +13,7 @@
     let(:approval_project_rules) { table(:approval_project_rules) }
     let(:approval_project_rules_users) { table(:approval_project_rules_users) }
     let(:users) { table(:users) }
+    let(:features) { table(:features) }
 
     before do
       namespaces.create(id: 1, name: 'gitlab-org', path: 'gitlab-org')
@@ -35,6 +36,7 @@
 
       expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.first).pluck(:user_id)).to contain_exactly(1)
       expect(approval_project_rules_users.where(approval_project_rule_id: rule_ids.last).pluck(:user_id)).to contain_exactly(2)
+      expect(features.where(key: 'approval_rule').exists?).to eq(true)
     end
   end
 end
-- 
GitLab


From 375bf7d9d58d00a9130f5b9eea3f2c11db08bf45 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Tue, 18 Dec 2018 15:37:41 +0800
Subject: [PATCH 06/25] Set MR rule to point to project MR

---
 .../migrate_approver_to_approval_rules.rb     | 18 +++++++++++----
 ...pprover_to_approval_rules_in_batch_spec.rb | 17 ++++++++++++++
 ...migrate_approver_to_approval_rules_spec.rb | 22 +++++++++++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 4faad150cab39d..8884371fa1bfab 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -29,6 +29,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
         has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_merge_request_rules_groups
         has_and_belongs_to_many :approvals # This is only populated after merge request is merged
         has_many :approved_approvers, through: :approvals, source: :user
+        has_one :approval_merge_request_rule_source
+        has_one :approval_project_rule, through: :approval_merge_request_rule_source
 
         def project
           merge_request.target_project
@@ -63,6 +65,12 @@ def sync_approvals
         end
       end
 
+      class ApprovalMergeRequestRuleSource < ApplicationRecord
+        self.table_name = 'approval_merge_request_rule_sources'
+        belongs_to :approval_merge_request_rule
+        belongs_to :approval_project_rule
+      end
+
       class ApprovalProjectRule < ActiveRecord::Base
         self.table_name = 'approval_project_rules'
 
@@ -158,7 +166,10 @@ def perform(target_type, target_id)
 
       def handle_merge_request
         ActiveRecord::Base.transaction do
-          sync_rule
+          if rule = sync_rule
+            rule.approval_project_rule = target.target_project.approval_rules.regular.first
+          end
+
           target.finalize_approvals if target.merged?
         end
       end
@@ -171,7 +182,6 @@ def handle_project
         schedule_to_migrate_merge_requests(target)
       end
 
-      # Sync users and groups on rule
       def sync_rule
         unless approvers_exists?
           target.approval_rules.regular.delete_all
@@ -179,9 +189,9 @@ def sync_rule
         end
 
         rule = find_or_create_rule
-
         rule.user_ids = target.approvers.pluck(:user_id)
         rule.group_ids = target.approver_groups.pluck(:group_id)
+        rule
       end
 
       def schedule_to_migrate_merge_requests(project)
@@ -204,7 +214,7 @@ def target
       end
 
       def find_or_create_rule
-        rule = target.approval_rules.find_or_initialize_by(name: 'Default')
+        rule = target.approval_rules.regular.find_or_initialize_by(name: 'Default')
 
         unless rule.persisted?
           rule.approvals_required = target.approvals_before_merge || 0
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
new file mode 100644
index 00000000000000..af97a757e4cd2b
--- /dev/null
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesInBatch do
+  context 'when there is no more MigrateApproverToApprovalRules jobs' do
+    let(:job) { double(:job) }
+
+    it 'enables feature' do
+      allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(job)
+
+      expect(job).to receive(:perform).exactly(3).times
+
+      described_class.new.perform('Foo', [1, 2, 3])
+    end
+  end
+end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index 29c0aba2fb9bcc..62749f70b04cf7 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -83,6 +83,28 @@ def create_member_in(member, *populate_in)
 
       it_behaves_like 'sync approval member'
 
+      context 'when project rule is present' do
+        let!(:project_rule) { create(:approval_project_rule, project: target.target_project) }
+
+        it "sets MR rule's source to project rule" do
+          create_member_in(create(:user), :old_schema)
+
+          described_class.new.perform(target_type, target.id)
+
+          expect(target.approval_rules.regular.first.approval_project_rule).to eq(project_rule)
+        end
+      end
+
+      context 'when project rule is absent' do
+        it "has MR rule's source as nil" do
+          create_member_in(create(:user), :old_schema)
+
+          described_class.new.perform(target_type, target.id)
+
+          expect(target.approval_rules.regular.first.approval_project_rule).to eq(nil)
+        end
+      end
+
       context 'when approver is no longer overwritten' do
         before do
           create_member_in(create(:user), :new_schema)
-- 
GitLab


From d34584630de184b8bb601a9f85f09bdcacdc85c6 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 19 Dec 2018 16:34:34 +0800
Subject: [PATCH 07/25] Check if specific type of background migration are done

Useful for checking progress.
---
 lib/gitlab/background_migration.rb           | 13 ++++++
 spec/lib/gitlab/background_migration_spec.rb | 44 ++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/lib/gitlab/background_migration.rb b/lib/gitlab/background_migration.rb
index d72befce571880..6cf40e2d4ca4a3 100644
--- a/lib/gitlab/background_migration.rb
+++ b/lib/gitlab/background_migration.rb
@@ -51,6 +51,19 @@ def self.perform(class_name, arguments)
       migration_class_for(class_name).new.perform(*arguments)
     end
 
+    def self.exists?(migration_class)
+      enqueued = Sidekiq::Queue.new(self.queue)
+      scheduled = Sidekiq::ScheduledSet.new
+
+      [enqueued, scheduled].each do |queue|
+        queue.each do |job|
+          return true if job.queue == self.queue && job.args.first == migration_class
+        end
+      end
+
+      false
+    end
+
     def self.migration_class_for(class_name)
       const_get(class_name)
     end
diff --git a/spec/lib/gitlab/background_migration_spec.rb b/spec/lib/gitlab/background_migration_spec.rb
index 4ad69aeba434fb..8a83b76fd945a6 100644
--- a/spec/lib/gitlab/background_migration_spec.rb
+++ b/spec/lib/gitlab/background_migration_spec.rb
@@ -119,4 +119,48 @@
       described_class.perform('Foo', [10, 20])
     end
   end
+
+  describe '.exists?' do
+    context 'when there are enqueued jobs present' do
+      let(:queue) do
+        [double(args: ['Foo', [10, 20]], queue: described_class.queue)]
+      end
+
+      before do
+        allow(Sidekiq::Queue).to receive(:new)
+          .with(described_class.queue)
+          .and_return(queue)
+      end
+
+      it 'returns true if specific job exists' do
+        expect(described_class.exists?('Foo')).to eq(true)
+      end
+
+      it 'returns false if specific job does not exist' do
+        expect(described_class.exists?('Bar')).to eq(false)
+      end
+    end
+
+    context 'when there are scheduled jobs present', :sidekiq, :redis do
+      before do
+        Sidekiq::Testing.disable! do
+          BackgroundMigrationWorker.perform_in(10.minutes, 'Foo')
+
+          expect(Sidekiq::ScheduledSet.new).to be_one
+        end
+      end
+
+      after do
+        Sidekiq::ScheduledSet.new.clear
+      end
+
+      it 'returns true if specific job exists' do
+        expect(described_class.exists?('Foo')).to eq(true)
+      end
+
+      it 'returns false if specific job does not exist' do
+        expect(described_class.exists?('Bar')).to eq(false)
+      end
+    end
+  end
 end
-- 
GitLab


From fc73f55162c898dd590c2d895c94cceb79797a32 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 19 Dec 2018 16:40:33 +0800
Subject: [PATCH 08/25] Update progress checker

Check presence of type of jobs in queue
---
 ...prover_to_approval_rules_check_progress.rb | 28 ++++---------------
 ...r_to_approval_rules_check_progress_spec.rb | 25 +++++++++++++++++
 2 files changed, 30 insertions(+), 23 deletions(-)
 create mode 100644 ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb
index 0d4e6f79cbe6c6..a5b45c467b3e41 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress.rb
@@ -6,35 +6,17 @@ class MigrateApproverToApprovalRulesCheckProgress
       RESCHEDULE_DELAY = 1.day
 
       def perform
-        if remaining('MergeRequest') == 0 && remaining('Project') == 0
-          Feature.enable(:approval_rule)
-        else
+        if remaining?
           BackgroundMigrationWorker.perform_in(RESCHEDULE_DELAY, self.class.name)
+        else
+          Feature.enable(:approval_rule)
         end
       end
 
       private
 
-      def remaining(class_name)
-        target_type = ActiveRecord::Base.connection.quote(class_name)
-
-        sql_old_schema = <<-SQL.strip_heredoc
-          SELECT count(*) FROM (
-            SELECT target_id FROM "approvers" WHERE "approvers"."target_type" = #{target_type}
-            UNION
-            SELECT target_id FROM "approver_groups" WHERE "approver_groups"."target_type" = #{target_type}
-          ) AS target_count
-        SQL
-
-        sql_new_schema = <<-SQL.strip_heredoc
-          SELECT count(distinct #{class_name.foreign_key}) from approval_#{class_name.underscore}_rules
-        SQL
-
-        count(sql_old_schema) - count(sql_new_schema)
-      end
-
-      def count(sql)
-        ActiveRecord::Base.connection.exec_query(sql).first['count']
+      def remaining?
+        Gitlab::BackgroundMigration.exists?('MigrateApproverToApprovalRulesInBatch')
       end
     end
   end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb
new file mode 100644
index 00000000000000..3bbae78efcc9d8
--- /dev/null
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress do
+  context 'when there is MigrateApproverToApprovalRulesInBatch jobs' do
+    it 'reschedules check' do
+      allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(true)
+
+      expect(BackgroundMigrationWorker).to receive(:perform_in).with(described_class::RESCHEDULE_DELAY, described_class.name)
+
+      described_class.new.perform
+    end
+  end
+
+  context 'when there is no more MigrateApproverToApprovalRulesInBatch jobs' do
+    it 'enables feature' do
+      allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
+
+      expect(Feature).to receive(:enable).and_return(:approval_rule)
+
+      described_class.new.perform
+    end
+  end
+end
-- 
GitLab


From 3487761d28cd816ccfb3826bb43aa9ca60858a09 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Fri, 28 Dec 2018 11:57:17 +0800
Subject: [PATCH 09/25] Assign code owners in data migration

The sync_code_owners_with_approvers call uses
original implementation, as it requires full
MR implementation to compute code owners.
However the specs are copied over,
so in the future if its behavior changes,
spec failure would notify developer to copy
existing code over.
---
 ee/app/models/ee/merge_request.rb             | 16 ++++--
 .../migrate_approver_to_approval_rules.rb     |  6 +++
 ...migrate_approver_to_approval_rules_spec.rb | 50 +++++++++++++++++++
 ee/spec/models/merge_request_spec.rb          | 39 ++++++++++-----
 4 files changed, 94 insertions(+), 17 deletions(-)

diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 238fea3a96be40..02564f29633c50 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -83,11 +83,19 @@ def compare_license_management_reports
     end
 
     def sync_code_owners_with_approvers
-      ActiveRecord::Base.transaction do
-        rule = approval_rules.code_owner.first
-        rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
+      return if merged?
 
-        rule.users = code_owners
+      owners = code_owners
+
+      if owners.present?
+        ActiveRecord::Base.transaction do
+          rule = approval_rules.code_owner.first
+          rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
+
+          rule.users = code_owners
+        end
+      else
+        approval_rules.code_owner.delete_all
       end
     end
   end
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 8884371fa1bfab..41552d2b4454b9 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -102,6 +102,10 @@ def merged?
           state == 'merged'
         end
 
+        def sync_code_owners_with_approvers
+          ::MergeRequest.find(id).sync_code_owners_with_approvers
+        end
+
         def finalize_approvals
           return unless merged?
 
@@ -170,6 +174,8 @@ def handle_merge_request
             rule.approval_project_rule = target.target_project.approval_rules.regular.first
           end
 
+          target.sync_code_owners_with_approvers
+
           target.finalize_approvals if target.merged?
         end
       end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index 62749f70b04cf7..b3f441e9afdb1a 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -120,6 +120,56 @@ def create_member_in(member, *populate_in)
           expect(target.approval_rules.exists?(approval_rule.id)).to eq(false)
         end
       end
+
+      context '#sync_code_owners_with_approvers' do
+        let(:owners) { create_list(:user, 2) }
+
+        before do
+          allow(::Gitlab::CodeOwners).to receive(:for_merge_request).and_return(owners)
+        end
+
+        context 'when merge request is merged' do
+          let(:target) { create(:merged_merge_request) }
+
+          it 'does nothing' do
+            expect do
+              described_class.new.perform(target_type, target.id)
+            end.not_to change { target.approval_rules.count }
+          end
+        end
+
+        context 'when code owner rule does not exist' do
+          it 'creates rule' do
+            expect do
+              described_class.new.perform(target_type, target.id)
+            end.to change { target.approval_rules.code_owner.count }.by(1)
+
+            expect(target.approval_rules.code_owner.first.users).to contain_exactly(*owners)
+          end
+        end
+
+        context 'when code owner rule exists' do
+          let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: target, code_owner: true, users: [create(:user)]) }
+
+          it 'reuses and updates existing rule' do
+            expect do
+              described_class.new.perform(target_type, target.id)
+            end.not_to change { target.approval_rules.count }
+
+            expect(code_owner_rule.reload.users).to contain_exactly(*owners)
+          end
+
+          context 'when there is no code owner' do
+            let(:owners) { [] }
+
+            it 'removes rule' do
+              described_class.new.perform(target_type, target.id)
+
+              expect(target.approval_rules.exists?(code_owner_rule.id)).to eq(false)
+            end
+          end
+        end
+      end
     end
 
     context 'project' do
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index 647eec9e24307e..a8488bb9e5c22a 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -71,36 +71,49 @@
   end
 
   describe '#sync_code_owners_with_approvers' do
-    let(:owners) { [create(:user), create(:user)] }
+    let(:owners) { create_list(:user, 2) }
 
     before do
       allow(subject).to receive(:code_owners).and_return(owners)
     end
 
-    it 'sync code owner to the code owner rule' do
+    it 'does nothing when merge request is merged' do
+      allow(subject).to receive(:merged?).and_return(true)
+
       expect do
         subject.sync_code_owners_with_approvers
-      end.to change { subject.approval_rules.count }.by(1)
-
-      expect(subject.approval_rules.code_owner.first.users).to contain_exactly(*owners)
+      end.not_to change { subject.approval_rules.count }
     end
 
-    context 'when code owner rule already exists' do
-      let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner') }
+    context 'when code owner rule does not exist' do
+      it 'creates rule' do
+        expect do
+          subject.sync_code_owners_with_approvers
+        end.to change { subject.approval_rules.code_owner.count }.by(1)
 
-      before do
-        code_owner_rule.users << create(:user)
+        expect(subject.approval_rules.code_owner.first.users).to contain_exactly(*owners)
       end
+    end
 
-      it 'reuses existing rule' do
+    context 'when code owner rule exists' do
+      let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner', users: [create(:user)]) }
+
+      it 'reuses and updates existing rule' do
         expect do
           subject.sync_code_owners_with_approvers
         end.not_to change { subject.approval_rules.count }
 
-        rule = subject.approval_rules.code_owner.first
+        expect(code_owner_rule.reload.users).to contain_exactly(*owners)
+      end
+
+      context 'when there is no code owner' do
+        let(:owners) { [] }
 
-        expect(rule).to eq(code_owner_rule)
-        expect(rule.users).to contain_exactly(*owners)
+        it 'removes rule' do
+          subject.sync_code_owners_with_approvers
+
+          expect(subject.approval_rules.exists?(code_owner_rule.id)).to eq(false)
+        end
       end
     end
   end
-- 
GitLab


From b855cb6b07c2a6a915f5c3c53fea815dc0d1eaf8 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 3 Jan 2019 17:04:12 +0800
Subject: [PATCH 10/25] Remove finalize MR approval rule in data migration

Past MR's approval requirement may change if project level setting
ever changed, so finalizing a past MR won't be accurate.

If there is a need to create estimated data,
another optional migration can be implemented.
---
 .../migrate_approver_to_approval_rules.rb     |  27 -----
 ...migrate_approver_to_approval_rules_spec.rb | 113 ------------------
 2 files changed, 140 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 41552d2b4454b9..09b496a6545b10 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -56,13 +56,6 @@ def approvers
             scope
           end
         end
-
-        def sync_approvals
-          # Before being merged, approvals are dynamically calculated instead of being persisted in the db.
-          return unless merge_request.merged?
-
-          self.approvals = merge_request.approvals.where(user_id: approvers.map(&:id))
-        end
       end
 
       class ApprovalMergeRequestRuleSource < ApplicationRecord
@@ -105,24 +98,6 @@ def merged?
         def sync_code_owners_with_approvers
           ::MergeRequest.find(id).sync_code_owners_with_approvers
         end
-
-        def finalize_approvals
-          return unless merged?
-
-          copy_project_approval_rules unless approval_rules.regular.exists?
-
-          approval_rules.reload.each(&:sync_approvals)
-        end
-
-        private
-
-        def copy_project_approval_rules
-          target_project.approval_rules.each do |project_rule|
-            rule = approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
-            rule.users = project_rule.users
-            rule.groups = project_rule.groups
-          end
-        end
       end
 
       class Project < ActiveRecord::Base
@@ -175,8 +150,6 @@ def handle_merge_request
           end
 
           target.sync_code_owners_with_approvers
-
-          target.finalize_approvals if target.merged?
         end
       end
 
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index b3f441e9afdb1a..68dd9b878ecd6e 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -190,117 +190,4 @@ def create_member_in(member, *populate_in)
       end
     end
   end
-
-  # Copied and modified from merge_request_spec.rb
-  describe '#finalize_approvals' do
-    let(:project) { create(:project, :repository) }
-    subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-    let(:target) { merge_request }
-
-    let!(:member1) { create(:user) }
-    let!(:member2) { create(:user) }
-    let!(:member3) { create(:user) }
-    let!(:group1) { create(:group) }
-    let!(:group2) { create(:group) }
-    let!(:group1_member) { create(:user) }
-    let!(:group2_member) { create(:user) }
-    let!(:approval1) { create(:approval, merge_request: subject, user: member1) }
-    let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
-    let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
-    let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
-
-    before do
-      group1.add_guest(group1_member)
-      group2.add_guest(group2_member)
-
-      rule = create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12)
-
-      rule.users = [member1, member2]
-      rule.groups << group1
-    end
-
-    context 'when without MR rules (project rule not overwritten)' do
-      it 'does nothing if unmerged' do
-        expect do
-          described_class.new.perform(target.class.name, target.id)
-        end.not_to change { ApprovalMergeRequestRule.count }
-
-        expect(approval1.approval_rules).to be_empty
-        expect(approval2.approval_rules).to be_empty
-        expect(approval3.approval_rules).to be_empty
-        expect(approval4.approval_rules).to be_empty
-      end
-
-      context 'when merged' do
-        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
-
-        it 'copies project rules to MR' do
-          expect do
-            described_class.new.perform(target.class.name, target.id)
-          end.to change { ApprovalMergeRequestRule.count }.by(1)
-
-          rule = subject.approval_rules.first
-
-          expect(rule.name).to eq('foo')
-          expect(rule.approvals_required).to eq(12)
-
-          expect(rule.users).to contain_exactly(member1, member2)
-          expect(rule.groups).to contain_exactly(group1)
-
-          rule = subject.approval_rules.first
-
-          expect(approval1.approval_rules).to contain_exactly(rule)
-          expect(approval2.approval_rules).to be_empty
-          expect(approval3.approval_rules).to contain_exactly(rule)
-          expect(approval4.approval_rules).to be_empty
-        end
-      end
-    end
-
-    context 'when with MR approver exists (project rule overwritten)' do
-      before do
-        create_skip_sync(:approver, target: subject, user: member2)
-        create_skip_sync(:approver, target: subject, user: member3)
-        create_skip_sync(:approver_group, target: subject, group: group2)
-
-        merge_request.update(approvals_before_merge: 32)
-      end
-
-      it 'does not call finalize_approvals if unmerged' do
-        expect do
-          described_class.new.perform(target.class.name, target.id)
-        end.to change { ApprovalMergeRequestRule.count }.by(1)
-
-        expect(approval1.approval_rules).to be_empty
-        expect(approval2.approval_rules).to be_empty
-        expect(approval3.approval_rules).to be_empty
-        expect(approval4.approval_rules).to be_empty
-      end
-
-      context 'when merged' do
-        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
-
-        it 'does not copy project rules, and updates approval mapping with MR rules' do
-          expect(subject).not_to receive(:copy_project_approval_rules)
-
-          expect do
-            described_class.new.perform(target.class.name, target.id)
-          end.to change { ApprovalMergeRequestRule.count }.by(1)
-
-          rule = subject.approval_rules.first
-
-          expect(rule.name).to eq('Default')
-          expect(rule.approvals_required).to eq(32)
-
-          expect(rule.users).to contain_exactly(member2, member3)
-          expect(rule.groups).to contain_exactly(group2)
-
-          expect(approval1.approval_rules).to be_empty
-          expect(approval2.approval_rules).to contain_exactly(rule)
-          expect(approval3.approval_rules).to be_empty
-          expect(approval4.approval_rules).to contain_exactly(rule)
-        end
-      end
-    end
-  end
 end
-- 
GitLab


From 489c337b4cb80b6b51cfd30a9293daabbe02abea Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 3 Jan 2019 17:26:34 +0800
Subject: [PATCH 11/25] Remove further unused code

Related to removing finalize MR
---
 .../migrate_approver_to_approval_rules.rb     | 39 ++-----------------
 ...r_to_approval_rules_check_progress_spec.rb |  2 +-
 2 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 09b496a6545b10..a96cd5c2d0da7f 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -3,7 +3,7 @@
 module Gitlab
   module BackgroundMigration
     # A Project/MergeRequest level migration, aiming to convert existing data
-    # (from approvers, approver_groups and approvals tables)
+    # (from approvers, approver_groups tables)
     # to new rule based schema.
     class MigrateApproverToApprovalRules
       include Gitlab::Utils::StrongMemoize
@@ -16,7 +16,7 @@ class ApproverGroup < ActiveRecord::Base
         self.table_name = 'approver_groups'
       end
 
-      class ApprovalMergeRequestRule < ApplicationRecord
+      class ApprovalMergeRequestRule < ActiveRecord::Base
         self.table_name = 'approval_merge_request_rules'
 
         include Gitlab::Utils::StrongMemoize
@@ -27,38 +27,15 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
         has_and_belongs_to_many :users
         has_and_belongs_to_many :groups, class_name: 'Group', join_table: :approval_merge_request_rules_groups
-        has_and_belongs_to_many :approvals # This is only populated after merge request is merged
-        has_many :approved_approvers, through: :approvals, source: :user
         has_one :approval_merge_request_rule_source
         has_one :approval_project_rule, through: :approval_merge_request_rule_source
 
         def project
           merge_request.target_project
         end
-
-        # Users who are eligible to approve, including specified group members.
-        # Excludes the author if 'self-approval' isn't explicitly
-        # enabled on project settings.
-        # @return [Array<User>]
-        def approvers
-          strong_memoize(:approvers) do
-            scope = User.from_union(
-              [
-                users,
-                User.joins(:group_members).where(members: { source_id: groups })
-              ]
-            )
-
-            if merge_request.author && !project.merge_requests_author_approval?
-              scope = scope.where.not(id: merge_request.author)
-            end
-
-            scope
-          end
-        end
       end
 
-      class ApprovalMergeRequestRuleSource < ApplicationRecord
+      class ApprovalMergeRequestRuleSource < ActiveRecord::Base
         self.table_name = 'approval_merge_request_rule_sources'
         belongs_to :approval_merge_request_rule
         belongs_to :approval_project_rule
@@ -79,9 +56,7 @@ class MergeRequest < ActiveRecord::Base
         include ::EachBatch
 
         belongs_to :target_project, class_name: "Project"
-        belongs_to :author, class_name: "User"
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
-        has_many :approvals
 
         def approvers
           Approver.where(target_type: 'MergeRequest', target_id: id)
@@ -91,10 +66,6 @@ def approver_groups
           ApproverGroup.where(target_type: 'MergeRequest', target_id: id)
         end
 
-        def merged?
-          state == 'merged'
-        end
-
         def sync_code_owners_with_approvers
           ::MergeRequest.find(id).sync_code_owners_with_approvers
         end
@@ -116,11 +87,7 @@ def approver_groups
       end
 
       class User < ActiveRecord::Base
-        include FromUnion
-
         self.table_name = 'users'
-
-        has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
       end
 
       ALLOWED_TARGET_TYPES = %w{MergeRequest Project}.freeze
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb
index 3bbae78efcc9d8..f2f7c811da0ed2 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb
@@ -17,7 +17,7 @@
     it 'enables feature' do
       allow(Gitlab::BackgroundMigration).to receive(:exists?).with('MigrateApproverToApprovalRulesInBatch').and_return(false)
 
-      expect(Feature).to receive(:enable).and_return(:approval_rule)
+      expect(Feature).to receive(:enable).with(:approval_rule)
 
       described_class.new.perform
     end
-- 
GitLab


From d79b7d3f198980148b4e86c2f30bc313c21fe625 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Fri, 4 Jan 2019 12:14:59 +0800
Subject: [PATCH 12/25] Bulk migrate multiple project's MR

This speeds up migration, as previously each project has its own job,
now they can be in a batch, avoiding 2 minutes wait interval per job.
---
 .../migrate_approver_to_approval_rules.rb     | 12 ---------
 ...ate_approver_to_approval_rules_in_batch.rb | 17 ++++++++++++
 ...pprover_to_approval_rules_in_batch_spec.rb | 26 ++++++++++++++++++-
 ...migrate_approver_to_approval_rules_spec.rb | 10 -------
 4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index a96cd5c2d0da7f..c5c49d01577797 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -53,7 +53,6 @@ class ApprovalProjectRule < ActiveRecord::Base
 
       class MergeRequest < ActiveRecord::Base
         self.table_name = 'merge_requests'
-        include ::EachBatch
 
         belongs_to :target_project, class_name: "Project"
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
@@ -74,7 +73,6 @@ def sync_code_owners_with_approvers
       class Project < ActiveRecord::Base
         self.table_name = 'projects'
 
-        has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project
         has_many :approval_rules, class_name: 'ApprovalProjectRule'
 
         def approvers
@@ -124,8 +122,6 @@ def handle_project
         ActiveRecord::Base.transaction do
           sync_rule
         end
-
-        schedule_to_migrate_merge_requests(target)
       end
 
       def sync_rule
@@ -140,14 +136,6 @@ def sync_rule
         rule
       end
 
-      def schedule_to_migrate_merge_requests(project)
-        jobs = []
-        project.merge_requests.each_batch do |scope, _|
-          jobs << ['MigrateApproverToApprovalRulesInBatch', ['MergeRequest', scope.pluck(:id)]]
-        end
-        BackgroundMigrationWorker.bulk_perform_async(jobs)
-      end
-
       def target
         strong_memoize(:target) do
           case @target_type
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
index cf776af7ec0224..fe12e440ac064a 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
@@ -3,10 +3,27 @@
 module Gitlab
   module BackgroundMigration
     class MigrateApproverToApprovalRulesInBatch
+      class MergeRequest < ActiveRecord::Base
+        self.table_name = 'merge_requests'
+        include ::EachBatch
+      end
+
       def perform(target_type, target_ids)
         target_ids.each do |target_id|
           MigrateApproverToApprovalRules.new.perform(target_type, target_id)
         end
+
+        schedule_to_migrate_merge_requests(target_ids) if target_type == 'Project'
+      end
+
+      private
+
+      def schedule_to_migrate_merge_requests(project_ids)
+        jobs = []
+        MergeRequest.where(target_project_id: project_ids).each_batch do |scope, _|
+          jobs << [self.class.name, ['MergeRequest', scope.pluck(:id)]]
+        end
+        BackgroundMigrationWorker.bulk_perform_async(jobs) unless jobs.empty?
       end
     end
   end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
index af97a757e4cd2b..4247040cd40387 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
@@ -6,12 +6,36 @@
   context 'when there is no more MigrateApproverToApprovalRules jobs' do
     let(:job) { double(:job) }
 
-    it 'enables feature' do
+    it 'migrates individual target' do
       allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(job)
 
       expect(job).to receive(:perform).exactly(3).times
 
       described_class.new.perform('Foo', [1, 2, 3])
     end
+
+    context 'when targets are projects' do
+      let(:projects) { create_list(:project, 3) }
+
+      context 'when projects contain merge requests' do
+        it 'schedules migrations for merge requests' do
+          merge_requests = projects.flat_map do |project|
+            create(:merge_request, source_project: project, target_project: project)
+          end
+
+          expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([[described_class.name, ["MergeRequest", merge_requests.map(&:id)]]])
+
+          described_class.new.perform('Project', projects.map(&:id))
+        end
+      end
+
+      context 'when merge request do not exist' do
+        it 'does nothing' do
+          expect(BackgroundMigrationWorker).not_to receive(:bulk_perform_async)
+
+          described_class.new.perform('Project', projects.map(&:id))
+        end
+      end
+    end
   end
 end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index 68dd9b878ecd6e..f958282a1414a0 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -178,16 +178,6 @@ def create_member_in(member, *populate_in)
       let(:approval_rule) { create(:approval_project_rule, project: target) }
 
       it_behaves_like 'sync approval member'
-
-      context 'when project contains some merge requests' do
-        let!(:merge_request) { create(:merge_request, source_project: target, target_project: target) }
-
-        it 'schedules migrations for all its merge requests' do
-          expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([['MigrateApproverToApprovalRulesInBatch', ["MergeRequest", [merge_request.id]]]])
-
-          described_class.new.perform(target.class.name, target.id)
-        end
-      end
     end
   end
 end
-- 
GitLab


From b5f388ccf93c703963b2e9f0942a110a0ed12f2b Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Fri, 4 Jan 2019 13:05:21 +0800
Subject: [PATCH 13/25] Refactor code around transaction

Extract transaction
---
 .../migrate_approver_to_approval_rules.rb     | 26 +++++++++----------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index c5c49d01577797..3ead34e8869522 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -98,30 +98,28 @@ def perform(target_type, target_id)
 
         raise "Incorrect target_type #{target_type}" unless ALLOWED_TARGET_TYPES.include?(@target_type)
 
-        case target
-        when MergeRequest
-          handle_merge_request
-        when Project
-          handle_project
+        ActiveRecord::Base.transaction do
+          case target
+          when MergeRequest
+            handle_merge_request
+          when Project
+            handle_project
+          end
         end
       end
 
       private
 
       def handle_merge_request
-        ActiveRecord::Base.transaction do
-          if rule = sync_rule
-            rule.approval_project_rule = target.target_project.approval_rules.regular.first
-          end
-
-          target.sync_code_owners_with_approvers
+        if rule = sync_rule
+          rule.approval_project_rule = target.target_project.approval_rules.regular.first
         end
+
+        target.sync_code_owners_with_approvers
       end
 
       def handle_project
-        ActiveRecord::Base.transaction do
-          sync_rule
-        end
+        sync_rule
       end
 
       def sync_rule
-- 
GitLab


From 18bbba99c3d822f20a77592d1244ad60c1c3c144 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Fri, 4 Jan 2019 15:12:48 +0800
Subject: [PATCH 14/25] Allow git access

This in run in bulk migration update loop,
so access can't be avoided.
---
 .../migrate_approver_to_approval_rules.rb                     | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 3ead34e8869522..11ba17f8da3070 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -66,7 +66,9 @@ def approver_groups
         end
 
         def sync_code_owners_with_approvers
-          ::MergeRequest.find(id).sync_code_owners_with_approvers
+          Gitlab::GitalyClient.allow_n_plus_1_calls do
+            ::MergeRequest.find(id).sync_code_owners_with_approvers
+          end
         end
       end
 
-- 
GitLab


From 1adde758e6a816d14bbc372ab5c6dd485413e2c1 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sat, 5 Jan 2019 23:50:45 +0800
Subject: [PATCH 15/25] Avoid pushing too many jobs in one go

Push 1000 jobs at a time.
---
 .../20181204040404_migrate_project_approvers.rb       | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
index 3daef03d682009..1da3f7c5d35dc3 100644
--- a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -13,8 +13,15 @@ class Project < ActiveRecord::Base
 
   def up
     jobs = []
-    Project.each_batch(of: BATCH_SIZE) do |scope, _|
-      jobs << ['MigrateApproverToApprovalRulesInBatch', ['Project', scope.pluck(:id)]]
+    Project.each_batch(of: BATCH_SIZE) do |scope|
+      project_ids = scope.pluck(:id)
+
+      if jobs.length >= BACKGROUND_MIGRATION_JOB_BUFFER_SIZE
+        BackgroundMigrationWorker.bulk_perform_async(jobs)
+        jobs.clear
+      end
+
+      jobs << ['MigrateApproverToApprovalRulesInBatch', ['Project', project_ids]]
     end
     BackgroundMigrationWorker.bulk_perform_async(jobs)
 
-- 
GitLab


From 0c3092e94f44c273ba09141b4c27f7c204b5041b Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sun, 6 Jan 2019 00:02:22 +0800
Subject: [PATCH 16/25] Add spec to handle destroyed target

It should not err.
---
 .../migrate_approver_to_approval_rules_spec.rb      | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index f958282a1414a0..210a611e69e1cb 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -180,4 +180,17 @@ def create_member_in(member, *populate_in)
       it_behaves_like 'sync approval member'
     end
   end
+
+  context 'when target is deleted' do
+    let(:target) { create(:project) }
+    let(:target_type) { 'Project' }
+
+    it "does not err" do
+      target.destroy
+
+      expect do
+        described_class.new.perform(target_type, target.id)
+      end.not_to raise_error
+    end
+  end
 end
-- 
GitLab


From 05f868b9bdb3b784e222ec7dcf2ae550d17184ff Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sun, 6 Jan 2019 11:57:05 +0800
Subject: [PATCH 17/25] Process all projects synchronously

This simplifies things.
And amount of projects are quite small.
---
 ...0181204040404_migrate_project_approvers.rb | 34 ++++++++++++-------
 ...ate_approver_to_approval_rules_in_batch.rb | 20 +++--------
 ...pprover_to_approval_rules_in_batch_spec.rb | 29 +++-------------
 3 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
index 1da3f7c5d35dc3..d6edbc06429c87 100644
--- a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -6,24 +6,25 @@ class MigrateProjectApprovers < ActiveRecord::Migration[5.0]
   DOWNTIME = false
   BATCH_SIZE = 1000
 
-  class Project < ActiveRecord::Base
+  class MergeRequest < ActiveRecord::Base
     include ::EachBatch
-    self.table_name = 'projects'
+    self.table_name = 'merge_requests'
   end
 
-  def up
-    jobs = []
-    Project.each_batch(of: BATCH_SIZE) do |scope|
-      project_ids = scope.pluck(:id)
+  class Approver < ActiveRecord::Base
+    self.table_name = 'approvers'
+  end
 
-      if jobs.length >= BACKGROUND_MIGRATION_JOB_BUFFER_SIZE
-        BackgroundMigrationWorker.bulk_perform_async(jobs)
-        jobs.clear
-      end
+  class ApproverGroup < ActiveRecord::Base
+    self.table_name = 'approver_groups'
+  end
 
-      jobs << ['MigrateApproverToApprovalRulesInBatch', ['Project', project_ids]]
+  def up
+    get_project_ids.each do |project_id|
+      Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform('Project', project_id)
     end
-    BackgroundMigrationWorker.bulk_perform_async(jobs)
+
+    bulk_queue_background_migration_jobs_by_range(MergeRequest, 'MigrateApproverToApprovalRulesInBatch')
 
     check_time = Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress::RESCHEDULE_DELAY
     BackgroundMigrationWorker.bulk_perform_in(check_time, [['MigrateApproverToApprovalRulesCheckProgress']])
@@ -31,4 +32,13 @@ def up
 
   def down
   end
+
+  private
+
+  def get_project_ids
+    project_ids = Approver.where('target_type = ?', 'Project').pluck(:target_id)
+    project_ids += ApproverGroup.where('target_type = ?', 'Project').pluck(:target_id)
+    project_ids.uniq!
+    project_ids
+  end
 end
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
index fe12e440ac064a..5808b5d434feae 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch.rb
@@ -5,25 +5,13 @@ module BackgroundMigration
     class MigrateApproverToApprovalRulesInBatch
       class MergeRequest < ActiveRecord::Base
         self.table_name = 'merge_requests'
-        include ::EachBatch
       end
 
-      def perform(target_type, target_ids)
-        target_ids.each do |target_id|
-          MigrateApproverToApprovalRules.new.perform(target_type, target_id)
+      def perform(start_id, end_id)
+        merge_request_ids = MergeRequest.where('id >= ? AND id <= ?', start_id, end_id).pluck(:id)
+        merge_request_ids.each do |merge_request_id|
+          MigrateApproverToApprovalRules.new.perform('MergeRequest', merge_request_id)
         end
-
-        schedule_to_migrate_merge_requests(target_ids) if target_type == 'Project'
-      end
-
-      private
-
-      def schedule_to_migrate_merge_requests(project_ids)
-        jobs = []
-        MergeRequest.where(target_project_id: project_ids).each_batch do |scope, _|
-          jobs << [self.class.name, ['MergeRequest', scope.pluck(:id)]]
-        end
-        BackgroundMigrationWorker.bulk_perform_async(jobs) unless jobs.empty?
       end
     end
   end
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
index 4247040cd40387..b34c38f74abc06 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb
@@ -5,37 +5,16 @@
 describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesInBatch do
   context 'when there is no more MigrateApproverToApprovalRules jobs' do
     let(:job) { double(:job) }
+    let(:project) { create(:project) }
 
     it 'migrates individual target' do
       allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(job)
 
-      expect(job).to receive(:perform).exactly(3).times
-
-      described_class.new.perform('Foo', [1, 2, 3])
-    end
-
-    context 'when targets are projects' do
-      let(:projects) { create_list(:project, 3) }
+      merge_requests = create_list(:merge_request, 3)
 
-      context 'when projects contain merge requests' do
-        it 'schedules migrations for merge requests' do
-          merge_requests = projects.flat_map do |project|
-            create(:merge_request, source_project: project, target_project: project)
-          end
-
-          expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([[described_class.name, ["MergeRequest", merge_requests.map(&:id)]]])
-
-          described_class.new.perform('Project', projects.map(&:id))
-        end
-      end
-
-      context 'when merge request do not exist' do
-        it 'does nothing' do
-          expect(BackgroundMigrationWorker).not_to receive(:bulk_perform_async)
+      expect(job).to receive(:perform).exactly(3).times
 
-          described_class.new.perform('Project', projects.map(&:id))
-        end
-      end
+      described_class.new.perform(merge_requests.first.id, merge_requests.last.id)
     end
   end
 end
-- 
GitLab


From fb7c62b84fb1d8d05a67b29f32eb5ea242a9eaec Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sun, 6 Jan 2019 21:11:18 +0800
Subject: [PATCH 18/25] Increase batch size to cut down time

---
 .../post_migrate/20181204040404_migrate_project_approvers.rb  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
index d6edbc06429c87..7480ff8d3b843e 100644
--- a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -4,7 +4,7 @@ class MigrateProjectApprovers < ActiveRecord::Migration[5.0]
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = false
-  BATCH_SIZE = 1000
+  BATCH_SIZE = 3000
 
   class MergeRequest < ActiveRecord::Base
     include ::EachBatch
@@ -24,7 +24,7 @@ def up
       Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform('Project', project_id)
     end
 
-    bulk_queue_background_migration_jobs_by_range(MergeRequest, 'MigrateApproverToApprovalRulesInBatch')
+    bulk_queue_background_migration_jobs_by_range(MergeRequest, 'MigrateApproverToApprovalRulesInBatch', batch_size: BATCH_SIZE)
 
     check_time = Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckProgress::RESCHEDULE_DELAY
     BackgroundMigrationWorker.bulk_perform_in(check_time, [['MigrateApproverToApprovalRulesCheckProgress']])
-- 
GitLab


From 0e186a05a202eabacf570259c65febea981e7d28 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sun, 6 Jan 2019 22:12:45 +0800
Subject: [PATCH 19/25] Avoid query and model initialization

Finding id is enough for update
---
 .../migrate_approver_to_approval_rules.rb     | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 11ba17f8da3070..c5e48ed005311f 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -19,8 +19,6 @@ class ApproverGroup < ActiveRecord::Base
       class ApprovalMergeRequestRule < ActiveRecord::Base
         self.table_name = 'approval_merge_request_rules'
 
-        include Gitlab::Utils::StrongMemoize
-
         belongs_to :merge_request
         scope :code_owner, -> { where(code_owner: true) }
         scope :regular, -> { where(code_owner: false) } # Non code owner rule
@@ -57,15 +55,17 @@ class MergeRequest < ActiveRecord::Base
         belongs_to :target_project, class_name: "Project"
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
 
-        def approvers
-          Approver.where(target_type: 'MergeRequest', target_id: id)
+        def approver_ids
+          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck(:user_id)
         end
 
-        def approver_groups
-          ApproverGroup.where(target_type: 'MergeRequest', target_id: id)
+        def approver_group_ids
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck(:group_id)
         end
 
         def sync_code_owners_with_approvers
+          return if state == 'merged' || state == 'closed'
+
           Gitlab::GitalyClient.allow_n_plus_1_calls do
             ::MergeRequest.find(id).sync_code_owners_with_approvers
           end
@@ -77,12 +77,12 @@ class Project < ActiveRecord::Base
 
         has_many :approval_rules, class_name: 'ApprovalProjectRule'
 
-        def approvers
-          Approver.where(target_type: 'Project', target_id: id)
+        def approver_ids
+          @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck(:user_id)
         end
 
-        def approver_groups
-          ApproverGroup.where(target_type: 'Project', target_id: id)
+        def approver_group_ids
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck(:group_id)
         end
       end
 
@@ -131,8 +131,8 @@ def sync_rule
         end
 
         rule = find_or_create_rule
-        rule.user_ids = target.approvers.pluck(:user_id)
-        rule.group_ids = target.approver_groups.pluck(:group_id)
+        rule.user_ids = target.approver_ids
+        rule.group_ids = target.approver_group_ids
         rule
       end
 
@@ -159,7 +159,7 @@ def find_or_create_rule
       end
 
       def approvers_exists?
-        target.approvers.to_a.any? || target.approver_groups.to_a.any?
+        target.approver_ids.any? || target.approver_group_ids.any?
       end
     end
   end
-- 
GitLab


From a818d9963749ba083413fec549798ad8ee3787e0 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sun, 6 Jan 2019 22:14:04 +0800
Subject: [PATCH 20/25] Use existing constant

Fix spec
---
 .../background_migration/migrate_approver_to_approval_rules.rb  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index c5e48ed005311f..57f91fc13ae8c8 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -148,7 +148,7 @@ def target
       end
 
       def find_or_create_rule
-        rule = target.approval_rules.regular.find_or_initialize_by(name: 'Default')
+        rule = target.approval_rules.regular.find_or_initialize_by(name: ApprovalRuleLike::DEFAULT_NAME)
 
         unless rule.persisted?
           rule.approvals_required = target.approvals_before_merge || 0
-- 
GitLab


From 5bd6b44d0ba9622b09c6188ceaf73f7c87a30712 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 7 Jan 2019 12:12:56 +0800
Subject: [PATCH 21/25] Refactor find rule

Do no force a find by the name
---
 .../migrate_approver_to_approval_rules.rb              | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 57f91fc13ae8c8..71e67c7469e1f5 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -130,9 +130,8 @@ def sync_rule
           return
         end
 
-        rule = find_or_create_rule
-        rule.user_ids = target.approver_ids
-        rule.group_ids = target.approver_group_ids
+        rule = first_or_initialize
+        rule.update(user_ids: target.approver_ids, group_ids: target.approver_group_ids)
         rule
       end
 
@@ -147,10 +146,11 @@ def target
         end
       end
 
-      def find_or_create_rule
-        rule = target.approval_rules.regular.find_or_initialize_by(name: ApprovalRuleLike::DEFAULT_NAME)
+      def first_or_initialize
+        rule = target.approval_rules.regular.first_or_initialize
 
         unless rule.persisted?
+          rule.name ||= ApprovalRuleLike::DEFAULT_NAME
           rule.approvals_required = target.approvals_before_merge || 0
           rule.save!
         end
-- 
GitLab


From 5d91db5bda335f915cc4774a31b28d169a8f5523 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 18:57:17 +0800
Subject: [PATCH 22/25] 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                     |  2 +
 ee/app/models/approver_group.rb               |  2 +
 .../models/concerns/approver_migrate_hook.rb  | 21 ++++
 ee/lib/api/merge_request_approvals.rb         |  2 +
 .../migrate_approver_to_approval_rules.rb     |  5 +-
 .../concerns/approver_migrate_hook_spec.rb    | 98 +++++++++++++++++++
 6 files changed, 128 insertions(+), 2 deletions(-)
 create mode 100644 ee/app/models/concerns/approver_migrate_hook.rb
 create mode 100644 ee/spec/models/concerns/approver_migrate_hook_spec.rb

diff --git a/ee/app/models/approver.rb b/ee/app/models/approver.rb
index 9bf17aca25fab1..26c3502fde44d5 100644
--- a/ee/app/models/approver.rb
+++ b/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)
diff --git a/ee/app/models/approver_group.rb b/ee/app/models/approver_group.rb
index d1177657361d48..3c1cd1cb0d6947 100644
--- a/ee/app/models/approver_group.rb
+++ b/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
diff --git a/ee/app/models/concerns/approver_migrate_hook.rb b/ee/app/models/concerns/approver_migrate_hook.rb
new file mode 100644
index 00000000000000..738dc48bdda492
--- /dev/null
+++ b/ee/app/models/concerns/approver_migrate_hook.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb
index 1a189f1edbb8c9..48b5100daf0018 100644
--- a/ee/lib/api/merge_request_approvals.rb
+++ b/ee/lib/api/merge_request_approvals.rb
@@ -77,6 +77,8 @@ def handle_merge_request_errors!(errors)
           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)
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 71e67c7469e1f5..bb660275548a4c 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -94,9 +94,10 @@ class User < ActiveRecord::Base
 
       # @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 @@ def handle_merge_request
           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
diff --git a/ee/spec/models/concerns/approver_migrate_hook_spec.rb b/ee/spec/models/concerns/approver_migrate_hook_spec.rb
new file mode 100644
index 00000000000000..b254b1d88999c2
--- /dev/null
+++ b/ee/spec/models/concerns/approver_migrate_hook_spec.rb
@@ -0,0 +1,98 @@
+# 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
-- 
GitLab


From 6864c1f27ab6ea1eaaea2c735c897a72218820e9 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 7 Jan 2019 17:45:02 +0800
Subject: [PATCH 23/25] Use union to get distinct project ids

---
 .../20181204040404_migrate_project_approvers.rb     | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
index 7480ff8d3b843e..913e036eafb1fa 100644
--- a/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
+++ b/ee/db/post_migrate/20181204040404_migrate_project_approvers.rb
@@ -36,9 +36,14 @@ def down
   private
 
   def get_project_ids
-    project_ids = Approver.where('target_type = ?', 'Project').pluck(:target_id)
-    project_ids += ApproverGroup.where('target_type = ?', 'Project').pluck(:target_id)
-    project_ids.uniq!
-    project_ids
+    results = ActiveRecord::Base.connection.exec_query <<~SQL
+      SELECT DISTINCT target_id FROM (
+        SELECT target_id FROM approvers WHERE target_type='Project'
+        UNION
+        SELECT target_id FROM approver_groups WHERE target_type='Project'
+      ) t
+    SQL
+
+    results.rows.flatten
   end
 end
-- 
GitLab


From aaf1e00b1caf100b34bedb1d91cf92cc6ca03098 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 7 Jan 2019 21:45:54 +0800
Subject: [PATCH 24/25] Fix duplicated approvers scenario

Prevents duplicated record.
---
 ee/app/models/ee/merge_request.rb                         | 2 +-
 .../migrate_approver_to_approval_rules.rb                 | 8 ++++----
 .../migrate_approver_to_approval_rules_spec.rb            | 6 ++++--
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 02564f29633c50..bbd9f900b65916 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -92,7 +92,7 @@ def sync_code_owners_with_approvers
           rule = approval_rules.code_owner.first
           rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
 
-          rule.users = code_owners
+          rule.users = code_owners.uniq
         end
       else
         approval_rules.code_owner.delete_all
diff --git a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index bb660275548a4c..ee5006991d9156 100644
--- a/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -56,11 +56,11 @@ class MergeRequest < ActiveRecord::Base
         has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
 
         def approver_ids
-          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck(:user_id)
+          @approver_ids ||= Approver.where(target_type: 'MergeRequest', target_id: id).pluck('distinct user_id')
         end
 
         def approver_group_ids
-          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck(:group_id)
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'MergeRequest', target_id: id).pluck('distinct group_id')
         end
 
         def sync_code_owners_with_approvers
@@ -78,11 +78,11 @@ class Project < ActiveRecord::Base
         has_many :approval_rules, class_name: 'ApprovalProjectRule'
 
         def approver_ids
-          @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck(:user_id)
+          @approver_ids ||= Approver.where(target_type: 'Project', target_id: id).pluck('distinct user_id')
         end
 
         def approver_group_ids
-          @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck(:group_id)
+          @approver_group_ids ||= ApproverGroup.where(target_type: 'Project', target_id: id).pluck('distinct group_id')
         end
       end
 
diff --git a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index 210a611e69e1cb..e9c89df6b0d47f 100644
--- a/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -86,8 +86,10 @@ def create_member_in(member, *populate_in)
       context 'when project rule is present' do
         let!(:project_rule) { create(:approval_project_rule, project: target.target_project) }
 
-        it "sets MR rule's source to project rule" do
-          create_member_in(create(:user), :old_schema)
+        it "sets MR rule's source to project rule without duplication" do
+          user = create(:user)
+          create_member_in(user, :old_schema)
+          create_member_in(user, :old_schema)
 
           described_class.new.perform(target_type, target.id)
 
-- 
GitLab


From 32d449d13ff0bf09647fdced8a9b3cc2ae1d2410 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Tue, 8 Jan 2019 11:58:32 +0800
Subject: [PATCH 25/25] Ensure group_users to return distinct users

Caused uniqueness constraint when inserting as users
---
 ee/app/models/concerns/approval_rule_like.rb       |  2 +-
 ee/spec/models/concerns/approval_rule_like_spec.rb | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index bb4826d622725e..03d0dc29faa786 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -8,7 +8,7 @@ module ApprovalRuleLike
   included do
     has_and_belongs_to_many :users
     has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
-    has_many :group_users, through: :groups, source: :users
+    has_many :group_users, -> { distinct }, through: :groups, source: :users
 
     validates :name, presence: true
   end
diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb
index ee7e02bbb2d284..7df935b6e402ed 100644
--- a/ee/spec/models/concerns/approval_rule_like_spec.rb
+++ b/ee/spec/models/concerns/approval_rule_like_spec.rb
@@ -87,4 +87,16 @@
 
     it_behaves_like 'approval rule like'
   end
+
+  context '.group_users' do
+    subject { create(:approval_project_rule) }
+
+    it 'returns distinct users' do
+      group1.add_guest(user1)
+      group2.add_guest(user1)
+      subject.groups = [group1, group2]
+
+      expect(subject.group_users).to eq([user1])
+    end
+  end
 end
-- 
GitLab