From 121147b0a6fb7f3bbd396382b2ee4cf135a5faf6 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Tue, 4 Jun 2019 10:51:24 +0800 Subject: [PATCH 01/13] Rename approval_settings endpoints to approval_rules Remove project API's "rules" segment --- ee/lib/api/merge_request_approvals.rb | 2 +- ee/lib/api/project_approval_rules.rb | 92 +++++++++++++-------------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb index 2e825ef7da58de..b58a9ceabdf57f 100644 --- a/ee/lib/api/merge_request_approvals.rb +++ b/ee/lib/api/merge_request_approvals.rb @@ -53,7 +53,7 @@ def handle_merge_request_errors!(errors) success: ::EE::API::Entities::MergeRequestApprovalRules, hidden: true } - get 'approval_settings' do + get 'approval_rules' do 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 diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index a40af59c796a29..92ffdf28c035e7 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -10,7 +10,7 @@ class ProjectApprovalRules < ::Grape::API 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 + segment ':id/approval_rules' do desc 'Get all project approval rules' do detail 'Private API subject to change' success EE::API::Entities::ProjectApprovalRules @@ -21,22 +21,48 @@ class ProjectApprovalRules < ::Grape::API present user_project, with: EE::API::Entities::ProjectApprovalRules, current_user: current_user end - segment 'rules' do - desc 'Create new approval rule' 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 :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' + 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 :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 :rule_type, type: String, desc: 'The type of approval rule' + 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' + optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end - post do + put do authorize! :admin_project, user_project - result = ::ApprovalRules::CreateService.new(user_project, current_user, declared_params(include_missing: false)).execute + 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 @@ -45,47 +71,19 @@ class ProjectApprovalRules < ::Grape::API 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' - optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' - 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 + 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]) + approval_rule = user_project.approval_rules.find(params[:approval_rule_id]) - destroy_conditionally!(approval_rule) do |rule| - ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute - end + destroy_conditionally!(approval_rule) do |rule| + ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute end end end -- GitLab From 5a15f149b6696372fd9c78e9002f9641fe8b7117 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Tue, 4 Jun 2019 11:02:58 +0800 Subject: [PATCH 02/13] Update references to new endpoint Rename path helper --- .../presenters/ee/merge_request_presenter.rb | 8 ++++---- .../ee/merge_request_widget_entity.rb | 4 ++-- ...e_request_approvals_settings_form.html.haml | 4 ++-- .../views/shared/issuable/_approvals.html.haml | 4 ++-- .../modules/project_settings/actions_spec.js | 4 ++-- .../presenters/merge_request_presenter_spec.rb | 12 ++++++------ .../api/merge_request_approvals_spec.rb | 8 ++++---- .../api/project_approval_rules_spec.rb | 18 +++++++++--------- 8 files changed, 31 insertions(+), 31 deletions(-) diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb index a377815e08b452..550ea9c084b061 100644 --- a/ee/app/presenters/ee/merge_request_presenter.rb +++ b/ee/app/presenters/ee/merge_request_presenter.rb @@ -17,15 +17,15 @@ def api_approvals_path end end - def api_approval_settings_path + def api_approval_rules_path if expose_mr_approval_path? - expose_path(api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid)) + expose_path(api_v4_projects_merge_requests_approval_rules_path(id: project.id, merge_request_iid: merge_request.iid)) end end - def api_project_approval_settings_path + def api_project_approval_rules_path if approval_feature_available? - expose_path(api_v4_projects_approval_settings_path(id: project.id)) + expose_path(api_v4_projects_approval_rules_path(id: project.id)) end end diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb index cf5cae57af908d..33e21b0a0dd740 100644 --- a/ee/app/serializers/ee/merge_request_widget_entity.rb +++ b/ee/app/serializers/ee/merge_request_widget_entity.rb @@ -152,8 +152,8 @@ module MergeRequestWidgetEntity 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 + expose :api_approval_rules_path do |merge_request| + presenter(merge_request).api_approval_rules_path end expose :api_approve_path do |merge_request| presenter(merge_request).api_approve_path diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index 49179755d8ebc4..c651f2ffb4c32f 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -5,8 +5,8 @@ = _("Add approvers") #js-mr-approvals-settings{ data: { 'project_id': @project.id, 'project_path': expose_path(api_v4_projects_path(id: @project.id)), - 'settings_path': expose_path(api_v4_projects_approval_settings_path(id: @project.id)), - 'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: @project.id)), + 'settings_path': expose_path(api_v4_projects_approval_rules_path(id: @project.id)), + 'rules_path': expose_path(api_v4_projects_approval_rules_path(id: @project.id)), 'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } } .text-center.prepend-top-default = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') diff --git a/ee/app/views/shared/issuable/_approvals.html.haml b/ee/app/views/shared/issuable/_approvals.html.haml index 51d6fecd79e0b6..a520a2c112419c 100644 --- a/ee/app/views/shared/issuable/_approvals.html.haml +++ b/ee/app/views/shared/issuable/_approvals.html.haml @@ -15,8 +15,8 @@ 'can_edit': can?(current_user, :update_approvers, issuable).to_s, 'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s, 'mr_id': issuable.iid, - 'mr_settings_path': presenter.api_approval_settings_path, - 'project_settings_path': presenter.api_project_approval_settings_path } } + 'mr_settings_path': presenter.api_approval_rules_path, + 'project_settings_path': presenter.api_project_approval_rules_path } } = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') - if can_update_approvers - approver_presenter = MergeRequestApproverPresenter.new(issuable, skip_user: current_user) diff --git a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js index 9c69f1f91b4844..bd31519ea8dc94 100644 --- a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js @@ -21,8 +21,8 @@ const TEST_RULE_RESPONSE = { groups: [{ id: 4 }], users: [{ id: 7 }, { id: 8 }], }; -const TEST_SETTINGS_PATH = 'projects/9/approval_settings'; -const TEST_RULES_PATH = 'projects/9/approval_settings/rules'; +const TEST_SETTINGS_PATH = 'projects/9/approval_rules'; +const TEST_RULES_PATH = 'projects/9/approval_rules'; describe('EE approvals project settings module actions', () => { let state; diff --git a/ee/spec/presenters/merge_request_presenter_spec.rb b/ee/spec/presenters/merge_request_presenter_spec.rb index a931a755b6e219..de5d122fdb643f 100644 --- a/ee/spec/presenters/merge_request_presenter_spec.rb +++ b/ee/spec/presenters/merge_request_presenter_spec.rb @@ -45,18 +45,18 @@ it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approvals")) } end - describe '#api_approval_settings_path' do - subject { described_class.new(merge_request, current_user: user).api_approval_settings_path } + describe '#api_approval_rules_path' do + subject { described_class.new(merge_request, current_user: user).api_approval_rules_path } it_behaves_like 'is nil when needed' - it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approval_settings")) } + it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approval_rules")) } end - describe '#api_project_approval_settings_path' do - subject { described_class.new(merge_request, current_user: user).api_project_approval_settings_path } + describe '#api_project_approval_rules_path' do + subject { described_class.new(merge_request, current_user: user).api_project_approval_rules_path } - it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/approval_settings")) } + it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/approval_rules")) } context "when approvals not available" do let(:approval_feature_available) { false } diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 65b63d48b749c1..1f951d320e1597 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -95,7 +95,7 @@ end end - describe 'GET :id/merge_requests/:merge_request_iid/approval_settings' do + describe 'GET :id/merge_requests/:merge_request_iid/approval_rules' do let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 2, name: 'foo') } it 'retrieves the approval rules details' do @@ -103,7 +103,7 @@ merge_request.approvals.create(user: approver) rule.users << approver - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) @@ -127,7 +127,7 @@ rule.users << approver rule.create_approval_merge_request_rule_source!(approval_project_rule: source_rule) - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) @@ -144,7 +144,7 @@ rule.users << approver rule.groups << private_group - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) diff --git a/ee/spec/requests/api/project_approval_rules_spec.rb b/ee/spec/requests/api/project_approval_rules_spec.rb index 5c86db2948d5da..8d2a7a9820bd34 100644 --- a/ee/spec/requests/api/project_approval_rules_spec.rb +++ b/ee/spec/requests/api/project_approval_rules_spec.rb @@ -10,10 +10,10 @@ 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" } + let(:url) { "/projects/#{project.id}/approval_rules" } - describe 'GET /projects/:id/approval_settings' do - let(:url) { "/projects/#{project.id}/approval_settings" } + describe 'GET /projects/:id/approval_rules' do + let(:url) { "/projects/#{project.id}/approval_rules" } context 'when the request is correct' do let!(:rule) do @@ -89,7 +89,7 @@ end end - describe 'POST /projects/:id/approval_settings/rules' do + describe 'POST /projects/:id/approval_rules' do let(:current_user) { user } let(:params) do { @@ -144,9 +144,9 @@ end end - describe 'PUT /projects/:id/approval_settings/:approval_rule_id' do + describe 'PUT /projects/:id/approval_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}" } + let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule.id}" } shared_examples_for 'a user with access' do before do @@ -210,9 +210,9 @@ end end - describe 'DELETE /projects/:id/approval_settings/rules/:approval_rule_id' do + describe 'DELETE /projects/:id/approval_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}" } + let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule.id}" } it 'destroys' do delete api(url, user) @@ -223,7 +223,7 @@ 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}" } + let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule_2.id}" } it 'returns not found' do delete api(url, user) -- GitLab From 41e771e7f57b0bff34469475bb8db597ace88385 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 5 Jun 2019 14:59:29 +0800 Subject: [PATCH 03/13] Mark API endpoints as public Remove hidden --- ee/lib/api/merge_request_approvals.rb | 3 +-- ee/lib/api/project_approval_rules.rb | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb index b58a9ceabdf57f..cd7e89079bcc8d 100644 --- a/ee/lib/api/merge_request_approvals.rb +++ b/ee/lib/api/merge_request_approvals.rb @@ -50,8 +50,7 @@ def handle_merge_request_errors!(errors) end desc 'List approval rules for merge request', { - success: ::EE::API::Entities::MergeRequestApprovalRules, - hidden: true + success: ::EE::API::Entities::MergeRequestApprovalRules } get 'approval_rules' do merge_request = find_merge_request_with_access(params[:merge_request_iid]) diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index 92ffdf28c035e7..0cb7d63d8907c7 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -12,7 +12,6 @@ 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 - detail 'Private API subject to change' success EE::API::Entities::ProjectApprovalRules end get do @@ -22,7 +21,6 @@ class ProjectApprovalRules < ::Grape::API end desc 'Create new approval rule' do - detail 'Private API subject to change' success EE::API::Entities::ApprovalRule end params do @@ -46,7 +44,6 @@ class ProjectApprovalRules < ::Grape::API segment ':approval_rule_id' do desc 'Update approval rule' do - detail 'Private API subject to change' success EE::API::Entities::ApprovalRule end params do @@ -72,7 +69,6 @@ class ProjectApprovalRules < ::Grape::API 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' -- GitLab From 32b7d882ccfb58dbe00bb41e87af437220ec386f Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 31 May 2019 14:04:24 +0800 Subject: [PATCH 04/13] Doc: add project approval rule endpoints Deprecate older approval APIs Remove deprecated attribute Add changelog --- doc/api/merge_request_approvals.md | 310 ++++++++++++++---- .../unreleased/11877-public-approval-api.yml | 5 + 2 files changed, 248 insertions(+), 67 deletions(-) create mode 100644 ee/changelogs/unreleased/11877-public-approval-api.yml diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index cc95689a65f62a..8de227312027c7 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -23,36 +23,6 @@ GET /projects/:id/approvals ```json { - "approvers": [ - { - "user": { - "id": 5, - "name": "John Doe6", - "username": "user5", - "state":"active","avatar_url":"https://www.gravatar.com/avatar/4aea8cf834ed91844a2da4ff7ae6b491?s=80\u0026d=identicon","web_url":"http://localhost/user5" - } - } - ], - "approver_groups": [ - { - "group": { - "id": 1, - "name": "group1", - "path": "group1", - "description": "", - "visibility": "public", - "lfs_enabled": false, - "avatar_url": null, - "web_url": "http://localhost/groups/group1", - "request_access_enabled": false, - "full_name": "group1", - "full_path": "group1", - "parent_id": null, - "ldap_cn": null, - "ldap_access": null - } - } - ], "approvals_before_merge": 2, "reset_approvals_on_push": true, "disable_overriding_approvers_per_merge_request": false @@ -75,7 +45,7 @@ POST /projects/:id/approvals | Attribute | Type | Required | Description | | ------------------------------------------------ | ------- | -------- | --------------------------------------------------------------------------------------------------- | | `id` | integer | yes | The ID of a project | -| `approvals_before_merge` | integer | no | How many approvals are required before an MR can be merged | +| `approvals_before_merge` | integer | no | How many approvals are required before an MR can be merged. Deprecated in 12.0 in favor of Approval Rules API. | | `reset_approvals_on_push` | boolean | no | Reset approvals on a new push | | `disable_overriding_approvers_per_merge_request` | boolean | no | Allow/Disallow overriding approvers per MR | | `merge_requests_author_approval` | boolean | no | Allow/Disallow authors from self approving merge requests; `true` means authors cannot self approve | @@ -83,46 +53,192 @@ POST /projects/:id/approvals ```json { + "approvals_before_merge": 2, + "reset_approvals_on_push": true, + "disable_overriding_approvers_per_merge_request": false, + "merge_requests_author_approval": false, + "merge_requests_disable_committers_approval": false +} +``` + +### Get project-level rules + +>**Note:** This API endpoint is only available on 12.0 Starter and above. + +You can request information about a project's approval rules using the following endpoint: + +``` +GET /projects/:id/approval_rules +``` + +**Parameters:** + +| Attribute | Type | Required | Description | +|----------------------|---------|----------|-----------------------------------------------------------| +| `id` | integer | yes | The ID of a project | + +```json +{ + "rules": [ + { + "id": 1, + "name": "security", + "rule_type": "regular", + "approvers": [ + { + "id": 5, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "approvals_required": 3, + "users": [ + { + "id": 5, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "groups": [], + "contains_hidden_groups": false + } + ], + "fallback_approvals_required": 3 +} +``` + +### Create project-level rule + +>**Note:** This API endpoint is only available on 12.0 Starter and above. + +You can create project approval rules using the following endpoint: + +``` +POST /projects/:id/approval_rules +``` + +**Parameters:** + +| Attribute | Type | Required | Description | +|----------------------|---------|----------|-----------------------------------------------------------| +| `id` | integer | yes | The ID of a project | +| `name` | string | yes | The name of the approval rule | +| `approvals_required` | integer | yes | The number of required approvals for this rule | +| `users` | integer | no | The ids of users as approvers | +| `groups` | integer | no | The ids of groups as approvers | + +```json +{ + "id": 1, + "name": "security", + "rule_type": "regular", "approvers": [ { - "user": { - "id": 5, - "name": "John Doe6", - "username": "user5", - "state":"active","avatar_url":"https://www.gravatar.com/avatar/4aea8cf834ed91844a2da4ff7ae6b491?s=80\u0026d=identicon","web_url":"http://localhost/user5" - } + "id": 2, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" } ], - "approver_groups": [ + "approvals_required": 1, + "users": [ { - "group": { - "id": 1, - "name": "group1", - "path": "group1", - "description": "", - "visibility": "public", - "lfs_enabled": false, - "avatar_url": null, - "web_url": "http://localhost/groups/group1", - "request_access_enabled": false, - "full_name": "group1", - "full_path": "group1", - "parent_id": null, - "ldap_cn": null, - "ldap_access": null - } + "id": 2, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" } ], - "approvals_before_merge": 2, - "reset_approvals_on_push": true, - "disable_overriding_approvers_per_merge_request": false, - "merge_requests_author_approval": false, - "merge_requests_disable_committers_approval": false + "groups": [], + "contains_hidden_groups": false +} +``` + +### Update project-level rule + +>**Note:** This API endpoint is only available on 12.0 Starter and above. + +You can update project approval rules using the following endpoint: + +``` +PUT /projects/:id/approval_rules/:approval_rule_id +``` + +**Important:** Approvers and groups not in the `users`/`groups` param will be **removed** + +**Parameters:** + +| Attribute | Type | Required | Description | +|----------------------|---------|----------|-----------------------------------------------------------| +| `id` | integer | yes | The ID of a project | +| `approval_rule_id` | integer | yes | The ID of a approval rule | +| `name` | string | yes | The name of the approval rule | +| `approvals_required` | integer | yes | The number of required approvals for this rule | +| `users` | integer | no | The ids of users as approvers | +| `groups` | integer | no | The ids of groups as approvers | + +```json +{ + "id": 1, + "name": "security", + "rule_type": "regular", + "approvers": [ + { + "id": 2, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "approvals_required": 1, + "users": [ + { + "id": 2, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "groups": [], + "contains_hidden_groups": false } ``` +### Delete project-level rule + +>**Note:** This API endpoint is only available on 12.0 Starter and above. + +You can delete project approval rules using the following endpoint: + +``` +DELETE /projects/:id/approval_rules/:approval_rule_id +``` + +**Parameters:** + +| Attribute | Type | Required | Description | +|----------------------|---------|----------|-----------------------------------------------------------| +| `id` | integer | yes | The ID of a project | +| `approval_rule_id` | integer | yes | The ID of a approval rule + ### Change allowed approvers +>**Note:** This API endpoint has been deprecated. Please use Approval Rule API instead. + >**Note:** This API endpoint is only available on 10.6 Starter and above. If you are allowed to, you can change approvers and approver groups using @@ -227,8 +343,6 @@ GET /projects/:id/merge_requests/:merge_request_iid/approvals } } ], - "approvers": [], - "approver_groups": [] } ``` @@ -249,7 +363,7 @@ POST /projects/:id/merge_requests/:merge_request_iid/approvals |----------------------|---------|----------|--------------------------------------------| | `id` | integer | yes | The ID of a project | | `merge_request_iid` | integer | yes | The IID of MR | -| `approvals_required` | integer | yes | Approvals required before MR can be merged | +| `approvals_required` | integer | yes | Approvals required before MR can be merged. Deprecated in 12.0 in favor of Approval Rules API. | ```json { @@ -264,14 +378,14 @@ POST /projects/:id/merge_requests/:merge_request_iid/approvals "merge_status": "cannot_be_merged", "approvals_required": 2, "approvals_left": 2, - "approved_by": [], - "approvers": [], - "approver_groups": [] + "approved_by": [] } ``` ### Change allowed approvers for Merge Request +>**Note:** This API endpoint has been deprecated. Please use Approval Rule API instead. + >**Note:** This API endpoint is only available on 10.6 Starter and above. If you are allowed to, you can change approvers and approver groups using @@ -341,6 +455,70 @@ PUT /projects/:id/merge_requests/:merge_request_iid/approvers } ``` +### Get approval rules for Merge Request + +>**Note:** This API endpoint is only available on 12.0 Starter and above. + +You can request information about a merge request's approval rules using the following endpoint: + +``` +GET /projects/:id/merge_requests/:merge_request_iid/approval_rules +``` + +**Parameters:** + +| Attribute | Type | Required | Description | +|----------------------|---------|----------|-----------------------------------------------------------| +| `id` | integer | yes | The ID of a project | +| `merge_request_iid` | integer | yes | The IID of MR | + +```json +{ + "approval_rules_overwritten": true, + "rules": [ + { + "id": 1, + "name": "Ruby", + "rule_type": "regular", + "approvers": [ + { + "id": 4, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "approvals_required": 2, + "users": [ + { + "id": 4, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "groups": [], + "contains_hidden_groups": false, + "approved_by": [ + { + "id": 4, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "source_rule": null, + "approved": true + } + ] +} +``` ## Approve Merge Request >**Note:** This API endpoint is only available on 8.9 Starter and above. @@ -401,8 +579,6 @@ does not match, the response code will be `409`. } } ], - "approvers": [], - "approver_groups": [] } ``` diff --git a/ee/changelogs/unreleased/11877-public-approval-api.yml b/ee/changelogs/unreleased/11877-public-approval-api.yml new file mode 100644 index 00000000000000..ba35851b1dac77 --- /dev/null +++ b/ee/changelogs/unreleased/11877-public-approval-api.yml @@ -0,0 +1,5 @@ +--- +title: Public project-level approval rule API +merge_request: 13895 +author: +type: added -- GitLab From b169a81fd06c3b2f155dee04f964b1b42e869bc0 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sun, 4 Aug 2019 15:49:42 -0500 Subject: [PATCH 05/13] Remove redundant settings_path from MR project FE --- .../approvals/stores/modules/project_settings/actions.js | 4 ++-- .../_merge_request_approvals_settings_form.html.haml | 1 - .../stores/modules/project_settings/actions_spec.js | 8 +++----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 79f9e7d93c2436..53a8e227e05a0f 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js @@ -22,12 +22,12 @@ export const receiveRulesError = () => { }; export const fetchRules = ({ rootState, dispatch }) => { - const { settingsPath } = rootState.settings; + const { rulesPath } = rootState.settings; dispatch('requestRules'); return axios - .get(settingsPath) + .get(rulesPath) .then(response => dispatch('receiveRulesSuccess', mapApprovalSettingsResponse(response.data))) .catch(() => dispatch('receiveRulesError')); }; diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index c651f2ffb4c32f..5183326dcc982c 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -5,7 +5,6 @@ = _("Add approvers") #js-mr-approvals-settings{ data: { 'project_id': @project.id, 'project_path': expose_path(api_v4_projects_path(id: @project.id)), - 'settings_path': expose_path(api_v4_projects_approval_rules_path(id: @project.id)), 'rules_path': expose_path(api_v4_projects_approval_rules_path(id: @project.id)), 'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } } .text-center.prepend-top-default diff --git a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js index bd31519ea8dc94..d112cfb282442c 100644 --- a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js @@ -21,7 +21,6 @@ const TEST_RULE_RESPONSE = { groups: [{ id: 4 }], users: [{ id: 7 }, { id: 8 }], }; -const TEST_SETTINGS_PATH = 'projects/9/approval_rules'; const TEST_RULES_PATH = 'projects/9/approval_rules'; describe('EE approvals project settings module actions', () => { @@ -33,7 +32,6 @@ describe('EE approvals project settings module actions', () => { state = { settings: { projectId: TEST_PROJECT_ID, - settingsPath: TEST_SETTINGS_PATH, rulesPath: TEST_RULES_PATH, }, }; @@ -90,7 +88,7 @@ describe('EE approvals project settings module actions', () => { describe('fetchRules', () => { it('dispatches request/receive', done => { const data = { rules: [TEST_RULE_RESPONSE] }; - mock.onGet(TEST_SETTINGS_PATH).replyOnce(200, data); + mock.onGet(TEST_RULES_PATH).replyOnce(200, data); testAction( actions.fetchRules, @@ -102,7 +100,7 @@ describe('EE approvals project settings module actions', () => { { type: 'receiveRulesSuccess', payload: mapApprovalSettingsResponse(data) }, ], () => { - expect(mock.history.get.map(x => x.url)).toEqual([TEST_SETTINGS_PATH]); + expect(mock.history.get.map(x => x.url)).toEqual([TEST_RULES_PATH]); done(); }, @@ -110,7 +108,7 @@ describe('EE approvals project settings module actions', () => { }); it('dispatches request/receive on error', done => { - mock.onGet(TEST_SETTINGS_PATH).replyOnce(500); + mock.onGet(TEST_RULES_PATH).replyOnce(500); testAction( actions.fetchRules, -- GitLab From 8a8a6c3b357946a40efd9e2d89e70bdd6b78c9c2 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Mon, 5 Aug 2019 15:05:26 +0800 Subject: [PATCH 06/13] Rename js setting to rule --- .../vue_merge_request_widget/mr_widget_options.vue | 2 +- .../vue_merge_request_widget/services/mr_widget_service.js | 4 ++-- .../vue_merge_request_widget/stores/mr_widget_store.js | 2 +- .../javascripts/vue_mr_widget/ee_mr_widget_options_spec.js | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue b/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue index 58aadf35018ea4..4db974ac052c78 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue +++ b/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue @@ -162,7 +162,7 @@ export default { return { ...base, apiApprovalsPath: store.apiApprovalsPath, - apiApprovalSettingsPath: store.apiApprovalSettingsPath, + apiApprovalRulesPath: store.apiApprovalRulesPath, apiApprovePath: store.apiApprovePath, apiUnapprovePath: store.apiUnapprovePath, }; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js b/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js index eb1625cff550de..f0fe04a116bb19 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js @@ -6,7 +6,7 @@ export default class MRWidgetService extends CEWidgetService { super(mr); this.apiApprovalsPath = mr.apiApprovalsPath; - this.apiApprovalSettingsPath = mr.apiApprovalSettingsPath; + this.apiApprovalRulesPath = mr.apiApprovalRulesPath; this.apiApprovePath = mr.apiApprovePath; this.apiUnapprovePath = mr.apiUnapprovePath; } @@ -16,7 +16,7 @@ export default class MRWidgetService extends CEWidgetService { } fetchApprovalSettings() { - return axios.get(this.apiApprovalSettingsPath).then(res => res.data); + return axios.get(this.apiApprovalRulesPath).then(res => res.data); } approveMergeRequest() { diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 32883b656ad058..cc31898b8ce687 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -61,7 +61,7 @@ export default class MergeRequestStore extends CEMergeRequestStore { data.has_approvals_available || this.hasApprovalsAvailable, ); this.apiApprovalsPath = data.api_approvals_path || this.apiApprovalsPath; - this.apiApprovalSettingsPath = data.api_approval_settings_path || this.apiApprovalSettingsPath; + this.apiApprovalRulesPath = data.api_approval_rules_path || this.apiApprovalRulesPath; this.apiApprovePath = data.api_approve_path || this.apiApprovePath; this.apiUnapprovePath = data.api_unapprove_path || this.apiUnapprovePath; } diff --git a/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js b/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js index 92257ece4a4258..a54462302d2225 100644 --- a/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js +++ b/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js @@ -1017,7 +1017,7 @@ describe('ee merge request widget options', () => { it('passes approval api paths to service', () => { const paths = { api_approvals_path: `${TEST_HOST}/api/approvals/path`, - api_approval_settings_path: `${TEST_HOST}/api/approval/settings/path`, + api_approval_rules_path: `${TEST_HOST}/api/approval/rules/path`, api_approve_path: `${TEST_HOST}/api/approve/path`, api_unapprove_path: `${TEST_HOST}/api/unapprove/path`, }; -- GitLab From c2d9296bf7fe055a2de28dea88097f94dddfcfb1 Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 7 Aug 2019 18:00:24 +0800 Subject: [PATCH 07/13] Fix doc extra blank line --- doc/api/merge_request_approvals.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index 8de227312027c7..f34afb7ab7ae9c 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -238,7 +238,6 @@ DELETE /projects/:id/approval_rules/:approval_rule_id ### Change allowed approvers >**Note:** This API endpoint has been deprecated. Please use Approval Rule API instead. - >**Note:** This API endpoint is only available on 10.6 Starter and above. If you are allowed to, you can change approvers and approver groups using @@ -385,7 +384,6 @@ POST /projects/:id/merge_requests/:merge_request_iid/approvals ### Change allowed approvers for Merge Request >**Note:** This API endpoint has been deprecated. Please use Approval Rule API instead. - >**Note:** This API endpoint is only available on 10.6 Starter and above. If you are allowed to, you can change approvers and approver groups using -- GitLab From 00d4b32cf47e64c8ee38004915d7312b033b659b Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Fri, 9 Aug 2019 14:55:10 +0800 Subject: [PATCH 08/13] Fix path renaming --- ee/spec/fixtures/api/schemas/entities/merge_request_widget.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json b/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json index fe990128c3f74e..7d493400936a7a 100644 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -9,7 +9,7 @@ "approved": { "type": "boolean" }, "approvals_path": { "type": ["string", "null"] }, "api_approvals_path": { "type": ["string", "null"] }, - "api_approval_settings_path": { "type": ["string", "null"] }, + "api_approval_rules_path": { "type": ["string", "null"] }, "api_approve_path": { "type": ["string", "null"] }, "api_unapprove_path": { "type": ["string", "null"] }, "codeclimate": { -- GitLab From 5f13729163e38cbee05a56c9fc1e99bb611e0ade Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 14 Aug 2019 09:54:23 +0800 Subject: [PATCH 09/13] Update release version to 12.2 --- doc/api/merge_request_approvals.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index f34afb7ab7ae9c..d58ed8eae460c3 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -63,7 +63,7 @@ POST /projects/:id/approvals ### Get project-level rules ->**Note:** This API endpoint is only available on 12.0 Starter and above. +>**Note:** This API endpoint is only available on 12.2 Starter and above. You can request information about a project's approval rules using the following endpoint: @@ -115,7 +115,7 @@ GET /projects/:id/approval_rules ### Create project-level rule ->**Note:** This API endpoint is only available on 12.0 Starter and above. +>**Note:** This API endpoint is only available on 12.2 Starter and above. You can create project approval rules using the following endpoint: @@ -166,7 +166,7 @@ POST /projects/:id/approval_rules ### Update project-level rule ->**Note:** This API endpoint is only available on 12.0 Starter and above. +>**Note:** This API endpoint is only available on 12.2 Starter and above. You can update project approval rules using the following endpoint: @@ -220,7 +220,7 @@ PUT /projects/:id/approval_rules/:approval_rule_id ### Delete project-level rule ->**Note:** This API endpoint is only available on 12.0 Starter and above. +>**Note:** This API endpoint is only available on 12.2 Starter and above. You can delete project approval rules using the following endpoint: @@ -455,7 +455,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid/approvers ### Get approval rules for Merge Request ->**Note:** This API endpoint is only available on 12.0 Starter and above. +>**Note:** This API endpoint is only available on 12.2 Starter and above. You can request information about a merge request's approval rules using the following endpoint: -- GitLab From ed46eeed912353555fd2287edf205b9ef8f5ea7c Mon Sep 17 00:00:00 2001 From: Mark Chao Date: Wed, 14 Aug 2019 10:41:17 +0800 Subject: [PATCH 10/13] Doc: add group in response sample --- doc/api/merge_request_approvals.md | 108 +++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index d58ed8eae460c3..8dfc9cf69d3ae5 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -92,6 +92,14 @@ GET /projects/:id/approval_rules "state": "active", "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", "web_url": "http://localhost/jdoe" + }, + { + "id": 50, + "name": "Group Member 1", + "username": "group_member_1", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/group_member_1" } ], "approvals_required": 3, @@ -105,7 +113,24 @@ GET /projects/:id/approval_rules "web_url": "http://localhost/jdoe" } ], - "groups": [], + "groups": [ + { + "id": 5, + "name": "group1", + "path": "group1", + "description": "", + "visibility": "public", + "lfs_enabled": false, + "avatar_url": null, + "web_url": "http://localhost/groups/group1", + "request_access_enabled": false, + "full_name": "group1", + "full_path": "group1", + "parent_id": null, + "ldap_cn": null, + "ldap_access": null + } + ], "contains_hidden_groups": false } ], @@ -146,6 +171,14 @@ POST /projects/:id/approval_rules "state": "active", "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", "web_url": "http://localhost/jdoe" + }, + { + "id": 50, + "name": "Group Member 1", + "username": "group_member_1", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/group_member_1" } ], "approvals_required": 1, @@ -159,7 +192,24 @@ POST /projects/:id/approval_rules "web_url": "http://localhost/jdoe" } ], - "groups": [], + "groups": [ + { + "id": 5, + "name": "group1", + "path": "group1", + "description": "", + "visibility": "public", + "lfs_enabled": false, + "avatar_url": null, + "web_url": "http://localhost/groups/group1", + "request_access_enabled": false, + "full_name": "group1", + "full_path": "group1", + "parent_id": null, + "ldap_cn": null, + "ldap_access": null + } + ], "contains_hidden_groups": false } ``` @@ -200,6 +250,14 @@ PUT /projects/:id/approval_rules/:approval_rule_id "state": "active", "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", "web_url": "http://localhost/jdoe" + }, + { + "id": 50, + "name": "Group Member 1", + "username": "group_member_1", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/group_member_1" } ], "approvals_required": 1, @@ -213,7 +271,24 @@ PUT /projects/:id/approval_rules/:approval_rule_id "web_url": "http://localhost/jdoe" } ], - "groups": [], + "groups": [ + { + "id": 5, + "name": "group1", + "path": "group1", + "description": "", + "visibility": "public", + "lfs_enabled": false, + "avatar_url": null, + "web_url": "http://localhost/groups/group1", + "request_access_enabled": false, + "full_name": "group1", + "full_path": "group1", + "parent_id": null, + "ldap_cn": null, + "ldap_access": null + } + ], "contains_hidden_groups": false } ``` @@ -486,6 +561,14 @@ GET /projects/:id/merge_requests/:merge_request_iid/approval_rules "state": "active", "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", "web_url": "http://localhost/jdoe" + }, + { + "id": 50, + "name": "Group Member 1", + "username": "group_member_1", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/group_member_1" } ], "approvals_required": 2, @@ -499,7 +582,24 @@ GET /projects/:id/merge_requests/:merge_request_iid/approval_rules "web_url": "http://localhost/jdoe" } ], - "groups": [], + "groups": [ + { + "id": 5, + "name": "group1", + "path": "group1", + "description": "", + "visibility": "public", + "lfs_enabled": false, + "avatar_url": null, + "web_url": "http://localhost/groups/group1", + "request_access_enabled": false, + "full_name": "group1", + "full_path": "group1", + "parent_id": null, + "ldap_cn": null, + "ldap_access": null + } + ], "contains_hidden_groups": false, "approved_by": [ { -- GitLab From 6037e713f58f3891f1e902f06e290f835f3bc2c5 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Fri, 16 Aug 2019 17:24:57 +0800 Subject: [PATCH 11/13] Revert changes to existing private APIs This also reverts the rename and changes to FE. --- .../modules/project_settings/actions.js | 4 +- .../mr_widget_options.vue | 2 +- .../services/mr_widget_service.js | 4 +- .../stores/mr_widget_store.js | 2 +- .../presenters/ee/merge_request_presenter.rb | 8 +- .../ee/merge_request_widget_entity.rb | 4 +- ..._request_approvals_settings_form.html.haml | 3 +- .../shared/issuable/_approvals.html.haml | 4 +- ee/lib/api/merge_request_approvals.rb | 5 +- ee/lib/api/project_approval_rules.rb | 92 ++++++++++--------- .../entities/merge_request_widget.json | 2 +- .../modules/project_settings/actions_spec.js | 10 +- .../ee_mr_widget_options_spec.js | 2 +- .../merge_request_presenter_spec.rb | 12 +-- .../api/merge_request_approvals_spec.rb | 8 +- .../api/project_approval_rules_spec.rb | 18 ++-- 16 files changed, 95 insertions(+), 85 deletions(-) diff --git a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js index 53a8e227e05a0f..79f9e7d93c2436 100644 --- a/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js +++ b/ee/app/assets/javascripts/approvals/stores/modules/project_settings/actions.js @@ -22,12 +22,12 @@ export const receiveRulesError = () => { }; export const fetchRules = ({ rootState, dispatch }) => { - const { rulesPath } = rootState.settings; + const { settingsPath } = rootState.settings; dispatch('requestRules'); return axios - .get(rulesPath) + .get(settingsPath) .then(response => dispatch('receiveRulesSuccess', mapApprovalSettingsResponse(response.data))) .catch(() => dispatch('receiveRulesError')); }; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue b/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue index 4db974ac052c78..58aadf35018ea4 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue +++ b/ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue @@ -162,7 +162,7 @@ export default { return { ...base, apiApprovalsPath: store.apiApprovalsPath, - apiApprovalRulesPath: store.apiApprovalRulesPath, + apiApprovalSettingsPath: store.apiApprovalSettingsPath, apiApprovePath: store.apiApprovePath, apiUnapprovePath: store.apiUnapprovePath, }; diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js b/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js index f0fe04a116bb19..eb1625cff550de 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js @@ -6,7 +6,7 @@ export default class MRWidgetService extends CEWidgetService { super(mr); this.apiApprovalsPath = mr.apiApprovalsPath; - this.apiApprovalRulesPath = mr.apiApprovalRulesPath; + this.apiApprovalSettingsPath = mr.apiApprovalSettingsPath; this.apiApprovePath = mr.apiApprovePath; this.apiUnapprovePath = mr.apiUnapprovePath; } @@ -16,7 +16,7 @@ export default class MRWidgetService extends CEWidgetService { } fetchApprovalSettings() { - return axios.get(this.apiApprovalRulesPath).then(res => res.data); + return axios.get(this.apiApprovalSettingsPath).then(res => res.data); } approveMergeRequest() { diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index cc31898b8ce687..32883b656ad058 100644 --- a/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -61,7 +61,7 @@ export default class MergeRequestStore extends CEMergeRequestStore { data.has_approvals_available || this.hasApprovalsAvailable, ); this.apiApprovalsPath = data.api_approvals_path || this.apiApprovalsPath; - this.apiApprovalRulesPath = data.api_approval_rules_path || this.apiApprovalRulesPath; + this.apiApprovalSettingsPath = data.api_approval_settings_path || this.apiApprovalSettingsPath; this.apiApprovePath = data.api_approve_path || this.apiApprovePath; this.apiUnapprovePath = data.api_unapprove_path || this.apiUnapprovePath; } diff --git a/ee/app/presenters/ee/merge_request_presenter.rb b/ee/app/presenters/ee/merge_request_presenter.rb index 550ea9c084b061..a377815e08b452 100644 --- a/ee/app/presenters/ee/merge_request_presenter.rb +++ b/ee/app/presenters/ee/merge_request_presenter.rb @@ -17,15 +17,15 @@ def api_approvals_path end end - def api_approval_rules_path + def api_approval_settings_path if expose_mr_approval_path? - expose_path(api_v4_projects_merge_requests_approval_rules_path(id: project.id, merge_request_iid: merge_request.iid)) + expose_path(api_v4_projects_merge_requests_approval_settings_path(id: project.id, merge_request_iid: merge_request.iid)) end end - def api_project_approval_rules_path + def api_project_approval_settings_path if approval_feature_available? - expose_path(api_v4_projects_approval_rules_path(id: project.id)) + expose_path(api_v4_projects_approval_settings_path(id: project.id)) end end diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb index 33e21b0a0dd740..cf5cae57af908d 100644 --- a/ee/app/serializers/ee/merge_request_widget_entity.rb +++ b/ee/app/serializers/ee/merge_request_widget_entity.rb @@ -152,8 +152,8 @@ module MergeRequestWidgetEntity expose :api_approvals_path do |merge_request| presenter(merge_request).api_approvals_path end - expose :api_approval_rules_path do |merge_request| - presenter(merge_request).api_approval_rules_path + 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 diff --git a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml index 5183326dcc982c..49179755d8ebc4 100644 --- a/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml +++ b/ee/app/views/projects/_merge_request_approvals_settings_form.html.haml @@ -5,7 +5,8 @@ = _("Add approvers") #js-mr-approvals-settings{ data: { 'project_id': @project.id, 'project_path': expose_path(api_v4_projects_path(id: @project.id)), - 'rules_path': expose_path(api_v4_projects_approval_rules_path(id: @project.id)), + 'settings_path': expose_path(api_v4_projects_approval_settings_path(id: @project.id)), + 'rules_path': expose_path(api_v4_projects_approval_settings_rules_path(id: @project.id)), 'allow_multi_rule': @project.multiple_approval_rules_available?.to_s } } .text-center.prepend-top-default = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') diff --git a/ee/app/views/shared/issuable/_approvals.html.haml b/ee/app/views/shared/issuable/_approvals.html.haml index a520a2c112419c..51d6fecd79e0b6 100644 --- a/ee/app/views/shared/issuable/_approvals.html.haml +++ b/ee/app/views/shared/issuable/_approvals.html.haml @@ -15,8 +15,8 @@ 'can_edit': can?(current_user, :update_approvers, issuable).to_s, 'allow_multi_rule': @target_project.multiple_approval_rules_available?.to_s, 'mr_id': issuable.iid, - 'mr_settings_path': presenter.api_approval_rules_path, - 'project_settings_path': presenter.api_project_approval_rules_path } } + 'mr_settings_path': presenter.api_approval_settings_path, + 'project_settings_path': presenter.api_project_approval_settings_path } } = sprite_icon('spinner', size: 24, css_class: 'gl-spinner') - if can_update_approvers - approver_presenter = MergeRequestApproverPresenter.new(issuable, skip_user: current_user) diff --git a/ee/lib/api/merge_request_approvals.rb b/ee/lib/api/merge_request_approvals.rb index cd7e89079bcc8d..2e825ef7da58de 100644 --- a/ee/lib/api/merge_request_approvals.rb +++ b/ee/lib/api/merge_request_approvals.rb @@ -50,9 +50,10 @@ def handle_merge_request_errors!(errors) end desc 'List approval rules for merge request', { - success: ::EE::API::Entities::MergeRequestApprovalRules + success: ::EE::API::Entities::MergeRequestApprovalRules, + hidden: true } - get 'approval_rules' do + get 'approval_settings' do 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 diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index 0cb7d63d8907c7..a40af59c796a29 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -10,8 +10,9 @@ class ProjectApprovalRules < ::Grape::API requires :id, type: String, desc: 'The ID of a project' end resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - segment ':id/approval_rules' 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 @@ -20,46 +21,22 @@ class ProjectApprovalRules < ::Grape::API present user_project, with: EE::API::Entities::ProjectApprovalRules, current_user: current_user end - desc 'Create new approval rule' do - 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 :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' - 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 + segment 'rules' do + desc 'Create new 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' + 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 :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 :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end - put do + post 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 + 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 @@ -68,18 +45,47 @@ class ProjectApprovalRules < ::Grape::API end end - desc 'Delete an approval rule' do - end - params do - requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule' - end - delete do - authorize! :admin_project, user_project + 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' + optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' + 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]) + approval_rule = user_project.approval_rules.find(params[:approval_rule_id]) - destroy_conditionally!(approval_rule) do |rule| - ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute + destroy_conditionally!(approval_rule) do |rule| + ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute + end end end end diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json b/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json index 7d493400936a7a..fe990128c3f74e 100644 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -9,7 +9,7 @@ "approved": { "type": "boolean" }, "approvals_path": { "type": ["string", "null"] }, "api_approvals_path": { "type": ["string", "null"] }, - "api_approval_rules_path": { "type": ["string", "null"] }, + "api_approval_settings_path": { "type": ["string", "null"] }, "api_approve_path": { "type": ["string", "null"] }, "api_unapprove_path": { "type": ["string", "null"] }, "codeclimate": { diff --git a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js index d112cfb282442c..9c69f1f91b4844 100644 --- a/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js +++ b/ee/spec/javascripts/approvals/stores/modules/project_settings/actions_spec.js @@ -21,7 +21,8 @@ const TEST_RULE_RESPONSE = { groups: [{ id: 4 }], users: [{ id: 7 }, { id: 8 }], }; -const TEST_RULES_PATH = 'projects/9/approval_rules'; +const TEST_SETTINGS_PATH = 'projects/9/approval_settings'; +const TEST_RULES_PATH = 'projects/9/approval_settings/rules'; describe('EE approvals project settings module actions', () => { let state; @@ -32,6 +33,7 @@ describe('EE approvals project settings module actions', () => { state = { settings: { projectId: TEST_PROJECT_ID, + settingsPath: TEST_SETTINGS_PATH, rulesPath: TEST_RULES_PATH, }, }; @@ -88,7 +90,7 @@ describe('EE approvals project settings module actions', () => { describe('fetchRules', () => { it('dispatches request/receive', done => { const data = { rules: [TEST_RULE_RESPONSE] }; - mock.onGet(TEST_RULES_PATH).replyOnce(200, data); + mock.onGet(TEST_SETTINGS_PATH).replyOnce(200, data); testAction( actions.fetchRules, @@ -100,7 +102,7 @@ describe('EE approvals project settings module actions', () => { { type: 'receiveRulesSuccess', payload: mapApprovalSettingsResponse(data) }, ], () => { - expect(mock.history.get.map(x => x.url)).toEqual([TEST_RULES_PATH]); + expect(mock.history.get.map(x => x.url)).toEqual([TEST_SETTINGS_PATH]); done(); }, @@ -108,7 +110,7 @@ describe('EE approvals project settings module actions', () => { }); it('dispatches request/receive on error', done => { - mock.onGet(TEST_RULES_PATH).replyOnce(500); + mock.onGet(TEST_SETTINGS_PATH).replyOnce(500); testAction( actions.fetchRules, diff --git a/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js b/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js index a54462302d2225..92257ece4a4258 100644 --- a/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js +++ b/ee/spec/javascripts/vue_mr_widget/ee_mr_widget_options_spec.js @@ -1017,7 +1017,7 @@ describe('ee merge request widget options', () => { it('passes approval api paths to service', () => { const paths = { api_approvals_path: `${TEST_HOST}/api/approvals/path`, - api_approval_rules_path: `${TEST_HOST}/api/approval/rules/path`, + api_approval_settings_path: `${TEST_HOST}/api/approval/settings/path`, api_approve_path: `${TEST_HOST}/api/approve/path`, api_unapprove_path: `${TEST_HOST}/api/unapprove/path`, }; diff --git a/ee/spec/presenters/merge_request_presenter_spec.rb b/ee/spec/presenters/merge_request_presenter_spec.rb index de5d122fdb643f..a931a755b6e219 100644 --- a/ee/spec/presenters/merge_request_presenter_spec.rb +++ b/ee/spec/presenters/merge_request_presenter_spec.rb @@ -45,18 +45,18 @@ it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approvals")) } end - describe '#api_approval_rules_path' do - subject { described_class.new(merge_request, current_user: user).api_approval_rules_path } + describe '#api_approval_settings_path' do + subject { described_class.new(merge_request, current_user: user).api_approval_settings_path } it_behaves_like 'is nil when needed' - it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approval_rules")) } + it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/merge_requests/#{merge_request.iid}/approval_settings")) } end - describe '#api_project_approval_rules_path' do - subject { described_class.new(merge_request, current_user: user).api_project_approval_rules_path } + describe '#api_project_approval_settings_path' do + subject { described_class.new(merge_request, current_user: user).api_project_approval_settings_path } - it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/approval_rules")) } + it { is_expected.to eq(expose_path("/api/v4/projects/#{merge_request.project.id}/approval_settings")) } context "when approvals not available" do let(:approval_feature_available) { false } diff --git a/ee/spec/requests/api/merge_request_approvals_spec.rb b/ee/spec/requests/api/merge_request_approvals_spec.rb index 1f951d320e1597..65b63d48b749c1 100644 --- a/ee/spec/requests/api/merge_request_approvals_spec.rb +++ b/ee/spec/requests/api/merge_request_approvals_spec.rb @@ -95,7 +95,7 @@ end end - describe 'GET :id/merge_requests/:merge_request_iid/approval_rules' do + describe 'GET :id/merge_requests/:merge_request_iid/approval_settings' do let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 2, name: 'foo') } it 'retrieves the approval rules details' do @@ -103,7 +103,7 @@ merge_request.approvals.create(user: approver) rule.users << approver - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) @@ -127,7 +127,7 @@ rule.users << approver rule.create_approval_merge_request_rule_source!(approval_project_rule: source_rule) - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) @@ -144,7 +144,7 @@ rule.users << approver rule.groups << private_group - get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_rules", user) + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approval_settings", user) expect(response).to have_gitlab_http_status(200) expect(json_response['rules'].size).to eq(1) diff --git a/ee/spec/requests/api/project_approval_rules_spec.rb b/ee/spec/requests/api/project_approval_rules_spec.rb index 8d2a7a9820bd34..5c86db2948d5da 100644 --- a/ee/spec/requests/api/project_approval_rules_spec.rb +++ b/ee/spec/requests/api/project_approval_rules_spec.rb @@ -10,10 +10,10 @@ 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_rules" } + let(:url) { "/projects/#{project.id}/approval_settings/rules" } - describe 'GET /projects/:id/approval_rules' do - let(:url) { "/projects/#{project.id}/approval_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 @@ -89,7 +89,7 @@ end end - describe 'POST /projects/:id/approval_rules' do + describe 'POST /projects/:id/approval_settings/rules' do let(:current_user) { user } let(:params) do { @@ -144,9 +144,9 @@ end end - describe 'PUT /projects/:id/approval_rules/:approval_rule_id' do + 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_rules/#{approval_rule.id}" } + let(:url) { "/projects/#{project.id}/approval_settings/rules/#{approval_rule.id}" } shared_examples_for 'a user with access' do before do @@ -210,9 +210,9 @@ end end - describe 'DELETE /projects/:id/approval_rules/:approval_rule_id' do + 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_rules/#{approval_rule.id}" } + let(:url) { "/projects/#{project.id}/approval_settings/rules/#{approval_rule.id}" } it 'destroys' do delete api(url, user) @@ -223,7 +223,7 @@ context 'when approval rule not found' do let!(:approval_rule_2) { create(:approval_project_rule) } - let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule_2.id}" } + let(:url) { "/projects/#{project.id}/approval_settings/#{approval_rule_2.id}" } it 'returns not found' do delete api(url, user) -- GitLab From c0b4a9711e3f24a0698e44c6c47b3948cc7f372a Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Fri, 16 Aug 2019 19:28:18 +0800 Subject: [PATCH 12/13] Add new public API endpoints for project rules Rename approvers to eligible_approvers in public API --- .../helpers/project_approval_rules_helpers.rb | 72 +++++++ ee/lib/api/project_approval_rules.rb | 92 +++------ ee/lib/api/project_approval_settings.rb | 62 ++++++ ee/lib/ee/api/endpoints.rb | 1 + ee/lib/ee/api/entities.rb | 12 +- .../public_api/v4/project_approval_rule.json | 2 +- .../public_api/v4/project_approval_rules.json | 11 +- .../v4/project_approval_setting.json | 32 ++++ .../v4/project_approval_settings.json | 11 ++ .../api/project_approval_rules_spec.rb | 180 +++--------------- .../api/project_approval_settings_spec.rb | 111 +++++++++++ ...ject_approval_rules_api_shared_examples.rb | 147 ++++++++++++++ 12 files changed, 497 insertions(+), 236 deletions(-) create mode 100644 ee/lib/api/helpers/project_approval_rules_helpers.rb create mode 100644 ee/lib/api/project_approval_settings.rb create mode 100644 ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json create mode 100644 ee/spec/fixtures/api/schemas/public_api/v4/project_approval_settings.json create mode 100644 ee/spec/requests/api/project_approval_settings_spec.rb create mode 100644 ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb new file mode 100644 index 00000000000000..0822a7e0a9f400 --- /dev/null +++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +module API + module Helpers + module ProjectApprovalRulesHelpers + extend Grape::API::Helpers + + ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } + + params :create_project_approval_rule 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 :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' + end + + params :update_project_approval_rule 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' + optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' + end + + params :delete_project_approval_rule do + requires :approval_rule_id, type: Integer, desc: 'The ID of an approval_rule' + end + + def authorize_create_merge_request_in_project + authorize! :create_merge_request_in, user_project + end + + def create_project_approval_rule(present_with:) + 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: present_with, current_user: current_user + else + render_api_error!(result[:message], 400) + end + end + + def update_project_approval_rule(present_with:) + 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: present_with, current_user: current_user + else + render_api_error!(result[:message], 400) + end + end + + def destroy_project_approval_rule + authorize! :admin_project, user_project + + approval_rule = user_project.approval_rules.find(params[:approval_rule_id]) + + destroy_conditionally!(approval_rule) do |rule| + ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute + end + end + end + end +end diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index a40af59c796a29..a251e568e98a30 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -4,89 +4,49 @@ module API class ProjectApprovalRules < ::Grape::API before { authenticate! } - ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) } + helpers ::API::Helpers::ProjectApprovalRulesHelpers 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 + segment ':id/approval_rules' do desc 'Get all project approval rules' do - detail 'Private API subject to change' - success EE::API::Entities::ProjectApprovalRules + success EE::API::Entities::ApprovalRule end get do - authorize! :create_merge_request_in, user_project + authorize_create_merge_request_in_project - present user_project, with: EE::API::Entities::ProjectApprovalRules, current_user: current_user + present user_project.visible_approval_rules, with: EE::API::Entities::ApprovalRule, current_user: current_user end - segment 'rules' do - desc 'Create new approval rule' do - detail 'Private API subject to change' + desc 'Create new project approval rule' do + success EE::API::Entities::ApprovalRule + end + params do + use :create_project_approval_rule + end + post do + create_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) + end + + segment ':approval_rule_id' do + desc 'Update project approval rule' do 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 :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' + use :update_project_approval_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 + put do + update_project_approval_rule(present_with: EE::API::Entities::ApprovalRule) 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' - optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' - 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) do |rule| - ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute - end - end + desc 'Destroy project approval rule' + params do + use :delete_project_approval_rule + end + delete do + destroy_project_approval_rule end end end diff --git a/ee/lib/api/project_approval_settings.rb b/ee/lib/api/project_approval_settings.rb new file mode 100644 index 00000000000000..1a2afe02b9ecdb --- /dev/null +++ b/ee/lib/api/project_approval_settings.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module API + class ProjectApprovalSettings < ::Grape::API + before { authenticate! } + + helpers ::API::Helpers::ProjectApprovalRulesHelpers + + 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::ProjectApprovalSettings + end + get do + authorize_create_merge_request_in_project + + present user_project, with: EE::API::Entities::ProjectApprovalSettings, current_user: current_user + end + + segment 'rules' do + desc 'Create new approval rule' do + detail 'Private API subject to change' + success EE::API::Entities::ApprovalSettingRule + end + params do + use :create_project_approval_rule + end + post do + create_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) + end + + segment ':approval_rule_id' do + desc 'Update approval rule' do + detail 'Private API subject to change' + success EE::API::Entities::ApprovalSettingRule + end + params do + use :update_project_approval_rule + end + put do + update_project_approval_rule(present_with: EE::API::Entities::ApprovalSettingRule) + end + + desc 'Delete an approval rule' do + detail 'Private API subject to change' + end + params do + use :delete_project_approval_rule + end + delete do + destroy_project_approval_rule + end + end + end + end + end + end +end diff --git a/ee/lib/ee/api/endpoints.rb b/ee/lib/ee/api/endpoints.rb index ac724008185c16..62c032c61b2f16 100644 --- a/ee/lib/ee/api/endpoints.rb +++ b/ee/lib/ee/api/endpoints.rb @@ -10,6 +10,7 @@ module Endpoints mount ::EE::API::GroupBoards mount ::API::ProjectApprovalRules + mount ::API::ProjectApprovalSettings mount ::API::Unleash mount ::API::EpicIssues mount ::API::EpicLinks diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 0af462a241763e..c48ffdd9bd266c 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -309,7 +309,7 @@ class ApprovalRuleShort < Grape::Entity expose :id, :name, :rule_type end - class ApprovalRule < ApprovalRuleShort + class ApprovalSettingRule < ApprovalRuleShort def initialize(object, options = {}) presenter = ::ApprovalRulePresenter.new(object, current_user: options[:current_user]) super(presenter, options) @@ -322,7 +322,11 @@ def initialize(object, options = {}) expose :contains_hidden_groups?, as: :contains_hidden_groups end - class MergeRequestApprovalRule < ApprovalRule + class ApprovalRule < ApprovalSettingRule + expose :approvers, as: :eligible_approvers, using: ::API::Entities::UserBasic, override: true + end + + class MergeRequestApprovalRule < ApprovalSettingRule class SourceRule < Grape::Entity expose :approvals_required end @@ -343,8 +347,8 @@ class MergeRequestApprovalRules < Grape::Entity end # Decorates Project - class ProjectApprovalRules < Grape::Entity - expose :visible_approval_rules, as: :rules, using: ApprovalRule + class ProjectApprovalSettings < Grape::Entity + expose :visible_approval_rules, as: :rules, using: ApprovalSettingRule 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 226c692a2f3ee0..d27a218bf41556 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 @@ -6,7 +6,7 @@ "approvals_required": { "type": "integer" }, "contains_hidden_groups": { "type": "boolean" }, "rule_type": { "type": "tring" }, - "approvers": { + "eligible_approvers": { "type": "array", "items": { "type": "object", diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rules.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rules.json index 3753f8cf2700c7..4460d376ea5d67 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rules.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rules.json @@ -1,11 +1,4 @@ { - "type": "object", - "properties": { - "rules": { - "type": "array", - "items": { "$ref": "project_approval_rule.json" } - }, - "fallback_approvals_required": { "type": "integer" } - }, - "additionalProperties": false + "type": "array", + "items": { "$ref": "./project_approval_rule.json" } } 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 new file mode 100644 index 00000000000000..226c692a2f3ee0 --- /dev/null +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_setting.json @@ -0,0 +1,32 @@ +{ + "type": "object", + "properties": { + "id": { "type": "integer" }, + "name": { "type": "string" }, + "approvals_required": { "type": "integer" }, + "contains_hidden_groups": { "type": "boolean" }, + "rule_type": { "type": "tring" }, + "approvers": { + "type": "array", + "items": { + "type": "object", + "properties": {} + } + }, + "groups": { + "type": "array", + "items": { + "type": "object", + "properties": {} + } + }, + "users": { + "type": "array", + "items": { + "type": "object", + "properties": {} + } + } + }, + "additionalProperties": false +} diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_settings.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_settings.json new file mode 100644 index 00000000000000..5f37fe7289ad18 --- /dev/null +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_settings.json @@ -0,0 +1,11 @@ +{ + "type": "object", + "properties": { + "rules": { + "type": "array", + "items": { "$ref": "project_approval_setting.json" } + }, + "fallback_approvals_required": { "type": "integer" } + }, + "additionalProperties": false +} diff --git a/ee/spec/requests/api/project_approval_rules_spec.rb b/ee/spec/requests/api/project_approval_rules_spec.rb index 5c86db2948d5da..6d74931a18c492 100644 --- a/ee/spec/requests/api/project_approval_rules_spec.rb +++ b/ee/spec/requests/api/project_approval_rules_spec.rb @@ -3,17 +3,15 @@ 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(: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" } + describe 'GET /projects/:id/approval_rules' do + let(:url) { "/projects/#{project.id}/approval_rules" } context 'when the request is correct' do let!(:rule) do @@ -36,9 +34,9 @@ json = json_response - expect(json['rules'].size).to eq(1) + expect(json.size).to eq(1) - rule = json['rules'].first + rule = json.first expect(rule['approvals_required']).to eq(7) expect(rule['name']).to eq('security') @@ -55,7 +53,7 @@ get api(url, developer) json = json_response - rule = json['rules'].first + rule = json.first expect(rule['groups'].size).to eq(0) end @@ -66,7 +64,7 @@ get api(url, developer) json = json_response - rule = json['rules'].first + rule = json.first expect(rule['groups'].size).to eq(1) end @@ -82,162 +80,32 @@ json = json_response - expect(json['rules'].size).to eq(2) - expect(json['rules'].map { |rule| rule['name'] }).to contain_exactly(rule.name, report_approver_rule.name) + expect(json.size).to eq(2) + expect(json.map { |rule| rule['name'] }).to contain_exactly(rule.name, report_approver_rule.name) 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: 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: 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) + describe 'POST /projects/:id/approval_rules' do + let(:schema) { 'public_api/v4/project_approval_rule' } + let(:url) { "/projects/#{project.id}/approval_rules" } - project.reset_approvals_on_push = false - project.disable_overriding_approvers_per_merge_request = true - project.save - - post api(url, current_user), params: params - - expect(json_response.symbolize_keys).to include(params) - end - - it 'sets rule_type as report_approver if name matches default name for security reports' do - expect do - post api(url, current_user), params: params.merge(name: ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT) - end.to change { ApprovalProjectRule.report_approver.count }.from(0).to(1) - - expect(response).to have_gitlab_http_status(201) - end - end + it_behaves_like 'an API endpoint for creating project approval rule' end - describe 'PUT /projects/:id/approval_settings/:approval_rule_id' do + describe 'PUT /projects/:id/approval_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}" } - - 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) + let(:schema) { 'public_api/v4/project_approval_rule' } + let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule.id}" } - expect do - put api(url, user2), params: { users: [], groups: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' } - end.not_to change { approval_rule.approvers.size } - - expect(response).to have_gitlab_http_status(403) - end - end + it_behaves_like 'an API endpoint for updating project approval rule' end - describe 'DELETE /projects/:id/approval_settings/rules/:approval_rule_id' do + describe 'DELETE /projects/:id/approval_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}" } + let(:url) { "/projects/#{project.id}/approval_rules/#{approval_rule.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 + it_behaves_like 'an API endpoint for deleting project approval rule' end end diff --git a/ee/spec/requests/api/project_approval_settings_spec.rb b/ee/spec/requests/api/project_approval_settings_spec.rb new file mode 100644 index 00000000000000..dc103769a4f708 --- /dev/null +++ b/ee/spec/requests/api/project_approval_settings_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::ProjectApprovalSettings 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) } + + 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_settings', 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 + + context 'report_approver rules' do + let!(:report_approver_rule) do + create(:approval_project_rule, :security_report, project: project) + end + + it 'includes report_approver rules' do + get api(url, developer) + + json = json_response + + expect(json['rules'].size).to eq(2) + expect(json['rules'].map { |rule| rule['name'] }).to contain_exactly(rule.name, report_approver_rule.name) + end + end + end + end + + describe 'POST /projects/:id/approval_settings/rules' do + let(:schema) { 'public_api/v4/project_approval_setting' } + let(:url) { "/projects/#{project.id}/approval_settings/rules" } + + it_behaves_like 'an API endpoint for creating project approval rule' + end + + describe 'PUT /projects/:id/approval_settings/:approval_rule_id' do + let!(:approval_rule) { create(:approval_project_rule, project: project) } + let(:schema) { 'public_api/v4/project_approval_setting' } + let(:url) { "/projects/#{project.id}/approval_settings/rules/#{approval_rule.id}" } + + it_behaves_like 'an API endpoint for updating project approval rule' + 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_behaves_like 'an API endpoint for deleting project approval rule' + end +end 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 new file mode 100644 index 00000000000000..b5f57f5d6aa12d --- /dev/null +++ b/ee/spec/support/shared_examples/project_approval_rules_api_shared_examples.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +shared_examples_for 'an API endpoint for creating project approval rule' 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: 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: params + + expect(response).to have_gitlab_http_status(201) + expect(response).to match_response_schema(schema, 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: params + + expect(json_response.symbolize_keys).to include(params) + end + + it 'sets rule_type as report_approver if name matches default name for security reports' do + expect do + post api(url, current_user), params: params.merge(name: ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT) + end.to change { ApprovalProjectRule.report_approver.count }.from(0).to(1) + + expect(response).to have_gitlab_http_status(201) + end + end +end + +shared_examples_for 'an API endpoint for updating project approval rule' do + 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) + expect(response).to match_response_schema(schema, dir: 'ee') + 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) + 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), params: { users: [], groups: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' } + end.not_to change { approval_rule.approvers.size } + + expect(response).to have_gitlab_http_status(403) + end + end +end + +shared_examples_for 'an API endpoint for deleting project approval rule' do + 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 -- GitLab From e9f306810d21b222ab72da5b8312123cd510da15 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Sun, 18 Aug 2019 11:18:50 +0800 Subject: [PATCH 13/13] Doc: Update with new version of public API --- doc/api/merge_request_approvals.md | 214 ++++++++--------------------- 1 file changed, 61 insertions(+), 153 deletions(-) diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md index 8dfc9cf69d3ae5..bf83bfd7e6ace1 100644 --- a/doc/api/merge_request_approvals.md +++ b/doc/api/merge_request_approvals.md @@ -63,7 +63,7 @@ POST /projects/:id/approvals ### Get project-level rules ->**Note:** This API endpoint is only available on 12.2 Starter and above. +>**Note:** This API endpoint is only available on 12.3 Starter and above. You can request information about a project's approval rules using the following endpoint: @@ -78,69 +78,66 @@ GET /projects/:id/approval_rules | `id` | integer | yes | The ID of a project | ```json -{ - "rules": [ - { - "id": 1, - "name": "security", - "rule_type": "regular", - "approvers": [ - { - "id": 5, - "name": "John Doe", - "username": "jdoe", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/jdoe" - }, - { - "id": 50, - "name": "Group Member 1", - "username": "group_member_1", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/group_member_1" - } - ], - "approvals_required": 3, - "users": [ - { - "id": 5, - "name": "John Doe", - "username": "jdoe", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/jdoe" - } - ], - "groups": [ - { - "id": 5, - "name": "group1", - "path": "group1", - "description": "", - "visibility": "public", - "lfs_enabled": false, - "avatar_url": null, - "web_url": "http://localhost/groups/group1", - "request_access_enabled": false, - "full_name": "group1", - "full_path": "group1", - "parent_id": null, - "ldap_cn": null, - "ldap_access": null - } - ], - "contains_hidden_groups": false - } - ], - "fallback_approvals_required": 3 -} +[ + { + "id": 1, + "name": "security", + "rule_type": "regular", + "eligible_approvers": [ + { + "id": 5, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + }, + { + "id": 50, + "name": "Group Member 1", + "username": "group_member_1", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/group_member_1" + } + ], + "approvals_required": 3, + "users": [ + { + "id": 5, + "name": "John Doe", + "username": "jdoe", + "state": "active", + "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", + "web_url": "http://localhost/jdoe" + } + ], + "groups": [ + { + "id": 5, + "name": "group1", + "path": "group1", + "description": "", + "visibility": "public", + "lfs_enabled": false, + "avatar_url": null, + "web_url": "http://localhost/groups/group1", + "request_access_enabled": false, + "full_name": "group1", + "full_path": "group1", + "parent_id": null, + "ldap_cn": null, + "ldap_access": null + } + ], + "contains_hidden_groups": false + } +] ``` ### Create project-level rule ->**Note:** This API endpoint is only available on 12.2 Starter and above. +>**Note:** This API endpoint is only available on 12.3 Starter and above. You can create project approval rules using the following endpoint: @@ -163,7 +160,7 @@ POST /projects/:id/approval_rules "id": 1, "name": "security", "rule_type": "regular", - "approvers": [ + "eligible_approvers": [ { "id": 2, "name": "John Doe", @@ -216,7 +213,7 @@ POST /projects/:id/approval_rules ### Update project-level rule ->**Note:** This API endpoint is only available on 12.2 Starter and above. +>**Note:** This API endpoint is only available on 12.3 Starter and above. You can update project approval rules using the following endpoint: @@ -242,7 +239,7 @@ PUT /projects/:id/approval_rules/:approval_rule_id "id": 1, "name": "security", "rule_type": "regular", - "approvers": [ + "eligible_approvers": [ { "id": 2, "name": "John Doe", @@ -295,7 +292,7 @@ PUT /projects/:id/approval_rules/:approval_rule_id ### Delete project-level rule ->**Note:** This API endpoint is only available on 12.2 Starter and above. +>**Note:** This API endpoint is only available on 12.3 Starter and above. You can delete project approval rules using the following endpoint: @@ -528,95 +525,6 @@ PUT /projects/:id/merge_requests/:merge_request_iid/approvers } ``` -### Get approval rules for Merge Request - ->**Note:** This API endpoint is only available on 12.2 Starter and above. - -You can request information about a merge request's approval rules using the following endpoint: - -``` -GET /projects/:id/merge_requests/:merge_request_iid/approval_rules -``` - -**Parameters:** - -| Attribute | Type | Required | Description | -|----------------------|---------|----------|-----------------------------------------------------------| -| `id` | integer | yes | The ID of a project | -| `merge_request_iid` | integer | yes | The IID of MR | - -```json -{ - "approval_rules_overwritten": true, - "rules": [ - { - "id": 1, - "name": "Ruby", - "rule_type": "regular", - "approvers": [ - { - "id": 4, - "name": "John Doe", - "username": "jdoe", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/jdoe" - }, - { - "id": 50, - "name": "Group Member 1", - "username": "group_member_1", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/group_member_1" - } - ], - "approvals_required": 2, - "users": [ - { - "id": 4, - "name": "John Doe", - "username": "jdoe", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/jdoe" - } - ], - "groups": [ - { - "id": 5, - "name": "group1", - "path": "group1", - "description": "", - "visibility": "public", - "lfs_enabled": false, - "avatar_url": null, - "web_url": "http://localhost/groups/group1", - "request_access_enabled": false, - "full_name": "group1", - "full_path": "group1", - "parent_id": null, - "ldap_cn": null, - "ldap_access": null - } - ], - "contains_hidden_groups": false, - "approved_by": [ - { - "id": 4, - "name": "John Doe", - "username": "jdoe", - "state": "active", - "avatar_url": "https://www.gravatar.com/avatar/0?s=80&d=identicon", - "web_url": "http://localhost/jdoe" - } - ], - "source_rule": null, - "approved": true - } - ] -} -``` ## Approve Merge Request >**Note:** This API endpoint is only available on 8.9 Starter and above. -- GitLab