Commit 16272d61 authored by Patrick Bajao's avatar Patrick Bajao

Append inapplicable rules when creating MR

When creating an MR, we need to associate all the rules to the MR
so they will still be available when MR's target branch changes.

This will be useful when FE starts to filter rules on MR create.
parent cdf6a458
...@@ -23,6 +23,10 @@ class ApprovalProjectRule < ApplicationRecord ...@@ -23,6 +23,10 @@ class ApprovalProjectRule < ApplicationRecord
includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) } includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
end end
def self.inapplicable_to_branch(branch)
includes(:protected_branches).reject { |rule| rule.applies_to_branch?(branch) }
end
def applies_to_branch?(branch) def applies_to_branch?(branch)
return true if protected_branches.empty? return true if protected_branches.empty?
......
...@@ -31,12 +31,15 @@ module Approvable ...@@ -31,12 +31,15 @@ module Approvable
end end
end end
# rubocop:disable Gitlab/ModuleWithInstanceVariables
def approval_state(target_branch: nil) def approval_state(target_branch: nil)
@approval_state ||= {} approval_state = strong_memoize(:approval_state) do
@approval_state[target_branch] ||= ApprovalState.new(self, target_branch: target_branch) Hash.new do |h, key|
h[key] = ApprovalState.new(self, target_branch: key)
end
end
approval_state[target_branch]
end end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def approvals_given def approvals_given
approvals.size approvals.size
......
...@@ -404,9 +404,13 @@ module EE ...@@ -404,9 +404,13 @@ module EE
end end
def visible_approval_rules(target_branch: nil) def visible_approval_rules(target_branch: nil)
strong_memoize(:"visible_approval_rules_#{target_branch}") do rules = strong_memoize(:visible_approval_rules) do
visible_user_defined_rules(branch: target_branch) + approval_rules.report_approver Hash.new do |h, key|
h[key] = visible_user_defined_rules(branch: key) + approval_rules.report_approver
end
end end
rules[target_branch]
end end
def visible_user_defined_rules(branch: nil) def visible_user_defined_rules(branch: nil)
...@@ -416,6 +420,12 @@ module EE ...@@ -416,6 +420,12 @@ module EE
user_defined_rules.applicable_to_branch(branch) user_defined_rules.applicable_to_branch(branch)
end end
def visible_user_defined_inapplicable_rules(branch)
return [] unless multiple_approval_rules_available?
user_defined_rules.inapplicable_to_branch(branch)
end
# TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329 # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329
def min_fallback_approvals def min_fallback_approvals
strong_memoize(:min_fallback_approvals) do strong_memoize(:min_fallback_approvals) do
......
...@@ -25,10 +25,15 @@ module ApprovalRules ...@@ -25,10 +25,15 @@ module ApprovalRules
return params unless params.key?(:approval_rules_attributes) return params unless params.key?(:approval_rules_attributes)
source_rule_ids = []
params[:approval_rules_attributes].each do |rule_attributes| params[:approval_rules_attributes].each do |rule_attributes|
source_rule_ids << rule_attributes[:approval_project_rule_id].presence
handle_rule(rule_attributes) handle_rule(rule_attributes)
end end
append_user_defined_inapplicable_rules(source_rule_ids.compact)
params params
end end
...@@ -113,5 +118,37 @@ module ApprovalRules ...@@ -113,5 +118,37 @@ module ApprovalRules
source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord
end end
end end
# Append inapplicable rules on create so they're still associated to the MR
# and will be available when the MR's target branch changes.
#
# Inapplicable rules are approval rules scoped to protected branches that
# does not match the specified `target_branch`.
#
# For example, there are 2 rules, one scoped to `master`, one scoped to `dev`.
# The MR's `target_branch` is set to `dev`, so the rule for `master` is
# inapplicable. But in case the MR's `target_branch` changes to `master`, the
# `master` rule should be available.
def append_user_defined_inapplicable_rules(source_rule_ids)
return if updating?
return unless project.scoped_approval_rules_enabled? && project.multiple_approval_rules_available?
project
.visible_user_defined_inapplicable_rules(params[:target_branch])
.each do |rule|
# Check if rule is already set as a source rule in one of the rules
# from params to prevent duplicates
next if source_rule_ids.include?(rule.id)
params[:approval_rules_attributes] << {
name: rule.name,
approval_project_rule_id: rule.id,
user_ids: rule.user_ids,
group_ids: rule.group_ids,
approvals_required: rule.approvals_required,
rule_type: rule.rule_type
}
end
end
end end
end end
---
title: Append inapplicable rules when creating MR
merge_request: 27886
author:
type: fixed
...@@ -180,4 +180,33 @@ describe ApprovalProjectRule do ...@@ -180,4 +180,33 @@ describe ApprovalProjectRule do
end end
end end
end end
describe '.inapplicable_to_branch' do
let!(:rule) { create(:approval_project_rule) }
let(:branch) { 'stable' }
subject { described_class.inapplicable_to_branch(branch) }
context 'when there are no associated protected branches' do
it { is_expected.to be_empty }
end
context 'when there are associated protected branches' do
before do
rule.update!(protected_branches: protected_branches)
end
context 'and branch does not match anything' do
let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
it { is_expected.to eq([rule]) }
end
context 'but branch matches' do
let(:protected_branches) { [create(:protected_branch, name: branch)] }
it { is_expected.to be_empty }
end
end
end
end end
...@@ -1150,6 +1150,49 @@ describe Project do ...@@ -1150,6 +1150,49 @@ describe Project do
end end
end end
describe '#visible_user_defined_inapplicable_rules' do
let_it_be(:project) { create(:project) }
let!(:rule) { create(:approval_project_rule, project: project) }
let!(:another_rule) { create(:approval_project_rule, project: project) }
context 'when multiple approval rules is available' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
let(:another_protected_branch) { create(:protected_branch, project: project, name: 'test-*') }
context 'when rules are scoped' do
before do
rule.update!(protected_branches: [protected_branch])
another_rule.update!(protected_branches: [another_protected_branch])
end
it 'returns rules that are not applicable to target_branch' do
expect(project.visible_user_defined_inapplicable_rules('stable-1'))
.to match_array([another_rule])
end
end
context 'when rules are not scoped' do
it 'returns empty array' do
expect(project.visible_user_defined_inapplicable_rules('stable-1')).to be_empty
end
end
end
context 'when multiple approval rules is not available' do
before do
stub_licensed_features(multiple_approval_rules: false)
end
it 'returns empty array' do
expect(project.visible_user_defined_inapplicable_rules('stable-1')).to be_empty
end
end
end
describe '#min_fallback_approvals' do describe '#min_fallback_approvals' do
let(:project) { create(:project) } let(:project) { create(:project) }
......
...@@ -17,16 +17,18 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -17,16 +17,18 @@ describe ApprovalRules::ParamsFilteringService do
project.add_reporter(project_member) project.add_reporter(project_member)
accessible_group.add_developer(user) accessible_group.add_developer(user)
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability)
.to receive(:allowed?)
.with(user, :update_approvers, merge_request)
.and_return(can_update_approvers?)
end end
shared_examples_for(:assigning_users_and_groups) do shared_examples_for(:assigning_users_and_groups) do
before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability)
.to receive(:allowed?)
.with(user, :update_approvers, merge_request)
.and_return(can_update_approvers?)
end
context 'user can update approvers' do context 'user can update approvers' do
let(:can_update_approvers?) { true } let(:can_update_approvers?) { true }
...@@ -77,6 +79,89 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -77,6 +79,89 @@ describe ApprovalRules::ParamsFilteringService do
let(:expected_groups) { [accessible_group] } let(:expected_groups) { [accessible_group] }
end end
context 'inapplicable user defined rules' do
let!(:source_rule) { create(:approval_project_rule, project: project) }
let!(:another_source_rule) { create(:approval_project_rule, project: project) }
let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
let(:approval_rules_attributes) do
[
{ name: another_source_rule.name, approval_project_rule_id: another_source_rule.id, user_ids: [project_member.id, outsider.id] }
]
end
before do
source_rule.update!(protected_branches: [protected_branch])
end
context 'when multiple_approval_rules feature is available' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
it 'adds inapplicable user defined rules' do
params = service.execute
approval_rules_attrs = params[:approval_rules_attributes]
aggregate_failures do
expect(approval_rules_attrs.size).to eq(2)
expect(approval_rules_attrs.first).to include(
name: another_source_rule.name,
approval_project_rule_id: another_source_rule.id
)
expect(approval_rules_attrs.last).to include(
name: source_rule.name,
approval_project_rule_id: source_rule.id,
user_ids: source_rule.user_ids,
group_ids: source_rule.group_ids,
approvals_required: source_rule.approvals_required,
rule_type: source_rule.rule_type
)
end
end
context 'when scoped_approval_rules feature is disabled' do
before do
stub_feature_flags(scoped_approval_rules: false)
end
it 'does not add inapplicable user defined rules' do
params = service.execute
approval_rules_attrs = params[:approval_rules_attributes]
aggregate_failures do
expect(approval_rules_attrs.size).to eq(1)
expect(approval_rules_attrs.first).to include(
name: another_source_rule.name,
approval_project_rule_id: another_source_rule.id
)
end
end
end
end
context 'when multiple_approval_rules feature is not available' do
before do
stub_licensed_features(multiple_approval_rules: false)
end
it 'does not add inapplicable user defined rules' do
params = service.execute
approval_rules_attrs = params[:approval_rules_attributes]
aggregate_failures do
expect(approval_rules_attrs.size).to eq(1)
expect(approval_rules_attrs.first).to include(
name: another_source_rule.name,
approval_project_rule_id: another_source_rule.id
)
end
end
end
end
context 'any approver rule' do context 'any approver rule' do
let(:can_update_approvers?) { true } let(:can_update_approvers?) { true }
let(:approval_rules_attributes) do let(:approval_rules_attributes) do
...@@ -96,7 +181,7 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -96,7 +181,7 @@ describe ApprovalRules::ParamsFilteringService do
end end
context 'update' do context 'update' do
let(:merge_request) { create(:merge_request, target_project: project, source_project: project)} let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
let(:existing_private_group) { create(:group, :private) } let(:existing_private_group) { create(:group, :private) }
let!(:rule1) { create(:approval_merge_request_rule, merge_request: merge_request, users: [create(:user)]) } let!(:rule1) { create(:approval_merge_request_rule, merge_request: merge_request, users: [create(:user)]) }
let!(:rule2) { create(:approval_merge_request_rule, merge_request: merge_request, groups: [existing_private_group]) } let!(:rule2) { create(:approval_merge_request_rule, merge_request: merge_request, groups: [existing_private_group]) }
...@@ -113,6 +198,30 @@ describe ApprovalRules::ParamsFilteringService do ...@@ -113,6 +198,30 @@ describe ApprovalRules::ParamsFilteringService do
let(:expected_groups) { [accessible_group, existing_private_group] } let(:expected_groups) { [accessible_group, existing_private_group] }
end end
context 'inapplicable user defined rules' do
let!(:source_rule) { create(:approval_project_rule, project: project) }
let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
let(:params) do
{
approval_rules_attributes: [
{ id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] }
]
}
end
before do
source_rule.update!(protected_branches: [protected_branch])
end
it 'does not add inapplicable user defined rules' do
params = service.execute
expect(params[:approval_rules_attributes].size).to eq(1)
expect(params[:approval_rules_attributes].first[:name]).to eq('foo')
end
end
context 'with remove_hidden_groups being true' do context 'with remove_hidden_groups being true' do
it_behaves_like :assigning_users_and_groups do it_behaves_like :assigning_users_and_groups do
let(:params) do let(:params) 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