Refactor MR approval_rules.code_owners bool to rule_type enum

Adds enum to better represent the different categories of
approval_merge_request_approval_rules. This will later be expanded but
by initially deprecating the boolean, it should be easier to expand the
functionality with new approval rule types

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190527194900) do
+ActiveRecord::Schema.define(version: 20190528173628) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -244,8 +244,11 @@ ActiveRecord::Schema.define(version: 20190527194900) do
     t.integer "approvals_required", limit: 2, default: 0, null: false
     t.boolean "code_owner", default: false, null: false
     t.string "name", null: false
+    t.integer "rule_type", limit: 2, default: 1, null: false
     t.index ["merge_request_id", "code_owner", "name"], name: "approval_rule_name_index_for_code_owners", unique: true, where: "(code_owner = true)", using: :btree
     t.index ["merge_request_id", "code_owner"], name: "index_approval_merge_request_rules_1", using: :btree
+    t.index ["merge_request_id", "rule_type", "name"], name: "index_approval_rule_name_for_code_owners_rule_type", unique: true, where: "(rule_type = 2)", using: :btree
+    t.index ["merge_request_id", "rule_type"], name: "index_approval_rules_code_owners_rule_type", where: "(rule_type = 2)", using: :btree
   end
 
   create_table "approval_merge_request_rules_approved_approvers", force: :cascade do |t|

ee/app/models/approval_merge_request_rule.rb

@@ -6,12 +6,15 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   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
   scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
+  # Deprecated scope until code_owner column has been migrated to rule_type
+  scope :code_owner, -> { where(code_owner: true).or(rule_type: :code_owner) }
 
   validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
+  # Temporary validations until `code_owner` can be dropped in favor of `rule_type`
+  validates :code_owner, inclusion: { in: [true], if: :code_owner? }
+  validates :code_owner, inclusion: { in: [false], if: :regular? }
 
   belongs_to :merge_request, inverse_of: :approval_rules
 
@@ -23,9 +26,15 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   validate :validate_approvals_required
 
+  enum rule_type: {
+    regular: 1,
+    code_owner: 2
+  }
+
   def self.find_or_create_code_owner_rule(merge_request, pattern)
     merge_request.approval_rules.safe_find_or_create_by(
-      code_owner: true,
+      rule_type: :code_owner,
+      code_owner: true, # deprecated, replaced with `rule_type: :code_owner`
       name: pattern
     )
   end
@@ -65,10 +74,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 
-  def regular
-    !code_owner?
-  end
-  alias_method :regular?, :regular
+  # ApprovalRuleLike interface
+  alias_method :regular, :regular?
 
   private
 

ee/app/models/ee/merge_request.rb

@@ -177,7 +177,10 @@ module EE
       if owners.present?
         ApplicationRecord.transaction do
           rule = approval_rules.code_owner.first
-          rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
+          rule ||= ApprovalMergeRequestRule.find_or_create_code_owner_rule(
+            self,
+            ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER
+          )
 
           rule.users = owners.uniq
         end

ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml

+---
+title: Migrate code_owners to rule_type enum on approval_merge_request_rules
+merge_request: 13036
+author:
+type: changed

ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.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 AddRuleTypeToApprovalMergeRequestApprovalRules < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:approval_merge_request_rules, :rule_type, :integer, limit: 2, default: 1)
+  end
+
+  def down
+    remove_column(:approval_merge_request_rules, :rule_type)
+  end
+end

ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.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 AddIndexForCodeOwnerRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME = 'index_approval_rule_name_for_code_owners_rule_type'
+  INDEX_CODE_OWNERS_RULES_QUERY_NAME = 'index_approval_rules_code_owners_rule_type'
+
+  class ApprovalMergeRequestRule < ActiveRecord::Base
+    include EachBatch
+
+    enum rule_types: {
+      regular: 1,
+      code_owner: 2
+    }
+  end
+
+  def up
+    # Ensure only 1 code_owner rule per merge_request
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :name],
+      unique: true,
+      where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}",
+      name: INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
+    )
+
+    # Support lookups for all code_owner rules per merge_request
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id],
+      where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}",
+      name: INDEX_CODE_OWNERS_RULES_QUERY_NAME
+    )
+  end
+
+  def down
+    remove_concurrent_index_by_name(
+      :approval_merge_request_rules,
+      INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
+    )
+
+    remove_concurrent_index_by_name(
+      :approval_merge_request_rules,
+      INDEX_CODE_OWNERS_RULES_QUERY_NAME
+    )
+  end
+end

ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.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 PopulateRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class ApprovalMergeRequestRule < ActiveRecord::Base
+    include EachBatch
+
+    enum rule_types: {
+      regular: 1,
+      code_owner: 2
+    }
+  end
+
+  def up
+    # On Gitlab.com, this should update about 17k rows. Since our updates are
+    # small and we are populating prior to indexing, the overhead should be small
+    ApprovalMergeRequestRule.where(code_owner: true).each_batch do |batch|
+      batch.update_all(rule_type: ApprovalMergeRequestRule.rule_types[:code_owner])
+    end
+  end
+
+  def down
+    # code_owner is already kept in sync with `rule_type`, so no changes are needed
+  end
+end

ee/spec/factories/approval_rules.rb

@@ -8,7 +8,8 @@ FactoryBot.define do
 
   factory :code_owner_rule, parent: :approval_merge_request_rule do
     merge_request
-    code_owner true
+    rule_type :code_owner
+    code_owner true # deprecated, replaced with `rule_type: :code_owner`
     sequence(:name) { |n| "*-#{n}.js" }
   end
 

ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('ee', 'db', 'post_migrate', '20190520201748_populate_rule_type_on_approval_merge_request_rules.rb')
+
+describe PopulateRuleTypeOnApprovalMergeRequestRules, :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_rules) { table(:approval_merge_request_rules) }
+    let(:regular_rule_type) { ApprovalMergeRequestRule.rule_types[:regular] }
+    let(:code_owner_rule_type) { ApprovalMergeRequestRule.rule_types[:code_owner] }
+
+    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')
+
+      approval_rules.create!(id: 1, merge_request_id: 1, name: "Default", code_owner: false, rule_type: regular_rule_type)
+      approval_rules.create!(id: 2, merge_request_id: 1, name: "Code Owners", code_owner: true, rule_type: regular_rule_type)
+    end
+
+    it 'backfills ApprovalMergeRequestRules code_owner rule_type' do
+      expect(approval_rules.where(rule_type: regular_rule_type).pluck(:id)).to contain_exactly(1, 2)
+      expect(approval_rules.where(rule_type: code_owner_rule_type).pluck(:id)).to be_empty
+
+      migrate!
+
+      expect(approval_rules.where(rule_type: regular_rule_type).pluck(:id)).to contain_exactly(1)
+      expect(approval_rules.where(rule_type: code_owner_rule_type).pluck(:id)).to contain_exactly(2)
+    end
+  end
+end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -38,6 +38,22 @@ describe ApprovalMergeRequestRule do
         expect(new).to be_valid
         expect { new.save! }.not_to raise_error
       end
+
+      it 'validates code_owner when rule_type code_owner' do
+        new = build(:code_owner_rule)
+        expect(new).to be_valid
+
+        new.code_owner = false
+        expect(new).not_to be_valid
+      end
+
+      it 'validates code_owner when rule_type regular' do
+        new = build(:approval_merge_request_rule)
+        expect(new).to be_valid
+
+        new.code_owner = true
+        expect(new).not_to be_valid
+      end
     end
   end
 
@@ -60,6 +76,13 @@ describe ApprovalMergeRequestRule do
           .to contain_exactly(rb_rule, js_rule)
       end
     end
+
+    describe '.code_owners' do
+      it 'returns the correct rules' do
+        expect(described_class.code_owner)
+          .to contain_exactly(rb_rule, js_rule, css_rule)
+      end
+    end
   end
 
   describe '.find_or_create_code_owner_rule' do
@@ -100,7 +123,7 @@ describe ApprovalMergeRequestRule do
     end
 
     it 'returns false for code owner records' do
-      subject = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true)
+      subject = create(:code_owner_rule, merge_request: merge_request)
 
       expect(subject.regular).to eq(false)
       expect(subject.regular?).to eq(false)

ee/spec/models/merge_request_spec.rb

@@ -490,7 +490,7 @@ describe MergeRequest do
     end
 
     context 'when code owner rule exists' do
-      let!(:code_owner_rule) { subject.approval_rules.code_owner.create!(name: 'Code Owner', users: [create(:user)]) }
+      let!(:code_owner_rule) { create(:code_owner_rule, merge_request: subject, name: 'Code Owner', users: [create(:user)]) }
 
       it 'reuses and updates existing rule' do
         expect do

ee/spec/models/visible_approvable_for_rule_spec.rb

@@ -35,7 +35,7 @@ describe VisibleApprovableForRule do
     let(:code_owner) { build(:user) }
 
     let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
-    let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [code_owner], code_owner: true) }
+    let!(:code_owner_rule) { create(:code_owner_rule, merge_request: resource, users: [code_owner]) }
 
     before do
       project.add_developer(approver)

ee/spec/services/approval_rules/finalize_service_spec.rb

@@ -45,7 +45,7 @@ describe ApprovalRules::FinalizeService do
         let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
 
         before do
-          merge_request.approval_rules.code_owner.create(name: 'Code Owner')
+          merge_request.approval_rules.code_owner.create(name: 'Code Owner', rule_type: :code_owner, code_owner: true)
         end
 
         it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do