Merge branch 'kerrizor/rever-rever-of-code-owner-column-removal' into 'master'

Revert revert of code owner column ignore/removal

See merge request gitlab-org/gitlab!38341

ee/app/models/approval_merge_request_rule.rb

@@ -5,6 +5,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
   include ApprovalRuleLike
   include UsageStatistics
 
+  include IgnorableColumns
+  ignore_column :code_owner, remove_with: '13.5', remove_after: '2020-10-22'
+
   scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
 
@@ -30,10 +33,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
   validates :name, uniqueness: { scope: [:merge_request_id, :rule_type, :section] }
   validates :rule_type, uniqueness: { scope: :merge_request_id, message: proc { _('any-approver for the merge request already exists') } }, if: :any_approver?
   validates :report_type, presence: true, if: :report_approver?
-  # Temporary validations until `code_owner` can be dropped in favor of `rule_type`
-  # To be removed with https://gitlab.com/gitlab-org/gitlab/issues/11834
-  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
 
@@ -52,14 +51,14 @@ class ApprovalMergeRequestRule < ApplicationRecord
     any_approver: 4
   }
 
+  alias_method :regular, :regular?
+  alias_method :code_owner, :code_owner?
+
   enum report_type: {
     security: 1,
     license_scanning: 2
   }
 
-  # Deprecated scope until code_owner column has been migrated to rule_type
-  # To be removed with https://gitlab.com/gitlab-org/gitlab/issues/11834
-  scope :code_owner, -> { where(code_owner: true).or(where(rule_type: :code_owner)) }
   scope :security_report, -> { report_approver.where(report_type: :security) }
   scope :license_compliance, -> { report_approver.where(report_type: :license_scanning) }
   scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) }
@@ -69,7 +68,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
   def self.find_or_create_code_owner_rule(merge_request, entry)
     merge_request.approval_rules.code_owner.where(name: entry.pattern).where(section: entry.section).first_or_create do |rule|
       rule.rule_type = :code_owner
-      rule.code_owner = true # deprecated, replaced with `rule_type: :code_owner`
     end
   rescue ActiveRecord::RecordNotUnique
     retry
@@ -98,20 +96,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
     merge_request.target_project
   end
 
-  # ApprovalRuleLike interface
-  # Temporary override to handle legacy records that have not yet been migrated
-  # To be removed with https://gitlab.com/gitlab-org/gitlab/issues/11834
-  def regular?
-    read_attribute(:rule_type) == 'regular' || (!report_approver? && !code_owner && !any_approver?)
-  end
-  alias_method :regular, :regular?
-
-  # Temporary override to handle legacy records that have not yet been migrated
-  # To be removed with https://gitlab.com/gitlab-org/gitlab/issues/11834
-  def code_owner?
-    read_attribute(:rule_type) == 'code_owner' || code_owner
-  end
-
   def approval_project_rule_id=(approval_project_rule_id)
     self.approval_merge_request_rule_source ||= build_approval_merge_request_rule_source
     self.approval_merge_request_rule_source.approval_project_rule_id = approval_project_rule_id

ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -32,21 +32,28 @@ module EE
           self.table_name = 'approval_merge_request_rules'
 
           belongs_to :merge_request
-          scope :code_owner, -> { where(code_owner: true) }
-          scope :regular, -> { where(code_owner: false) } # Non code owner rule
+          scope :code_owner, -> { where(code_owner: true).or(where(rule_type: :code_owner)) }
+          scope :regular, -> { where(code_owner: false).or(where(rule_type: :regular)) } # 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_one :approval_merge_request_rule_source
           has_one :approval_project_rule, through: :approval_merge_request_rule_source
 
+          enum rule_type: {
+            regular: 1,
+            code_owner: 2,
+            report_approver: 3,
+            any_approver: 4
+          }
+
           def project
             merge_request.target_project
           end
 
           def self.find_or_create_code_owner_rule(merge_request, entry)
             merge_request.approval_rules.safe_find_or_create_by(
-              code_owner: true,
+              rule_type: :code_owner,
               name: entry.pattern
             )
           end

ee/spec/factories/approval_rules.rb

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

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

@@ -221,7 +221,7 @@ RSpec.describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRules do
 
             rule = target.approval_rules.first
 
-            expect(rule.read_attribute(:code_owner)).to eq(true)
+            expect(rule.code_owner).to eq(true)
             expect(rule.users).to contain_exactly(*owners)
           end
         end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -61,22 +61,6 @@ RSpec.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
 
     context 'report_approver rules' do
@@ -184,12 +168,15 @@ RSpec.describe ApprovalMergeRequestRule do
         .to change { merge_request.approval_rules.matching_pattern('*.js').count }.by(1)
     end
 
-    it 'finds an existing rule using deprecated code_owner column' do
-      deprecated_code_owner_rule = create(:code_owner_rule, name: '*.js', merge_request: merge_request)
-      deprecated_code_owner_rule.update_column(:rule_type, described_class.rule_types[:regular])
+    it 'finds an existing rule using rule_type column' do
+      regular_rule_type_rule = create(
+        :code_owner_rule,
+        name:          entry.pattern,
+        merge_request: merge_request,
+        rule_type:     described_class.rule_types[:regular]
+      )
 
-      expect(rule)
-        .to eq(deprecated_code_owner_rule)
+      expect(rule).not_to eq(regular_rule_type_rule)
     end
 
     it 'retries when a record was created between the find and the create' do
@@ -303,14 +290,22 @@ RSpec.describe ApprovalMergeRequestRule do
   end
 
   describe '#code_owner?' do
-    it 'returns true when deprecated code_owner bool is set' do
-      code_owner_rule = build(:code_owner_rule)
+    let(:code_owner_rule) { build(:code_owner_rule) }
 
+    context "rule_type is :code_owner" do
+      it "returns true" do
         expect(code_owner_rule.code_owner?).to be true
+      end
+    end
 
+    context "rule_type is :regular" do
+      before do
         code_owner_rule.rule_type = :regular
+      end
 
-      expect(code_owner_rule.code_owner?).to be true
+      it "returns false" do
+        expect(code_owner_rule.code_owner?).to be false
+      end
     end
   end
 

ee/spec/services/approval_rules/finalize_service_spec.rb

@@ -45,7 +45,7 @@ RSpec.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', rule_type: :code_owner, code_owner: true)
+          merge_request.approval_rules.code_owner.create(name: 'Code Owner', rule_type: :code_owner)
         end
 
         it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do