Commit a8f152d8 authored by Stan Hu's avatar Stan Hu

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

Remove deprecated `code_owner` references from ApprovalMergeRequestRule

Closes #230843

See merge request gitlab-org/gitlab!37260
parents 6f8822f0 ffa42ea7
...@@ -5,6 +5,9 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -5,6 +5,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
include ApprovalRuleLike include ApprovalRuleLike
include UsageStatistics 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 :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) } scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
...@@ -30,10 +33,6 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -30,10 +33,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
validates :name, uniqueness: { scope: [:merge_request_id, :rule_type, :section] } 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 :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? 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 belongs_to :merge_request, inverse_of: :approval_rules
...@@ -52,14 +51,14 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -52,14 +51,14 @@ class ApprovalMergeRequestRule < ApplicationRecord
any_approver: 4 any_approver: 4
} }
alias_method :regular, :regular?
alias_method :code_owner, :code_owner?
enum report_type: { enum report_type: {
security: 1, security: 1,
license_scanning: 2 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 :security_report, -> { report_approver.where(report_type: :security) }
scope :license_compliance, -> { report_approver.where(report_type: :license_scanning) } scope :license_compliance, -> { report_approver.where(report_type: :license_scanning) }
scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) } scope :with_head_pipeline, -> { includes(merge_request: [:head_pipeline]) }
...@@ -69,7 +68,6 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -69,7 +68,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
def self.find_or_create_code_owner_rule(merge_request, entry) 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| 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.rule_type = :code_owner
rule.code_owner = true # deprecated, replaced with `rule_type: :code_owner`
end end
rescue ActiveRecord::RecordNotUnique rescue ActiveRecord::RecordNotUnique
retry retry
...@@ -98,20 +96,6 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -98,20 +96,6 @@ class ApprovalMergeRequestRule < ApplicationRecord
merge_request.target_project merge_request.target_project
end 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) 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 ||= build_approval_merge_request_rule_source
self.approval_merge_request_rule_source.approval_project_rule_id = approval_project_rule_id self.approval_merge_request_rule_source.approval_project_rule_id = approval_project_rule_id
......
...@@ -14,7 +14,6 @@ FactoryBot.define do ...@@ -14,7 +14,6 @@ FactoryBot.define do
factory :code_owner_rule, parent: :approval_merge_request_rule do factory :code_owner_rule, parent: :approval_merge_request_rule do
merge_request merge_request
rule_type { :code_owner } rule_type { :code_owner }
code_owner { true } # deprecated, replaced with `rule_type: :code_owner`
sequence(:name) { |n| "*-#{n}.js" } sequence(:name) { |n| "*-#{n}.js" }
section { Gitlab::CodeOwners::Entry::DEFAULT_SECTION } section { Gitlab::CodeOwners::Entry::DEFAULT_SECTION }
end end
......
...@@ -61,22 +61,6 @@ RSpec.describe ApprovalMergeRequestRule do ...@@ -61,22 +61,6 @@ RSpec.describe ApprovalMergeRequestRule do
expect(new).to be_valid expect(new).to be_valid
expect { new.save! }.not_to raise_error expect { new.save! }.not_to raise_error
end 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 end
context 'report_approver rules' do context 'report_approver rules' do
...@@ -184,12 +168,15 @@ RSpec.describe ApprovalMergeRequestRule do ...@@ -184,12 +168,15 @@ RSpec.describe ApprovalMergeRequestRule do
.to change { merge_request.approval_rules.matching_pattern('*.js').count }.by(1) .to change { merge_request.approval_rules.matching_pattern('*.js').count }.by(1)
end end
it 'finds an existing rule using deprecated code_owner column' do it 'finds an existing rule using rule_type column' do
deprecated_code_owner_rule = create(:code_owner_rule, name: '*.js', merge_request: merge_request) regular_rule_type_rule = create(
deprecated_code_owner_rule.update_column(:rule_type, described_class.rule_types[:regular]) :code_owner_rule,
name: entry.pattern,
merge_request: merge_request,
rule_type: described_class.rule_types[:regular]
)
expect(rule) expect(rule).not_to eq(regular_rule_type_rule)
.to eq(deprecated_code_owner_rule)
end end
it 'retries when a record was created between the find and the create' do it 'retries when a record was created between the find and the create' do
...@@ -303,14 +290,22 @@ RSpec.describe ApprovalMergeRequestRule do ...@@ -303,14 +290,22 @@ RSpec.describe ApprovalMergeRequestRule do
end end
describe '#code_owner?' do describe '#code_owner?' do
it 'returns true when deprecated code_owner bool is set' do let(:code_owner_rule) { build(:code_owner_rule) }
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 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 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
end end
......
...@@ -45,7 +45,7 @@ RSpec.describe ApprovalRules::FinalizeService do ...@@ -45,7 +45,7 @@ RSpec.describe ApprovalRules::FinalizeService do
let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) } let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
before do 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 end
it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do 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