Commit 2184b87b authored by Max Woolf's avatar Max Woolf

Add group <> project level MR approval settings cascade

Adds a new API endpoint to be the canonical list of
MR approval settings for a project, which inherits
from the instance and group. It also contains
information about which level defined the rule.

Adds logic to the Resovler class to calculate.

Adds specs for every concievable input.
parent 6c981091
...@@ -32,6 +32,10 @@ module EE ...@@ -32,6 +32,10 @@ module EE
::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request
end end
condition(:group_merge_request_approval_settings_enabled) do
@subject.feature_available?(:group_merge_request_approval_settings)
end
with_scope :global with_scope :global
condition(:locked_merge_request_author_setting) do condition(:locked_merge_request_author_setting) do
License.feature_available?(:admin_merge_request_approvers_rules) && License.feature_available?(:admin_merge_request_approvers_rules) &&
...@@ -387,6 +391,10 @@ module EE ...@@ -387,6 +391,10 @@ module EE
end end
rule { auditor | can?(:developer_access) }.enable :add_project_to_instance_security_dashboard rule { auditor | can?(:developer_access) }.enable :add_project_to_instance_security_dashboard
rule { (admin | owner) & group_merge_request_approval_settings_enabled }.policy do
enable :admin_merge_request_approval_settings
end
end end
override :lookup_access_level! override :lookup_access_level!
......
...@@ -5,6 +5,7 @@ module MergeRequestApprovalSettings ...@@ -5,6 +5,7 @@ module MergeRequestApprovalSettings
def execute def execute
return ServiceResponse.error(message: 'Insufficient permissions') unless allowed? return ServiceResponse.error(message: 'Insufficient permissions') unless allowed?
if container.is_a?(Group)
setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(container) setting = GroupMergeRequestApprovalSetting.find_or_initialize_by_group(container)
setting.assign_attributes(params) setting.assign_attributes(params)
...@@ -13,6 +14,22 @@ module MergeRequestApprovalSettings ...@@ -13,6 +14,22 @@ module MergeRequestApprovalSettings
else else
ServiceResponse.error(message: setting.errors.messages) ServiceResponse.error(message: setting.errors.messages)
end end
elsif container.is_a?(Project)
resolved_params = {
merge_requests_author_approval: params[:allow_author_approval],
merge_requests_disable_committers_approval: !params[:allow_committer_approval],
disable_overriding_approvers_per_merge_request: !params[:allow_overrides_to_approver_list_per_merge_request],
reset_approvals_on_push: !params[:retain_approvals_on_push],
require_password_to_approve: params[:require_password_to_approve]
}
result = ::Projects::UpdateService.new(container, current_user, resolved_params).execute
if result[:status] == :success
ServiceResponse.success(payload: container)
else
ServiceResponse.error(message: container.errors.messages)
end
end
end end
private private
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module API module API
module Entities module Entities
class GroupMergeRequestApprovalSetting < Grape::Entity class MergeRequestApprovalSetting < Grape::Entity
expose :allow_author_approval expose :allow_author_approval
expose :allow_committer_approval expose :allow_committer_approval
expose :allow_overrides_to_approver_list_per_merge_request expose :allow_overrides_to_approver_list_per_merge_request
......
# frozen_string_literal: true
module API
class GroupMergeRequestApprovalSettings < ::API::Base
feature_category :source_code_management
before do
authenticate!
not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group)
authorize! :admin_merge_request_approval_settings, user_group
end
params do
requires :id, type: String, desc: 'The ID of a group'
end
resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
segment ':id/merge_request_approval_setting' do
desc 'Get group merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::GroupMergeRequestApprovalSetting
end
get do
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
end
desc 'Update existing merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::GroupMergeRequestApprovalSetting
end
params do
optional :allow_author_approval, type: Boolean, desc: 'Allow authors to self-approve merge requests'
optional :allow_committer_approval, type: Boolean, desc: 'Allow committers to approve merge requests'
optional :allow_overrides_to_approver_list_per_merge_request,
type: Boolean, desc: 'Allow overrides to approver list per merge request'
optional :retain_approvals_on_push, type: Boolean, desc: 'Retain approval count on a new push'
optional :require_password_to_approve,
type: Boolean, desc: 'Require approver to authenticate before approving'
at_least_one_of :allow_author_approval,
:allow_committer_approval,
:allow_overrides_to_approver_list_per_merge_request,
:retain_approvals_on_push,
:require_password_to_approve
end
put do
setting_params = declared_params(include_missing: false)
response = ::MergeRequestApprovalSettings::UpdateService
.new(container: user_group, current_user: current_user, params: setting_params).execute
if response.success?
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
present setting, with: ::API::Entities::GroupMergeRequestApprovalSetting
else
render_api_error!(response.message, :bad_request)
end
end
end
end
end
end
# frozen_string_literal: true
module API
class MergeRequestApprovalSettings < ::API::Base
feature_category :source_code_management
before do
authenticate!
end
helpers do
params :merge_request_approval_settings do
optional :allow_author_approval, type: Boolean, desc: 'Allow authors to self-approve merge requests', allow_blank: false
optional :allow_committer_approval, type: Boolean, desc: 'Allow committers to approve merge requests', allow_blank: false
optional :allow_overrides_to_approver_list_per_merge_request,
type: Boolean, desc: 'Allow overrides to approver list per merge request', allow_blank: false
optional :retain_approvals_on_push, type: Boolean, desc: 'Retain approval count on a new push', allow_blank: false
optional :require_password_to_approve,
type: Boolean, desc: 'Require approver to authenticate before approving', allow_blank: false
at_least_one_of :allow_author_approval,
:allow_committer_approval,
:allow_overrides_to_approver_list_per_merge_request,
:retain_approvals_on_push,
:require_password_to_approve
end
end
params do
requires :id, type: String, desc: 'The ID of a project'
end
resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
before do
authorize! :admin_merge_request_approval_settings, user_project
end
segment ':id/merge_request_approval_setting' do
desc 'Get project-level MR approval settings' do
detail 'This feature was introduced in 14.3 behind the :group_merge_request_approval_settings_feature_flag'
success EE::API::Entities::MergeRequestApprovalSettings
end
get do
not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_project.root_ancestor)
group = user_project.group.present? ? user_project.root_ancestor : nil
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute
present setting, with: ::API::Entities::MergeRequestApprovalSetting
end
desc 'Update existing merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting
end
params do
use :merge_request_approval_settings
end
put do
setting_params = declared_params(include_missing: false)
response = ::MergeRequestApprovalSettings::UpdateService
.new(container: user_project, current_user: current_user, params: setting_params).execute
if response.success?
group = user_project.group.present? ? user_project.root_ancestor : nil
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: user_project).execute
present setting, with: ::API::Entities::MergeRequestApprovalSetting
else
render_api_error!(response.message, :bad_request)
end
end
end
end
params do
requires :id, type: String, desc: 'The ID of a group'
end
resource :groups, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
before do
not_found! unless ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, user_group)
authorize! :admin_merge_request_approval_settings, user_group
end
segment ':id/merge_request_approval_setting' do
desc 'Get group merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting
end
get do
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
present setting, with: ::API::Entities::MergeRequestApprovalSetting
end
desc 'Update existing merge request approval setting' do
detail 'This feature is gated by the :group_merge_request_approval_settings_feature_flag'
success ::API::Entities::MergeRequestApprovalSetting
end
params do
use :merge_request_approval_settings
end
put do
setting_params = declared_params(include_missing: false)
response = ::MergeRequestApprovalSettings::UpdateService
.new(container: user_group, current_user: current_user, params: setting_params).execute
if response.success?
setting = ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(user_group).execute
present setting, with: ::API::Entities::MergeRequestApprovalSetting
else
render_api_error!(response.message, :bad_request)
end
end
end
end
end
end
...@@ -2,8 +2,9 @@ ...@@ -2,8 +2,9 @@
module ComplianceManagement module ComplianceManagement
module MergeRequestApprovalSettings module MergeRequestApprovalSettings
class Resolver class Resolver
def initialize(group) def initialize(group, project: nil)
@group = group @group = group
@project = project
end end
def execute def execute
...@@ -17,38 +18,45 @@ module ComplianceManagement ...@@ -17,38 +18,45 @@ module ComplianceManagement
end end
def allow_author_approval def allow_author_approval
instance_value = instance_settings.current_application_settings.prevent_merge_requests_author_approval instance_value = !instance_settings.prevent_merge_requests_author_approval
group_value = group_settings&.allow_author_approval group_value = group_settings&.allow_author_approval
project_value = @project&.merge_requests_author_approval
setting(:allow_author_approval, instance_value, group_value, default: true) setting(instance_value, group_value, project_value)
end end
def allow_committer_approval def allow_committer_approval
instance_value = instance_settings.prevent_merge_requests_committers_approval instance_value = !instance_settings.prevent_merge_requests_committers_approval
group_value = group_settings&.allow_committer_approval group_value = group_settings&.allow_committer_approval
project_value = @project ? !@project.merge_requests_disable_committers_approval : nil
setting(:allow_committer_approval, instance_value, group_value, default: true) setting(instance_value, group_value, project_value)
end end
def allow_overrides_to_approver_list_per_merge_request def allow_overrides_to_approver_list_per_merge_request
instance_value = instance_settings.disable_overriding_approvers_per_merge_request instance_value = !instance_settings.disable_overriding_approvers_per_merge_request
group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request
project_value = @project ? !@project.disable_overriding_approvers_per_merge_request : nil
setting(:allow_overrides_to_approver_list_per_merge_request, instance_value, group_value, default: true) setting(instance_value, group_value, project_value)
end end
def retain_approvals_on_push def retain_approvals_on_push
instance_value = nil
group_value = group_settings&.retain_approvals_on_push group_value = group_settings&.retain_approvals_on_push
project_value = @project ? !@project.reset_approvals_on_push : nil
setting(:retain_approvals_on_push, instance_value, group_value, default: true) setting(nil, group_value, project_value)
end end
def require_password_to_approve def require_password_to_approve
instance_value = nil
group_value = group_settings&.require_password_to_approve group_value = group_settings&.require_password_to_approve
project_value = @project&.require_password_to_approve
setting(:require_password_to_approve, instance_value, group_value, default: false) ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
value: require_password_value(group_value, project_value),
locked: require_password_locked?(group_value, false),
inherited_from: (:group if require_password_locked?(group_value, false))
)
end end
private private
...@@ -61,16 +69,39 @@ module ComplianceManagement ...@@ -61,16 +69,39 @@ module ComplianceManagement
@group&.group_merge_request_approval_setting @group&.group_merge_request_approval_setting
end end
def setting(name, instance_value, group_value, default:) def value(instance_value, group_value, project_value)
# Setting value should be the instance-level, if set. [instance_value, group_value, project_value].compact.all?
# Otherwise group-level. end
# Finally, fallback on to the default defined here.
value = instance_value ? !instance_value : group_value def require_password_value(group_value, project_value)
[group_value, project_value].any?
end
def locked?(instance_value, group_value, project_value)
_, *inherited = [project_value, group_value, instance_value].compact
inherited.any? { |value| value == false }
end
def require_password_locked?(group_value, default)
return false if @project.nil?
return false if @group.nil?
group_value || default
end
def inherited_from(instance_value, group_value, project_value)
return :instance if instance_value == false
return :group if group_value == false && !project_value.nil?
nil
end
def setting(instance_value, group_value, project_value)
ComplianceManagement::MergeRequestApprovalSettings::Setting.new( ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
value: value.nil? ? default : value, value: value(instance_value, group_value, project_value),
locked: instance_value.nil? ? false : instance_value, locked: locked?(instance_value, group_value, project_value),
inherited_from: instance_value ? :instance : nil inherited_from: inherited_from(instance_value, group_value, project_value)
) )
end end
end end
......
...@@ -30,7 +30,7 @@ module EE ...@@ -30,7 +30,7 @@ module EE
mount ::API::GroupPushRule mount ::API::GroupPushRule
mount ::API::MergeTrains mount ::API::MergeTrains
mount ::API::GroupHooks mount ::API::GroupHooks
mount ::API::GroupMergeRequestApprovalSettings mount ::API::MergeRequestApprovalSettings
mount ::API::Scim mount ::API::Scim
mount ::API::ManagedLicenses mount ::API::ManagedLicenses
mount ::API::ProjectApprovals mount ::API::ProjectApprovals
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe API::Entities::GroupMergeRequestApprovalSetting do RSpec.describe API::Entities::MergeRequestApprovalSetting do
let(:group_merge_request_approval_setting) { build(:group_merge_request_approval_setting) } let(:group_merge_request_approval_setting) { build(:group_merge_request_approval_setting) }
subject { described_class.new(group_merge_request_approval_setting).as_json } subject { described_class.new(group_merge_request_approval_setting).as_json }
......
...@@ -7,6 +7,9 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do ...@@ -7,6 +7,9 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do
let_it_be(:group) { create(:group) } let_it_be(:group) { create(:group) }
let(:project) { build(:project, group: group) }
let(:namespace_project) { build(:project) }
before(:all) do before(:all) do
group.create_group_merge_request_approval_setting group.create_group_merge_request_approval_setting
end end
...@@ -17,19 +20,7 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do ...@@ -17,19 +20,7 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do
expect(resolver).not_to be_nil expect(resolver).not_to be_nil
end end
shared_examples 'resolvable but has no instance setting' do shared_examples 'a MR approval setting' do
where(:group_allows_approval, :value, :locked, :inherited_from) do
true | true | false | nil
false | false | false | nil
end
with_them do
before do
group.group_merge_request_approval_setting.update!(instance_method => group_allows_approval)
end
let(:object) { described_class.new(group).send(instance_method) }
it 'has the correct value' do it 'has the correct value' do
expect(object.value).to eq(value) expect(object.value).to eq(value)
end end
...@@ -42,68 +33,195 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do ...@@ -42,68 +33,195 @@ RSpec.describe ::ComplianceManagement::MergeRequestApprovalSettings::Resolver do
expect(object.inherited_from).to eq(inherited_from) expect(object.inherited_from).to eq(inherited_from)
end end
end end
end
shared_examples 'resolvable' do describe '#allow_author_approval' do
where(:instance_prevents_approval, :group_allows_approval, :value, :locked, :inherited_from) do where(:instance_prevents_approval, :group_allows_approval, :project_allows_approval, :value, :locked, :inherited_from) do
true | true | false | true | :instance # Cases where the project is nil
true | false | false | true | :instance true | true | nil | false | true | :instance
false | true | true | false | nil true | false | nil | false | true | :instance
false | false | false | false | nil false | true | nil | true | false | nil
false | false | nil | false | false | nil
# Cases which do not include a group
false | nil | true | true | false | nil
false | nil | false | false | false | nil
true | nil | false | false | true | :instance
# Cases which include a project
false | true | true | true | false | nil
false | false | true | false | true | :group
true | true | true | false | true | :instance
false | true | false | false | false | nil
end end
with_them do with_them do
before do before do
stub_ee_application_setting(instance_flag => instance_prevents_approval) stub_ee_application_setting(prevent_merge_requests_author_approval: instance_prevents_approval)
group.group_merge_request_approval_setting.update!(group_flag => group_allows_approval) group.group_merge_request_approval_setting.update!(allow_author_approval: !!group_allows_approval)
project.update!(merge_requests_author_approval: project_allows_approval)
end end
let(:object) { described_class.new(group).send(group_flag) } let(:object) do
if project_allows_approval.nil?
described_class.new(group).allow_author_approval
elsif group_allows_approval.nil?
described_class.new(nil, project: project).allow_author_approval
else
described_class.new(group, project: project).allow_author_approval
end
end
it 'has the correct value' do it_behaves_like 'a MR approval setting'
expect(object.value).to eq(value) end
end end
it 'has the correct locked status' do describe '#allow_committer_approval' do
expect(object.locked).to eq(locked) where(:instance_prevents_approval, :group_allows_approval, :project_prevents_approval, :value, :locked, :inherited_from) do
# Cases where the project is nil
true | true | nil | false | true | :instance
true | false | nil | false | true | :instance
false | true | nil | true | false | nil
false | false | nil | false | false | nil
# Cases which do not include a group
false | nil | true | false | false | nil
true | nil | false | false | true | :instance
# Cases which include a project
false | true | true | false | false | nil
false | false | false | false | true | :group
true | true | false | false | true | :instance
end end
it 'has the correct inheritance' do with_them do
expect(object.inherited_from).to eq(inherited_from) before do
stub_ee_application_setting(prevent_merge_requests_committers_approval: instance_prevents_approval)
group.group_merge_request_approval_setting.update!(allow_committer_approval: !!group_allows_approval)
project.update!(merge_requests_disable_committers_approval: project_prevents_approval)
end end
let(:object) do
if project_prevents_approval.nil?
described_class.new(group).allow_committer_approval
elsif group_allows_approval.nil?
described_class.new(nil, project: project).allow_committer_approval
else
described_class.new(group, project: project).allow_committer_approval
end end
end end
describe '#allow_author_approval' do it_behaves_like 'a MR approval setting'
let(:instance_flag) { :prevent_merge_requests_author_approval } end
let(:group_flag) { :allow_author_approval }
it_behaves_like 'resolvable'
end end
describe '#allow_committer_approval' do describe '#allow_overrides_to_approver_list_per_merge_request' do
let(:instance_flag) { :prevent_merge_requests_committers_approval } where(:instance_prevents_approval, :group_allows_approval, :project_prevents_approval, :value, :locked, :inherited_from) do
let(:group_flag) { :allow_committer_approval } # Cases where the project is nil
true | true | nil | false | true | :instance
true | false | nil | false | true | :instance
false | true | nil | true | false | nil
false | false | nil | false | false | nil
it_behaves_like 'resolvable' # Cases which do not include a group
false | nil | true | false | false | nil
true | nil | false | false | true | :instance
# Cases which include a project
false | true | true | false | false | nil
false | false | false | false | true | :group
true | true | false | false | true | :instance
end end
describe '#allow_overrides_to_approver_list_per_merge_request' do with_them do
let(:instance_flag) { :disable_overriding_approvers_per_merge_request } before do
let(:group_flag) { :allow_overrides_to_approver_list_per_merge_request } stub_ee_application_setting(disable_overriding_approvers_per_merge_request: instance_prevents_approval)
group.group_merge_request_approval_setting.update!(allow_overrides_to_approver_list_per_merge_request: !!group_allows_approval)
project.update!(disable_overriding_approvers_per_merge_request: project_prevents_approval)
end
it_behaves_like 'resolvable' let(:object) do
if project_prevents_approval.nil?
described_class.new(group).allow_overrides_to_approver_list_per_merge_request
elsif group_allows_approval.nil?
described_class.new(nil, project: project).allow_overrides_to_approver_list_per_merge_request
else
described_class.new(group, project: project).allow_overrides_to_approver_list_per_merge_request
end
end
it_behaves_like 'a MR approval setting'
end
end end
describe '#retain_approvals_on_push' do describe '#retain_approvals_on_push' do
let(:instance_method) { :retain_approvals_on_push } where(:group_retains_approvals, :project_resets_approvals, :value, :locked, :inherited_from) do
true | nil | true | false | nil
false | nil | false | false | nil
it_behaves_like 'resolvable but has no instance setting' # Cases which do not include a group
nil | true | false | false | nil
nil | false | true | false | nil
# Cases with a project
true | false | true | false | nil
false | true | false | true | :group
false | false | false | true | :group
true | true | false | false | nil
end
with_them do
before do
group.group_merge_request_approval_setting.update!(retain_approvals_on_push: !!group_retains_approvals)
project.update!(reset_approvals_on_push: project_resets_approvals)
end
let(:object) do
if project_resets_approvals.nil?
described_class.new(group).retain_approvals_on_push
elsif group_retains_approvals.nil?
described_class.new(nil, project: project).retain_approvals_on_push
else
described_class.new(group, project: project).retain_approvals_on_push
end
end
it_behaves_like 'a MR approval setting'
end
end end
describe '#require_password_to_approve' do describe '#require_password_to_approve' do
let(:instance_method) { :require_password_to_approve } where(:group_requires_password, :project_requires_password, :value, :locked, :inherited_from) do
true | nil | true | false | nil
false | nil | false | false | nil
# Cases which do not include a group
nil | true | true | false | nil
nil | false | false | false | nil
# Cases with a project
true | false | true | true | :group
true | true | true | true | :group
false | false | false | false | nil
false | true | true | false | nil
end
it_behaves_like 'resolvable but has no instance setting' with_them do
before do
group.group_merge_request_approval_setting.update!(require_password_to_approve: !!group_requires_password)
project.update!(require_password_to_approve: project_requires_password)
end
let(:object) do
if project_requires_password.nil?
described_class.new(group).require_password_to_approve
elsif group_requires_password.nil?
described_class.new(nil, project: project).require_password_to_approve
else
described_class.new(group, project: project).require_password_to_approve
end
end
it_behaves_like 'a MR approval setting'
end
end end
end end
...@@ -1404,6 +1404,35 @@ RSpec.describe ProjectPolicy do ...@@ -1404,6 +1404,35 @@ RSpec.describe ProjectPolicy do
end end
end end
describe ':admin_merge_request_approval_settings' do
let(:project) { private_project }
using RSpec::Parameterized::TableSyntax
where(:role, :licensed, :allowed) do
:guest | true | false
:reporter | true | false
:developer | true | false
:maintainer | false | false
:maintainer | true | false
:owner | false | false
:owner | true | true
:admin | true | true
:admin | false | false
end
with_them do
let(:current_user) { public_send(role) }
before do
stub_licensed_features(group_merge_request_approval_settings: licensed)
enable_admin_mode!(current_user) if role == :admin
end
it { is_expected.to(allowed ? be_allowed(:admin_merge_request_approval_settings) : be_disallowed(:admin_merge_request_approval_settings)) }
end
end
describe ':modify_approvers_rules' do describe ':modify_approvers_rules' do
it_behaves_like 'merge request approval settings' do it_behaves_like 'merge request approval settings' do
let(:app_setting) { :disable_overriding_approvers_per_merge_request } let(:app_setting) { :disable_overriding_approvers_per_merge_request }
......
...@@ -2,13 +2,12 @@ ...@@ -2,13 +2,12 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe API::GroupMergeRequestApprovalSettings do RSpec.describe API::MergeRequestApprovalSettings do
let_it_be_with_reload(:group) { create(:group) } let_it_be_with_reload(:group) { create(:group) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be_with_reload(:project) { create(:project, group: group) }
let_it_be(:setting) { create(:group_merge_request_approval_setting, group: group) } let_it_be(:setting) { create(:group_merge_request_approval_setting, group: group) }
let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
shared_examples "resolvable" do shared_examples "resolvable" do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
...@@ -44,6 +43,8 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do ...@@ -44,6 +43,8 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
end end
describe 'GET /groups/:id/merge_request_approval_settings' do describe 'GET /groups/:id/merge_request_approval_settings' do
let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do context 'when feature flag is disabled' do
before do before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false) stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
...@@ -131,6 +132,7 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do ...@@ -131,6 +132,7 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
end end
describe 'PUT /groups/:id/merge_request_approval_setting' do describe 'PUT /groups/:id/merge_request_approval_setting' do
let(:url) { "/groups/#{group.id}/merge_request_approval_setting" }
let(:params) { { allow_author_approval: true } } let(:params) { { allow_author_approval: true } }
context 'when feature flag is disabled' do context 'when feature flag is disabled' do
...@@ -172,13 +174,13 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do ...@@ -172,13 +174,13 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
end end
context 'when update fails' do context 'when update fails' do
let(:params) { { allow_author_approval: nil } } let(:params) { { allow_author_approval: 45 } }
it 'returns 400 status', :aggregate_failures do it 'returns 400 status', :aggregate_failures do
put api(url, user), params: params put api(url, user), params: params
expect(response).to have_gitlab_http_status(:bad_request) expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['message']).to eq('allow_author_approval' => ['must be a boolean value']) expect(json_response['error']).to match(/allow_.*_approval is invalid/)
end end
end end
...@@ -209,4 +211,99 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do ...@@ -209,4 +211,99 @@ RSpec.describe API::GroupMergeRequestApprovalSettings do
end end
end end
end end
describe 'GET /projects/:id/merge_request_approval_settings' do
let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
get api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
before do
group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(group_merge_request_approval_settings: true)
allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project)
.and_return(true)
end
it 'matches the response schema' do
get api(url, user)
expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee')
end
context 'when the project does not have existing settings' do
before do
project.update!(
merge_requests_author_approval: nil,
merge_requests_disable_committers_approval: nil,
disable_overriding_approvers_per_merge_request: nil,
reset_approvals_on_push: nil,
require_password_to_approve: nil
)
end
it 'returns in-memory default settings', :aggregate_failures do
get api(url, user)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['allow_author_approval']['value']).to eq(false)
expect(json_response['allow_committer_approval']['value']).to eq(false)
expect(json_response['allow_overrides_to_approver_list_per_merge_request']['value']).to eq(false)
expect(json_response['retain_approvals_on_push']['value']).to eq(false)
expect(json_response['require_password_to_approve']['value']).to eq(false)
end
end
end
end
describe 'PUT /projects/:id/merge_request_approval_settings' do
let(:url) { "/projects/#{project.id}/merge_request_approval_setting" }
context 'when feature flag is disabled' do
before do
stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
end
it 'returns 404 status' do
put api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when feature flag is enabled' do
let(:params) { { retain_approvals_on_push: false } }
before do
group.add_owner(user)
allow(Ability).to receive(:allowed?).and_call_original
stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
stub_licensed_features(group_merge_request_approval_settings: true)
allow(Ability).to receive(:allowed?)
.with(user, :admin_merge_request_approval_settings, project)
.and_return(true)
end
it 'matches the response schema and updates the params' do
put api(url, user), params: params
expect(project.reset_approvals_on_push).to be true
expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee')
end
end
end
end end
...@@ -3,20 +3,58 @@ ...@@ -3,20 +3,58 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe MergeRequestApprovalSettings::UpdateService do RSpec.describe MergeRequestApprovalSettings::UpdateService do
let_it_be_with_reload(:group) { create(:group) } let!(:group) { create(:group) }
let!(:project) { create(:project, merge_requests_author_approval: true) }
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let(:params) { { allow_author_approval: true } } let(:params) { { allow_author_approval: false } }
subject(:service) do subject(:service) do
described_class.new( described_class.new(
container: group, container: container,
current_user: user, current_user: user,
params: params params: params
) )
end end
describe 'execute' do describe 'execute with a Project as container' do
let(:container) { project }
context 'user does not have permissions' do
before do
allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, container).and_return(false)
end
it 'responds with an error response', :aggregate_failures do
response = subject.execute
expect(response.status).to eq(:error)
expect(response.message).to eq('Insufficient permissions')
end
it 'does not change any of the approval settings' do
expect { subject.execute }.not_to change { project.attributes }
end
end
context 'user has permissions' do
before do
allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, container).and_return(true)
end
it 'responds with a successful service response', :aggregate_failures do
response = subject.execute
expect(response).to be_success
expect(response.payload.reload.merge_requests_author_approval).to be(false)
expect(project.reload.merge_requests_author_approval).to be(false)
end
end
end
describe 'execute with a Group as container' do
let(:container) { group }
context 'user does not have permissions' do context 'user does not have permissions' do
before do before do
allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, group).and_return(false) allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, group).and_return(false)
...@@ -45,7 +83,7 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do ...@@ -45,7 +83,7 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do
response = subject.execute response = subject.execute
expect(response).to be_success expect(response).to be_success
expect(response.payload.allow_author_approval).to be(true) expect(response.payload.allow_author_approval).to be(false)
end end
context 'when group has an existing setting' do context 'when group has an existing setting' do
...@@ -53,16 +91,14 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do ...@@ -53,16 +91,14 @@ RSpec.describe MergeRequestApprovalSettings::UpdateService do
let_it_be(:existing_setting) { create(:group_merge_request_approval_setting, group: group) } let_it_be(:existing_setting) { create(:group_merge_request_approval_setting, group: group) }
it 'does not create a new setting' do it 'does not create a new setting' do
expect { subject.execute } expect { subject.execute }.not_to change { GroupMergeRequestApprovalSetting.count }
.to change { GroupMergeRequestApprovalSetting.count }.by(0)
.and change { existing_setting.reload.allow_author_approval }.to(true)
end end
it 'responds with a successful service response', :aggregate_failures do it 'responds with a successful service response', :aggregate_failures do
response = subject.execute response = subject.execute
expect(response).to be_success expect(response).to be_success
expect(response.payload.allow_author_approval).to be(true) expect(response.payload.allow_author_approval).to be(false)
end end
end end
......
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