Commit 4081fbd1 authored by Patrick Bajao's avatar Patrick Bajao Committed by Douglas Barbosa Alexandre

Remove validation of MR level approval rules in MR

This removes `MergeRequest#validate_approval_rule_source` which
was responsible to check whether the MR level approval rules
matches the existing project level rules.

Added the validation in `ApprovalMergeRequestRule` to check if the
project of `approval_project_rule` matches the merge request's
project. This way we can ensure that the source rule should still
be in the same project.

This will make the logic simpler for the upcoming API to create
MR level approval rules.
parent 60d6eb18
......@@ -39,6 +39,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
alias_method :source_rule, :approval_project_rule
validate :validate_approvals_required, unless: :report_approver?
validate :validate_approval_project_rule
enum rule_type: {
regular: 1,
......@@ -123,4 +124,11 @@ class ApprovalMergeRequestRule < ApplicationRecord
errors.add(:approvals_required, :greater_than_or_equal_to, count: approval_project_rule.approvals_required)
end
end
def validate_approval_project_rule
return if approval_project_rule.blank?
return if merge_request.project == approval_project_rule.project
errors.add(:approval_project_rule, 'must be for the same project')
end
end
......@@ -35,8 +35,6 @@ module EE
has_many :blocked_merge_requests, through: :blocks_as_blocker
validate :validate_approval_rule_source
delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
delegate :merge_requests_author_approval?, to: :target_project, allow_nil: true
......@@ -106,21 +104,6 @@ module EE
hidden.count
end
def validate_approval_rule_source
return unless approval_rules.any?
local_project_rule_ids = approval_rules.map { |rule| rule.approval_merge_request_rule_source&.approval_project_rule_id }
local_project_rule_ids.compact!
invalid = if new_record?
local_project_rule_ids.to_set != project.visible_regular_approval_rules.pluck(:id).to_set
else
(local_project_rule_ids - project.approval_rule_ids).present?
end
errors.add(:approval_rules, :invalid_sourcing_to_project_rules) if invalid
end
def participant_approvers
strong_memoize(:participant_approvers) do
next [] unless approval_needed?
......
---
title: Remove validation of MR level approval rules in merge requests
merge_request: 14968
author:
type: changed
......@@ -16,6 +16,25 @@ describe ApprovalMergeRequestRule do
expect(build(:approval_merge_request_rule, name: nil)).not_to be_valid
end
context 'approval_project_rule is set' do
let(:approval_project_rule) { build(:approval_project_rule) }
let(:merge_request_rule) { build(:approval_merge_request_rule, merge_request: merge_request, approval_project_rule: approval_project_rule) }
context 'when project of approval_project_rule and merge request matches' do
let(:merge_request) { build(:merge_request, project: approval_project_rule.project) }
it 'is valid' do
expect(merge_request_rule).to be_valid
end
end
context 'when the project of approval_project_rule and merge request does not match' do
it 'is invalid' do
expect(merge_request_rule).to be_invalid
end
end
end
context 'code owner rules' do
it 'is valid' do
expect(build(:code_owner_rule)).to be_valid
......
......@@ -50,92 +50,6 @@ describe MergeRequest do
end
end
describe 'approval_rules' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
context 'when project contains approval_rules' do
let!(:project_rule1) { project.approval_rules.create(name: 'p1') }
let!(:project_rule2) { project.approval_rules.create(name: 'p2') }
context "when creating" do
subject(:merge_request) { build(:merge_request, source_project: project, target_project: project) }
context "when MR has no rule" do
it 'is valid as project rule will be active' do
expect(merge_request).to be_valid
end
end
context "when MR rules exists but do not reference all project rules" do
it 'is invalid' do
merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
expect(merge_request).to be_invalid
expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
end
end
context "when MR rules exists but reference rules other than the project's" do
let(:other_project_rule) { create(:approval_project_rule) }
it 'is invalid' do
merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
merge_request.approval_rules.build(name: 'mr3', approval_project_rule_id: other_project_rule.id)
expect(merge_request).to be_invalid
expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
end
end
context "when MR's rules exists and reference all project's rules" do
it 'is valid' do
merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
expect(merge_request).to be_valid
end
end
end
context "when updating" do
subject!(:merge_request) do
merge_request = build(:merge_request, source_project: project, target_project: project)
merge_request.approval_rules.build(name: 'mr1', approval_project_rule_id: project_rule1.id)
merge_request.approval_rules.build(name: 'mr2', approval_project_rule_id: project_rule2.id)
merge_request.save!
merge_request
end
context "when MR rules reference rules other than the project's" do
let(:other_project_rule) { create(:approval_project_rule) }
it 'is invalid' do
merge_request.approval_rules.build(name: 'mr3', approval_project_rule_id: other_project_rule.id)
expect(merge_request).to be_invalid
expect(merge_request.errors.added?(:approval_rules, :invalid_sourcing_to_project_rules)).to eq(true)
end
end
context 'when project later added a new rule' do
before do
project.approval_rules.create(name: 'p3')
end
it 'can still be saved' do
subject.reload
subject.title = 'foobar'
expect(subject.save).to eq(true)
end
end
end
end
end
describe '#participant_approvers' do
let(:approvers) { create_list(:user, 2) }
let(:code_owners) { create_list(:user, 2) }
......
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