Incorporate feedback from Nick

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