From 367ef6f6661bbbe405adb4a67d125e9b7552a301 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Mon, 20 May 2019 15:43:06 -0700
Subject: [PATCH 01/16] 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                                  |  4 +-
 ee/app/models/approval_merge_request_rule.rb  | 17 +++++---
 ...nvert-approval_rules_rule_type_to_enum.yml |  5 +++
 ...o_approval_merge_request_approval_rules.rb | 21 +++++++++
 ...le_type_on_approval_merge_request_rules.rb | 43 +++++++++++++++++++
 ee/spec/factories/approval_rules.rb           |  3 +-
 .../approval_merge_request_rule_spec.rb       |  2 +-
 .../visible_approvable_for_rule_spec.rb       |  2 +-
 8 files changed, 86 insertions(+), 11 deletions(-)
 create mode 100644 ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml
 create mode 100644 ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb
 create mode 100644 ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb

diff --git a/db/schema.rb b/db/schema.rb
index 9ac866d9f43a39..5502d26b2f8233 100644
--- a/db/schema.rb
+++ b/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: 20190520201748) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -244,8 +244,10 @@
     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 ["rule_type"], name: "index_approval_merge_request_rules_on_rule_type", using: :btree
   end
 
   create_table "approval_merge_request_rules_approved_approvers", force: :cascade do |t|
diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 199a3da9ff5eb8..c0ec6ddb0cd2d8 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -6,8 +6,8 @@ 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 :regular, -> { where(rule_type: :regular) }
+  scope :code_owner, -> { where(rule_type: :code_owner) } # 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) }
 
@@ -23,9 +23,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 +71,7 @@ def sync_approved_approvers
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 
-  def regular
-    !code_owner?
-  end
-  alias_method :regular?, :regular
+  alias_method :regular, :regular?
 
   private
 
diff --git a/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml b/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml
new file mode 100644
index 00000000000000..13b9abf9a0abc8
--- /dev/null
+++ b/ee/changelogs/unreleased/9928-convert-approval_rules_rule_type_to_enum.yml
@@ -0,0 +1,5 @@
+---
+title: Migrate code_owners to rule_type enum on approval_merge_request_rules
+merge_request: 13036
+author:
+type: changed
diff --git a/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb b/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb
new file mode 100644
index 00000000000000..7339a4fccbad81
--- /dev/null
+++ b/ee/db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
new file mode 100644
index 00000000000000..374403a2f941a1
--- /dev/null
+++ b/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
@@ -0,0 +1,43 @@
+# 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 IndexRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  CODE_OWNERS_INDEX_NAME = 'approval_rule_name_index_for_code_owners'
+
+  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
+
+    remove_concurrent_index :approval_merge_request_rules, :rule_type
+    add_concurrent_index :approval_merge_request_rules, :rule_type
+  end
+
+  def down
+    ApprovalMergeRequestRule.where(rule_type: ApprovalMergeRequestRule.rule_types[:code_owner]).each_batch do |batch|
+      batch.update_all(code_owner: true)
+    end
+
+    remove_concurrent_index :approval_merge_request_rules, :rule_type
+  end
+end
diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb
index 6065871cdc0188..99baad91b4605a 100644
--- a/ee/spec/factories/approval_rules.rb
+++ b/ee/spec/factories/approval_rules.rb
@@ -8,7 +8,8 @@
 
   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
 
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index e9bc5c55c55c01..cc773492a095a4 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -100,7 +100,7 @@
     end
 
     it 'returns false for code owner records' do
-      subject = create(:approval_merge_request_rule, merge_request: merge_request, code_owner: true)
+      subject = create(:approval_merge_request_rule, merge_request: merge_request, rule_type: :code_owner)
 
       expect(subject.regular).to eq(false)
       expect(subject.regular?).to eq(false)
diff --git a/ee/spec/models/visible_approvable_for_rule_spec.rb b/ee/spec/models/visible_approvable_for_rule_spec.rb
index 33c9558c765faa..fecc68bf377c52 100644
--- a/ee/spec/models/visible_approvable_for_rule_spec.rb
+++ b/ee/spec/models/visible_approvable_for_rule_spec.rb
@@ -35,7 +35,7 @@
     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)
-- 
GitLab


From 52bffa269abb5faf1238ca987a7c5f0daedbc1ee Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Tue, 21 May 2019 17:05:26 -0700
Subject: [PATCH 02/16] Remove unnecessary code_owner logic in approval_rule
 index migration

---
 ...0201748_index_rule_type_on_approval_merge_request_rules.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
index 374403a2f941a1..62837363a2814b 100644
--- a/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
+++ b/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
@@ -34,10 +34,6 @@ def up
   end
 
   def down
-    ApprovalMergeRequestRule.where(rule_type: ApprovalMergeRequestRule.rule_types[:code_owner]).each_batch do |batch|
-      batch.update_all(code_owner: true)
-    end
-
     remove_concurrent_index :approval_merge_request_rules, :rule_type
   end
 end
-- 
GitLab


From 94fa1afe5ed3db899ef4cccaefe2477c6421836d Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 22 May 2019 14:33:49 -0700
Subject: [PATCH 03/16] Drop unnecessary index from rule_type_on_approval MR
 rules migration

---
 db/schema.rb                                             | 1 -
 ...opulate_rule_type_on_approval_merge_request_rules.rb} | 9 ++-------
 2 files changed, 2 insertions(+), 8 deletions(-)
 rename ee/db/migrate/{20190520201748_index_rule_type_on_approval_merge_request_rules.rb => 20190520201748_populate_rule_type_on_approval_merge_request_rules.rb} (70%)

diff --git a/db/schema.rb b/db/schema.rb
index 5502d26b2f8233..302b19156beb02 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -247,7 +247,6 @@
     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 ["rule_type"], name: "index_approval_merge_request_rules_on_rule_type", using: :btree
   end
 
   create_table "approval_merge_request_rules_approved_approvers", force: :cascade do |t|
diff --git a/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
similarity index 70%
rename from ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
rename to ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
index 62837363a2814b..9c37425b79a81d 100644
--- a/ee/db/migrate/20190520201748_index_rule_type_on_approval_merge_request_rules.rb
+++ b/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
@@ -3,14 +3,12 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
-class IndexRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
+class PopualteRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
   include Gitlab::Database::MigrationHelpers
 
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
-  CODE_OWNERS_INDEX_NAME = 'approval_rule_name_index_for_code_owners'
-
   disable_ddl_transaction!
 
   class ApprovalMergeRequestRule < ActiveRecord::Base
@@ -28,12 +26,9 @@ def up
     ApprovalMergeRequestRule.where(code_owner: true).each_batch do |batch|
       batch.update_all(rule_type: ApprovalMergeRequestRule.rule_types[:code_owner])
     end
-
-    remove_concurrent_index :approval_merge_request_rules, :rule_type
-    add_concurrent_index :approval_merge_request_rules, :rule_type
   end
 
   def down
-    remove_concurrent_index :approval_merge_request_rules, :rule_type
+    # code_owner is already kept in sync with `rule_type`, so no changes are needd
   end
 end
-- 
GitLab


From 14121c030c7fb9e8fe9fa18c27dc1fa0593f73e3 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 22 May 2019 14:40:48 -0700
Subject: [PATCH 04/16] Remove redundant enum scopes for
 ApprovalMergeRequestRules

---
 ee/app/models/approval_merge_request_rule.rb       | 2 --
 ee/spec/models/approval_merge_request_rule_spec.rb | 7 +++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index c0ec6ddb0cd2d8..78108a41be1d90 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -6,8 +6,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   DEFAULT_NAME_FOR_CODE_OWNER = 'Code Owner'
 
-  scope :regular, -> { where(rule_type: :regular) }
-  scope :code_owner, -> { where(rule_type: :code_owner) } # 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) }
 
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index cc773492a095a4..d79dc33dff47e1 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -60,6 +60,13 @@
           .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
-- 
GitLab


From 7086d4dfd374426cafcae28a6071f960416d83b8 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 22 May 2019 21:42:58 +0000
Subject: [PATCH 05/16] Apply suggestion to
 ee/app/models/approval_merge_request_rule.rb

---
 ee/app/models/approval_merge_request_rule.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 78108a41be1d90..284a3fb86246ca 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -69,6 +69,7 @@ def sync_approved_approvers
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 
+  # ApprovalRuleLike interface
   alias_method :regular, :regular?
 
   private
-- 
GitLab


From 1a8856b825f6d0b6fc0a257c6c597cb0677002da Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 22 May 2019 21:48:17 +0000
Subject: [PATCH 06/16] Rename index_rule_type to populate_rule_type

---
 ...1748_populate_rule_type_on_approval_merge_request_rules.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
index 9c37425b79a81d..0f0df45613444e 100644
--- a/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
+++ b/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
@@ -3,7 +3,7 @@
 # See http://doc.gitlab.com/ce/development/migration_style_guide.html
 # for more information on how to write migrations for GitLab.
 
-class PopualteRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
+class PopulateRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Migration[5.1]
   include Gitlab::Database::MigrationHelpers
 
   # Set this constant to true if this migration requires downtime.
@@ -29,6 +29,6 @@ def up
   end
 
   def down
-    # code_owner is already kept in sync with `rule_type`, so no changes are needd
+    # code_owner is already kept in sync with `rule_type`, so no changes are needed
   end
 end
-- 
GitLab


From 09e05d5d226cfc0c46bfb2383e7f25b7fe91e3d6 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Thu, 23 May 2019 16:38:50 -0700
Subject: [PATCH 07/16] Validate code_owner=true if approval rule_type is
 :code_owner

Ensures approval records are in sync during transition between
boolean code_owner and introduction of rule_type enum
---
 ee/app/models/approval_merge_request_rule.rb       |  1 +
 ee/spec/models/approval_merge_request_rule_spec.rb | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 284a3fb86246ca..d566488b8a3113 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -10,6 +10,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
 
   validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
+  validates :code_owner, inclusion: { in: [true], if: :code_owner? }
 
   belongs_to :merge_request, inverse_of: :approval_rules
 
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index d79dc33dff47e1..194c0304e65bf4 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -38,6 +38,14 @@
         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
     end
   end
 
@@ -107,7 +115,7 @@
     end
 
     it 'returns false for code owner records' do
-      subject = create(:approval_merge_request_rule, merge_request: merge_request, rule_type: :code_owner)
+      subject = create(:code_owner_rule, merge_request: merge_request)
 
       expect(subject.regular).to eq(false)
       expect(subject.regular?).to eq(false)
-- 
GitLab


From b65974f61f5063b72ae20a9a2b0c714ad385af8e Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Thu, 23 May 2019 17:08:02 -0700
Subject: [PATCH 08/16] Validate code_owner=false if approval rule_type is
 :regular

Ensures approval records are in sync during transition between
boolean code_owner and introduction of rule_type enum
---
 ee/app/models/approval_merge_request_rule.rb       | 2 ++
 ee/spec/models/approval_merge_request_rule_spec.rb | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index d566488b8a3113..c50b2ea9b16383 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -10,7 +10,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
 
   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
 
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 194c0304e65bf4..a6c1d6853c4974 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -46,6 +46,14 @@
         new.code_owner = false
         expect(new).not_to be_valid
       end
+
+      it 'validates code_owner when rule_type code_owner' 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
 
-- 
GitLab


From 9c8cef7933f0b60a28dbd6c8db3f7b1ec6d98ad7 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Tue, 28 May 2019 10:32:17 -0700
Subject: [PATCH 09/16] Re-add code_owner approval scope until post_migrate has
 ran

---
 ee/app/models/approval_merge_request_rule.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index c50b2ea9b16383..080b730eb1c48a 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -8,6 +8,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   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`
-- 
GitLab


From 021fa2be5ef3cb939fa06b0fb0141482d88585a9 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Tue, 28 May 2019 10:50:43 -0700
Subject: [PATCH 10/16] Move PopulateRuleTypeOnApprovalMRRules to post_migrate

---
 ...20201748_populate_rule_type_on_approval_merge_request_rules.rb | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename ee/db/{migrate => post_migrate}/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb (100%)

diff --git a/ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb b/ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
similarity index 100%
rename from ee/db/migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
rename to ee/db/post_migrate/20190520201748_populate_rule_type_on_approval_merge_request_rules.rb
-- 
GitLab


From faa2218e1980da8d09d0a74fc4707d255671bd98 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Tue, 28 May 2019 10:51:01 -0700
Subject: [PATCH 11/16] Add index migration for code_owner approval rule_type

Adds indexes for querying against approvals table for
rule_type=code_owner
---
 db/schema.rb                                  |  4 +-
 ...le_type_on_approval_merge_request_rules.rb | 48 +++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb

diff --git a/db/schema.rb b/db/schema.rb
index 302b19156beb02..6543da73547597 100644
--- a/db/schema.rb
+++ b/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: 20190520201748) do
+ActiveRecord::Schema.define(version: 20190528173628) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -247,6 +247,8 @@
     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|
diff --git a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
new file mode 100644
index 00000000000000..6c34c267a52e99
--- /dev/null
+++ b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
@@ -0,0 +1,48 @@
+# 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'
+
+  def up
+    # Ensure only 1 code_owner rule per merge_request
+    add_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :rule_type, :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, :rule_type],
+      where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}",
+      name: INDEX_CODE_OWNERS_RULES_QUERY_NAME
+    )
+  end
+
+  def down
+    remove_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :rule_type, :name],
+      name: INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
+    )
+
+    remove_concurrent_index(
+      :approval_merge_request_rules,
+      [:merge_request_id, :rule_type],
+      name: INDEX_CODE_OWNERS_RULES_QUERY_NAME
+    )
+  end
+end
-- 
GitLab


From 912f46c6ab686d3ae1aaff8df44b9774763a56e5 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Tue, 28 May 2019 12:46:12 -0700
Subject: [PATCH 12/16] Update sync_code_owners_with_approvers to use rule_type
 enum

---
 ee/app/models/ee/merge_request.rb                        | 5 ++++-
 ee/spec/models/merge_request_spec.rb                     | 2 +-
 ee/spec/services/approval_rules/finalize_service_spec.rb | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index fb26e85e2634c6..aca946d6b6340e 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -177,7 +177,10 @@ def sync_code_owners_with_approvers
       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
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index e10febcd416681..d3874959d43d17 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -490,7 +490,7 @@
     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
diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb
index 2ed4dec67d5390..b6861f2e2813c4 100644
--- a/ee/spec/services/approval_rules/finalize_service_spec.rb
+++ b/ee/spec/services/approval_rules/finalize_service_spec.rb
@@ -45,7 +45,7 @@
         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
-- 
GitLab


From 61eb1b4f25ec229c528013cc63a38213b97de60e Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 29 May 2019 07:55:46 -0700
Subject: [PATCH 13/16] Update code_owner approval rule_type migration to not
 depend on model

---
 ...de_owner_rule_type_on_approval_merge_request_rules.rb | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
index 6c34c267a52e99..7c1836f463b92d 100644
--- a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
+++ b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
@@ -13,6 +13,15 @@ class AddIndexForCodeOwnerRuleTypeOnApprovalMergeRequestRules < ActiveRecord::Mi
   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(
-- 
GitLab


From 8c0e3738f9e9542bd76a71ebc44c034c3d935553 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Wed, 29 May 2019 11:40:51 -0700
Subject: [PATCH 14/16] Add populate rule_type migration spec

---
 ...pe_on_approval_merge_request_rules_spec.rb | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb

diff --git a/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb b/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb
new file mode 100644
index 00000000000000..143a7c53a831b7
--- /dev/null
+++ b/ee/spec/migrations/populate_rule_type_on_approval_merge_request_rules_spec.rb
@@ -0,0 +1,36 @@
+# 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
-- 
GitLab


From 2b3cffb8bcabf446e54c99b67b5bb1a54ab86c15 Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Thu, 30 May 2019 16:11:47 -0700
Subject: [PATCH 15/16] Remove redundant rule_type in code_owner indexes

---
 ...er_rule_type_on_approval_merge_request_rules.rb | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
index 7c1836f463b92d..d43319ad1f0626 100644
--- a/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
+++ b/ee/db/migrate/20190528173628_add_index_for_code_owner_rule_type_on_approval_merge_request_rules.rb
@@ -26,7 +26,7 @@ def up
     # Ensure only 1 code_owner rule per merge_request
     add_concurrent_index(
       :approval_merge_request_rules,
-      [:merge_request_id, :rule_type, :name],
+      [:merge_request_id, :name],
       unique: true,
       where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}",
       name: INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
@@ -35,23 +35,21 @@ def up
     # Support lookups for all code_owner rules per merge_request
     add_concurrent_index(
       :approval_merge_request_rules,
-      [:merge_request_id, :rule_type],
+      [:merge_request_id],
       where: "rule_type = #{ApprovalMergeRequestRule.rule_types[:code_owner]}",
       name: INDEX_CODE_OWNERS_RULES_QUERY_NAME
     )
   end
 
   def down
-    remove_concurrent_index(
+    remove_concurrent_index_by_name(
       :approval_merge_request_rules,
-      [:merge_request_id, :rule_type, :name],
-      name: INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
+      INDEX_CODE_OWNERS_RULES_UNIQUENESS_NAME
     )
 
-    remove_concurrent_index(
+    remove_concurrent_index_by_name(
       :approval_merge_request_rules,
-      [:merge_request_id, :rule_type],
-      name: INDEX_CODE_OWNERS_RULES_QUERY_NAME
+      INDEX_CODE_OWNERS_RULES_QUERY_NAME
     )
   end
 end
-- 
GitLab


From a5f77f946b63ce6cbee74333ca6973989a37b53a Mon Sep 17 00:00:00 2001
From: Lucas Charles <me@lucascharles.me>
Date: Fri, 31 May 2019 00:30:43 +0000
Subject: [PATCH 16/16] Apply suggestion to
 ee/spec/models/approval_merge_request_rule_spec.rb

---
 ee/spec/models/approval_merge_request_rule_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index a6c1d6853c4974..0928c3161f2c62 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -47,7 +47,7 @@
         expect(new).not_to be_valid
       end
 
-      it 'validates code_owner when rule_type code_owner' do
+      it 'validates code_owner when rule_type regular' do
         new = build(:approval_merge_request_rule)
         expect(new).to be_valid
 
-- 
GitLab