diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 1d9f81aaa23e3decdb6a1c4b2f1203d9271cc841..76acca3b3bc45ac02b290201ed8ee9c7837df714 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -51,7 +51,7 @@ def new def edit @badge_api_endpoint = expose_url(api_v4_projects_badges_path(id: @project.id)) - render 'edit' + render_edit end def create @@ -85,7 +85,7 @@ def update else flash.now[:alert] = result[:message] - format.html { render 'edit' } + format.html { render_edit } end format.js @@ -387,7 +387,6 @@ def project_params_attributes :merge_method, :initialize_with_readme, :autoclose_referenced_issues, - :suggestion_commit_message, project_feature_attributes: %i[ builds_access_level @@ -488,6 +487,10 @@ def export_rate_limit def rate_limiter ::Gitlab::ApplicationRateLimiter end + + def render_edit + render 'edit' + end end ProjectsController.prepend_if_ee('EE::ProjectsController') diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index d9a7f0a96dc65796adcb138c16b14d5284cd9ef0..cddca72f91f77999b8bf91a626f3136340355fac 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -10,6 +10,8 @@ module ProtectedRef validates :project, presence: true delegate :matching, :matches?, :wildcard?, to: :ref_matcher + + scope :for_project, ->(project) { where(project: project) } end def commit diff --git a/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb new file mode 100644 index 0000000000000000000000000000000000000000..4e75f7d41fdf09116df08941928993aa3ddaf895 --- /dev/null +++ b/db/migrate/20200109085206_create_approval_project_rules_protected_branches.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class CreateApprovalProjectRulesProtectedBranches < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + create_table :approval_project_rules_protected_branches, id: false do |t| + t.references :approval_project_rule, + null: false, + index: false, + foreign_key: { on_delete: :cascade } + t.references :protected_branch, + null: false, + index: { name: 'index_approval_project_rules_protected_branches_pb_id' }, + foreign_key: { on_delete: :cascade } + t.index [:approval_project_rule_id, :protected_branch_id], name: 'index_approval_project_rules_protected_branches_unique', unique: true, using: :btree + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c2c5bb43c5e744c32662f3bd619ab533ec11501c..158f8cc100d9be94a42b2e3d8bfc1f66ca0fedbd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -437,6 +437,13 @@ t.index ["group_id"], name: "index_approval_project_rules_groups_2" end + create_table "approval_project_rules_protected_branches", id: false, force: :cascade do |t| + t.bigint "approval_project_rule_id", null: false + t.bigint "protected_branch_id", null: false + t.index ["approval_project_rule_id", "protected_branch_id"], name: "index_approval_project_rules_protected_branches_unique", unique: true + t.index ["protected_branch_id"], name: "index_approval_project_rules_protected_branches_pb_id" + end + create_table "approval_project_rules_users", force: :cascade do |t| t.bigint "approval_project_rule_id", null: false t.integer "user_id", null: false @@ -4441,6 +4448,8 @@ add_foreign_key "approval_project_rules", "projects", on_delete: :cascade add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "approval_project_rules_protected_branches", "approval_project_rules", on_delete: :cascade + add_foreign_key "approval_project_rules_protected_branches", "protected_branches", on_delete: :cascade add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade diff --git a/ee/app/controllers/ee/projects_controller.rb b/ee/app/controllers/ee/projects_controller.rb index 169cd6b831e78b5a6373bba6ee227e8b44e22a87..004e3e905a554ff60615c0ca6e74d1eddfc41ce9 100644 --- a/ee/app/controllers/ee/projects_controller.rb +++ b/ee/app/controllers/ee/projects_controller.rb @@ -23,7 +23,7 @@ def restore else flash.now[:alert] = result[:message] - render 'edit' + render_edit end end @@ -41,7 +41,7 @@ def destroy else flash.now[:alert] = result[:message] - render 'edit' + render_edit end end @@ -137,5 +137,11 @@ def log_archive_audit_event def log_unarchive_audit_event log_audit_event(message: 'Project unarchived') end + + override :render_edit + def render_edit + push_frontend_feature_flag(:scoped_approval_rules, project, default_enabled: true) + super + end end end diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index 055a14f78eb9a776e3fe635bdbb88ac8cb2c92fe..7f69a02eb5f917f1e750fd256030c8d52ff2be92 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -70,6 +70,14 @@ def self.find_or_create_code_owner_rule(merge_request, pattern) retry end + def self.applicable_to_branch(branch) + includes(approval_project_rule: :protected_branches).select do |rule| + next true unless rule.approval_project_rule.present? + + rule.approval_project_rule.applies_to_branch?(branch) + end + end + def project merge_request.target_project end diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb index 4fc3a185734d2568fe6138489f7d088d5669c4d8..85427f8b3d0e1acfabcacfa093209f1be491cd92 100644 --- a/ee/app/models/approval_project_rule.rb +++ b/ee/app/models/approval_project_rule.rb @@ -4,6 +4,7 @@ class ApprovalProjectRule < ApplicationRecord include ApprovalRuleLike belongs_to :project + has_and_belongs_to_many :protected_branches enum rule_type: { regular: 0, @@ -17,6 +18,16 @@ class ApprovalProjectRule < ApplicationRecord validates :name, uniqueness: { scope: :project_id } + def self.applicable_to_branch(branch) + includes(:protected_branches).select { |rule| rule.applies_to_branch?(branch) } + end + + def applies_to_branch?(branch) + return true if protected_branches.empty? + + protected_branches.matching(branch).any? + end + def source_rule nil end diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb index b92e0efb7cb8942c97d7e202e60c6f0d36038247..6fe4ef13d6a82d02333df71ec5aa5bcf1cb60bd8 100644 --- a/ee/app/models/approval_state.rb +++ b/ee/app/models/approval_state.rb @@ -157,7 +157,9 @@ def user_defined_rules if approval_rules_overwritten? user_defined_merge_request_rules else - project.visible_user_defined_rules.map do |rule| + branch = project.scoped_approval_rules_enabled? ? target_branch : nil + + project.visible_user_defined_rules(branch: branch).map do |rule| ApprovalWrappedRule.wrap(merge_request, rule) end end @@ -201,9 +203,18 @@ def report_approver_rules def wrapped_rules strong_memoize(:wrapped_rules) do - merge_request.approval_rules.map do |rule| + merge_request_rules = merge_request.approval_rules + merge_request_rules = merge_request_rules.applicable_to_branch(target_branch) if project.scoped_approval_rules_enabled? + + merge_request_rules.map do |rule| ApprovalWrappedRule.wrap(merge_request, rule) end end end + + def target_branch + strong_memoize(:target_branch) do + merge_request.target_branch + end + end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a4f151687274bc25f8096e04843dc69b24bb4150..4cef4bc20e2176e5a45e6f6536b8fb5a839703e9 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -323,6 +323,10 @@ def code_owner_approval_required_available? feature_available?(:code_owner_approval_required) end + def scoped_approval_rules_enabled? + ::Feature.enabled?(:scoped_approval_rules, self, default_enabled: true) + end + def service_desk_enabled ::EE::Gitlab::ServiceDesk.enabled?(project: self) && super end @@ -408,14 +412,11 @@ def visible_approval_rules end end - def visible_user_defined_rules - strong_memoize(:visible_user_defined_rules) do - rules = approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc) + def visible_user_defined_rules(branch: nil) + return user_defined_rules.take(1) unless multiple_approval_rules_available? + return user_defined_rules unless branch - next rules.take(1) unless multiple_approval_rules_available? - - rules - end + user_defined_rules.applicable_to_branch(branch) end # TODO: Clean up this method in the https://gitlab.com/gitlab-org/gitlab/issues/33329 @@ -771,5 +772,11 @@ def check_pull_mirror_branch_prefix errors.add(:pull_mirror_branch_prefix, _('Invalid Git ref')) end end + + def user_defined_rules + strong_memoize(:user_defined_rules) do + approval_rules.regular_or_any_approver.order(rule_type: :desc, id: :asc) + end + end end end diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb index f360c944c6a999a81a9461b4d192afdc5d392e0c..a9dd5acbc24d44e8dbe54e28deb061ac6a0d808d 100644 --- a/ee/app/services/concerns/approval_rules/updater.rb +++ b/ee/app/services/concerns/approval_rules/updater.rb @@ -5,6 +5,7 @@ module Updater def action filter_eligible_users! filter_eligible_groups! + filter_eligible_protected_branches! if rule.update(params) rule.reset @@ -27,5 +28,18 @@ def filter_eligible_groups! params[:groups] = Group.id_in(params.delete(:group_ids)).public_or_visible_to_user(current_user) end + + def filter_eligible_protected_branches! + return unless params.key?(:protected_branch_ids) + + protected_branch_ids = params.delete(:protected_branch_ids) + + return unless project.multiple_approval_rules_available? && can?(current_user, :admin_project, project) + + params[:protected_branches] = + ProtectedBranch + .id_in(protected_branch_ids) + .for_project(project) + end end end diff --git a/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml b/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml new file mode 100644 index 0000000000000000000000000000000000000000..22a48d715d688d2ad1e0c35567c559749fc55ff9 --- /dev/null +++ b/ee/changelogs/unreleased/460-approval-project-rules-protected-branches-api.yml @@ -0,0 +1,5 @@ +--- +title: Scope approval rules by protected branches via API +merge_request: 22673 +author: +type: added diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb index 90f7e3c5f25104285298ca961f01f293eb38b924..3e43e7c27fd7f78759844415373d64618f14f8fe 100644 --- a/ee/lib/api/helpers/project_approval_rules_helpers.rb +++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb @@ -13,6 +13,7 @@ module ProjectApprovalRulesHelpers optional :rule_type, type: String, desc: 'The type of approval rule' optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' + optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule' end params :update_project_approval_rule do @@ -21,6 +22,7 @@ module ProjectApprovalRulesHelpers 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' + optional :protected_branch_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The protected branch ids for this rule' optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index a251e568e98a300f63db558154d9ae16075ed2db..2e7a26870d6fdfe47592b89d0599360717308442 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -12,33 +12,33 @@ class ProjectApprovalRules < ::Grape::API resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do segment ':id/approval_rules' do desc 'Get all project approval rules' do - success EE::API::Entities::ApprovalRule + success EE::API::Entities::ProjectApprovalRule end get do authorize_create_merge_request_in_project - present user_project.visible_approval_rules, with: EE::API::Entities::ApprovalRule, current_user: current_user + present user_project.visible_approval_rules, with: EE::API::Entities::ProjectApprovalRule, current_user: current_user end desc 'Create new project approval rule' do - success EE::API::Entities::ApprovalRule + success EE::API::Entities::ProjectApprovalRule end params do use :create_project_approval_rule end post do - create_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) + create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule) end segment ':approval_rule_id' do desc 'Update project approval rule' do - success EE::API::Entities::ApprovalRule + success EE::API::Entities::ProjectApprovalRule end params do use :update_project_approval_rule end put do - update_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) + update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalRule) end desc 'Destroy project approval rule' diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb index 1a2afe02b9ecdb915fa483dca66a7ee4838f5e2d..9c54b07016148c09a22372c66e583be4e7842f4a 100644 --- a/ee/lib/api/project_approval_settings.rb +++ b/ee/lib/api/project_approval_settings.rb @@ -24,25 +24,25 @@ class ProjectApprovalSettings < ::Grape::API segment 'rules' do desc 'Create new approval rule' do detail 'Private API subject to change' - success EE::API::Entities::ApprovalSettingRule + success EE::API::Entities::ProjectApprovalSettingRule end params do use :create_project_approval_rule end post do - create_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) + create_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule) end segment ':approval_rule_id' do desc 'Update approval rule' do detail 'Private API subject to change' - success EE::API::Entities::ApprovalSettingRule + success EE::API::Entities::ProjectApprovalSettingRule end params do use :update_project_approval_rule end put do - update_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) + update_project_approval_rule(present_with: EE::API::Entities::ProjectApprovalSettingRule) end desc 'Delete an approval rule' do diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index c03b1672aed1f138bbf6796a9c353572f42858d2..bda0822dc69a70bffd3afb97f9f0fbaabad89af0 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -420,6 +420,10 @@ def initialize(object, options = {}) expose :contains_hidden_groups?, as: :contains_hidden_groups end + class ProjectApprovalRule < ApprovalRule + expose :protected_branches, using: ::API::Entities::ProtectedBranch, if: -> (rule, _) { rule.project.multiple_approval_rules_available? } + end + class MergeRequestApprovalRule < ApprovalRule class SourceRule < Grape::Entity expose :approvals_required @@ -446,7 +450,7 @@ class MergeRequestApprovalState < Grape::Entity # This overrides the `eligible_approvers` to be exposed as `approvers`. # # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574. - class ApprovalSettingRule < ApprovalRule + class ProjectApprovalSettingRule < ProjectApprovalRule expose :approvers, using: ::API::Entities::UserBasic, override: true end @@ -454,7 +458,7 @@ class ApprovalSettingRule < ApprovalRule # # To be removed in https://gitlab.com/gitlab-org/gitlab/issues/13574. class ProjectApprovalSettings < Grape::Entity - expose :visible_approval_rules, as: :rules, using: ApprovalSettingRule + expose :visible_approval_rules, as: :rules, using: ProjectApprovalSettingRule expose :min_fallback_approvals, as: :fallback_approvals_required end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json index d27a218bf41556ae7c355a3ec5799ce600e9f6de..76ecb81514aa38720f80bb9cee4b4d52ed888716 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json @@ -26,6 +26,13 @@ "type": "object", "properties": {} } + }, + "protected_branches": { + "type": "array", + "items": { + "type": "object", + "properties": {} + } } }, "additionalProperties": false diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json index 226c692a2f3ee080f87a861a0dbad143dd087122..77b135e52d800d3b6ff9ed074733751bd0fac6ac 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json @@ -26,6 +26,13 @@ "type": "object", "properties": {} } + }, + "protected_branches": { + "type": "array", + "items": { + "type": "object", + "properties": {} + } } }, "additionalProperties": false diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb index e389f3294d576b8874dde3683e0d19efd922afe2..0dd0b7a2725ad3427ee3bf4af266beef62ee6ac1 100644 --- a/ee/spec/models/approval_merge_request_rule_spec.rb +++ b/ee/spec/models/approval_merge_request_rule_spec.rb @@ -116,12 +116,12 @@ end end - context 'scopes' do - set(:rb_rule) { create(:code_owner_rule, name: '*.rb') } - set(:js_rule) { create(:code_owner_rule, name: '*.js') } - set(:css_rule) { create(:code_owner_rule, name: '*.css') } - set(:approval_rule) { create(:approval_merge_request_rule) } - set(:report_approver_rule) { create(:report_approver_rule) } + context 'scopes' do + let!(:rb_rule) { create(:code_owner_rule, name: '*.rb') } + let!(:js_rule) { create(:code_owner_rule, name: '*.js') } + let!(:css_rule) { create(:code_owner_rule, name: '*.css') } + let!(:approval_rule) { create(:approval_merge_request_rule) } + let!(:report_approver_rule) { create(:report_approver_rule) } describe '.not_matching_pattern' do it 'returns the correct rules' do @@ -153,8 +153,7 @@ end describe '.find_or_create_code_owner_rule' do - set(:merge_request) { create(:merge_request) } - set(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) } + let!(:existing_code_owner_rule) { create(:code_owner_rule, name: '*.rb', merge_request: merge_request) } it 'finds an existing rule' do expect(described_class.find_or_create_code_owner_rule(merge_request, '*.rb')) @@ -182,6 +181,47 @@ end end + describe '.applicable_to_branch' do + let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request) } + let(:branch) { 'stable' } + + subject { described_class.applicable_to_branch(branch) } + + context 'when there are no associated source rules' do + it { is_expected.to eq([rule]) } + end + + context 'when there are associated source rules' do + let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) } + + before do + rule.update!(approval_project_rule: source_rule) + end + + context 'and there are no associated protected branches to source rule' do + it { is_expected.to eq([rule]) } + end + + context 'and there are associated protected branches to source rule' do + before do + source_rule.update!(protected_branches: protected_branches) + end + + context 'and branch matches' do + let(:protected_branches) { [create(:protected_branch, name: branch)] } + + it { is_expected.to eq([rule]) } + end + + context 'but branch does not match anything' do + let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] } + + it { is_expected.to be_empty } + end + end + end + end + describe '#project' do it 'returns project of MergeRequest' do expect(subject.project).to be_present diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb index b738a2c43100f7a8c151758ffbc72ac9837313d5..31bdc409da60ac8328626dc06fea6a36c521d222 100644 --- a/ee/spec/models/approval_project_rule_spec.rb +++ b/ee/spec/models/approval_project_rule_spec.rb @@ -147,4 +147,33 @@ expect { rule.save }.to raise_error(ActiveRecord::RecordNotUnique) end end + + describe '.applicable_to_branch' do + let!(:rule) { create(:approval_project_rule) } + let(:branch) { 'stable' } + + subject { described_class.applicable_to_branch(branch) } + + context 'when there are no associated protected branches' do + it { is_expected.to eq([rule]) } + end + + context 'when there are associated protected branches' do + before do + rule.update!(protected_branches: protected_branches) + end + + context 'and branch matches' do + let(:protected_branches) { [create(:protected_branch, name: branch)] } + + it { is_expected.to eq([rule]) } + end + + context 'but branch does not match anything' do + let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] } + + it { is_expected.to be_empty } + end + end + end end diff --git a/ee/spec/models/approval_state_spec.rb b/ee/spec/models/approval_state_spec.rb index f44581c274ec4c8b26423c01bdbdde420941a6fd..be580fd802cb8d0d349d72105d0e1c13b51781ac 100644 --- a/ee/spec/models/approval_state_spec.rb +++ b/ee/spec/models/approval_state_spec.rb @@ -1380,4 +1380,130 @@ def create_project_member(role) end end end + + describe '#user_defined_rules' do + context 'when approval rules are not overwritten' do + let!(:project_rule) { create(:approval_project_rule, project: project) } + let!(:another_project_rule) { create(:approval_project_rule, project: project) } + + context 'and multiple approval rules is disabled' do + it 'returns the first rule' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + project_rule + ]) + end + end + + context 'and multiple approval rules is enabled' do + before do + stub_licensed_features(multiple_approval_rules: true) + end + + it 'returns the rules as is' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + project_rule, + another_project_rule + ]) + end + + context 'and rules are scoped by protected branches' do + let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') } + let(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') } + + before do + merge_request.update!(target_branch: 'stable-1') + another_project_rule.update!(protected_branches: [protected_branch]) + project_rule.update!(protected_branches: [another_protected_branch]) + end + + context 'and scoped_approval_rules feature is enabled' do + it 'returns the rules that are applicable to the merge request target branch' do + expect(subject.user_defined_rules.map(&:approval_rule)).to eq([ + another_project_rule + ]) + end + end + + context 'but scoped_approval_rules feature is disabled' do + before do + stub_feature_flags(scoped_approval_rules: false) + end + + it 'returns unscoped rules' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + project_rule, + another_project_rule + ]) + end + end + end + end + end + + context 'when approval rules are overwritten' do + let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } + let!(:another_mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request) } + + before do + project.update!(disable_overriding_approvers_per_merge_request: false) + end + + context 'when multiple approval rules is disabled' do + it 'returns the first rule' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + mr_rule + ]) + end + end + + context 'when multiple approval rules is enabled' do + before do + stub_licensed_features(multiple_approval_rules: true) + end + + it 'returns the rules as is' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + mr_rule, + another_mr_rule + ]) + end + + context 'and rules have source rules that are scoped by protected branches' do + let(:source_rule) { create(:approval_project_rule, project: project) } + let(:another_source_rule) { create(:approval_project_rule, project: project) } + let(:protected_branch) { create(:protected_branch, project: project, name: 'stable-*') } + let(:another_protected_branch) { create(:protected_branch, project: project, name: '*-stable') } + + before do + merge_request.update!(target_branch: 'stable-1') + source_rule.update!(protected_branches: [protected_branch]) + another_source_rule.update!(protected_branches: [another_protected_branch]) + mr_rule.update!(approval_project_rule: another_source_rule) + another_mr_rule.update!(approval_project_rule: source_rule) + end + + context 'and scoped_approval_rules feature is enabled' do + it 'returns the rules that are applicable to the merge request target branch' do + expect(subject.user_defined_rules.map(&:approval_rule)).to eq([ + another_mr_rule + ]) + end + end + + context 'but scoped_approval_rules feature is disabled' do + before do + stub_feature_flags(scoped_approval_rules: false) + end + + it 'returns unscoped rules' do + expect(subject.user_defined_rules.map(&:approval_rule)).to match_array([ + mr_rule, + another_mr_rule + ]) + end + end + end + end + end + end end diff --git a/ee/spec/services/approval_rules/create_service_spec.rb b/ee/spec/services/approval_rules/create_service_spec.rb index 42674cb87ad6d8a3ab969e91de3f376e4250e2ef..73fcd502dda7a08dbdf50e598ad0af700d523168 100644 --- a/ee/spec/services/approval_rules/create_service_spec.rb +++ b/ee/spec/services/approval_rules/create_service_spec.rb @@ -83,6 +83,60 @@ it_behaves_like "creatable" + context 'when protected_branch_ids param is present' do + let(:protected_branch) { create(:protected_branch, project: target) } + + subject do + described_class.new( + target, + user, + name: 'developers', + approvals_required: 1, + protected_branch_ids: [protected_branch.id] + ).execute + end + + context 'and multiple approval rules is enabled' do + before do + stub_licensed_features(multiple_approval_rules: true) + end + + it 'associates the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to eq([protected_branch]) + end + + context 'but user cannot administer project' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + end + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + + context 'but protected branch is for another project' do + let(:another_project) { create(:project) } + let(:protected_branch) { create(:protected_branch, project: another_project) } + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + end + + context 'and multiple approval rules is disabled' do + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + end + ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name| context "when the rule name is `#{rule_name}`" do subject { described_class.new(target, user, { name: rule_name, approvals_required: 1 }) } diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb index f00b4d2e0025cfd2b33c71b65b5f62226a0a66b6..5f948b979c1d441d1800ae890e08654639fe8360 100644 --- a/ee/spec/services/approval_rules/update_service_spec.rb +++ b/ee/spec/services/approval_rules/update_service_spec.rb @@ -5,9 +5,9 @@ describe ApprovalRules::UpdateService do let(:project) { create(:project) } let(:user) { project.creator } + let(:approval_rule) { target.approval_rules.create(name: 'foo') } 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) } @@ -138,6 +138,58 @@ let(:target) { project } it_behaves_like "editable" + + context 'when protected_branch_ids param is present' do + let(:protected_branch) { create(:protected_branch, project: target) } + + subject do + described_class.new( + approval_rule, + user, + protected_branch_ids: [protected_branch.id] + ).execute + end + + context 'and multiple approval rules is enabled' do + before do + stub_licensed_features(multiple_approval_rules: true) + end + + it 'associates the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to eq([protected_branch]) + end + + context 'but user cannot administer project' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_project, target).and_return(false) + end + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + + context 'but protected branch is for another project' do + let(:another_project) { create(:project) } + let(:protected_branch) { create(:protected_branch, project: another_project) } + + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + end + + context 'and multiple approval rules is disabled' do + it 'does not associate the approval rule to the protected branch' do + expect(subject[:status]).to eq(:success) + expect(subject[:rule].protected_branches).to be_empty + end + end + end end context 'when target is merge request' do diff --git a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb index 5ef1b198173a579f7e90d0b6aecfe804bf6db4c6..33a1c5838351c652ee2d808f59762317c115a8c2 100644 --- a/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb +++ b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb @@ -45,6 +45,22 @@ expect(json_response.symbolize_keys).to include(params) end + context 'when protected_branch_ids param is present' do + let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } } + + before do + stub_licensed_features(multiple_approval_rules: true) + post api(url, current_user), params: params.merge(protected_branch_ids: protected_branches.map(&:id)) + end + + it 'creates approval rule associated to specified protected branches' do + expect(json_response['protected_branches']).to contain_exactly( + a_hash_including('id' => protected_branches.first.id), + a_hash_including('id' => protected_branches.last.id) + ) + end + end + ApprovalProjectRule::REPORT_TYPES_BY_DEFAULT_NAME.keys.each do |rule_name| context "when creating a '#{rule_name}' approval rule" do it 'specifies a `rule_type` of `report_approver`' do @@ -65,6 +81,22 @@ project.add_developer(approver) end + context 'when protected_branch_ids param is present' do + let(:protected_branches) { Array.new(2).map { create(:protected_branch, project: project) } } + + before do + stub_licensed_features(multiple_approval_rules: true) + put api(url, current_user), params: { protected_branch_ids: protected_branches.map(&:id) } + end + + it 'associates approval rule to specified protected branches' do + expect(json_response['protected_branches']).to contain_exactly( + a_hash_including('id' => protected_branches.first.id), + a_hash_including('id' => protected_branches.last.id) + ) + end + end + context 'when approver already exists' do before do approval_rule.users << approver diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 74f8edb07847ff000f923e0ccc8600f8cf17a461..1be73e606d9f9469c55a5412b7ec7e4b6c665434 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -613,6 +613,7 @@ class ProtectedRefAccess < Grape::Entity end class ProtectedBranch < Grape::Entity + expose :id expose :name expose :push_access_levels, using: Entities::ProtectedRefAccess expose :merge_access_levels, using: Entities::ProtectedRefAccess