Merge branch 'id-fix-any-approver-rule-for-projects' into 'master'

Fix any-approver rule data for projects

See merge request gitlab-org/gitlab!18265

changelogs/unreleased/id-fix-any-approver-rule-for-projects.yml

+---
+title: Fix any approver project rule records
+merge_request: 18265
+author:
+type: changed

db/post_migrate/20191008143850_fix_any_approver_rule_for_projects.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class FixAnyApproverRuleForProjects < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+  BATCH_SIZE = 1000
+
+  disable_ddl_transaction!
+
+  class ApprovalProjectRule < ActiveRecord::Base
+    NON_EXISTENT_RULE_TYPE = 4
+    ANY_APPROVER_RULE_TYPE = 3
+
+    include EachBatch
+
+    self.table_name = 'approval_project_rules'
+
+    scope :any_approver, -> { where(rule_type: ANY_APPROVER_RULE_TYPE) }
+    scope :non_existent_rule_type, -> { where(rule_type: NON_EXISTENT_RULE_TYPE) }
+  end
+
+  def up
+    return unless Gitlab.ee?
+
+    # Remove approval project rule with rule type 4 if the project has a rule with rule_type 3
+    #
+    # Currently, there is no projects on gitlab.com which have both rules with 3 and 4 rule type
+    # There's a code-level validation for a rule, which doesn't allow to create rules with the same names
+    #
+    # But in order to avoid failing the update query due to uniqueness constraint
+    # Let's run the delete query to be sure
+    project_ids = FixAnyApproverRuleForProjects::ApprovalProjectRule.any_approver.select(:project_id)
+    FixAnyApproverRuleForProjects::ApprovalProjectRule
+      .non_existent_rule_type
+      .where(project_id: project_ids)
+      .delete_all
+
+    # Set approval project rule types to 3
+    # Currently there are 18_445 records to be updated
+    FixAnyApproverRuleForProjects::ApprovalProjectRule.non_existent_rule_type.each_batch(of: BATCH_SIZE) do |rules|
+      rules.update_all(rule_type: FixAnyApproverRuleForProjects::ApprovalProjectRule::ANY_APPROVER_RULE_TYPE)
+    end
+  end
+
+  def down
+    # The migration doesn't leave the database in an inconsistent state
+    # And can be run multiple times
+  end
+end

ee/lib/gitlab/background_migration/populate_any_approval_rule_for_projects.rb

@@ -13,7 +13,7 @@ module Gitlab
             .where(project_approval_rules_not_exists_clause)
             .where(id: from_id..to_id)
             .where('approvals_before_merge <> 0')
-            .select("id, LEAST(#{MAX_VALUE}, approvals_before_merge), created_at, updated_at, 4, '#{ApprovalRuleLike::ALL_MEMBERS}'")
+            .select("id, LEAST(#{MAX_VALUE}, approvals_before_merge), created_at, updated_at, #{ApprovalProjectRule.rule_types[:any_approver]}, '#{ApprovalRuleLike::ALL_MEMBERS}'")
             .to_sql
 
         execute("INSERT INTO approval_project_rules (project_id, approvals_required, created_at, updated_at, rule_type, name) #{select_sql}")

ee/spec/lib/gitlab/background_migration/populate_any_approval_rule_for_projects_spec.rb

@@ -21,14 +21,20 @@ describe Gitlab::BackgroundMigration::PopulateAnyApprovalRuleForProjects, :migra
     create_project(3, approvals_before_merge: 0)
 
     # Test filtering already migrated rows
-    create_project(4, approvals_before_merge: 3)
+    project_with_any_approver_rule = create_project(4, approvals_before_merge: 3)
     approval_project_rules.create(id: 4,
-      project_id: 4, approvals_required: 3, rule_type: 4, name: ApprovalRuleLike::ALL_MEMBERS)
+      project_id: project_with_any_approver_rule.id,
+      approvals_required: 3,
+      rule_type: ApprovalProjectRule.rule_types[:any_approver],
+      name: ApprovalRuleLike::ALL_MEMBERS)
 
     # Test filtering MRs with existing rules
-    create_project(5, approvals_before_merge: 3)
+    project_with_regular_rule = create_project(5, approvals_before_merge: 3)
     approval_project_rules.create(id: 5,
-      project_id: 5, approvals_required: 3, rule_type: 1, name: 'Regular rules')
+      project_id: project_with_regular_rule.id,
+      approvals_required: 3,
+      rule_type: ApprovalProjectRule.rule_types[:regular],
+      name: 'Regular rules')
 
     create_project(6, approvals_before_merge: 5)
     create_project(7, approvals_before_merge: 2**30)
@@ -47,7 +53,8 @@ describe Gitlab::BackgroundMigration::PopulateAnyApprovalRuleForProjects, :migra
         { 'project_id' => 7, 'approvals_required' => 2**15 - 1 }
       ]
 
-      rows = approval_project_rules.where(rule_type: 4).order(:id).map do |row|
+      rule_type = ApprovalProjectRule.rule_types[:any_approver]
+      rows = approval_project_rules.where(rule_type: rule_type).order(:id).map do |row|
         row.attributes.slice('project_id', 'approvals_required')
       end
 

ee/spec/migrations/fix_any_approver_rule_for_projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20191008143850_fix_any_approver_rule_for_projects.rb')
+
+describe FixAnyApproverRuleForProjects, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:projects) { table(:projects) }
+  let(:approval_project_rules) { table(:approval_project_rules) }
+
+  def create_project(id)
+    projects.create(id: id, namespace_id: namespace.id)
+  end
+
+  def create_rule(id, project_id:, rule_type:)
+    approval_project_rules.create(
+      id: id, project_id: project_id, rule_type: rule_type,
+      approvals_required: 3, name: ApprovalRuleLike::ALL_MEMBERS)
+  end
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+
+    create_project(1)
+    create_rule(1, project_id: 1, rule_type: 3)
+
+    create_project(2)
+    create_rule(2, project_id: 2, rule_type: 4)
+
+    create_project(3)
+    create_rule(3, project_id: 3, rule_type: 3)
+    create_rule(4, project_id: 3, rule_type: 4)
+
+    create_project(4)
+    create_rule(5, project_id: 4, rule_type: 3)
+    create_rule(6, project_id: 4, rule_type: 4)
+
+    create_project(5)
+    create_rule(7, project_id: 5, rule_type: 4)
+  end
+
+  it 'sets all rule types to 3 and removes duplicates' do
+    expect(approval_project_rules.where(rule_type: 4).count).to eq(4)
+    expect(approval_project_rules.where(rule_type: 3).count).to eq(3)
+
+    expect { migrate! }.to change(approval_project_rules, :count).from(7).to(5)
+
+    expect(approval_project_rules.where(rule_type: 4)).to eq([])
+
+    rows = approval_project_rules.where(rule_type: 3).order(:id).map do |row|
+      row.attributes.slice('id', 'project_id')
+    end
+
+    expect(rows).to eq([
+      { "id" => 1, "project_id" => 1 },
+      { "id" => 2, "project_id" => 2 },
+      { "id" => 3, "project_id" => 3 },
+      { "id" => 5, "project_id" => 4 },
+      { "id" => 7, "project_id" => 5 }
+    ])
+  end
+end