Merge branch '12035-fix-regression-modifying-mr-approval-rules' into 'master'

Add ApprovalMRRule fallback for legacy code_owner rules

Closes #12035

See merge request gitlab-org/gitlab-ee!14037

ee/app/models/approval_merge_request_rule.rb

@@ -8,8 +8,6 @@ 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`
@@ -31,18 +29,34 @@ class ApprovalMergeRequestRule < ApplicationRecord
     code_owner: 2
   }
 
+  # Deprecated scope until code_owner column has been migrated to rule_type
+  scope :code_owner, -> { where(code_owner: true).or(where(rule_type: :code_owner)) }
+
   def self.find_or_create_code_owner_rule(merge_request, pattern)
-    merge_request.approval_rules.safe_find_or_create_by(
-      rule_type: :code_owner,
-      code_owner: true, # deprecated, replaced with `rule_type: :code_owner`
-      name: pattern
-    )
+    merge_request.approval_rules.code_owner.where(name: pattern).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
   end
 
   def project
     merge_request.target_project
   end
 
+  # ApprovalRuleLike interface
+  # Temporary override to handle legacy records that have not yet been migrated
+  def regular?
+    read_attribute(:rule_type) == 'regular' || code_owner == false
+  end
+  alias_method :regular, :regular?
+
+  # Temporary override to handle legacy records that have not yet been migrated
+  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
@@ -74,9 +88,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
     self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 
-  # ApprovalRuleLike interface
-  alias_method :regular, :regular?
-
   private
 
   def validate_approvals_required

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -96,12 +96,20 @@ describe ApprovalMergeRequestRule do
 
     it 'creates a new rule if it does not exist' do
       expect { described_class.find_or_create_code_owner_rule(merge_request, '*.js') }
-        .to change { merge_request.approval_rules.count }.by(1)
+        .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: '*.md', merge_request: merge_request)
+      deprecated_code_owner_rule.update_column(:rule_type, described_class.rule_types[:regular])
+
+      expect(described_class.find_or_create_code_owner_rule(merge_request, '*.md'))
+        .to eq(deprecated_code_owner_rule)
     end
 
     it 'retries when a record was created between the find and the create' do
-      expect(described_class).to receive(:find_or_create_by).and_raise(ActiveRecord::RecordNotUnique)
-      allow(described_class).to receive(:find_or_create_by).and_call_original
+      expect(described_class).to receive(:where).and_raise(ActiveRecord::RecordNotUnique)
+      allow(described_class).to receive(:where).and_call_original
 
       expect(described_class.find_or_create_code_owner_rule(merge_request, '*.js')).not_to be_nil
     end
@@ -130,6 +138,18 @@ describe ApprovalMergeRequestRule do
     end
   end
 
+  describe '#code_owner?' do
+    it 'returns true when deprecated code_owner bool is set' do
+      code_owner_rule = build(:code_owner_rule)
+
+      expect(code_owner_rule.code_owner?).to be true
+
+      code_owner_rule.rule_type = :regular
+
+      expect(code_owner_rule.code_owner?).to be true
+    end
+  end
+
   describe '#approvers' do
     before do
       create(:group) do |group|