Commit c419fc2f authored by Nick Thomas's avatar Nick Thomas

Merge branch '1979-2-controller' into 'master'

[API] Approval rule endpoints

See merge request gitlab-org/gitlab-ee!8769
parents 3625563b ba3002dc
......@@ -14,6 +14,7 @@ module EE
def merge_request_params_attributes
attrs = super.push(
{ approval_rules_attributes: [:id, :name, { user_ids: [] }, { group_ids: [] }, :approvals_required, :approval_project_rule_id, :_destroy] },
:approvals_before_merge,
:approver_group_ids,
:approver_ids
......
......@@ -14,11 +14,19 @@ class ApprovalMergeRequestRule < ApplicationRecord
has_and_belongs_to_many :approved_approvers, class_name: 'User', join_table: :approval_merge_request_rules_approved_approvers
has_one :approval_merge_request_rule_source
has_one :approval_project_rule, through: :approval_merge_request_rule_source
alias_method :source_rule, :approval_project_rule
validate :validate_approvals_required
def project
merge_request.target_project
end
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.approval_project_rule_id = approval_project_rule_id
end
# Users who are eligible to approve, including specified group members.
# Excludes the author if 'self-approval' isn't explicitly
# enabled on project settings.
......@@ -44,4 +52,15 @@ class ApprovalMergeRequestRule < ApplicationRecord
!code_owner?
end
alias_method :regular?, :regular
private
def validate_approvals_required
return unless approval_project_rule
return unless approvals_required_changed?
if approvals_required < approval_project_rule.approvals_required
errors.add(:approvals_required, :greater_than_or_equal_to, count: approval_project_rule.approvals_required)
end
end
end
......@@ -4,4 +4,16 @@
class ApprovalMergeRequestRuleSource < ApplicationRecord
belongs_to :approval_merge_request_rule
belongs_to :approval_project_rule
validate :validate_project_rule
private
def validate_project_rule
project = approval_merge_request_rule.merge_request.target_project
unless project.approval_rules.where(id: approval_project_rule_id).exists?
errors.add(:approval_project_rule, :invalid)
end
end
end
......@@ -18,4 +18,8 @@ class ApprovalProjectRule < ApplicationRecord
false
end
alias_method :code_owner?, :code_owner
def source_rule
nil
end
end
......@@ -35,6 +35,14 @@ class ApprovalState
end
end
def has_approval_rules?
!wrapped_approval_rules.empty?
end
def use_fallback?
regular_rules.empty?
end
def approval_rules_overwritten?
merge_request.approval_rules.any?(&:regular?)
end
......@@ -43,28 +51,42 @@ class ApprovalState
def approval_needed?
return false unless project.feature_available?(:merge_request_approvers)
overall_approvals_required > 0 || wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
result = wrapped_approval_rules.any? { |rule| rule.approvals_required > 0 }
result ||= fallback_approvals_required > 0 if use_fallback?
result
end
def overall_approvals_required
@overall_approvals_required ||= project.approvals_before_merge
def fallback_approvals_required
@fallback_approvals_required ||= [project.approvals_before_merge, merge_request.approvals_before_merge || 0].max
end
def approved?
strong_memoize(:approved) do
(overall_approvals_required == 0 || approvals.size >= overall_approvals_required) && wrapped_approval_rules.all?(&:approved?)
result = wrapped_approval_rules.all?(&:approved?)
result &&= approvals.size >= fallback_approvals_required if use_fallback?
result
end
end
def any_approver_allowed?
approved? || overall_approvals_required > approvers.size
regular_rules.empty? || approved?
end
def approvals_required
strong_memoize(:approvals_required) do
result = wrapped_approval_rules.sum(&:approvals_required)
result = [result, fallback_approvals_required].max if use_fallback?
result
end
end
# Number of approvals remaining (excluding existing approvals) before the MR is
# considered approved.
def approvals_left
strong_memoize(:approvals_left) do
wrapped_approval_rules.sum(&:approvals_left)
result = wrapped_approval_rules.sum(&:approvals_left)
result = [result, fallback_approvals_required - approved_approvers.size].max if use_fallback?
result
end
end
......
......@@ -8,7 +8,7 @@ class ApprovalWrappedRule
attr_reader :merge_request
attr_reader :approval_rule
def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner
def_delegators :@approval_rule, :id, :name, :users, :groups, :approvals_required, :code_owner, :source_rule
def initialize(merge_request, approval_rule)
@merge_request = merge_request
......
......@@ -12,7 +12,7 @@ module ApprovalRuleLike
has_many :group_users, -> { distinct }, through: :groups, source: :users
validates :name, presence: true
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX }
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 }
end
# Users who are eligible to approve, including specified group members.
......
......@@ -79,6 +79,7 @@ module VisibleApprovable
def reset_approval_cache!
approvals.reload
approved_by_users.reload
approval_rules.reload
clear_memoization(:approvers_left)
clear_memoization(:all_approvers_including_groups)
......
......@@ -22,12 +22,15 @@ module EE
has_many :draft_notes
validate :validate_approvals_before_merge, unless: :importing?
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
participant :participant_approvers
accepts_nested_attributes_for :approval_rules, allow_destroy: true
end
override :mergeable?
......@@ -54,6 +57,22 @@ module EE
end
end
def validate_approval_rule_source
return if ::Feature.disabled?(:approval_rules, project)
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.approval_rule_ids.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?
......@@ -99,7 +118,7 @@ module EE
rule = approval_rules.code_owner.first
rule ||= approval_rules.code_owner.create!(name: ApprovalMergeRequestRule::DEFAULT_NAME_FOR_CODE_OWNER)
rule.users = code_owners.uniq
rule.users = owners.uniq
end
else
approval_rules.code_owner.delete_all
......
......@@ -120,6 +120,8 @@ module EE
accepts_nested_attributes_for :tracing_setting, update_only: true, allow_destroy: true
accepts_nested_attributes_for :alerting_setting, update_only: true
alias_attribute :fallback_approvals_required, :approvals_before_merge
end
class_methods do
......
# frozen_string_literal: true
class ApprovalMergeRequestRulePolicy < BasePolicy
delegate { @subject.merge_request }
condition(:editable) do
can?(:update_merge_request, @subject.merge_request)
end
rule { editable }.enable :edit_approval_rule
end
# frozen_string_literal: true
class ApprovalProjectRulePolicy < BasePolicy
delegate { @subject.project }
condition(:editable) do
can?(:admin_project, @subject.project)
end
rule { editable }.enable :edit_approval_rule
end
......@@ -11,6 +11,30 @@ module EE
end
end
def api_approvals_path
if requires_approve?
api_v4_projects_merge_requests_approvals_path(id: project.id, merge_request_iid: merge_request.iid)
end
end
def api_approval_settings_path
if requires_approve?
api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid)
end
end
def api_approve_path
if requires_approve?
api_v4_projects_merge_requests_approve_path(id: project.id, merge_request_iid: merge_request.iid)
end
end
def api_unapprove_path
if requires_approve?
api_v4_projects_merge_requests_unapprove_path(id: project.id, merge_request_iid: merge_request.iid)
end
end
def target_project
merge_request.target_project.present(current_user: current_user)
end
......
......@@ -128,6 +128,18 @@ module EE
expose :approvals_path do |merge_request|
presenter(merge_request).approvals_path
end
expose :api_approvals_path do |merge_request|
presenter(merge_request).api_approvals_path
end
expose :api_approval_settings_path do |merge_request|
presenter(merge_request).api_approval_settings_path
end
expose :api_approve_path do |merge_request|
presenter(merge_request).api_approve_path
end
expose :api_unapprove_path do |merge_request|
presenter(merge_request).api_unapprove_path
end
end
private
......
# frozen_string_literal: true
module ApprovalRules
class BaseService < ::BaseService
def execute
return error(['Prohibited']) unless can_edit?
filter_eligible_users!
filter_eligible_groups!
if rule.update(params)
rule.reload
success
else
error(rule.errors.messages)
end
end
private
attr_reader :rule
def can_edit?
can?(current_user, :edit_approval_rule, rule)
end
def success(*args, &blk)
super.tap { |hsh| hsh[:rule] = rule }
end
def filter_eligible_users!
return unless params.key?(:user_ids)
params[:users] = project.members_among(User.id_in(params.delete(:user_ids)))
end
def filter_eligible_groups!
return unless params.key?(:group_ids)
params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user)
end
end
end
# frozen_string_literal: true
module ApprovalRules
class CreateService < ::ApprovalRules::BaseService
# @param target [Project, MergeRequest]
def initialize(target, user, params)
@rule = target.approval_rules.build
super(@rule.project, user, params)
end
end
end
# frozen_string_literal: true
module ApprovalRules
class UpdateService < ::ApprovalRules::BaseService
attr_reader :rule
def initialize(approval_rule, user, params)
@rule = approval_rule
super(@rule.project, user, params)
end
end
end
......@@ -12,8 +12,36 @@ module EE
params.delete(:approver_group_ids)
end
filter_approval_rule_groups_and_users(merge_request)
super
end
def filter_approval_rule_groups_and_users(merge_request)
return unless params.key?(:approval_rules_attributes)
# For efficiency, we avoid repeated check per rule for eligibility of users and groups
# but instead consolidate all ids so eligibility can be checked in one go.
group_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:group_ids] }
user_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:user_ids] }
# rubocop: disable CodeReuse/ActiveRecord
group_ids = ::Group.id_in(group_ids).public_or_visible_to_user(current_user).pluck(:id) unless group_ids.empty?
user_ids = merge_request.project.members_among(::User.id_in(user_ids)).pluck(:id) unless user_ids.empty?
# rubocop: enable CodeReuse/ActiveRecord
params[:approval_rules_attributes].each do |rule_attributes|
if rule_attributes.key?(:group_ids)
provided_group_ids = rule_attributes[:group_ids].map(&:to_i)
rule_attributes[:group_ids] = provided_group_ids & group_ids
end
if rule_attributes.key?(:user_ids)
provided_user_ids = rule_attributes[:user_ids].map(&:to_i)
rule_attributes[:user_ids] = provided_user_ids & user_ids
end
end
end
end
end
end
......@@ -40,21 +40,27 @@ module EE
def update_approvers
return yield unless project.feature_available?(:code_owners)
previous_diffs = fetch_latest_merge_request_diffs
if ::Feature.enabled?(:approval_rules, project)
results = yield
results = yield
merge_requests_for_source_branch.each(&:sync_code_owners_with_approvers)
else
previous_diffs = fetch_latest_merge_request_diffs
merge_requests = merge_requests_for_source_branch
ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
results = yield
merge_requests.each do |merge_request|
previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
new_code_owners = merge_request.code_owners - previous_code_owners
merge_requests = merge_requests_for_source_branch
ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
create_approvers(merge_request, new_code_owners)
merge_requests.each do |merge_request|
previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
new_code_owners = merge_request.code_owners - previous_code_owners
merge_request.sync_code_owners_with_approvers
create_approvers(merge_request, new_code_owners)
merge_request.sync_code_owners_with_approvers
end
end
results
......
......@@ -13,6 +13,13 @@ module EE
old_approvers = merge_request.overall_approvers(exclude_code_owners: true)
merge_request = super(merge_request)
sync_approval_rules(merge_request)
if should_remove_old_approvers && merge_request.valid?
cleanup_approvers(merge_request, reload: true)
end
merge_request.reset_approval_cache!
new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
......@@ -21,12 +28,6 @@ module EE
notification_service.add_merge_request_approvers(merge_request, new_approvers, current_user)
end
if should_remove_old_approvers && merge_request.valid?
cleanup_approvers(merge_request, reload: true)
end
sync_approval_rules(merge_request)
merge_request
end
......
# frozen_string_literal: true
module API
module Helpers
module ApprovalHelpers
def present_approval(merge_request)
if Feature.enabled?(:approval_rules, merge_request.project)
present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
else
present merge_request.present(current_user: current_user), with: ::EE::API::Entities::MergeRequestApprovals, current_user: current_user
end
end
end
end
end
......@@ -6,6 +6,7 @@ module API
ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
helpers ::API::Helpers::ApprovalHelpers
helpers do
def handle_merge_request_errors!(errors)
if errors.has_key? :project_access
......@@ -38,18 +39,31 @@ module API
# Examples:
# GET /projects/:id/merge_requests/:merge_request_iid/approvals
#
# @deprecated
desc 'List approvals for merge request' do
success Entities::MergeRequestApprovals
success ::EE::API::Entities::ApprovalState
end
get 'approvals' do
merge_request = find_merge_request_with_access(params[:merge_request_iid])
present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
present_approval(merge_request)
end
desc 'List approval rules for merge request', {
success: ::EE::API::Entities::MergeRequestApprovalRules,
hidden: true
}
get 'approval_settings' do
not_found! unless ::Feature.enabled?(:approval_rules, user_project)
merge_request = find_merge_request_with_access(params[:merge_request_iid])
present merge_request.approval_state, with: ::EE::API::Entities::MergeRequestApprovalRules, current_user: current_user
end
desc 'Change approval-related configuration' do
detail 'This feature was introduced in 10.6'
success Entities::MergeRequestApprovals
success ::EE::API::Entities::ApprovalState
end
params do
requires :approvals_required, type: Integer, desc: 'The amount of approvals required. Must be higher than the project approvals'
......@@ -62,7 +76,7 @@ module API
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, approvals_before_merge: params[:approvals_required]).execute(merge_request)
if merge_request.valid?
present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
present_approval(merge_request)
else
handle_merge_request_errors! merge_request.errors
end
......@@ -70,7 +84,7 @@ module API
desc 'Update approvers and approver groups' do
detail 'This feature was introduced in 10.6'
success Entities::MergeRequestApprovals
success ::EE::API::Entities::ApprovalState
end
params do
requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
......@@ -84,7 +98,7 @@ module API
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
if merge_request.valid?
present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
present_approval(merge_request)
else
handle_merge_request_errors! merge_request.errors
end
......@@ -99,7 +113,7 @@ module API
# POST /projects/:id/merge_requests/:merge_request_iid/approve
#
desc 'Approve a merge request' do
success Entities::MergeRequestApprovals
success ::EE::API::Entities::ApprovalState
end
params do
optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
......@@ -115,11 +129,11 @@ module API
.new(user_project, current_user)
.execute(merge_request)
present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
present_approval(merge_request)
end
desc 'Remove an approval from a merge request' do
success Entities::MergeRequestApprovals
success ::EE::API::Entities::ApprovalState
end
post 'unapprove' do
merge_request = find_project_merge_request(params[:merge_request_iid])
......@@ -130,7 +144,7 @@ module API
.new(user_project, current_user)
.execute(merge_request)
present merge_request.present(current_user: current_user), with: Entities::MergeRequestApprovals, current_user: current_user
present_approval(merge_request)
end
end
end
......
# frozen_string_literal: true
module API
class ProjectApprovalRules < ::Grape::API
before { authenticate! }
before { not_found! unless ::Feature.enabled?(:approval_rules, user_project) }
ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
params do
requires :id, type: String, desc: 'The ID of a project'
end
resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
segment ':id/approval_settings' do
desc 'Get all project approval rules' do
detail 'Private API subject to change'
success EE::API::Entities::ProjectApprovalRules
end
get do
authorize! :create_merge_request_in, user_project
present user_project, with: EE::API::Entities::ProjectApprovalRules, current_user: current_user
end
segment 'rules' do
desc 'Create new approval rule' do
detail 'Private API subject to change'
success EE::API::Entities::ApprovalRule
end
params do
requires :name, type: String, desc: 'The name of the approval rule'
requires :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 :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
end
post do
authorize! :admin_project, user_project
result = ::ApprovalRules::CreateService.new(user_project, current_user, declared_params(include_missing: false)).execute
if result[:status] == :success
present result[:rule], with: EE::API::Entities::ApprovalRule, current_user: current_user
else
render_api_error!(result[:message], 400)
end
end
segment ':approval_rule_id' do
desc 'Update approval rule' do
detail 'Private API subject to change'
success EE::API::Entities::ApprovalRule
end
params do
requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule'
optional :name, type: String, desc: 'The name of the approval 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 :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
end
put do
authorize! :admin_project, user_project
params = declared_params(include_missing: false)
approval_rule = user_project.approval_rules.find(params.delete(:approval_rule_id))
result = ::ApprovalRules::UpdateService.new(approval_rule, current_user, params).execute
if result[:status] == :success
present result[:rule], with: EE::API::Entities::ApprovalRule, current_user: current_user
else
render_api_error!(result[:message], 400)
end
end
desc 'Delete an approval rule' do
detail 'Private API subject to change'
end
params do
requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule'
end
delete do
authorize! :admin_project, user_project
approval_rule = user_project.approval_rules.find(params[:approval_rule_id])
destroy_conditionally!(approval_rule)
no_content!
end
end
end
end
end
end
end
......@@ -14,15 +14,15 @@ module API
segment ':id/approvals' do
desc 'Get all project approvers and related configuration' do
detail 'This feature was introduced in 10.6'
success ::API::Entities::ApprovalSettings
success EE::API::Entities::ApprovalSettings
end
get '/' do
present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
present user_project.present(current_user: current_user), with: EE::API::Entities::ApprovalSettings
end
desc 'Change approval-related configuration' do
detail 'This feature was introduced in 10.6'
success ::API::Entities::ApprovalSettings
success EE::API::Entities::ApprovalSettings
end
params do
optional :approvals_before_merge, type: Integer, desc: 'The amount of approvals required before an MR can be merged'
......@@ -36,7 +36,7 @@ module API
result = ::Projects::UpdateService.new(user_project, current_user, project_params).execute
if result[:status] == :success
present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
present user_project.present(current_user: current_user), with: EE::API::Entities::ApprovalSettings
else
render_validation_error!(user_project)
end
......@@ -45,7 +45,7 @@ module API
desc 'Update approvers and approver groups' do
detail 'This feature was introduced in 10.6'
success ::API::Entities::ApprovalSettings
success EE::API::Entities::ApprovalSettings
end
params do
requires :approver_ids, type: Array[String], coerce_with: ARRAY_COERCION_LAMBDA, desc: 'Array of User IDs to set as approvers.'
......@@ -55,7 +55,7 @@ module API
result = ::Projects::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute
if result[:status] == :success
present user_project.present(current_user: current_user), with: ::API::Entities::ApprovalSettings
present user_project.present(current_user: current_user), with: EE::API::Entities::ApprovalSettings
else
render_validation_error!(user_project)
end
......
......@@ -9,6 +9,7 @@ module EE
mount ::EE::API::Boards
mount ::EE::API::GroupBoards
mount ::API::ProjectApprovalRules
mount ::API::Unleash
mount ::API::EpicIssues
mount ::API::EpicLinks
......
......@@ -243,10 +243,71 @@ module EE
expose :title
end
class ApprovalRule < Grape::Entity
def initialize(object, options = {})
presenter = ::ApprovalRulePresenter.new(object, current_user: options[:current_user])
super(presenter, options)
end
expose :id, :name
expose :approvers, using: ::API::Entities::UserBasic
expose :approvals_required
expose :users, using: ::API::Entities::UserBasic
expose :groups, using: ::API::Entities::Group
end
class MergeRequestApprovalRule < ApprovalRule
class SourceRule < Grape::Entity
expose :approvals_required
end
expose :approved_approvers, as: :approved_by, using: ::API::Entities::UserBasic
expose :code_owner
expose :source_rule, using: SourceRule
end
# Decorates ApprovalState
class MergeRequestApprovalRules < Grape::Entity
expose :approval_rules_overwritten do |approval_state|
approval_state.approval_rules_overwritten?
end
expose :wrapped_approval_rules, as: :rules, using: MergeRequestApprovalRule
expose :fallback_approvals_required
expose :use_fallback do |approval_state|
approval_state.use_fallback?
end
end
# Decorates Project
class ProjectApprovalRules < Grape::Entity
expose :approval_rules, as: :rules, using: ApprovalRule
expose :approvals_before_merge, as: :fallback_approvals_required
end
# @deprecated
class Approver < Grape::Entity
expose :user, using: ::API::Entities::UserBasic
end
# @deprecated
class ApproverGroup < Grape::Entity
expose :group, using: ::API::Entities::Group
end
class ApprovalSettings < Grape::Entity
expose :approvers, using: EE::API::Entities::Approver
expose :approver_groups, using: EE::API::Entities::ApproverGroup
expose :approvals_before_merge
expose :reset_approvals_on_push
expose :disable_overriding_approvers_per_merge_request
end
class Approvals < Grape::Entity
expose :user, using: ::API::Entities::UserBasic
end
# @deprecated, replaced with ApprovalState
class MergeRequestApprovals < ::API::Entities::ProjectEntity
def initialize(merge_request, options = {})
presenter = merge_request.present(current_user: options[:current_user])
......@@ -259,6 +320,10 @@ module EE
expose :approvals_left
expose :approvals, as: :approved_by, using: EE::API::Entities::Approvals
expose :approvers_left, as: :suggested_approvers, using: ::API::Entities::UserBasic
# @deprecated
expose :approvers, using: EE::API::Entities::Approver
# @deprecated
expose :approver_groups, using: EE::API::Entities::ApproverGroup
expose :user_has_approved do |merge_request, options|
merge_request.has_approved?(options[:current_user])
......@@ -269,6 +334,64 @@ module EE
end
end
class ApprovalState < Grape::Entity
expose :merge_request, merge: true, using: ::API::Entities::ProjectEntity
expose(:merge_status) { |approval_state| approval_state.merge_request.merge_status }
expose :approved?, as: :approved
expose :approvals_required
expose :approvals_left
expose :approved_by, using: EE::API::Entities::Approvals do |approval_state|
approval_state.merge_request.approvals
end
expose :suggested_approvers, using: ::API::Entities::UserBasic do |approval_state, options|
# TODO order by relevance
approval_state.unactioned_approvers
end
# @deprecated, reads from first regular rule instead
expose :approvers do |approval_state|
if rule = approval_state.first_regular_rule
rule.users.map do |user|
{ user: ::API::Entities::UserBasic.represent(user) }
end
else
[]
end
end
# @deprecated, reads from first regular rule instead
expose :approver_groups do |approval_state|
if rule = approval_state.first_regular_rule
presenter = ::ApprovalRulePresenter.new(rule, current_user: options[:current_user])
presenter.groups.map do |group|
{ group: ::API::Entities::Group.represent(group) }
end
else
[]
end
end
expose :user_has_approved do |approval_state, options|
approval_state.has_approved?(options[:current_user])
end
expose :user_can_approve do |approval_state, options|
approval_state.can_approve?(options[:current_user])
end
expose :approval_rules_left do |approval_state, options|
approval_state.approval_rules_left.map(&:name)
end
expose :has_approval_rules do |approval_state|
approval_state.has_approval_rules?
end
end
class LdapGroup < Grape::Entity
expose :cn
end
......
......@@ -9,6 +9,10 @@ module EE
helpers do
params :optional_params_ee do
optional :approvals_before_merge, type: Integer, desc: 'Number of approvals required before this can be merged'
optional :approval_rules_attributes, type: Array, documentation: { hidden: true } do
optional :id, type: Integer, desc: 'The ID of a rule'
optional :approvals_required, type: Integer, desc: 'Total number of approvals required'
end
end
end
......
......@@ -20,6 +20,7 @@ module EE
optional :mirror_overwrites_diverged_branches, type: Grape::API::Boolean, desc: 'Pull mirror overwrites diverged branches'
optional :import_url, type: String, desc: 'URL from which the project is imported'
optional :packages_enabled, type: Grape::API::Boolean, desc: 'Enable project packages feature'
optional :fallback_approvals_required, type: Integer, desc: 'Overall approvals required when no rule is present'
end
def apply_filters(projects)
......@@ -67,7 +68,8 @@ module EE
:repository_storage,
:external_authorization_classification_label,
:import_url,
:packages_enabled
:packages_enabled,
:fallback_approvals_required
]
end
end
......
......@@ -163,6 +163,22 @@ describe Projects::MergeRequestsController do
expect(merge_request.reload.approvals_before_merge).to eq(2)
end
it 'creates rules' do
users = create_list(:user, 3)
users.each { |user| project.add_developer(user) }
update_merge_request(approval_rules_attributes: [
{ name: 'foo', user_ids: users.map(&:id), approvals_required: 3 }
])
expect(merge_request.reload.approval_rules.size).to eq(1)
rule = merge_request.reload.approval_rules.first
expect(rule.name).to eq('foo')
expect(rule.approvals_required).to eq(3)
end
end
context 'disabled' do
......
{
"type": "object",
"properties": {
"id": { "type": "integer" },
"name": { "type": "string" },
"approvals_required": { "type": "integer" },
"approvers": {
"type": "array",
"items": {
"type": "object",
"properties": {}
}
},
"groups": {
"type": "array",
"items": {
"type": "object",
"properties": {}
}
},
"users": {
"type": "array",
"items": {
"type": "object",
"properties": {}
}
}
},
"additionalProperties": false
}
{
"type": "object",
"properties": {
"rules": {
"type": "array",
"items": { "$ref": "project_approval_rule.json" }
},
"fallback_approvals_required": { "type": "integer" }
},
"additionalProperties": false
}
# frozen_string_literal: true
require 'spec_helper'
# Based on approvable_spec.rb
describe ApprovableForRule do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:author) { merge_request.author }
describe '#approvers_overwritten?' do
subject { merge_request.approvers_overwritten? }
it 'returns false when merge request has no approvers' do
is_expected.to be false
end
it 'returns true when merge request has user approver' do
create(:approver, target: merge_request)
is_expected.to be true
end
it 'returns true when merge request has group approver' do
group = create(:group_with_members)
create(:approver_group, target: merge_request, group: group)
is_expected.to be true
end
end
describe '#can_approve?' do
subject { merge_request.can_approve?(user) }
it 'returns false if user is nil' do
expect(merge_request.can_approve?(nil)).to be false
end
it 'returns true when user is included in the approvers list' do
user = create(:approver, target: merge_request).user
expect(merge_request.can_approve?(user)).to be true
end
context 'when authors can approve' do
before do
project.update(merge_requests_author_approval: true)
end
context 'when the user is the author' do
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(author)).to be false
end
end
context 'when user is committer' do
let(:user) { create(:user, email: merge_request.commits.first.committer_email) }
before do
project.add_developer(user)
end
it 'returns true when user is approver' do
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when user is not approver' do
expect(merge_request.can_approve?(user)).to be false
end
end
end
context 'when authors cannot approve' do
before do
project.update(merge_requests_author_approval: false)
end
it 'returns false when user is the author' do
create(:approver, target: merge_request, user: author)
expect(merge_request.can_approve?(author)).to be false
end
it 'returns false when user is a committer' do
user = create(:user, email: merge_request.commits.first.committer_email)
project.add_developer(user)
create(:approver, target: merge_request, user: user)
expect(merge_request.can_approve?(user)).to be false
end
end
it 'returns false when user is unable to update the merge request' do
user = create(:user)
project.add_guest(user)
expect(merge_request.can_approve?(user)).to be false
end
context 'when approvals are required' do
before do
project.update(approvals_before_merge: 1)
end
it 'returns true when approvals are still accepted and user still has not approved' do
user = create(:user)
project.add_developer(user)
expect(merge_request.can_approve?(user)).to be true
end
it 'returns false when there is still one approver missing' do
user = create(:user)
project.add_developer(user)
create(:approver, target: merge_request)
expect(merge_request.can_approve?(user)).to be false
end
end
end
end
......@@ -85,4 +85,32 @@ describe ApprovalMergeRequestRule do
end
end
end
describe 'validations' do
describe 'approvals_required' do
subject { build(:approval_merge_request_rule, merge_request: merge_request) }
it 'is a natual number' do
subject.assign_attributes(approvals_required: 2)
expect(subject).to be_valid
subject.assign_attributes(approvals_required: 0)
expect(subject).to be_valid
subject.assign_attributes(approvals_required: -1)
expect(subject).to be_invalid
end
context 'when project rule is present' do
let(:project_rule) { create(:approval_project_rule, project: merge_request.project, approvals_required: 3) }
it 'has to be greater than or equal to project rule approvals_required' do
subject.assign_attributes(approval_project_rule: project_rule, approvals_required: 2)
subject.valid?
expect(subject.errors[:approvals_required]).to include("must be greater than or equal to 3")
end
end
end
end
end
......@@ -127,94 +127,106 @@ describe ApprovalState do
end
describe '#approved?' do
context 'when no rules' do
before do
project.update(approvals_before_merge: 1)
end
shared_examples_for 'when rules are present' do
context 'when all rules are approved' do
before do
subject.wrapped_approval_rules.each do |rule|
create(:approval, merge_request: merge_request, user: rule.users.first)
end
end
context 'when overall_approvals_required is not met' do
it 'returns false' do
expect(subject.wrapped_approval_rules.size).to eq(0)
expect(subject.approved?).to eq(false)
it 'returns true' do
expect(subject.approved?).to eq(true)
end
end
context 'when overall_approvals_required is met' do
it 'returns true' do
create(:approval, merge_request: merge_request)
context 'when some rules are not approved' do
before do
allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
end
expect(subject.wrapped_approval_rules.size).to eq(0)
expect(subject.approved?).to eq(true)
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
end
context 'when rules are present' do
shared_examples_for 'checking fallback_approvals_required' do
before do
2.times { create_rule(users: [create(:user)]) }
project.update(approvals_before_merge: 1)
subject.wrapped_approval_rules.each do |rule|
allow(rule).to receive(:approved?).and_return(true)
end
end
context 'when all rules are approved' do
before do
subject.wrapped_approval_rules.each do |rule|
create(:approval, merge_request: merge_request, user: rule.users.first)
end
context 'when it is not met' do
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
context 'when it is met' do
it 'returns true' do
create(:approval, merge_request: merge_request)
expect(subject.approved?).to eq(true)
end
end
end
context 'when overall_approvals_required is not met' do
before do
project.update(approvals_before_merge: 3)
end
context 'when no rules' do
it_behaves_like 'checking fallback_approvals_required'
end
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
context 'when only code owner rules present' do
before do
2.times { create_rule(users: [create(:user)], code_owner: true) }
end
context 'when some rules are not approved' do
before do
allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
end
it_behaves_like 'when rules are present'
it_behaves_like 'checking fallback_approvals_required'
end
it 'returns false' do
expect(subject.approved?).to eq(false)
end
context 'when regular rules present' do
before do
project.update(approvals_before_merge: 999)
2.times { create_rule(users: [create(:user)]) }
end
it_behaves_like 'when rules are present'
end
end
describe '#any_approver_allowed?' do
context 'when approved' do
before do
allow(subject).to receive(:approved?).and_return(true)
end
context 'when no rules' do
it 'returns true' do
expect(subject.any_approver_allowed?).to eq(true)
end
end
context 'when not approved' do
context 'when with rules' do
before do
allow(subject).to receive(:approved?).and_return(false)
create_rule(approvals_required: 1, users: [approver1])
end
it 'returns false' do
expect(subject.approved?).to eq(false)
context 'when approved' do
before do
allow(subject).to receive(:approved?).and_return(true)
end
it 'returns true' do
expect(subject.any_approver_allowed?).to eq(true)
end
end
context 'when overall_approvals_required cannot be met' do
context 'when not approved' do
before do
project.update(approvals_before_merge: 1)
allow(subject).to receive(:approved?).and_return(false)
end
it 'returns false' do
expect(subject.any_approver_allowed?).to eq(true)
expect(subject.approved?).to eq(false)
end
end
end
......@@ -671,64 +683,74 @@ describe ApprovalState do
end
describe '#approved?' do
context 'when no rules' do
before do
project.update(approvals_before_merge: 1)
end
shared_examples_for 'when rules are present' do
context 'when all rules are approved' do
before do
subject.wrapped_approval_rules.each do |rule|
create(:approval, merge_request: merge_request, user: rule.users.first)
end
end
context 'when overall_approvals_required is not met' do
it 'returns false' do
expect(subject.wrapped_approval_rules.size).to eq(0)
expect(subject.approved?).to eq(false)
it 'returns true' do
expect(subject.approved?).to eq(true)
end
end
context 'when overall_approvals_required is met' do
it 'returns true' do
create(:approval, merge_request: merge_request)
context 'when some rules are not approved' do
before do
allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
end
expect(subject.wrapped_approval_rules.size).to eq(0)
expect(subject.approved?).to eq(true)
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
end
context 'when rules are present' do
shared_examples_for 'checking fallback_approvals_required' do
before do
2.times { create_rule(users: [create(:user)]) }
project.update(approvals_before_merge: 1)
subject.wrapped_approval_rules.each do |rule|
allow(rule).to receive(:approved?).and_return(true)
end
end
context 'when all rules are approved' do
before do
subject.wrapped_approval_rules.each do |rule|
create(:approval, merge_request: merge_request, user: rule.users.first)
end
context 'when it is not met' do
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
context 'when it is met' do
it 'returns true' do
create(:approval, merge_request: merge_request)
expect(subject.approved?).to eq(true)
end
end
end
context 'when overall_approvals_required is not met' do
before do
project.update(approvals_before_merge: 3)
end
context 'when no rules' do
it_behaves_like 'checking fallback_approvals_required'
end
it 'returns false' do
expect(subject.approved?).to eq(false)
end
end
context 'when only code owner rules present' do
before do
2.times { create_rule(users: [create(:user)], code_owner: true) }
end
context 'when some rules are not approved' do
before do
allow(subject.wrapped_approval_rules.first).to receive(:approved?).and_return(false)
end
it_behaves_like 'when rules are present'
it_behaves_like 'checking fallback_approvals_required'
end
it 'returns false' do
expect(subject.approved?).to eq(false)
end
context 'when regular rules present' do
before do
project.update(approvals_before_merge: 999)
2.times { create_rule(users: [create(:user)]) }
end
it_behaves_like 'when rules are present'
end
end
......@@ -751,16 +773,6 @@ describe ApprovalState do
it 'returns false' do
expect(subject.approved?).to eq(false)
end
context 'when overall_approvals_required cannot be met' do
before do
project.update(approvals_before_merge: 1)
end
it 'returns false' do
expect(subject.any_approver_allowed?).to eq(true)
end
end
end
end
......
......@@ -17,6 +17,88 @@ describe MergeRequest do
it { is_expected.to have_many(:approved_by_users) }
end
describe 'approval_rules' do
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 'approvals' do
shared_examples_for 'authors self-approval authorization' do
context 'when authors are authorized to approve their own MRs' do
......
# frozen_string_literal: true
require 'spec_helper'
# Based on visible_approvable_spec.rb
describe VisibleApprovableForRule do
let(:resource) { create(:merge_request, source_project: project, target_project: project) }
let!(:project) { create(:project, :repository) }
let!(:user) { project.creator }
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.approvers_left }
it 'avoids N+1 queries' do
control = ActiveRecord::QueryRecorder.new { subject }
expect { subject }.not_to exceed_query_limit(control)
end
it 'returns all approvers left' do
resource.approvals.create!(user: approver.user)
is_expected.to match_array(public_approver_group.users + private_approver_group.users)
end
end
describe '#overall_approvers' do
let(:approver) { create(:user) }
let(:code_owner) { build(:user) }
let!(:project_regular_rule) { create(:approval_project_rule, project: project, users: [approver]) }
let!(:code_owner_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [code_owner], code_owner: true) }
before do
project.add_developer(approver)
project.add_developer(code_owner)
end
subject { resource.overall_approvers }
it 'returns a list of all the project approvers' do
is_expected.to contain_exactly(approver, code_owner)
end
context 'when exclude_code_owners is true' do
subject { resource.overall_approvers(exclude_code_owners: true) }
it 'excludes code owners' do
is_expected.to contain_exactly(approver)
end
end
context 'when approvers are overwritten' do
let!(:merge_request_regular_rule) { create(:approval_merge_request_rule, merge_request: resource, users: [create(:user)]) }
it 'returns the list of all the merge request level approvers' do
is_expected.to contain_exactly(*merge_request_regular_rule.users, code_owner)
end
end
shared_examples_for 'able to exclude authors' do
it 'excludes author if authors cannot approve' do
project.update(merge_requests_author_approval: false)
is_expected.not_to include(approver)
end
it 'includes author if authors are able to approve' do
project.update(merge_requests_author_approval: true)
is_expected.to include(approver)
end
end
context 'when author is approver' do
let!(:approver) { resource.author }
it_behaves_like 'able to exclude authors'
end
context 'when committer is approver' do
let(:approver) { create(:user, email: resource.commits.first.committer_email) }
it_behaves_like 'able to exclude authors'
end
end
describe '#all_approvers_including_groups' do
let!(:group) { create(:group_with_members) }
let!(:approver_group) { create(:approver_group, target: resource, group: group) }
let!(:approver) { create(:approver, target: resource) }
subject { resource.all_approvers_including_groups }
it 'returns all approvers (groups and users)' do
is_expected.to match_array(approver_group.users + [approver.user])
end
end
describe '#authors_can_approve?' do
subject { resource.authors_can_approve? }
it 'returns false when merge_requests_author_approval flag is off' do
is_expected.to be false
end
it 'returns true when merge_requests_author_approval flag is turned on' do
project.update(merge_requests_author_approval: true)
is_expected.to be true
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalMergeRequestRulePolicy do
let(:merge_request) { create(:merge_request) }
let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request) }
def permissions(user, approval_rule)
described_class.new(user, approval_rule)
end
context 'when user can update merge request' do
it 'allows updating approval rule' do
expect(permissions(merge_request.author, approval_rule)).to be_allowed(:edit_approval_rule)
end
end
context 'when user cannot update merge request' do
it 'disallow updating approval rule' do
expect(permissions(create(:user), approval_rule)).to be_disallowed(:edit_approval_rule)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalProjectRulePolicy do
let(:project) { create(:project) }
let!(:approval_rule) { create(:approval_project_rule, project: project) }
def permissions(user, approval_rule)
described_class.new(user, approval_rule)
end
context 'when user can admin project' do
it 'allows updating approval rule' do
expect(permissions(project.creator, approval_rule)).to be_allowed(:edit_approval_rule)
end
end
context 'when user cannot admin project' do
let(:user) { create(:user) }
before do
project.add_developer(user)
end
it 'disallow updating approval rule' do
expect(permissions(user, approval_rule)).to be_disallowed(:edit_approval_rule)
end
end
end
......@@ -13,6 +13,38 @@ describe MergeRequestPresenter do
end
end
describe '#api_approvals_path' do
subject { described_class.new(resource, current_user: user).api_approvals_path }
it 'returns path' do
is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approvals")
end
end
describe '#api_approval_settings_path' do
subject { described_class.new(resource, current_user: user).api_approval_settings_path }
it 'returns path' do
is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approval_settings")
end
end
describe '#api_approve_path' do
subject { described_class.new(resource, current_user: user).api_approve_path }
it 'returns path' do
is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/approve")
end
end
describe '#api_unapprove_path' do
subject { described_class.new(resource, current_user: user).api_unapprove_path }
it 'returns path' do
is_expected.to eq("/api/v4/projects/#{resource.project.id}/merge_requests/#{resource.iid}/unapprove")
end
end
describe '#approvers_left' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
......@@ -42,6 +74,26 @@ describe MergeRequestPresenter do
end
end
describe '#approvers_left with approval_rule enabled' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
before do
resource.approvals.create!(user: approver.user)
end
subject { described_class.new(resource, current_user: user).approvers_left }
it 'contains all approvers' do
approvers = public_approver_group.users + private_approver_group.users - [user]
is_expected.to match_array(approvers)
end
end
describe '#overall_approver_groups' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
......@@ -88,4 +140,20 @@ describe MergeRequestPresenter do
end
end
end
describe '#all_approvers_including_groups with approval_rule enabled' do
let!(:private_group) { create(:group_with_members, :private) }
let!(:public_group) { create(:group_with_members) }
let!(:public_approver_group) { create(:approver_group, target: resource, group: public_group) }
let!(:private_approver_group) { create(:approver_group, target: resource, group: private_group) }
let!(:approver) { create(:approver, target: resource) }
subject { described_class.new(resource, current_user: user).all_approvers_including_groups }
it do
approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
is_expected.to match_array(approvers)
end
end
end
......@@ -18,6 +18,32 @@ describe API::MergeRequests do
project.add_reporter(user)
end
describe 'PUT /projects/:id/merge_requests' do
context 'when updating existing approval rules' do
def update_merge_request(params)
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: params
end
let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1) }
it 'is successful' do
update_merge_request(
title: "New title",
approval_rules_attributes: [
{ id: rule.id, approvals_required: 2 }
]
)
expect(response).to have_gitlab_http_status(200)
merge_request.reload
expect(merge_request.approval_rules.size).to eq(1)
expect(merge_request.approval_rules.first.approvals_required).to eq(2)
end
end
end
describe "POST /projects/:id/merge_requests" do
def create_merge_request(args)
defaults = {
......
# frozen_string_literal: true
require 'spec_helper'
describe API::ProjectApprovalRules do
set(:group) { create(:group_with_members) }
set(:user) { create(:user) }
set(:user2) { create(:user) }
set(:admin) { create(:user, :admin) }
set(:project) { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
set(:approver) { create(:user) }
let(:url) { "/projects/#{project.id}/approval_settings/rules" }
describe 'GET /projects/:id/approval_settings' do
let(:url) { "/projects/#{project.id}/approval_settings" }
context 'when the request is correct' do
let!(:rule) do
rule = create(:approval_project_rule, name: 'security', project: project, approvals_required: 7)
rule.users << approver
rule
end
let(:developer) do
user = create(:user)
project.add_guest(user)
user
end
it 'matches the response schema' do
get api(url, developer)
expect(response).to have_gitlab_http_status(200)
expect(response).to match_response_schema('public_api/v4/project_approval_rules', dir: 'ee')
json = json_response
expect(json['rules'].size).to eq(1)
rule = json['rules'].first
expect(rule['approvals_required']).to eq(7)
expect(rule['name']).to eq('security')
end
context 'private group filtering' do
set(:private_group) { create :group, :private }
before do
rule.groups << private_group
end
it 'excludes private groups if user has no access' do
get api(url, developer)
json = json_response
rule = json['rules'].first
expect(rule['groups'].size).to eq(0)
end
it 'includes private groups if user has access' do
private_group.add_owner(developer)
get api(url, developer)
json = json_response
rule = json['rules'].first
expect(rule['groups'].size).to eq(1)
end
end
end
end
describe 'POST /projects/:id/approval_settings/rules' do
let(:current_user) { user }
let(:params) do
{
name: 'security',
approvals_required: 10
}
end
context 'when missing parameters' do
it 'returns 400 status' do
post api(url, current_user)
expect(response).to have_gitlab_http_status(400)
end
end
context 'when user is without access' do
it 'returns 403' do
post api(url, user2), params
expect(response).to have_gitlab_http_status(403)
end
end
context 'when the request is correct' do
it 'returns 201 status' do
post api(url, current_user), params
expect(response).to have_gitlab_http_status(201)
expect(response).to match_response_schema('public_api/v4/project_approval_rule', dir: 'ee')
end
it 'changes settings properly' do
create(:approval_project_rule, project: project, approvals_required: 2)
project.reset_approvals_on_push = false
project.disable_overriding_approvers_per_merge_request = true
project.save
post api(url, current_user), params
expect(json_response.symbolize_keys).to include(params)
end
end
end
describe 'PUT /projects/:id/approval_settings/:approval_rule_id' do
let!(:approval_rule) { create(:approval_project_rule, project: project) }
let(:url) { "/projects/#{project.id}/approval_settings/rules/#{approval_rule.id}" }
shared_examples_for 'a user with access' do
before do
project.add_developer(approver)
end
context 'when approver already exists' do
before do
approval_rule.users << approver
approval_rule.groups << group
end
context 'when sending json data' do
it 'removes all approvers if empty params are given' do
expect do
put api(url, current_user), params: { users: [], groups: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' }
end.to change { approval_rule.users.count + approval_rule.groups.count }.from(2).to(0)
expect(response).to have_gitlab_http_status(200)
end
end
end
it 'sets approvers' do
expect do
put api(url, current_user), params: { users: [approver.id] }
end.to change { approval_rule.users.count }.from(0).to(1)
expect(approval_rule.users).to contain_exactly(approver)
expect(approval_rule.groups).to be_empty
expect(response).to have_gitlab_http_status(200)
expect(json_response['approvers'][0]['id']).to eq(approver.id)
end
end
context 'as a project admin' do
it_behaves_like 'a user with access' do
let(:current_user) { user }
let(:visible_approver_groups_count) { 0 }
end
end
context 'as a global admin' do
it_behaves_like 'a user with access' do
let(:current_user) { admin }
let(:visible_approver_groups_count) { 1 }
end
end
context 'as a random user' do
it 'returns 403' do
project.approvers.create(user: approver)
expect do
put api(url, user2), { users: [], groups: [] }.to_json, { CONTENT_TYPE: 'application/json' }
end.not_to change { approval_rule.approvers.size }
expect(response).to have_gitlab_http_status(403)
end
end
end
describe 'DELETE /projects/:id/approval_settings/rules/:approval_rule_id' do
let!(:approval_rule) { create(:approval_project_rule, project: project) }
let(:url) { "/projects/#{project.id}/approval_settings/rules/#{approval_rule.id}" }
it 'destroys' do
delete api(url, user)
expect(ApprovalProjectRule.exists?(id: approval_rule.id)).to eq(false)
expect(response).to have_gitlab_http_status(204)
end
context 'when approval rule not found' do
let!(:approval_rule_2) { create(:approval_project_rule) }
let(:url) { "/projects/#{project.id}/approval_settings/#{approval_rule_2.id}" }
it 'returns not found' do
delete api(url, user)
expect(response).to have_gitlab_http_status(404)
end
end
context 'when user is not eligible to delete' do
it 'returns forbidden' do
delete api(url, user2)
expect(response).to have_gitlab_http_status(403)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalRules::CreateService do
let(:project) { create(:project) }
let(:user) { project.creator }
shared_examples 'creatable' do
let(:new_approvers) { create_list(:user, 2) }
let(:new_groups) { create_list(:group, 2, :private) }
it 'creates approval, excluding non-eligible users and groups' do
result = described_class.new(target, user, {
name: 'security',
approvals_required: 1,
user_ids: new_approvers.map(&:id),
group_ids: new_groups.map(&:id)
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.name).to eq('security')
expect(rule.approvals_required).to eq(1)
expect(rule.users).to be_empty
expect(rule.groups).to be_empty
end
context 'when some users and groups are eligible' do
before do
project.add_reporter new_approvers.first
new_groups.first.add_guest(user)
end
it 'creates and includes eligible users and groups' do
result = described_class.new(target, user, {
name: 'security',
approvals_required: 1,
user_ids: new_approvers.map(&:id),
group_ids: new_groups.map(&:id)
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.name).to eq('security')
expect(rule.approvals_required).to eq(1)
expect(rule.users).to contain_exactly(new_approvers.first)
expect(rule.groups).to contain_exactly(new_groups.first)
end
end
context 'when validation fails' do
it 'returns error message' do
result = described_class.new(target, user, {
name: nil,
approvals_required: 1
}).execute
expect(result[:status]).to eq(:error)
end
end
context 'when user does not have right to admin project' do
let(:user) { create(:user) }
it 'returns error message' do
result = described_class.new(target, user, {
approvals_required: 1
}).execute
expect(result[:status]).to eq(:error)
expect(result[:message]).to include('Prohibited')
end
end
end
context 'when target is project' do
let(:target) { project }
it_behaves_like "creatable"
end
context 'when target is merge request' do
let(:target) { create(:merge_request, source_project: project, target_project: project) }
it_behaves_like "creatable"
context 'when project rule id is present' do
let(:project_rule) { create(:approval_project_rule, project: project) }
it 'associates with project rule' do
result = described_class.new(target, user, {
name: 'foo',
approvals_required: 1,
approval_project_rule_id: project_rule.id
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.approval_project_rule).to eq(project_rule)
end
end
context "when project rule id is not the same as MR's project" do
let(:project_rule) { create(:approval_project_rule) }
it 'ignores assignment' do
result = described_class.new(target, user, {
name: 'foo',
approvals_required: 1,
approval_project_rule_id: project_rule.id
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.approval_project_rule).to eq(nil)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe ApprovalRules::UpdateService do
let(:project) { create(:project) }
let(:user) { project.creator }
shared_examples 'editable' do
let(:approval_rule) { target.approval_rules.create(name: 'foo') }
let(:new_approvers) { create_list(:user, 2) }
let(:new_groups) { create_list(:group, 2, :private) }
it 'updates approval, excluding non-eligible users and groups' do
result = described_class.new(approval_rule, user, {
name: 'security',
approvals_required: 1,
user_ids: new_approvers.map(&:id),
group_ids: new_groups.map(&:id)
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.name).to eq('security')
expect(rule.approvals_required).to eq(1)
expect(rule.users).to be_empty
expect(rule.groups).to be_empty
end
context 'when some users and groups are eligible' do
before do
project.add_reporter new_approvers.first
new_groups.first.add_guest user
end
it 'creates and includes eligible users and groups' do
result = described_class.new(approval_rule, user, {
name: 'security',
approvals_required: 1,
user_ids: new_approvers.map(&:id),
group_ids: new_groups.map(&:id)
}).execute
expect(result[:status]).to eq(:success)
rule = result[:rule]
expect(rule.name).to eq('security')
expect(rule.approvals_required).to eq(1)
expect(rule.users).to contain_exactly(new_approvers.first)
expect(rule.groups).to contain_exactly(new_groups.first)
end
end
context 'when validation fails' do
it 'returns error message' do
result = described_class.new(approval_rule, user, {
name: nil,
approvals_required: 1
}).execute
expect(result[:status]).to eq(:error)
end
end
context 'when user does not have right to edit' do
let(:user) { create(:user) }
it 'returns error message' do
result = described_class.new(approval_rule, user, {
approvals_required: 1
}).execute
expect(result[:status]).to eq(:error)
expect(result[:message]).to include('Prohibited')
end
end
end
context 'when target is project' do
let(:target) { project }
it_behaves_like "editable"
end
context 'when target is merge request' do
let(:target) { create(:merge_request, source_project: project, target_project: project) }
it_behaves_like "editable"
end
end
# frozen_string_literal: true
require 'spec_helper'
describe MergeRequests::BaseService do
include ProjectForksHelper
let(:project_member) { create(:user) }
let(:outsider) { create(:user) }
let(:accessible_group) { create(:group, :private) }
let(:inaccessible_group) { create(:group, :private) }
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
describe '#filter_params' do
context 'filter users and groups' do
shared_examples_for(:assigning_users_and_groups) do
before do
project.add_maintainer(user)
project.add_reporter(project_member)
accessible_group.add_developer(user)
allow(service).to receive(:execute_hooks)
end
it 'only assigns eligible users and groups' do
merge_request = subject
rule1 = merge_request.approval_rules.regular.first
expect(rule1.users).to contain_exactly(*project_member)
rule2 = merge_request.approval_rules.regular.last
expect(rule2.users).to be_empty
expect(rule2.groups).to contain_exactly(*accessible_group)
end
end
context 'create' do
it_behaves_like :assigning_users_and_groups do
let(:service) { MergeRequests::CreateService.new(project, user, opts) }
let(:opts) do
{
title: 'Awesome merge_request',
description: 'please fix',
source_branch: 'feature',
target_branch: 'master',
force_remove_source_branch: '1',
approval_rules_attributes: [
{ name: 'foo', user_ids: [project_member.id, outsider.id] },
{ name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
]
}
end
subject { service.execute }
end
end
context 'update' do
let(:merge_request) { create(:merge_request, target_project: project, source_project: project)}
it_behaves_like :assigning_users_and_groups do
let(:service) { MergeRequests::UpdateService.new(project, user, opts) }
let(:opts) do
{
approval_rules_attributes: [
{ name: 'foo', user_ids: [project_member.id, outsider.id] },
{ name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] }
]
}
end
subject { service.execute(merge_request) }
end
end
end
end
end
......@@ -137,6 +137,34 @@ describe MergeRequests::RefreshService do
end
end
end
context 'when code owners enabled, with approval_rule enabled' do
let(:relevant_merge_requests) { [merge_request, another_merge_request] }
let(:new_owners) { [owner] }
before do
relevant_merge_requests.each do |merge_request|
expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
end
[forked_merge_request].each do |merge_request|
expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request)
end
end
it 'triggers syncing of code owners' do
relevant_merge_requests.each do |merge_request|
expect(merge_request.approval_rules.code_owner.exists?).to eq(false)
end
subject
relevant_merge_requests.each do |merge_request|
code_owner_rule = merge_request.approval_rules.code_owner.first
expect(code_owner_rule.users).to eq(new_owners)
end
end
end
end
end
end
......@@ -251,6 +251,16 @@ describe Projects::UpdateService, '#execute' do
expect(rule.reload.approvals_required).to eq(0)
end
end
context 'when approval_rule feature is enabled' do
it "does not update approval_rules' approvals_required" do
rule = create(:approval_project_rule, project: project)
expect do
update_project(project, user, approvals_before_merge: 42)
end.not_to change { rule.reload.approvals_required }
end
end
end
def update_project(project, user, opts)
......
......@@ -741,32 +741,6 @@ module API
end
end
class Approver < Grape::Entity
expose :user, using: Entities::UserBasic
end
class ApproverGroup < Grape::Entity
expose :group, using: Entities::Group
end
class MergeRequestApprovals < ProjectEntity
expose :merge_status
expose :approvals_required
expose :approvals_left
expose :approvals, as: :approved_by, using: Entities::Approver
expose :approvers_left, as: :suggested_approvers, using: Entities::UserBasic
expose :approvers, using: Entities::Approver
expose :approver_groups, using: Entities::ApproverGroup
expose :user_has_approved do |merge_request, options|
merge_request.has_approved?(options[:current_user])
end
expose :user_can_approve do |merge_request, options|
merge_request.can_approve?(options[:current_user])
end
end
class MergeRequestDiff < Grape::Entity
expose :id, :head_commit_sha, :base_commit_sha, :start_commit_sha,
:created_at, :merge_request_id, :state, :real_size
......@@ -1560,14 +1534,6 @@ module API
klass.prepend(with)
end
class ApprovalSettings < Grape::Entity
expose :approvers, using: Entities::Approver
expose :approver_groups, using: Entities::ApproverGroup
expose :approvals_before_merge
expose :reset_approvals_on_push
expose :disable_overriding_approvers_per_merge_request
end
class ManagedLicense < Grape::Entity
expose :id, :name, :approval_status
end
......
......@@ -129,6 +129,10 @@
"approvals_before_merge": { "type": ["integer", "null"] },
"approved": { "type": "boolean" },
"approvals_path": { "type": ["string", "null"] },
"api_approvals_path": { "type": ["string", "null"] },
"api_approval_settings_path": { "type": ["string", "null"] },
"api_approve_path": { "type": ["string", "null"] },
"api_unapprove_path": { "type": ["string", "null"] },
"codeclimate": {
"head_path": { "type": "string" },
"base_path": { "type": "string" }
......
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