diff --git a/config/feature_flags/development/remove_deprecated_approvals.yml b/config/feature_flags/development/adapt_deprecated_approvals.yml similarity index 87% rename from config/feature_flags/development/remove_deprecated_approvals.yml rename to config/feature_flags/development/adapt_deprecated_approvals.yml index 4a3501cc7ff03643c60c8eeda5e175f30c8c45ce..697f7e210250a3326ab43a153d93720a5cdb7da8 100644 --- a/config/feature_flags/development/remove_deprecated_approvals.yml +++ b/config/feature_flags/development/adapt_deprecated_approvals.yml @@ -1,5 +1,5 @@ --- -name: remove_deprecated_approvals +name: adapt_deprecated_approvals introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118036 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/409686 milestone: '16.0' diff --git a/ee/lib/ee/api/merge_request_approvals.rb b/ee/lib/ee/api/merge_request_approvals.rb index fd3f7821f9138a88ae985a16ddce49cd3c38cbb0..36643fb385b1f73b424aab730667556ccc83e965 100644 --- a/ee/lib/ee/api/merge_request_approvals.rb +++ b/ee/lib/ee/api/merge_request_approvals.rb @@ -56,8 +56,8 @@ def present_merge_request_approval_state(presenter:, target_branch: nil) end # Deprecated in favor of approval rules API - desc 'Change approval-related configuration' do - detail 'This feature was introduced in 10.6' + desc 'Deprecated in 16.0: Use the merge request approvals API instead. Change approval-related configuration' do + detail 'This feature was introduced in 10.6 and deprecated in 16.0' success ::EE::API::Entities::ApprovalState deprecated true end @@ -67,18 +67,34 @@ def present_merge_request_approval_state(presenter:, target_branch: nil) documentation: { example: 2 } end post 'approvals' do - error!('Not found', 404) if ::Feature.enabled?(:remove_deprecated_approvals) - merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_merge_request) error!('Overriding approvals is disabled', 422) if merge_request.project.disable_overriding_approvers_per_merge_request - merge_request = ::MergeRequests::UpdateService.new(project: user_project, current_user: current_user, params: { approvals_before_merge: params[:approvals_required] }).execute(merge_request) + if ::Feature.enabled?(:adapt_deprecated_approvals) + approval_rule = merge_request.approval_rules.any_approver.first + + approval_params = declared_params(include_missing: false) + + result = if approval_rule + ::ApprovalRules::UpdateService.new(approval_rule, current_user, approval_params).execute + else + ::ApprovalRules::CreateService.new(merge_request, current_user, approval_params).execute + end + + if result[:status] == :success + present_approval(merge_request) + else + render_api_error!(result[:message], result[:http_status] || 400) + end + else + merge_request = ::MergeRequests::UpdateService.new(project: user_project, current_user: current_user, params: { approvals_before_merge: params[:approvals_required] }).execute(merge_request) - # Merge request shouldn't be in an invalid state after the changes, but handling errors to be safe - handle_merge_request_errors!(merge_request) + # Merge request shouldn't be in an invalid state after the changes, but handling errors to be safe + handle_merge_request_errors!(merge_request) - present_approval(merge_request) + present_approval(merge_request) + end end end end diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 0c12323a3dab53df63ffa2fe246d520adf589fa2..eef57e2848fc800c4e855305c9e7ed9e8a20dc38 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -280,19 +280,110 @@ let(:params) { { approvals_required: 5 } } let(:current_user) { user } - it 'returns 404 error (deprecated endpoint)' do - post api(path, current_user, admin_mode: true), params: params + context 'when feature flag "adapt_deprecated_approvals" is enabled' do + before do + stub_feature_flags(adapt_deprecated_approvals: true) + end + + it_behaves_like 'POST request permissions for admin mode' + + shared_examples_for 'cannot update approval rules' do + context 'when users cannot update approval rules' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :edit_approval_rule, anything).and_return(false) + end + + it 'returns 403 error' do + post api(path, current_user, admin_mode: true), params: params + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq ['Prohibited'] + end + end + end + + shared_examples_for 'user allowed to override approvals_before_merge' do + context 'when approval rule is missing for the merge request' do + it 'creates an approval rule with required number of approvals' do + post api(path, current_user, admin_mode: true), params: params + + approval_rule = merge_request.approval_rules.any_approver.first + expect(approval_rule.approvals_required).to eq(5) + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['approvals_required']).to eq(5) + end + + it_behaves_like 'cannot update approval rules' + end + + context 'when approval rules already exist for the merge request' do + let!(:any_approver_rule) { create(:any_approver_rule, merge_request: merge_request, approvals_required: 1) } + let!(:custom_rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 2) } + + it 'updates any approval rule with required number of approvals' do + expect do + post api(path, current_user, admin_mode: true), params: params + end.to change { any_approver_rule.reload.approvals_required }.from(1).to(5) + .and change { custom_rule.reload.approvals_required }.by(0) + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['approvals_required']).to eq(5) + end + + it_behaves_like 'cannot update approval rules' + end + end + + context 'when disable_overriding_approvers_per_merge_request is true on the project' do + before do + project.update!(disable_overriding_approvers_per_merge_request: true) + end + + it 'does not allow you to set approvals_before_merge' do + expect do + post api(path, current_user, admin_mode: true), params: params + end.not_to change { merge_request.approval_rules.any_approver.reload.first }.from(nil) + + expect(response).to have_gitlab_http_status(:unprocessable_entity) + end + end + + context 'as a project admin' do + it_behaves_like 'user allowed to override approvals_before_merge' do + let(:current_user) { user } + let(:expected_approver_group_size) { 0 } + end + end + + context 'as a global admin' do + it_behaves_like 'user allowed to override approvals_before_merge' do + let(:current_user) { admin } + let(:expected_approver_group_size) { 1 } + end + end - expect(response).to have_gitlab_http_status(:not_found) + context 'as a random user' do + before do + project.update!(disable_overriding_approvers_per_merge_request: false) + end + + it 'does not allow you to override approvals required' do + expect do + post api(path, user2), params: params + end.not_to change { merge_request.approval_rules.any_approver.reload.first }.from(nil) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end - context 'when feature flag "remove_deprecated_approvals" is disabled' do + context 'when feature flag "adapt_deprecated_approvals" is disabled' do before do - stub_feature_flags(remove_deprecated_approvals: false) + stub_feature_flags(adapt_deprecated_approvals: false) end - it_behaves_like 'POST request permissions for admin mode' - shared_examples_for 'user allowed to override approvals_before_merge' do context 'when disable_overriding_approvers_per_merge_request is false on the project' do before do @@ -324,6 +415,14 @@ end end + it 'additionally updates approvals_before_merge on merge request' do + expect do + post api(path, current_user, admin_mode: true), params: params + end.to change { merge_request.reload.approvals_before_merge }.from(nil).to(5) + + expect(response).to have_gitlab_http_status(:created) + end + context 'as a project admin' do it_behaves_like 'user allowed to override approvals_before_merge' do let(:current_user) { user }