Incorporate feedback from Nick

This commit is contained in:
Robert Schilling 2018-09-08 10:36:45 +02:00
parent f17d10c451
commit 82f09a91dd
7 changed files with 52 additions and 23 deletions

View File

@ -10,4 +10,8 @@ class GroupLabel < Label
def subject_foreign_key
'group_id'
end
def priority(parent)
nil
end
end

View File

@ -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)

View File

@ -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

View File

@ -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

View File

@ -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!

View File

@ -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
}

View File

@ -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',