From 910dc7e7ee9580fa7396da980a624602166eae55 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Thu, 20 Apr 2023 16:29:09 +0200 Subject: [PATCH] Inherit project approval rules for merge request creation Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/388140 **Problem** We don't create approval rules for merge request unless they are explicitly provided by the user. It currently works for creating MR via UI, because frontend provides a list of approval rules in `approval_rules_attributes` param. It doesn't work for merge_request API endpoint, because we don't expose of populate this attribute. **Solution** The code is complicated enough and requires a refactoring. The least invacive approach is to generate `approval_rules_attributes` list if it was not provided. That should ensure that MergeRequest::CreateService will inherit project approval rules even if the user didn't provide them. --- app/services/merge_requests/create_service.rb | 5 ++ .../inherit_approval_rules_on_creation.yml | 8 ++ ee/app/models/approval_project_rule.rb | 12 +++ .../params_filtering_service.rb | 9 +- .../ee/merge_requests/create_service.rb | 13 +++ ee/spec/factories/approval_rules.rb | 5 ++ ee/spec/requests/api/merge_requests_spec.rb | 26 ++++++ .../ee/merge_requests/create_service_spec.rb | 4 + .../merge_merge_requests_shared_examples.rb | 84 +++++++++++++++++++ 9 files changed, 158 insertions(+), 8 deletions(-) create mode 100644 config/feature_flags/development/inherit_approval_rules_on_creation.yml diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 75e1adec41b43e..39e1594d2151d9 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -4,6 +4,7 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute set_projects! + set_default_attributes! merge_request = MergeRequest.new merge_request.target_project = @project @@ -61,6 +62,10 @@ def set_projects! raise Gitlab::Access::AccessDeniedError end end + + def set_default_attributes! + # Implemented in EE + end end end diff --git a/config/feature_flags/development/inherit_approval_rules_on_creation.yml b/config/feature_flags/development/inherit_approval_rules_on_creation.yml new file mode 100644 index 00000000000000..f72b1730a65922 --- /dev/null +++ b/config/feature_flags/development/inherit_approval_rules_on_creation.yml @@ -0,0 +1,8 @@ +--- +name: inherit_approval_rules_on_creation +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118235 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/408218 +milestone: '16.0' +type: development +group: group::source code +default_enabled: false diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb index e221b7563e2e80..da4d7cc456f568 100644 --- a/ee/app/models/approval_project_rule.rb +++ b/ee/app/models/approval_project_rule.rb @@ -102,6 +102,18 @@ def scanners=(value) super(Array.wrap(value)) end + # For initialization of merge request approval rules + def to_nested_attributes + { + name: name, + approval_project_rule_id: id, + user_ids: user_ids, + group_ids: group_ids, + approvals_required: approvals_required, + rule_type: rule_type + } + end + private # There are NULL values in the database and we want to convert them to empty arrays. diff --git a/ee/app/services/approval_rules/params_filtering_service.rb b/ee/app/services/approval_rules/params_filtering_service.rb index ea49fc3bf2599e..091a041e948e05 100644 --- a/ee/app/services/approval_rules/params_filtering_service.rb +++ b/ee/app/services/approval_rules/params_filtering_service.rb @@ -142,14 +142,7 @@ def append_user_defined_inapplicable_rules(source_rule_ids) # from params to prevent duplicates next if source_rule_ids.include?(rule.id) - params[:approval_rules_attributes] << { - name: rule.name, - approval_project_rule_id: rule.id, - user_ids: rule.user_ids, - group_ids: rule.group_ids, - approvals_required: rule.approvals_required, - rule_type: rule.rule_type - } + params[:approval_rules_attributes] << rule.to_nested_attributes end end end diff --git a/ee/app/services/ee/merge_requests/create_service.rb b/ee/app/services/ee/merge_requests/create_service.rb index 77555b39442e92..e546c37641bd94 100644 --- a/ee/app/services/ee/merge_requests/create_service.rb +++ b/ee/app/services/ee/merge_requests/create_service.rb @@ -5,6 +5,19 @@ module MergeRequests module CreateService extend ::Gitlab::Utils::Override + override :set_default_attributes! + def set_default_attributes! + return unless ::Feature.enabled?(:inherit_approval_rules_on_creation, project) + return if params[:approval_rules_attributes].present? + + # Pick only regular or any_approver to match frontend behavior: + # See https://gitlab.com/gitlab-org/gitlab/blob/ea40bc69a9309e0c8691588a920383394ebc649c/ee/app/assets/javascripts/approvals/mappers.js#L80-88 + # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/408380 + approval_rules_attrs = project.approval_rules.regular_or_any_approver.map(&:to_nested_attributes) + + params[:approval_rules_attributes] = approval_rules_attrs if approval_rules_attrs.present? + end + override :after_create def after_create(issuable) issuable.run_after_commit do diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb index 2bd70dfb29df3a..cf3e4a5e9e93df 100644 --- a/ee/spec/factories/approval_rules.rb +++ b/ee/spec/factories/approval_rules.rb @@ -54,6 +54,11 @@ sequence(:name) { |n| "#{ApprovalRuleLike::DEFAULT_NAME}-#{n}" } rule_type { :regular } + trait :any_approver_rule do + rule_type { :any_approver } + name { "All Members" } + end + trait :requires_approval do approvals_required { rand(1..ApprovalProjectRule::APPROVALS_REQUIRED_MAX) } end diff --git a/ee/spec/requests/api/merge_requests_spec.rb b/ee/spec/requests/api/merge_requests_spec.rb index 9d9160653944a1..059dfc939f2469 100644 --- a/ee/spec/requests/api/merge_requests_spec.rb +++ b/ee/spec/requests/api/merge_requests_spec.rb @@ -315,6 +315,32 @@ def create_merge_request(args) end end end + + context 'when the project has approval rules' do + let(:project_approvers) { create_list(:user, 3) } + let!(:any_approver_rule) { create(:approval_project_rule, :any_approver_rule, project: project, approvals_required: 1) } + let!(:regular_rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 3) } + + before do + project_approvers.each { |u| project.add_developer(u) } + end + + it 'inherits project-level approval rules' do + create_merge_request({}) + + expect(response).to have_gitlab_http_status(:created) + + merge_request = MergeRequest.find(json_response['id']) + + merge_request_approval_rules = merge_request.approval_rules.map(&:attributes) + + expect(merge_request_approval_rules.count).to eq(2) + expect(merge_request_approval_rules).to include(a_hash_including('name' => any_approver_rule.name, 'rule_type' => 'any_approver', 'approvals_required' => 1)) + expect(merge_request_approval_rules).to include(a_hash_including('name' => regular_rule.name, 'rule_type' => 'regular', 'approvals_required' => 3)) + + expect(merge_request.approval_rules.map(&:user_ids)).to match_array([[], project_approvers.map(&:id)]) + end + end end describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge" do diff --git a/ee/spec/services/ee/merge_requests/create_service_spec.rb b/ee/spec/services/ee/merge_requests/create_service_spec.rb index 1d5629a468afab..5c45d293359538 100644 --- a/ee/spec/services/ee/merge_requests/create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/create_service_spec.rb @@ -55,6 +55,10 @@ let(:execute) { service.execute } end + it_behaves_like 'service with approval rules' do + let(:execute) { service.execute } + end + it 'sends the audit streaming event' do audit_context = { name: 'merge_request_create', diff --git a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb index beb8f04e6d978d..f298de1ff10ed2 100644 --- a/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/merge_merge_requests_shared_examples.rb @@ -213,3 +213,87 @@ def hooks_pass?(squashing: false) end end end + +RSpec.shared_examples 'service with approval rules' do + let(:opts) { super().merge(approval_attributes) } + let(:approval_attributes) { {} } + + context 'when project approval rules are missing' do + context 'when approval rules attributes are missing' do + it 'does not create approval rules' do + expect(execute.approval_rules).to be_empty + end + end + + context 'when approval rules attributes are provided' do + let(:approval_attributes) { { approval_rules_attributes: approval_rules } } + let(:approval_rules) do + [ + { + name: 'Test', + approvals_required: 5 + } + ] + end + + it 'creates approval rules' do + expect(execute.approval_rules.count).to eq(1) + expect(execute.approval_rules.first).to have_attributes(approvals_required: 5, name: 'Test') + end + end + end + + context 'when project has approval rules' do + let!(:any_approver_rule) do + create(:approval_project_rule, :any_approver_rule, project: project, approvals_required: 1) + end + + let!(:regular_rule) do + create(:approval_project_rule, project: project, approvals_required: 2) + end + + let!(:special_approver_rule) do + create(:approval_project_rule, :license_scanning, project: project, approvals_required: 3) + end + + context 'when approval rules attributes are missing' do + it 'inherits only regular and any_approver rules from the project' do + expect(execute.approval_rules.count).to eq(2) + + expect(execute.approval_rules.map(&:attributes)).to match( + [ + a_hash_including('approvals_required' => 1, 'rule_type' => 'any_approver'), + a_hash_including('approvals_required' => 2, 'rule_type' => 'regular') + ] + ) + end + + context 'when inherit_approval_rules_on_creation is disabled' do + before do + stub_feature_flags(inherit_approval_rules_on_creation: false) + end + + it 'does not create approval rules' do + expect(execute.approval_rules).to be_empty + end + end + end + + context 'when approval rules attributes are provided' do + let(:approval_attributes) { { approval_rules_attributes: approval_rules } } + let(:approval_rules) do + [ + { + name: 'Test', + approvals_required: 5 + } + ] + end + + it 'creates only requested approval rules' do + expect(execute.approval_rules.count).to eq(1) + expect(execute.approval_rules.first).to have_attributes(approvals_required: 5, name: 'Test') + end + end + end +end -- GitLab