Commit 6cd381eb authored by Patrick Bajao's avatar Patrick Bajao

Filter rules by target_branch in approval_settings

With scoped approval rules, it's possible that all approval rules
does not apply to an MR's target branch.

When changing target branch of an MR, we need to know the rules
that will be applicable for the MR before it changes. This adds
that support via `target_branch` param.
parent 2593da37
---
title: Filter rules by target_branch in approval_settings
merge_request: 26439
author:
type: added
...@@ -13,9 +13,10 @@ class ApprovalState ...@@ -13,9 +13,10 @@ class ApprovalState
def_delegators :@merge_request, :merge_status, :approved_by_users, :approvals, :approval_feature_available? def_delegators :@merge_request, :merge_status, :approved_by_users, :approvals, :approval_feature_available?
alias_method :approved_approvers, :approved_by_users alias_method :approved_approvers, :approved_by_users
def initialize(merge_request) def initialize(merge_request, target_branch: nil)
@merge_request = merge_request @merge_request = merge_request
@project = merge_request.target_project @project = merge_request.target_project
@target_branch = target_branch || merge_request.target_branch
end end
# Excludes the author if 'author-approval' is explicitly disabled on project settings. # Excludes the author if 'author-approval' is explicitly disabled on project settings.
...@@ -168,6 +169,8 @@ class ApprovalState ...@@ -168,6 +169,8 @@ class ApprovalState
private private
attr_reader :target_branch
def filter_approvers(approvers, unactioned:) def filter_approvers(approvers, unactioned:)
approvers = approvers.uniq approvers = approvers.uniq
approvers -= approved_approvers if unactioned approvers -= approved_approvers if unactioned
...@@ -211,10 +214,4 @@ class ApprovalState ...@@ -211,10 +214,4 @@ class ApprovalState
end end
end end
end end
def target_branch
strong_memoize(:target_branch) do
merge_request.target_branch
end
end
end end
...@@ -31,9 +31,12 @@ module Approvable ...@@ -31,9 +31,12 @@ module Approvable
end end
end end
def approval_state # rubocop:disable Gitlab/ModuleWithInstanceVariables
@approval_state ||= ApprovalState.new(self) def approval_state(target_branch: nil)
@approval_state ||= {}
@approval_state[target_branch] ||= ApprovalState.new(self, target_branch: target_branch)
end end
# rubocop:enable Gitlab/ModuleWithInstanceVariables
def approvals_given def approvals_given
approvals.size approvals.size
......
...@@ -399,9 +399,9 @@ module EE ...@@ -399,9 +399,9 @@ module EE
super super
end end
def visible_approval_rules def visible_approval_rules(target_branch: nil)
strong_memoize(:visible_approval_rules) do strong_memoize(:"visible_approval_rules_#{target_branch}") do
visible_user_defined_rules + approval_rules.report_approver visible_user_defined_rules(branch: target_branch) + approval_rules.report_approver
end end
end end
......
...@@ -27,10 +27,14 @@ module API ...@@ -27,10 +27,14 @@ module API
render_api_error!(errors, 400) render_api_error!(errors, 400)
end end
def present_merge_request_approval_state(presenter:) def present_merge_request_approval_state(presenter:, target_branch: nil)
merge_request = find_merge_request_with_access(params[:merge_request_iid]) merge_request = find_merge_request_with_access(params[:merge_request_iid])
present merge_request.approval_state, with: presenter, current_user: current_user present(
merge_request.approval_state(target_branch: target_branch),
with: presenter,
current_user: current_user
)
end end
end end
...@@ -62,8 +66,14 @@ module API ...@@ -62,8 +66,14 @@ module API
success: ::EE::API::Entities::MergeRequestApprovalSettings, success: ::EE::API::Entities::MergeRequestApprovalSettings,
hidden: true hidden: true
} }
params do
optional :target_branch, type: String, desc: 'Branch that scoped approval rules apply to'
end
get 'approval_settings' do get 'approval_settings' do
present_merge_request_approval_state(presenter: ::EE::API::Entities::MergeRequestApprovalSettings) present_merge_request_approval_state(
presenter: ::EE::API::Entities::MergeRequestApprovalSettings,
target_branch: declared_params[:target_branch]
)
end end
desc 'Get approval state of merge request' do desc 'Get approval state of merge request' do
......
...@@ -15,10 +15,18 @@ module API ...@@ -15,10 +15,18 @@ module API
detail 'Private API subject to change' detail 'Private API subject to change'
success EE::API::Entities::ProjectApprovalSettings success EE::API::Entities::ProjectApprovalSettings
end end
params do
optional :target_branch, type: String, desc: 'Branch that scoped approval rules apply to'
end
get do get do
authorize_create_merge_request_in_project authorize_create_merge_request_in_project
present user_project, with: EE::API::Entities::ProjectApprovalSettings, current_user: current_user present(
user_project,
with: EE::API::Entities::ProjectApprovalSettings,
current_user: current_user,
target_branch: declared_params[:target_branch]
)
end end
segment 'rules' do segment 'rules' do
......
...@@ -300,7 +300,10 @@ module EE ...@@ -300,7 +300,10 @@ module EE
# #
# To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574. # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574.
class ProjectApprovalSettings < Grape::Entity class ProjectApprovalSettings < Grape::Entity
expose :visible_approval_rules, as: :rules, using: ProjectApprovalSettingRule expose :rules, using: ProjectApprovalSettingRule do |project, options|
project.visible_approval_rules(target_branch: options[:target_branch])
end
expose :min_fallback_approvals, as: :fallback_approvals_required expose :min_fallback_approvals, as: :fallback_approvals_required
end end
......
...@@ -1519,6 +1519,16 @@ describe ApprovalState do ...@@ -1519,6 +1519,16 @@ describe ApprovalState do
another_project_rule another_project_rule
]) ])
end end
context 'and target_branch is specified' do
subject { described_class.new(merge_request, target_branch: 'v1-stable') }
it 'returns the rules that are applicable to the specified target_branch' do
expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
project_rule
])
end
end
end end
context 'but scoped_approval_rules feature is disabled' do context 'but scoped_approval_rules feature is disabled' do
...@@ -1585,6 +1595,16 @@ describe ApprovalState do ...@@ -1585,6 +1595,16 @@ describe ApprovalState do
another_mr_rule another_mr_rule
]) ])
end end
context 'and target_branch is specified' do
subject { described_class.new(merge_request, target_branch: 'v1-stable') }
it 'returns the rules that are applicable to the specified target_branch' do
expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
mr_rule
])
end
end
end end
context 'but scoped_approval_rules feature is disabled' do context 'but scoped_approval_rules feature is disabled' do
......
...@@ -165,6 +165,50 @@ describe API::MergeRequestApprovals do ...@@ -165,6 +165,50 @@ describe API::MergeRequestApprovals do
expect(rule_response['approved_by'][0]['username']).to eq(approver.username) expect(rule_response['approved_by'][0]['username']).to eq(approver.username)
expect(rule_response['source_rule']).to eq(nil) expect(rule_response['source_rule']).to eq(nil)
end end
context 'when target_branch is specified' do
let(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
let(:another_protected_branch) { create(:protected_branch, project: project, name: 'test') }
let(:project_rule) do
create(
:approval_project_rule,
project: project,
protected_branches: [protected_branch]
)
end
let(:another_project_rule) do
create(
:approval_project_rule,
project: project,
protected_branches: [another_protected_branch]
)
end
let!(:another_rule) do
create(
:approval_merge_request_rule,
approval_project_rule: another_project_rule,
merge_request: merge_request
)
end
before do
rule.update!(approval_project_rule: project_rule)
end
it 'filters the rules returned by target branch' do
get api("#{url}?target_branch=master", user)
expect(json_response['rules'].size).to eq(1)
rule_response = json_response['rules'].first
expect(rule_response['id']).to eq(rule.id)
expect(rule_response['name']).to eq('foo')
end
end
end end
describe 'GET :id/merge_requests/:merge_request_iid/approval_state' do describe 'GET :id/merge_requests/:merge_request_iid/approval_state' do
......
...@@ -42,6 +42,36 @@ describe API::ProjectApprovalSettings do ...@@ -42,6 +42,36 @@ describe API::ProjectApprovalSettings do
expect(rule['name']).to eq('security') expect(rule['name']).to eq('security')
end end
context 'when target_branch is specified' do
let(:protected_branch) { create(:protected_branch, project: project, name: 'master') }
let(:another_protected_branch) { create(:protected_branch, project: project, name: 'test') }
let!(:another_rule) do
create(
:approval_project_rule,
name: 'test',
project: project,
protected_branches: [another_protected_branch]
)
end
before do
stub_licensed_features(multiple_approval_rules: true)
rule.update!(protected_branches: [protected_branch])
end
it 'filters the rules returned by target branch' do
get api("#{url}?target_branch=test", developer)
expect(json_response['rules'].size).to eq(1)
rule_response = json_response['rules'].first
expect(rule_response['id']).to eq(another_rule.id)
expect(rule_response['name']).to eq('test')
end
end
context 'private group filtering' do context 'private group filtering' do
let_it_be(:private_group) { create :group, :private } let_it_be(:private_group) { create :group, :private }
......
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