From 82f09a91dd3abae48b74010f541ea50e0190276a Mon Sep 17 00:00:00 2001 From: Robert Schilling Date: Sat, 8 Sep 2018 10:36:45 +0200 Subject: [PATCH] Incorporate feedback from Nick --- app/models/group_label.rb | 4 +++ app/services/labels/update_service.rb | 1 + lib/api/entities.rb | 2 +- lib/api/group_labels.rb | 10 ++---- lib/api/subscriptions.rb | 21 ++++++------ .../schemas/public_api/v4/group_labels.json | 3 +- spec/requests/api/group_labels_spec.rb | 34 +++++++++++++++++-- 7 files changed, 52 insertions(+), 23 deletions(-) diff --git a/app/models/group_label.rb b/app/models/group_label.rb index ff14529c6e6..d1499129130 100644 --- a/app/models/group_label.rb +++ b/app/models/group_label.rb @@ -10,4 +10,8 @@ class GroupLabel < Label def subject_foreign_key 'group_id' end + + def priority(parent) + nil + end end diff --git a/app/services/labels/update_service.rb b/app/services/labels/update_service.rb index e563447c64c..be33947d0eb 100644 --- a/app/services/labels/update_service.rb +++ b/app/services/labels/update_service.rb @@ -8,6 +8,7 @@ module Labels # returns the updated label def execute(label) + params[:name] = params.delete(:new_name) if params.key?(:new_name) params[:color] = convert_color_name_to_hex if params[:color].present? label.update(params) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8f0dddb55ee..4edec631e8d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1019,7 +1019,7 @@ module API label.open_merge_requests_count(options[:current_user]) end - expose :priority, if: lambda { |_, options| options[:project].is_a?(::Project) } do |label, options| + expose :priority do |label, options| label.priority(options[:project]) end diff --git a/lib/api/group_labels.rb b/lib/api/group_labels.rb index 35c5a6deca5..fb2d7fedb90 100644 --- a/lib/api/group_labels.rb +++ b/lib/api/group_labels.rb @@ -40,7 +40,7 @@ module API label = ::Labels::CreateService.new(declared_params(include_missing: false)).execute(group: user_group) - if label.valid? + if label.persisted? present label, with: Entities::Label, current_user: current_user, parent: user_group else render_validation_error!(label) @@ -80,12 +80,8 @@ module API label = user_group.labels.find_by(title: params[:name]) not_found!('Label not found') unless label - label_params = declared_params(include_missing: false) - # Rename new name to the actual label attribute name - label_params[:name] = label_params.delete(:new_name) if label_params.key?(:new_name) - - label = ::Labels::UpdateService.new(label_params).execute(label) - render_validation_error!(label) unless label.valid? + label = ::Labels::UpdateService.new(declared_params(include_missing: false)).execute(label) + render_validation_error!(label) if label.changed? present label, with: Entities::Label, current_user: current_user, parent: user_group end diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index a8d5457efde..58c9dfcc1db 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -5,10 +5,10 @@ module API before { authenticate! } subscribables = [ - ['merge_requests', Project, proc { |id| find_merge_request_with_access(id, :update_merge_request) }, proc { user_project }], - ['issues', Project, proc { |id| find_project_issue(id) }, proc { user_project }], - ['labels', Project, proc { |id| find_label(user_project, id) }, proc { user_project }], - ['labels', Group, proc { |id| find_label(user_group, id) }, proc { nil }] + { type: 'merge_requests', source: Project, finder: ->(id) { find_merge_request_with_access(id, :update_merge_request) }, parent_resource: -> { user_project } }, + { type: 'issues', source: Project, finder: ->(id) { find_project_issue(id) }, parent_resource: -> { user_project } }, + { type: 'labels', source: Project, finder: ->(id) { find_label(user_project, id) }, parent_resource: -> { user_project } }, + { type: 'labels', source: Group, finder: ->(id) { find_label(user_group, id) }, parent_resource: -> { nil } } ] params do @@ -32,9 +32,9 @@ module API desc 'Subscribe to a resource' do success entity_class end - post ":id/#{type}/:subscribable_id/subscribe" do - parent = instance_exec(&parent_ressource) - resource = instance_exec(params[:subscribable_id], &finder) + post ":id/#{subscribable[:type]}/:subscribable_id/subscribe" do + parent = instance_exec(&subscribable[:parent_resource]) + resource = instance_exec(params[:subscribable_id], &subscribable[:finder]) if resource.subscribed?(current_user, parent) not_modified! @@ -47,10 +47,9 @@ module API desc 'Unsubscribe from a resource' do success entity_class end - post ":id/#{type}/:subscribable_id/unsubscribe" do - parent = instance_exec(&parent_ressource) - resource = instance_exec(params[:subscribable_id], &finder) - + post ":id/#{subscribable[:type]}/:subscribable_id/unsubscribe" do + parent = instance_exec(&subscribable[:parent_resource]) + resource = instance_exec(params[:subscribable_id], &subscribable[:finder]) if !resource.subscribed?(current_user, parent) not_modified! diff --git a/spec/fixtures/api/schemas/public_api/v4/group_labels.json b/spec/fixtures/api/schemas/public_api/v4/group_labels.json index bcd027da5aa..f6c327abfdd 100644 --- a/spec/fixtures/api/schemas/public_api/v4/group_labels.json +++ b/spec/fixtures/api/schemas/public_api/v4/group_labels.json @@ -10,7 +10,8 @@ "open_issues_count" : { "type": "integer "}, "closed_issues_count" : { "type": "integer "}, "open_merge_requests_count" : { "type": "integer "}, - "subscribed" : { "type": "boolean" } + "subscribed" : { "type": "boolean" }, + "priority" : { "type": "null" } }, "additionalProperties": false } diff --git a/spec/requests/api/group_labels_spec.rb b/spec/requests/api/group_labels_spec.rb index 8e368a0ce39..c1efa7ada06 100644 --- a/spec/requests/api/group_labels_spec.rb +++ b/spec/requests/api/group_labels_spec.rb @@ -18,11 +18,12 @@ describe API::GroupLabels do expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(json_response.size).to eq(2) + expect(json_response.map {|r| r['name'] }).to contain_exactly('feature', 'bug') end end describe 'POST /groups/:id/labels' do - it 'returns created label when all params' do + it 'returns created label when all params are given' do post api("/groups/#{group.id}/labels", user), name: 'Foo', color: '#FFAABB', @@ -34,7 +35,7 @@ describe API::GroupLabels do expect(json_response['description']).to eq('test') end - it 'returns created label when only required params' do + it 'returns created label when only required params are given' do post api("/groups/#{group.id}/labels", user), name: 'Foo & Bar', color: '#FFAABB' @@ -51,7 +52,7 @@ describe API::GroupLabels do expect(response).to have_gitlab_http_status(400) end - it 'returns a 400 bad request if color not given' do + it 'returns a 400 bad request if color is not given' do post api("/groups/#{group.id}/labels", user), name: 'Foobar' expect(response).to have_gitlab_http_status(400) @@ -114,6 +115,17 @@ describe API::GroupLabels do expect(response).to have_gitlab_http_status(400) end + it "does not delete parent's group labels" do + subgroup = create(:group, parent: group) + subgroup_label = create(:group_label, title: 'feature', group: subgroup) + + delete api("/groups/#{subgroup.id}/labels", user), name: subgroup_label.name + + expect(response).to have_gitlab_http_status(204) + expect(subgroup.labels.size).to eq(0) + expect(group.labels).to include(label1) + end + it_behaves_like '412 response' do let(:request) { api("/groups/#{group.id}/labels", user) } let(:params) { { name: label1.name } } @@ -127,6 +139,7 @@ describe API::GroupLabels do 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') @@ -137,6 +150,7 @@ describe API::GroupLabels do put api("/groups/#{group.id}/labels", user), name: label1.name, new_name: 'New Label' + expect(response).to have_gitlab_http_status(200) expect(json_response['name']).to eq('New Label') expect(json_response['color']).to eq(label1.color) @@ -146,6 +160,7 @@ describe API::GroupLabels do put api("/groups/#{group.id}/labels", user), name: label1.name, 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') @@ -161,6 +176,19 @@ describe API::GroupLabels do expect(json_response['description']).to eq('test') end + it "does not update parent's group label" do + subgroup = create(:group, parent: group) + subgroup_label = create(:group_label, title: 'feature', group: subgroup) + + put api("/groups/#{subgroup.id}/labels", user), + name: subgroup_label.name, + new_name: 'New Label' + + expect(response).to have_gitlab_http_status(200) + expect(subgroup.labels[0].name).to eq('New Label') + expect(label1.name).to eq('feature') + end + it 'returns 404 if label does not exist' do put api("/groups/#{group.id}/labels", user), name: 'label2',