Commit aa4a4e2e authored by Kerri Miller's avatar Kerri Miller Committed by Stan Hu

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

This reverts commit 4075ec82.
parent af1f4022
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment