Commit 66c9cbef authored by Patrick Bajao's avatar Patrick Bajao Committed by Nick Thomas

Update API endpoints to support new param

A new `protected_branch_ids` has been added so we can associate
`ProtectedBranch` records to a `ApprovalProjectRule`. The IDs
get filtered and will only allow those coming from the same
project.
parent eb0fbfb4
...@@ -51,7 +51,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -51,7 +51,7 @@ class ProjectsController < Projects::ApplicationController
def edit def edit
@badge_api_endpoint = expose_url(api_v4_projects_badges_path(id: @project.id)) @badge_api_endpoint = expose_url(api_v4_projects_badges_path(id: @project.id))
render 'edit' render_edit
end end
def create def create
...@@ -85,7 +85,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -85,7 +85,7 @@ class ProjectsController < Projects::ApplicationController
else else
flash.now[:alert] = result[:message] flash.now[:alert] = result[:message]
format.html { render 'edit' } format.html { render_edit }
end end
format.js format.js
...@@ -387,7 +387,6 @@ class ProjectsController < Projects::ApplicationController ...@@ -387,7 +387,6 @@ class ProjectsController < Projects::ApplicationController
:merge_method, :merge_method,
:initialize_with_readme, :initialize_with_readme,
:autoclose_referenced_issues, :autoclose_referenced_issues,
:suggestion_commit_message,
project_feature_attributes: %i[ project_feature_attributes: %i[
builds_access_level builds_access_level
...@@ -488,6 +487,10 @@ class ProjectsController < Projects::ApplicationController ...@@ -488,6 +487,10 @@ class ProjectsController < Projects::ApplicationController
def rate_limiter def rate_limiter
::Gitlab::ApplicationRateLimiter ::Gitlab::ApplicationRateLimiter
end end
def render_edit
render 'edit'
end
end end
ProjectsController.prepend_if_ee('EE::ProjectsController') ProjectsController.prepend_if_ee('EE::ProjectsController')
...@@ -10,6 +10,8 @@ module ProtectedRef ...@@ -10,6 +10,8 @@ module ProtectedRef
validates :project, presence: true validates :project, presence: true
delegate :matching, :matches?, :wildcard?, to: :ref_matcher delegate :matching, :matches?, :wildcard?, to: :ref_matcher
scope :for_project, ->(project) { where(project: project) }
end end
def commit def commit
......
# frozen_string_literal: true
class CreateApprovalProjectRulesProtectedBranches < ActiveRecord::Migration[5.2]
DOWNTIME = false
def change
create_table :approval_project_rules_protected_branches, id: false do |t|
t.references :approval_project_rule,
null: false,
index: false,
foreign_key: { on_delete: :cascade }
t.references :protected_branch,
null: false,
index: { name: 'index_approval_project_rules_protected_branches_pb_id' },
foreign_key: { on_delete: :cascade }
t.index [:approval_project_rule_id, :protected_branch_id], name: 'index_approval_project_rules_protected_branches_unique', unique: true, using: :btree
end
end
end
...@@ -437,6 +437,13 @@ ActiveRecord::Schema.define(version: 2020_01_13_133352) do ...@@ -437,6 +437,13 @@ ActiveRecord::Schema.define(version: 2020_01_13_133352) do
t.index ["group_id"], name: "index_approval_project_rules_groups_2" t.index ["group_id"], name: "index_approval_project_rules_groups_2"
end end
create_table "approval_project_rules_protected_branches", id: false, force: :cascade do |t|
t.bigint "approval_project_rule_id", null: false
t.bigint "protected_branch_id", null: false
t.index ["approval_project_rule_id", "protected_branch_id"], name: "index_approval_project_rules_protected_branches_unique", unique: true
t.index ["protected_branch_id"], name: "index_approval_project_rules_protected_branches_pb_id"
end
create_table "approval_project_rules_users", force: :cascade do |t| create_table "approval_project_rules_users", force: :cascade do |t|
t.bigint "approval_project_rule_id", null: false t.bigint "approval_project_rule_id", null: false
t.integer "user_id", null: false t.integer "user_id", null: false
...@@ -4448,6 +4455,8 @@ ActiveRecord::Schema.define(version: 2020_01_13_133352) do ...@@ -4448,6 +4455,8 @@ ActiveRecord::Schema.define(version: 2020_01_13_133352) do
add_foreign_key "approval_project_rules", "projects", on_delete: :cascade add_foreign_key "approval_project_rules", "projects", on_delete: :cascade
add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "approval_project_rules_protected_branches", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_project_rules_protected_branches", "protected_branches", on_delete: :cascade
add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade
add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade
add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade
......
...@@ -23,7 +23,7 @@ module EE ...@@ -23,7 +23,7 @@ module EE
else else
flash.now[:alert] = result[:message] flash.now[:alert] = result[:message]
render 'edit' render_edit
end end
end end
...@@ -41,7 +41,7 @@ module EE ...@@ -41,7 +41,7 @@ module EE
else else
flash.now[:alert] = result[:message] flash.now[:alert] = result[:message]
render 'edit' render_edit
end end
end end
...@@ -137,5 +137,11 @@ module EE ...@@ -137,5 +137,11 @@ module EE
def log_unarchive_audit_event def log_unarchive_audit_event
log_audit_event(message: 'Project unarchived') log_audit_event(message: 'Project unarchived')
end end
override :render_edit
def render_edit
push_frontend_feature_flag(:scoped_approval_rules, project, default_enabled: true)
super
end
end end
end end
...@@ -70,6 +70,14 @@ class ApprovalMergeRequestRule < ApplicationRecord ...@@ -70,6 +70,14 @@ class ApprovalMergeRequestRule < ApplicationRecord
retry retry
end end
def self.applicable_to_branch(branch)
includes(approval_project_rule: :protected_branches).select do |rule|
next true unless rule.approval_project_rule.present?
rule.approval_project_rule.applies_to_branch?(branch)
end
end
def project def project
merge_request.target_project merge_request.target_project
end end
......
...@@ -4,6 +4,7 @@ class ApprovalProjectRule < ApplicationRecord ...@@ -4,6 +4,7 @@ class ApprovalProjectRule < ApplicationRecord
include ApprovalRuleLike include ApprovalRuleLike
belongs_to :project belongs_to :project
has_and_belongs_to_many :protected_branches
enum rule_type: { enum rule_type: {
regular: 0, regular: 0,
...@@ -17,6 +18,16 @@ class ApprovalProjectRule < ApplicationRecord ...@@ -17,6 +18,16 @@ class ApprovalProjectRule < ApplicationRecord
validates :name, uniqueness: { scope: :project_id } validates :name, uniqueness: { scope: :project_id }
def self.applicable_to_branch(branch)
includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) }
end
def applies_to_branch?(branch)
return true if protected_branches.empty?
protected_branches.matching(branch).any?
end
def source_rule def source_rule
nil nil
end end
......
...@@ -157,7 +157,9 @@ class ApprovalState ...@@ -157,7 +157,9 @@ class ApprovalState
if approval_rules_overwritten? if approval_rules_overwritten?
user_defined_merge_request_rules user_defined_merge_request_rules
else else
project.visible_user_defined_rules.map do |rule| branch = project.scoped_approval_rules_enabled? ? target_branch : nil
project.visible_user_defined_rules(branch: branch).map do |rule|
ApprovalWrappedRule.wrap(merge_request, rule) ApprovalWrappedRule.wrap(merge_request, rule)
end end
end end
...@@ -201,9 +203,18 @@ class ApprovalState ...@@ -201,9 +203,18 @@ class ApprovalState
def wrapped_rules def wrapped_rules
strong_memoize(:wrapped_rules) do strong_memoize(:wrapped_rules) do
merge_request.approval_rules.map do |rule| merge_request_rules = merge_request.approval_rules
merge_request_rules = merge_request_rules.applicable_to_branch(target_branch) if project.scoped_approval_rules_enabled?
merge_request_rules.map do |rule|
ApprovalWrappedRule.wrap(merge_request, rule) ApprovalWrappedRule.wrap(merge_request, rule)
end end
end end
end end
def target_branch
strong_memoize(:target_branch) do
merge_request.target_branch
end
end
end end
...@@ -323,6 +323,10 @@ module EE ...@@ -323,6 +323,10 @@ module EE
feature_available?(:code_owner_approval_required) feature_available?(:code_owner_approval_required)
end end
def scoped_approval_rules_enabled?
::Feature.enabled?(:scoped_approval_rules, self, default_enabled: true)
end
def service_desk_enabled def service_desk_enabled
::EE::Gitlab::ServiceDesk.enabled?(project: self) && super ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super
end end
...@@ -408,14 +412,11 @@ module EE ...@@ -408,14 +412,11 @@ module EE
end end
end end
def visible_user_defined_rules def visible_user_defined_rules(branch: nil)
strong_memoize(:visible_user_defined_rules) do return user_defined_rules.take(1) unless multiple_approval_rules_available?
rules = approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc) return user_defined_rules unless branch
next rules.take(1) unless multiple_approval_rules_available? user_defined_rules.applicable_to_branch(branch)
rules
end
end 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
...@@ -771,5 +772,11 @@ module EE ...@@ -771,5 +772,11 @@ module EE
errors.add(:pull_mirror_branch_prefix, _('Invalid Git ref')) errors.add(:pull_mirror_branch_prefix, _('Invalid Git ref'))
end end
end end
def user_defined_rules
strong_memoize(:user_defined_rules) do
approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc)
end
end
end end
end end
...@@ -5,6 +5,7 @@ module ApprovalRules ...@@ -5,6 +5,7 @@ module ApprovalRules
def action def action
filter_eligible_users! filter_eligible_users!
filter_eligible_groups! filter_eligible_groups!
filter_eligible_protected_branches!
if rule.update(params) if rule.update(params)
rule.reset rule.reset
...@@ -27,5 +28,18 @@ module ApprovalRules ...@@ -27,5 +28,18 @@ module ApprovalRules
params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user) params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
end end
def filter_eligible_protected_branches!
return unless params.key?(:protected_branch_ids)
protected_branch_ids = params.delete(:protected_branch_ids)
return unless project.multiple_approval_rules_available? && can?(current_user, :admin_project, project)
params[:protected_branches] =
ProtectedBranch
.id_in(protected_branch_ids)
.for_project(project)
end
end end
end end
---
title: Scope approval rules by protected branches via API
merge_request: 22673
author:
type: added
...@@ -13,6 +13,7 @@ module API ...@@ -13,6 +13,7 @@ module API
optional :rule_type, type: String, desc: 'The type of approval rule' optional :rule_type, type: String, desc: 'The type of approval rule'
optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
end end
params :update_project_approval_rule do params :update_project_approval_rule do
...@@ -21,6 +22,7 @@ module API ...@@ -21,6 +22,7 @@ module API
optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule'
optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed'
end end
......
...@@ -12,33 +12,33 @@ module API ...@@ -12,33 +12,33 @@ module API
resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
segment ':id/approval_rules' do segment ':id/approval_rules' do
desc 'Get all project approval rules' do desc 'Get all project approval rules' do
success EE::API::Entities::ApprovalRule success EE::API::Entities::ProjectApprovalRule
end end
get do get do
authorize_create_merge_request_in_project authorize_create_merge_request_in_project
present user_project.visible_approval_rules, with: EE::API::Entities::ApprovalRule, current_user: current_user present user_project.visible_approval_rules, with: EE::API::Entities::ProjectApprovalRule, current_user: current_user
end end
desc 'Create new project approval rule' do desc 'Create new project approval rule' do
success EE::API::Entities::ApprovalRule success EE::API::Entities::ProjectApprovalRule
end end
params do params do
use :create_project_approval_rule use :create_project_approval_rule
end end
post do post do
create_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule)
end end
segment ':approval_rule_id' do segment ':approval_rule_id' do
desc 'Update project approval rule' do desc 'Update project approval rule' do
success EE::API::Entities::ApprovalRule success EE::API::Entities::ProjectApprovalRule
end end
params do params do
use :update_project_approval_rule use :update_project_approval_rule
end end
put do put do
update_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule)
end end
desc 'Destroy project approval rule' desc 'Destroy project approval rule'
......
...@@ -24,25 +24,25 @@ module API ...@@ -24,25 +24,25 @@ module API
segment 'rules' do segment 'rules' do
desc 'Create new approval rule' do desc 'Create new approval rule' do
detail 'Private API subject to change' detail 'Private API subject to change'
success EE::API::Entities::ApprovalSettingRule success EE::API::Entities::ProjectApprovalSettingRule
end end
params do params do
use :create_project_approval_rule use :create_project_approval_rule
end end
post do post do
create_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule)
end end
segment ':approval_rule_id' do segment ':approval_rule_id' do
desc 'Update approval rule' do desc 'Update approval rule' do
detail 'Private API subject to change' detail 'Private API subject to change'
success EE::API::Entities::ApprovalSettingRule success EE::API::Entities::ProjectApprovalSettingRule
end end
params do params do
use :update_project_approval_rule use :update_project_approval_rule
end end
put do put do
update_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule)
end end
desc 'Delete an approval rule' do desc 'Delete an approval rule' do
......
...@@ -420,6 +420,10 @@ module EE ...@@ -420,6 +420,10 @@ module EE
expose :contains_hidden_groups?, as: :contains_hidden_groups expose :contains_hidden_groups?, as: :contains_hidden_groups
end end
class ProjectApprovalRule < ApprovalRule
expose :protected_branches, using: ::API::Entities::ProtectedBranch, if: -> (rule, _) { rule.project.multiple_approval_rules_available? }
end
class MergeRequestApprovalRule < ApprovalRule class MergeRequestApprovalRule < ApprovalRule
class SourceRule < Grape::Entity class SourceRule < Grape::Entity
expose :approvals_required expose :approvals_required
...@@ -446,7 +450,7 @@ module EE ...@@ -446,7 +450,7 @@ module EE
# This overrides the `eligible_approvers` to be exposed as `approvers`. # This overrides the `eligible_approvers` to be exposed as `approvers`.
# #
# 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 ApprovalSettingRule < ApprovalRule class ProjectApprovalSettingRule < ProjectApprovalRule
expose :approvers, using: ::API::Entities::UserBasic, override: true expose :approvers, using: ::API::Entities::UserBasic, override: true
end end
...@@ -454,7 +458,7 @@ module EE ...@@ -454,7 +458,7 @@ 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: ApprovalSettingRule expose :visible_approval_rules, as: :rules, using: ProjectApprovalSettingRule
expose :min_fallback_approvals, as: :fallback_approvals_required expose :min_fallback_approvals, as: :fallback_approvals_required
end end
......
...@@ -26,6 +26,13 @@ ...@@ -26,6 +26,13 @@
"type": "object", "type": "object",
"properties": {} "properties": {}
} }
},
"protected_branches": {
"type": "array",
"items": {
"type": "object",
"properties": {}
}
} }
}, },
"additionalProperties": false "additionalProperties": false
......
...@@ -26,6 +26,13 @@ ...@@ -26,6 +26,13 @@
"type": "object", "type": "object",
"properties": {} "properties": {}
} }
},
"protected_branches": {
"type": "array",
"items": {
"type": "object",
"properties": {}
}
} }
}, },
"additionalProperties": false "additionalProperties": false
......
...@@ -116,12 +116,12 @@ describe ApprovalMergeRequestRule do ...@@ -116,12 +116,12 @@ describe ApprovalMergeRequestRule do
end end
end end
context 'scopes' do context 'scopes' do
set(:rb_rule) { create(:code_owner_rule, name: '*.rb') } let!(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
set(:js_rule) { create(:code_owner_rule, name: '*.js') } let!(:js_rule) { create(:code_owner_rule, name: '*.js') }
set(:css_rule) { create(:code_owner_rule, name: '*.css') } let!(:css_rule) { create(:code_owner_rule, name: '*.css') }
set(:approval_rule) { create(:approval_merge_request_rule) } let!(:approval_rule) { create(:approval_merge_request_rule) }
set(:report_approver_rule) { create(:report_approver_rule) } let!(:report_approver_rule) { create(:report_approver_rule) }
describe '.not_matching_pattern' do describe '.not_matching_pattern' do
it 'returns the correct rules' do it 'returns the correct rules' do
...@@ -153,8 +153,7 @@ describe ApprovalMergeRequestRule do ...@@ -153,8 +153,7 @@ describe ApprovalMergeRequestRule do
end end
describe '.find_or_create_code_owner_rule' do describe '.find_or_create_code_owner_rule' do
set(:merge_request) { create(:merge_request) } let!(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
set(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) }
it 'finds an existing rule' do it 'finds an existing rule' do
expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb')) expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb'))
...@@ -182,6 +181,47 @@ describe ApprovalMergeRequestRule do ...@@ -182,6 +181,47 @@ describe ApprovalMergeRequestRule do
end end
end end
describe '.applicable_to_branch' do
let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
let(:branch) { 'stable' }
subject { described_class.applicable_to_branch(branch) }
context 'when there are no associated source rules' do
it { is_expected.to eq([rule]) }
end
context 'when there are associated source rules' do
let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
before do
rule.update!(approval_project_rule: source_rule)
end
context 'and there are no associated protected branches to source rule' do
it { is_expected.to eq([rule]) }
end
context 'and there are associated protected branches to source rule' do
before do
source_rule.update!(protected_branches: protected_branches)
end
context 'and branch matches' do
let(:protected_branches) { [create(:protected_branch, name: branch)] }
it { is_expected.to eq([rule]) }
end
context 'but branch does not match anything' do
let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
it { is_expected.to be_empty }
end
end
end
end
describe '#project' do describe '#project' do
it 'returns project of MergeRequest' do it 'returns project of MergeRequest' do
expect(subject.project).to be_present expect(subject.project).to be_present
......
...@@ -147,4 +147,33 @@ describe ApprovalProjectRule do ...@@ -147,4 +147,33 @@ describe ApprovalProjectRule do
expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique) expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique)
end end
end end
describe '.applicable_to_branch' do
let!(:rule) { create(:approval_project_rule) }
let(:branch) { 'stable' }
subject { described_class.applicable_to_branch(branch) }
context 'when there are no associated protected branches' do
it { is_expected.to eq([rule]) }
end
context 'when there are associated protected branches' do
before do
rule.update!(protected_branches: protected_branches)
end
context 'and branch matches' do
let(:protected_branches) { [create(:protected_branch, name: branch)] }
it { is_expected.to eq([rule]) }
end
context 'but branch does not match anything' do
let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
it { is_expected.to be_empty }
end
end
end
end end
...@@ -1380,4 +1380,130 @@ describe ApprovalState do ...@@ -1380,4 +1380,130 @@ describe ApprovalState do
end end
end end
end end
describe '#user_defined_rules' do
context 'when approval rules are not overwritten' do
let!(:project_rule) { create(:approval_project_rule, project: project) }
let!(:another_project_rule) { create(:approval_project_rule, project: project) }
context 'and multiple approval rules is disabled' do
it 'returns the first rule' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
project_rule
])
end
end
context 'and multiple approval rules is enabled' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
it 'returns the rules as is' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
project_rule,
another_project_rule
])
end
context 'and rules are scoped by protected branches' do
let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') }
let(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') }
before do
merge_request.update!(target_branch: 'stable-1')
another_project_rule.update!(protected_branches: [protected_branch])
project_rule.update!(protected_branches: [another_protected_branch])
end
context 'and scoped_approval_rules feature is enabled' do
it 'returns the rules that are applicable to the merge request target branch' do
expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
another_project_rule
])
end
end
context 'but scoped_approval_rules feature is disabled' do
before do
stub_feature_flags(scoped_approval_rules: false)
end
it 'returns unscoped rules' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
project_rule,
another_project_rule
])
end
end
end
end
end
context 'when approval rules are overwritten' do
let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
let!(:another_mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
before do
project.update!(disable_overriding_approvers_per_merge_request: false)
end
context 'when multiple approval rules is disabled' do
it 'returns the first rule' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
mr_rule
])
end
end
context 'when multiple approval rules is enabled' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
it 'returns the rules as is' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
mr_rule,
another_mr_rule
])
end
context 'and rules have source rules that are scoped by protected branches' 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(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') }
before do
merge_request.update!(target_branch: 'stable-1')
source_rule.update!(protected_branches: [protected_branch])
another_source_rule.update!(protected_branches: [another_protected_branch])
mr_rule.update!(approval_project_rule: another_source_rule)
another_mr_rule.update!(approval_project_rule: source_rule)
end
context 'and scoped_approval_rules feature is enabled' do
it 'returns the rules that are applicable to the merge request target branch' do
expect(subject.user_defined_rules.map(&:approval_rule)).to eq([
another_mr_rule
])
end
end
context 'but scoped_approval_rules feature is disabled' do
before do
stub_feature_flags(scoped_approval_rules: false)
end
it 'returns unscoped rules' do
expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([
mr_rule,
another_mr_rule
])
end
end
end
end
end
end
end end
...@@ -83,6 +83,60 @@ describe ApprovalRules::CreateService do ...@@ -83,6 +83,60 @@ describe ApprovalRules::CreateService do
it_behaves_like "creatable" it_behaves_like "creatable"
context 'when protected_branch_ids param is present' do
let(:protected_branch) { create(:protected_branch, project: target) }
subject do
described_class.new(
target,
user,
name: 'developers',
approvals_required: 1,
protected_branch_ids: [protected_branch.id]
).execute
end
context 'and multiple approval rules is enabled' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
it 'associates the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to eq([protected_branch])
end
context 'but user cannot administer project' do
before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false)
end
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
context 'but protected branch is for another project' do
let(:another_project) { create(:project) }
let(:protected_branch) { create(:protected_branch, project: another_project) }
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
end
context 'and multiple approval rules is disabled' do
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
end
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name| ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
context "when the rule name is `#{rule_name}`" do context "when the rule name is `#{rule_name}`" do
subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) } subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) }
......
...@@ -5,9 +5,9 @@ require 'spec_helper' ...@@ -5,9 +5,9 @@ require 'spec_helper'
describe ApprovalRules::UpdateService do describe ApprovalRules::UpdateService do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { project.creator } let(:user) { project.creator }
let(:approval_rule) { target.approval_rules.create(name: 'foo') }
shared_examples 'editable' do shared_examples 'editable' do
let(:approval_rule) { target.approval_rules.create(name: 'foo') }
let(:new_approvers) { create_list(:user, 2) } let(:new_approvers) { create_list(:user, 2) }
let(:new_groups) { create_list(:group, 2, :private) } let(:new_groups) { create_list(:group, 2, :private) }
...@@ -138,6 +138,58 @@ describe ApprovalRules::UpdateService do ...@@ -138,6 +138,58 @@ describe ApprovalRules::UpdateService do
let(:target) { project } let(:target) { project }
it_behaves_like "editable" it_behaves_like "editable"
context 'when protected_branch_ids param is present' do
let(:protected_branch) { create(:protected_branch, project: target) }
subject do
described_class.new(
approval_rule,
user,
protected_branch_ids: [protected_branch.id]
).execute
end
context 'and multiple approval rules is enabled' do
before do
stub_licensed_features(multiple_approval_rules: true)
end
it 'associates the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to eq([protected_branch])
end
context 'but user cannot administer project' do
before do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false)
end
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
context 'but protected branch is for another project' do
let(:another_project) { create(:project) }
let(:protected_branch) { create(:protected_branch, project: another_project) }
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
end
context 'and multiple approval rules is disabled' do
it 'does not associate the approval rule to the protected branch' do
expect(subject[:status]).to eq(:success)
expect(subject[:rule].protected_branches).to be_empty
end
end
end
end end
context 'when target is merge request' do context 'when target is merge request' do
......
...@@ -45,6 +45,22 @@ shared_examples_for 'an API endpoint for creating project approval rule' do ...@@ -45,6 +45,22 @@ shared_examples_for 'an API endpoint for creating project approval rule' do
expect(json_response.symbolize_keys).to include(params) expect(json_response.symbolize_keys).to include(params)
end end
context 'when protected_branch_ids param is present' do
let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
before do
stub_licensed_features(multiple_approval_rules: true)
post api(url, current_user), params: params.merge(protected_branch_ids: protected_branches.map(&:id))
end
it 'creates approval rule associated to specified protected branches' do
expect(json_response['protected_branches']).to contain_exactly(
a_hash_including('id' => protected_branches.first.id),
a_hash_including('id' => protected_branches.last.id)
)
end
end
ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name| ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name|
context "when creating a '#{rule_name}' approval rule" do context "when creating a '#{rule_name}' approval rule" do
it 'specifies a `rule_type` of `report_approver`' do it 'specifies a `rule_type` of `report_approver`' do
...@@ -65,6 +81,22 @@ shared_examples_for 'an API endpoint for updating project approval rule' do ...@@ -65,6 +81,22 @@ shared_examples_for 'an API endpoint for updating project approval rule' do
project.add_developer(approver) project.add_developer(approver)
end end
context 'when protected_branch_ids param is present' do
let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } }
before do
stub_licensed_features(multiple_approval_rules: true)
put api(url, current_user), params: { protected_branch_ids: protected_branches.map(&:id) }
end
it 'associates approval rule to specified protected branches' do
expect(json_response['protected_branches']).to contain_exactly(
a_hash_including('id' => protected_branches.first.id),
a_hash_including('id' => protected_branches.last.id)
)
end
end
context 'when approver already exists' do context 'when approver already exists' do
before do before do
approval_rule.users << approver approval_rule.users << approver
......
...@@ -613,6 +613,7 @@ module API ...@@ -613,6 +613,7 @@ module API
end end
class ProtectedBranch < Grape::Entity class ProtectedBranch < Grape::Entity
expose :id
expose :name expose :name
expose :push_access_levels, using: Entities::ProtectedRefAccess expose :push_access_levels, using: Entities::ProtectedRefAccess
expose :merge_access_levels, using: Entities::ProtectedRefAccess expose :merge_access_levels, using: Entities::ProtectedRefAccess
......
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