Incorporate feedback from Robert

5 files changed, 83 insertions, 120 deletions

diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 25052a29c31..e361b419f16 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1006,7 +1006,7 @@ module API
       expose :id, :name, :color, :description
     end
 
-    class GroupLabel < LabelBasic
+    class Label < LabelBasic
       expose :open_issues_count do |label, options|
         label.open_issues_count(options[:current_user])
       end
@@ -1024,7 +1024,10 @@ module API
       end
     end
 
-    class ProjectLabel < GroupLabel
+    class GroupLabel < Label
+    end
+
+    class ProjectLabel < Label
       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 85fd470c10d..559e6a25f35 100644
--- a/lib/api/group_labels.rb
+++ b/lib/api/group_labels.rb
@@ -57,7 +57,7 @@ module API
       delete ':id/labels' do
         authorize! :admin_label, user_group
 
-        label = find_label(user_group, params[:name], false)
+        label = find_label(user_group, params[:name], include_ancestor_groups: false)
 
         destroy_conditionally!(label)
       end
@@ -76,7 +76,7 @@ module API
       put ':id/labels' do
         authorize! :admin_label, user_group
 
-        label = find_label(user_group, params[:name], false)
+        label = find_label(user_group, params[:name], include_ancestor_groups: false)
 
         label = ::Labels::UpdateService.new(declared_params(include_missing: false)).execute(label)
         render_validation_error!(label) unless label.valid?
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index afe0437c555..f769a5f78c0 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -84,7 +84,7 @@ module API
       page || not_found!('Wiki Page')
     end
 
-    def available_labels_for(label_parent, include_ancestor_groups = true)
+    def available_labels_for(label_parent, include_ancestor_groups: true)
       search_params = { include_ancestor_groups: include_ancestor_groups }
 
       if label_parent.is_a?(Project)
@@ -170,8 +170,8 @@ module API
       end
     end
 
-    def find_label(parent, id, include_ancestor_groups = true)
-      labels = available_labels_for(parent, include_ancestor_groups)
+    def find_label(parent, id, 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 || not_found!('Label')
diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb
index b15875027ee..10d747e231e 100644
--- a/lib/api/subscriptions.rb
+++ b/lib/api/subscriptions.rb
@@ -5,10 +5,30 @@ module API
     before { authenticate! }
 
     subscribables = [
-      { type: 'merge_requests', entity: Entities::MergeRequest, source: Project, finder: ->(id) { find_merge_request_with_access(id, :update_merge_request) }, parent_resource: -> { user_project } },
-      { type: 'issues', entity: Entities::Issue, source: Project, finder: ->(id) { find_project_issue(id) }, parent_resource: -> { user_project } },
-      { type: 'labels', entity: Entities::ProjectLabel, source: Project, finder: ->(id) { find_label(user_project, id) }, parent_resource: -> { user_project } },
-      { type: 'labels', entity: Entities::GroupLabel, source: Group, finder: ->(id) { find_label(user_group, id) }, parent_resource: -> { nil } }
+      {
+        type: 'merge_requests',
+        entity: Entities::MergeRequest,
+        source: Project,
+        finder: ->(id) { find_merge_request_with_access(id, :update_merge_request) }
+      },
+      {
+        type: 'issues',
+        entity: Entities::Issue,
+        source: Project,
+        finder: ->(id) { find_project_issue(id) }
+      },
+      {
+        type: 'labels',
+        entity: Entities::ProjectLabel,
+        source: Project,
+        finder: ->(id) { find_label(user_project, id) }
+      },
+      {
+        type: 'labels',
+        entity: Entities::GroupLabel,
+        source: Group,
+        finder: ->(id) { find_label(user_group, id) }
+      }
     ]
 
     subscribables.each do |subscribable|
@@ -23,7 +43,7 @@ module API
           success subscribable[:entity]
         end
         post ":id/#{subscribable[:type]}/:subscribable_id/subscribe" do
-          parent = instance_exec(&subscribable[:parent_resource])
+          parent = parent_resource(source_type)
           resource = instance_exec(params[:subscribable_id], &subscribable[:finder])
 
           if resource.subscribed?(current_user, parent)
@@ -38,7 +58,7 @@ module API
           success subscribable[:entity]
         end
         post ":id/#{subscribable[:type]}/:subscribable_id/unsubscribe" do
-          parent = instance_exec(&subscribable[:parent_resource])
+          parent = parent_resource(source_type)
           resource = instance_exec(params[:subscribable_id], &subscribable[:finder])
 
           if !resource.subscribed?(current_user, parent)
@@ -50,5 +70,18 @@ module API
         end
       end
     end
+
+    private
+
+    helpers do
+      def parent_resource(source_type)
+        case source_type
+        when 'project'
+          user_project
+        else
+          nil
+        end
+      end
+    end
   end
 end
diff --git a/spec/requests/api/group_labels_spec.rb b/spec/requests/api/group_labels_spec.rb
index 00063b51792..3769f8b78e4 100644
--- a/spec/requests/api/group_labels_spec.rb
+++ b/spec/requests/api/group_labels_spec.rb
@@ -25,9 +25,11 @@ describe API::GroupLabels do
   describe 'POST /groups/:id/labels' do
     it 'returns created label when all params are given' do
       post api("/groups/#{group.id}/labels", user),
-           name: 'Foo',
-           color: '#FFAABB',
-           description: 'test'
+           params: {
+             name: 'Foo',
+             color: '#FFAABB',
+             description: 'test'
+           }
 
       expect(response).to have_gitlab_http_status(201)
       expect(json_response['name']).to eq('Foo')
@@ -37,8 +39,10 @@ describe API::GroupLabels do
 
     it 'returns created label when only required params are given' do
       post api("/groups/#{group.id}/labels", user),
-           name: 'Foo & Bar',
-           color: '#FFAABB'
+           params: {
+             name: 'Foo & Bar',
+             color: '#FFAABB'
+           }
 
       expect(response.status).to eq(201)
       expect(json_response['name']).to eq('Foo & Bar')
@@ -47,48 +51,23 @@ describe API::GroupLabels do
     end
 
     it 'returns a 400 bad request if name not given' do
-      post api("/groups/#{group.id}/labels", user), color: '#FFAABB'
+      post api("/groups/#{group.id}/labels", user), params: { color: '#FFAABB' }
 
       expect(response).to have_gitlab_http_status(400)
     end
 
     it 'returns a 400 bad request if color is not given' do
-      post api("/groups/#{group.id}/labels", user), name: 'Foobar'
+      post api("/groups/#{group.id}/labels", user), params: { name: 'Foobar' }
 
       expect(response).to have_gitlab_http_status(400)
     end
 
-    it 'returns 400 for invalid color' do
-      post api("/groups/#{group.id}/labels", user),
-           name: 'Foo',
-           color: '#FFAA'
-
-      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
-      post api("/groups/#{group.id}/labels", user),
-           name: 'Foo',
-           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 name' do
-      post api("/groups/#{group.id}/labels", user),
-           name: ',',
-           color: '#FFAABB'
-
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['title']).to eq(['is invalid'])
-    end
-
     it 'returns 409 if label already exists' do
       post api("/groups/#{group.id}/labels", user),
-           name: label1.name,
-           color: '#FFAABB'
+           params: {
+             name: label1.name,
+             color: '#FFAABB'
+           }
 
       expect(response).to have_gitlab_http_status(409)
       expect(json_response['message']).to eq('Label already exists')
@@ -97,13 +76,13 @@ describe API::GroupLabels do
 
   describe 'DELETE /groups/:id/labels' do
     it 'returns 204 for existing label' do
-      delete api("/groups/#{group.id}/labels", user), name: label1.name
+      delete api("/groups/#{group.id}/labels", user), params: { name: label1.name }
 
       expect(response).to have_gitlab_http_status(204)
     end
 
     it 'returns 404 for non existing label' do
-      delete api("/groups/#{group.id}/labels", user), name: 'label2'
+      delete api("/groups/#{group.id}/labels", user), params: { name: 'label2' }
 
       expect(response).to have_gitlab_http_status(404)
       expect(json_response['message']).to eq('404 Label Not Found')
@@ -119,7 +98,7 @@ describe API::GroupLabels 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
+      delete api("/groups/#{subgroup.id}/labels", user), params: { name: subgroup_label.name }
 
       expect(response).to have_gitlab_http_status(204)
       expect(subgroup.labels.size).to eq(0)
@@ -135,44 +114,16 @@ describe API::GroupLabels do
   describe 'PUT /groups/:id/labels' do
     it 'returns 200 if name and colors and description are changed' do
       put api("/groups/#{group.id}/labels", user),
-          name: label1.name,
-          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')
-    end
-
-    it 'returns 200 if name is changed' do
-      put api("/groups/#{group.id}/labels", user),
-          name: label1.name,
-          new_name: 'New Label'
+          params: {
+            name: label1.name,
+            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(label1.color)
-    end
-
-    it 'returns 200 if colors is changed' 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')
-    end
-
-    it 'returns 200 if description is changed' do
-      put api("/groups/#{group.id}/labels", user),
-          name: label2.name,
-          description: 'test'
-
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['name']).to eq(label2.name)
       expect(json_response['description']).to eq('test')
     end
 
@@ -181,8 +132,10 @@ describe API::GroupLabels do
       subgroup_label = create(:group_label, title: 'feature', group: subgroup)
 
       put api("/groups/#{subgroup.id}/labels", user),
-          name: subgroup_label.name,
-          new_name: 'New Label'
+          params: {
+            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')
@@ -191,54 +144,28 @@ describe API::GroupLabels do
 
     it 'returns 404 if label does not exist' do
       put api("/groups/#{group.id}/labels", user),
-          name: 'label2',
-          new_name: 'label3'
+          params: {
+            name: 'label2',
+            new_name: 'label3'
+          }
 
       expect(response).to have_gitlab_http_status(404)
     end
 
     it 'returns 400 if no label name given' do
-      put api("/groups/#{group.id}/labels", user), new_name: label1.name
+      put api("/groups/#{group.id}/labels", user), params: { new_name: label1.name }
 
       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("/groups/#{group.id}/labels", user), name: label1.name
+      put api("/groups/#{group.id}/labels", user), params: { name: label1.name }
 
       expect(response).to have_gitlab_http_status(400)
       expect(json_response['error']).to eq('new_name, color, description are missing, '\
                                            'at least one parameter must be provided')
     end
-
-    it 'returns 400 for invalid name' do
-      put api("/groups/#{group.id}/labels", user),
-          name: label1.name,
-          new_name: ',',
-          color: '#FFFFFF'
-
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['title']).to eq(['is invalid'])
-    end
-
-    it 'returns 400 when color code is too short' do
-      put api("/groups/#{group.id}/labels", user),
-          name: label1.name,
-          color: '#FF'
-
-      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("/groups/#{group.id}/labels", user),
-           name: label1.name,
-           color: '#FFAAFFFF'
-
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['color']).to eq(['must be a valid color code'])
-    end
   end
 
   describe 'POST /groups/:id/labels/:label_id/subscribe' do