Merge branch 'revert-a8f152d8' into 'master'

Revert "Merge branch '11834-remove-and-ignore-code_owner-attribute' into 'master'"

See merge request gitlab-org/gitlab!38297

ee/app/models/approval_merge_request_rule.rb

@@ -5,9 +5,6 @@ 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) }
 
@@ -33,6 +30,10 @@ 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
 
@@ -51,14 +52,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]) }
@@ -68,6 +69,7 @@ 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
@@ -96,6 +98,20 @@ 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/spec/factories/approval_rules.rb

@@ -14,6 +14,7 @@ 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/models/approval_merge_request_rule_spec.rb

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