Merge branch '1979-1-3-fix-approvals-required' into 'master'

Consuming remaining jobs and correct approvals_required

See merge request gitlab-org/gitlab-ee!9267

ee/db/migrate/20190114040404_correct_approvals_required.rb

+# frozen_string_literal: true
+
+# Fix data for https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9143
+# Check MR rules which has 0 and update to project approval rate.
+class CorrectApprovalsRequired < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  class ApprovalMergeRequestRule < ActiveRecord::Base
+    self.table_name = 'approval_merge_request_rules'
+    has_one :approval_merge_request_rule_source
+    has_one :approval_project_rule, through: :approval_merge_request_rule_source
+  end
+
+  class ApprovalProjectRule < ActiveRecord::Base
+    self.table_name = 'approval_project_rules'
+
+    has_many :approval_merge_request_rule_sources
+    has_many :approval_merge_request_rules, through: :approval_merge_request_rule_sources
+  end
+
+  def up
+    ApprovalProjectRule
+      .joins(:approval_merge_request_rules)
+      .where('approval_merge_request_rules.approvals_required = 0 AND approval_project_rules.approvals_required > 0')
+      .distinct
+      .find_each do |project_rule|
+      # Pluck as MySQL prohibits subquery that references the table being updated
+      mr_rule_ids = ApprovalMergeRequestRule
+        .joins(:approval_merge_request_rule_source)
+        .where("approval_merge_request_rules.approvals_required = 0 AND approval_merge_request_rule_sources.approval_project_rule_id = #{project_rule.id}")
+        .pluck('approval_merge_request_rules.id')
+
+      ApprovalMergeRequestRule.where(id: mr_rule_ids).update_all(approvals_required: project_rule.approvals_required)
+    end
+  end
+
+  def down
+  end
+end

ee/db/migrate/20190114040405_consume_remaining_migrate_approver_to_approval_rules_in_batch_jobs.rb

+# frozen_string_literal: true
+
+class ConsumeRemainingMigrateApproverToApprovalRulesInBatchJobs < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  # Number of MRs to join in order to search for wrong MR ids.
+  # Wrong MRs appear to be aggregated in groups,
+  # because migration was grouped in blocks of 3000,
+  # and when one MR fails, later MRs in that group are not be migrated.
+  #
+  # Double the size since difference between lower and higher id of the group
+  # are a little bigger than 3000.
+  #
+  # By using this smaller join size, the query is faster.
+  JOIN_SIZE = 6000
+  # A bound for doing searches.
+  # Without it a search for the next bad MR can timeout if that MR's id is really high.
+  BOUND_SIZE = 1000000
+  BASE_QUERY = <<~SQL
+    SELECT DISTINCT merge_requests.id FROM merge_requests
+    LEFT JOIN approval_merge_request_rules
+    ON merge_requests.id = approval_merge_request_rules.merge_request_id AND approval_merge_request_rules.code_owner IS FALSE
+    LEFT JOIN approvers
+    ON merge_requests.id = approvers.target_id AND approvers.target_type = 'MergeRequest'
+    LEFT JOIN approver_groups
+    ON merge_requests.id = approver_groups.target_id AND approver_groups.target_type = 'MergeRequest'
+    WHERE (approval_merge_request_rules.id IS NULL) AND (approvers.id IS NOT NULL OR approver_groups.id IS NOT NULL)
+  SQL
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    Gitlab::BackgroundMigration.steal('MigrateApproverToApprovalRulesInBatch', retry_dead_jobs: true)
+
+    process_unmigrated
+  end
+
+  def down
+  end
+
+  private
+
+  def process_unmigrated
+    bad_ids = []
+
+    max_id = MergeRequest.maximum(:id)
+
+    return if max_id.nil?
+
+    (0..max_id).step(BOUND_SIZE) do |lower_bound|
+      bad_ids.concat search_bound(lower_bound, lower_bound + BOUND_SIZE)
+    end
+
+    bad_ids.uniq!
+    bad_ids.each do |id|
+      Gitlab::BackgroundMigration::MigrateApproverToApprovalRules.new.perform('MergeRequest', id)
+    end
+  end
+
+  def search_bound(lower_bound, upper_bound)
+    bad_ids = []
+
+    loop do
+      # search for next wrong MR
+      lower_bound = exec_query("#{BASE_QUERY} AND merge_requests.id BETWEEN #{lower_bound} AND #{upper_bound} ORDER BY merge_requests.id ASC LIMIT 1").dig(0, 0)
+
+      return bad_ids if lower_bound.nil?
+
+      end_id = lower_bound + JOIN_SIZE
+
+      bad_ids.concat exec_query("#{BASE_QUERY} AND merge_requests.id BETWEEN #{lower_bound} AND #{end_id} ORDER BY merge_requests.id ASC").flatten
+
+      lower_bound = end_id + 1
+    end
+  end
+
+  def exec_query(query)
+    ActiveRecord::Base.connection.exec_query(query).rows
+  end
+end

ee/spec/migrations/consume_remaining_migrate_approver_to_approval_rules_in_batch_jobs_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190114040405_consume_remaining_migrate_approver_to_approval_rules_in_batch_jobs.rb')
+
+describe ConsumeRemainingMigrateApproverToApprovalRulesInBatchJobs, :migration do
+  let(:migration) { described_class.new }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:approvers) { table(:approvers) }
+  let(:approver_groups) { table(:approver_groups) }
+  let(:approval_rules) { table(:approval_merge_request_rules) }
+  let(:migrator) { double(:migrator) }
+
+  describe '#up' do
+    before do
+      stub_const('ConsumeRemainingMigrateApproverToApprovalRulesInBatchJobs::JOIN_SIZE', 6)
+      stub_const('ConsumeRemainingMigrateApproverToApprovalRulesInBatchJobs::BOUND_SIZE', 100)
+      allow(Gitlab::BackgroundMigration::MigrateApproverToApprovalRules).to receive(:new).and_return(migrator)
+
+      namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab')
+      projects.create!(id: 101, namespace_id: 11, name: 'gitlab', path: 'gitlab')
+
+      namespaces.create!(id: 12, name: 'gitlab', path: 'gitlab')
+
+      (1..101).each do |id|
+        merge_requests.create!(id: id, target_project_id: 101, source_project_id: 101, target_branch: 'feature', source_branch: 'master', state: 'merged')
+
+        if id.odd?
+          approvers.create!(id: id, target_id: id, target_type: 'MergeRequest', user_id: 1)
+        else
+          approver_groups.create!(id: id, target_id: id, target_type: 'MergeRequest', group_id: 12)
+        end
+
+        # add some approval rules to simulate approvers that were already migrated
+        # to rules
+        if (id % 5) == 0
+          approval_rules.create!(merge_request_id: id, code_owner: false, name: "test-rule-#{id}")
+        end
+      end
+    end
+
+    it "migrates unmigrated merge requests" do
+      (1..101).each do |id|
+        if (id % 5) != 0
+          expect(migrator).to receive(:perform).with('MergeRequest', id)
+        else
+          expect(migrator).not_to receive(:perform).with('MergeRequest', id)
+        end
+      end
+
+      migrate!
+    end
+  end
+end

ee/spec/migrations/correct_approvals_required_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'migrate', '20190114040404_correct_approvals_required.rb')
+
+describe CorrectApprovalsRequired, :migration do
+  let(:migration) { described_class.new }
+
+  describe '#up' do
+    let(:namespaces) { table(:namespaces) }
+    let(:projects) { table(:projects) }
+    let(:merge_requests) { table(:merge_requests) }
+    let(:approval_merge_request_rules) { table(:approval_merge_request_rules) }
+    let(:approval_merge_request_rule_sources) { table(:approval_merge_request_rule_sources) }
+    let(:approval_project_rules) { table(:approval_project_rules) }
+
+    before do
+      namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab')
+      projects.create!(id: 101, namespace_id: 11, name: 'gitlab', path: 'gitlab')
+      merge_requests.create!(id: 1, target_project_id: 101, source_project_id: 101, target_branch: 'feature', source_branch: 'master', state: 'merged')
+
+      # When approvals_required is 0
+      approval_project_rules.create!(id: 1, project_id: 101, approvals_required: 3, name: 'Rule1')
+      approval_merge_request_rules.create!(id: 1, merge_request_id: 1, approvals_required: 0, name: 'Default')
+      approval_merge_request_rule_sources.create!(id: 1, approval_merge_request_rule_id: 1, approval_project_rule_id: 1)
+
+      # When approvals_required is not 0
+      approval_project_rules.create!(id: 2, project_id: 101, approvals_required: 3, name: 'Rule2')
+      approval_merge_request_rules.create!(id: 2, merge_request_id: 1, approvals_required: 5, name: 'Merge request rule 2')
+      approval_merge_request_rule_sources.create!(id: 2, approval_merge_request_rule_id: 2, approval_project_rule_id: 2)
+
+      # When MR rule does not have project rule
+      approval_merge_request_rules.create!(id: 3, merge_request_id: 1, approvals_required: 0, name: 'Rule3')
+    end
+
+    it "updates approvals_required when it is 0 and lower than that of the project rule's" do
+      migrate!
+
+      expect(approval_merge_request_rules.where(id: 1).pluck(:approvals_required)).to contain_exactly(3)
+      expect(approval_merge_request_rules.where(id: 2).pluck(:approvals_required)).to contain_exactly(5)
+      expect(approval_merge_request_rules.where(id: 3).pluck(:approvals_required)).to contain_exactly(0)
+    end
+  end
+end