From f1e24d4d31776f675cd4a7cdc21ddc9d496400cf Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Fri, 23 Aug 2019 17:45:42 +0000 Subject: Add label_id parameter to label API for PUT and DELETE Add specs for new parameter and updated documentation as well. --- ...-add-optional-id-to-label-api-put-delete-pd.yml | 5 + doc/api/labels.md | 8 +- lib/api/helpers/label_helpers.rb | 32 +- lib/api/labels.rb | 8 +- spec/lib/api/helpers/label_helpers_spec.rb | 33 ++ spec/requests/api/labels_spec.rb | 336 +++++++++++++-------- 6 files changed, 285 insertions(+), 137 deletions(-) create mode 100644 changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml create mode 100644 spec/lib/api/helpers/label_helpers_spec.rb diff --git a/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml b/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml new file mode 100644 index 00000000000..7e1eeddef32 --- /dev/null +++ b/changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml @@ -0,0 +1,5 @@ +--- +title: Add optional label_id parameter to label API for PUT and DELETE +merge_request: 31804 +author: +type: changed diff --git a/doc/api/labels.md b/doc/api/labels.md index 5db0edcf14d..fde1d861cf6 100644 --- a/doc/api/labels.md +++ b/doc/api/labels.md @@ -137,8 +137,9 @@ DELETE /projects/:id/labels | Attribute | Type | Required | Description | | --------- | ------- | -------- | --------------------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `name` | string | yes | The name of the label | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `label_id` | integer | yes (or `name`) | The id of the existing label | +| `name` | string | yes (or `label_id`) | The name of the existing label | ```bash curl --request DELETE --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/1/labels?name=bug" @@ -156,7 +157,8 @@ PUT /projects/:id/labels | Attribute | Type | Required | Description | | --------------- | ------- | --------------------------------- | ------------------------------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `name` | string | yes | The name of the existing label | +| `label_id` | integer | yes (or `name`) | The id of the existing label | +| `name` | string | yes (or `label_id`) | The name of the existing label | | `new_name` | string | yes if `color` is not provided | The new name of the label | | `color` | string | yes if `new_name` is not provided | The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the [CSS color names](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Color_keywords) | | `description` | string | no | The new description of the label | diff --git a/lib/api/helpers/label_helpers.rb b/lib/api/helpers/label_helpers.rb index 896b0aba52b..ec5b688dd1c 100644 --- a/lib/api/helpers/label_helpers.rb +++ b/lib/api/helpers/label_helpers.rb @@ -11,9 +11,9 @@ module API optional :description, type: String, desc: 'The description of label to be created' end - def find_label(parent, id, include_ancestor_groups: true) + def find_label(parent, id_or_title, include_ancestor_groups: true) labels = available_labels_for(parent, include_ancestor_groups: include_ancestor_groups) - label = labels.find_by_id(id) || labels.find_by_title(id) + label = labels.find_by_id(id_or_title) || labels.find_by_title(id_or_title) label || not_found!('Label') end @@ -35,12 +35,7 @@ module API priority = params.delete(:priority) label_params = declared_params(include_missing: false) - label = - if parent.is_a?(Project) - ::Labels::CreateService.new(label_params).execute(project: parent) - else - ::Labels::CreateService.new(label_params).execute(group: parent) - end + label = ::Labels::CreateService.new(label_params).execute(create_service_params(parent)) if label.persisted? if parent.is_a?(Project) @@ -56,10 +51,13 @@ module API def update_label(parent, entity) authorize! :admin_label, parent - label = find_label(parent, params[:name], include_ancestor_groups: false) + label = find_label(parent, params_id_or_title, include_ancestor_groups: false) update_priority = params.key?(:priority) priority = params.delete(:priority) + # params is used to update the label so we need to remove this field here + params.delete(:label_id) + label = ::Labels::UpdateService.new(declared_params(include_missing: false)).execute(label) render_validation_error!(label) unless label.valid? @@ -77,10 +75,24 @@ module API def delete_label(parent) authorize! :admin_label, parent - label = find_label(parent, params[:name], include_ancestor_groups: false) + label = find_label(parent, params_id_or_title, include_ancestor_groups: false) destroy_conditionally!(label) end + + def params_id_or_title + @params_id_or_title ||= params[:label_id] || params[:name] + end + + def create_service_params(parent) + if parent.is_a?(Project) + { project: parent } + elsif parent.is_a?(Group) + { group: parent } + else + raise TypeError, 'Parent type is not supported' + end + end end end end diff --git a/lib/api/labels.rb b/lib/api/labels.rb index c183198d3c6..83d645ca07a 100644 --- a/lib/api/labels.rb +++ b/lib/api/labels.rb @@ -38,11 +38,13 @@ module API success Entities::ProjectLabel end params do - requires :name, type: String, desc: 'The name of the label to be updated' + optional :label_id, type: Integer, desc: 'The id of the label to be updated' + optional :name, type: String, desc: 'The name of the label to be updated' optional :new_name, type: String, desc: 'The new name of the label' optional :color, type: String, desc: "The new color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the allowed CSS color names" optional :description, type: String, desc: 'The new description of label' optional :priority, type: Integer, desc: 'The priority of the label', allow_blank: true + exactly_one_of :label_id, :name at_least_one_of :new_name, :color, :description, :priority end put ':id/labels' do @@ -53,7 +55,9 @@ module API success Entities::ProjectLabel end params do - requires :name, type: String, desc: 'The name of the label to be deleted' + optional :label_id, type: Integer, desc: 'The id of the label to be deleted' + optional :name, type: String, desc: 'The name of the label to be deleted' + exactly_one_of :label_id, :name end delete ':id/labels' do delete_label(user_project) diff --git a/spec/lib/api/helpers/label_helpers_spec.rb b/spec/lib/api/helpers/label_helpers_spec.rb new file mode 100644 index 00000000000..138e9a22d70 --- /dev/null +++ b/spec/lib/api/helpers/label_helpers_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Helpers::LabelHelpers do + describe 'create_service_params' do + let(:label_helper) do + Class.new do + include API::Helpers::LabelHelpers + end.new + end + + context 'when a project is given' do + it 'returns the expected params' do + project = create(:project) + expect(label_helper.create_service_params(project)).to eq({ project: project }) + end + end + + context 'when a group is given' do + it 'returns the expected params' do + group = create(:group) + expect(label_helper.create_service_params(group)).to eq({ group: group }) + end + end + + context 'when something else is given' do + it 'raises a type error' do + expect { label_helper.create_service_params(Class.new) }.to raise_error(TypeError) + end + end + end +end diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index ad0974f55a3..f6640fe41d0 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -6,6 +6,180 @@ describe API::Labels do let!(:label1) { create(:label, title: 'label1', project: project) } let!(:priority_label) { create(:label, title: 'bug', project: project, priority: 3) } + shared_examples 'label update API' do + it 'returns 200 if name is changed' do + request_params = { + new_name: 'New Label' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq('New Label') + expect(json_response['color']).to eq(label1.color) + end + + it 'returns 200 if colors is changed' do + request_params = { + color: '#FFFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq(label1.name) + expect(json_response['color']).to eq('#FFFFFF') + end + + it 'returns 200 if a priority is added' do + request_params = { + priority: 3 + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['name']).to eq(label1.name) + expect(json_response['priority']).to eq(3) + end + + it 'returns 400 if no new parameters given' do + put api("/projects/#{project.id}/labels", user), params: spec_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\ + 'at least one parameter must be provided') + end + + it 'returns 400 when color code is too short' do + request_params = { + color: '#FF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['color']).to eq(['must be a valid color code']) + end + + it 'returns 400 for too long color code' do + request_params = { + color: '#FFAAFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['color']).to eq(['must be a valid color code']) + end + + it 'returns 400 for invalid priority' do + request_params = { + priority: 'foo' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns 200 if name and colors and description are changed' do + request_params = { + new_name: 'New Label', + color: '#FFFFFF', + description: 'test' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['name']).to eq('New Label') + expect(json_response['color']).to eq('#FFFFFF') + expect(json_response['description']).to eq('test') + end + + it 'returns 400 for invalid name' do + request_params = { + new_name: ',', + color: '#FFFFFF' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']['title']).to eq(['is invalid']) + end + + it 'returns 200 if description is changed' do + request_params = { + description: 'test' + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response).to have_gitlab_http_status(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['description']).to eq('test') + end + + it 'returns 200 if priority is changed' do + request_params = { + priority: 10 + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['priority']).to eq(10) + end + + it 'returns 200 if a priority is removed' do + label = find_by_spec_params(spec_params) + expect(label).not_to be_nil + + label.priorities.create(project: label.project, priority: 1) + label.save! + + request_params = { + priority: nil + }.merge(spec_params) + + put api("/projects/#{project.id}/labels", user), + params: request_params + + expect(response.status).to eq(200) + expect(json_response['id']).to eq(expected_response_label_id) + expect(json_response['priority']).to be_nil + end + + def find_by_spec_params(params) + if params.key?(:label_id) + Label.find(params[:label_id]) + else + Label.find_by(name: params[:name]) + end + end + end + + shared_examples 'label delete API' do + it 'returns 204 for existing label' do + delete api("/projects/#{project.id}/labels", user), params: spec_params + + expect(response).to have_gitlab_http_status(204) + end + end + before do project.add_maintainer(user) end @@ -208,174 +382,92 @@ describe API::Labels do end describe 'DELETE /projects/:id/labels' do - it 'returns 204 for existing label' do - delete api("/projects/#{project.id}/labels", user), params: { name: 'label1' } + it_behaves_like 'label delete API' do + let(:spec_params) { { name: 'label1' } } + end - expect(response).to have_gitlab_http_status(204) + it_behaves_like 'label delete API' do + let(:spec_params) { { label_id: label1.id } } end it 'returns 404 for non existing label' do delete api("/projects/#{project.id}/labels", user), params: { name: 'label2' } + expect(response).to have_gitlab_http_status(404) expect(json_response['message']).to eq('404 Label Not Found') end it 'returns 400 for wrong parameters' do delete api("/projects/#{project.id}/labels", user) - expect(response).to have_gitlab_http_status(400) - end - - it_behaves_like '412 response' do - let(:request) { api("/projects/#{project.id}/labels", user) } - let(:params) { { name: 'label1' } } - end - end - describe 'PUT /projects/:id/labels' do - it 'returns 200 if name and colors and description are changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - new_name: 'New Label', - color: '#FFFFFF', - description: 'test' - } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq('New Label') - expect(json_response['color']).to eq('#FFFFFF') - expect(json_response['description']).to eq('test') + expect(response).to have_gitlab_http_status(400) end - it 'returns 200 if name is changed' do - put api("/projects/#{project.id}/labels", user), + it 'fails if label_id and name are given in params' do + delete api("/projects/#{project.id}/labels", user), params: { - name: 'label1', - new_name: 'New Label' + label_id: label1.id, + name: priority_label.name } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq('New Label') - expect(json_response['color']).to eq(label1.color) - end - it 'returns 200 if colors is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - color: '#FFFFFF' - } - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq(label1.name) - expect(json_response['color']).to eq('#FFFFFF') + expect(response).to have_gitlab_http_status(400) end - it 'returns 200 if description is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'bug', - description: 'test' - } - - expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['description']).to eq('test') - expect(json_response['priority']).to eq(3) + it_behaves_like '412 response' do + let(:request) { api("/projects/#{project.id}/labels", user) } + let(:params) { { name: 'label1' } } end + end - it 'returns 200 if priority is changed' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'bug', - priority: 10 - } - - expect(response.status).to eq(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['priority']).to eq(10) + describe 'PUT /projects/:id/labels' do + context 'when using name' do + it_behaves_like 'label update API' do + let(:spec_params) { { name: 'label1' } } + let(:expected_response_label_id) { label1.id } + end end - it 'returns 200 if a priority is added' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - priority: 3 - } - - expect(response.status).to eq(200) - expect(json_response['name']).to eq(label1.name) - expect(json_response['priority']).to eq(3) + context 'when using label_id' do + it_behaves_like 'label update API' do + let(:spec_params) { { label_id: label1.id } } + let(:expected_response_label_id) { label1.id } + end end - it 'returns 200 if the priority is removed' do + it 'returns 404 if label does not exist' do put api("/projects/#{project.id}/labels", user), params: { - name: priority_label.name, - priority: nil + name: 'label2', + new_name: 'label3' } - expect(response.status).to eq(200) - expect(json_response['name']).to eq(priority_label.name) - expect(json_response['priority']).to be_nil + expect(response).to have_gitlab_http_status(404) end - it 'returns 404 if label does not exist' do + it 'returns 404 if label by id does not exist' do put api("/projects/#{project.id}/labels", user), params: { - name: 'label2', + label_id: 0, new_name: 'label3' } + expect(response).to have_gitlab_http_status(404) end - it 'returns 400 if no label name given' do + it 'returns 400 if no label name and id is given' do put api("/projects/#{project.id}/labels", user), params: { new_name: 'label2' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['error']).to eq('name is missing') - end - - it 'returns 400 if no new parameters given' do - put api("/projects/#{project.id}/labels", user), params: { name: 'label1' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\ - 'at least one parameter must be provided') - end - it 'returns 400 for invalid name' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - new_name: ',', - color: '#FFFFFF' - } expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['title']).to eq(['is invalid']) + expect(json_response['error']).to eq('label_id, name are missing, exactly one parameter must be provided') end - it 'returns 400 when color code is too short' do + it 'fails if label_id and name are given in params' do put api("/projects/#{project.id}/labels", user), params: { - name: 'label1', - color: '#FF' + label_id: label1.id, + name: priority_label.name, + new_name: 'New Label' } - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['color']).to eq(['must be a valid color code']) - end - - it 'returns 400 for too long color code' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - color: '#FFAAFFFF' - } - expect(response).to have_gitlab_http_status(400) - expect(json_response['message']['color']).to eq(['must be a valid color code']) - end - - it 'returns 400 for invalid priority' do - put api("/projects/#{project.id}/labels", user), - params: { - name: 'label1', - priority: 'foo' - } expect(response).to have_gitlab_http_status(400) end -- cgit v1.2.1