Merge branch 'epic-add-labels' into 'master'

Enable labels for epics

See merge request gitlab-org/gitlab-ee!4569

app/finders/labels_finder.rb

@@ -39,7 +39,7 @@ class LabelsFinder < UnionFinder
         end
       end
     elsif only_group_labels?
-      label_ids << Label.where(group_id: group.id)
+      label_ids << Label.where(group_id: group_ids)
     else
       label_ids << Label.where(group_id: projects.group_ids)
       label_ids << Label.where(project_id: projects.select(:id))
@@ -59,10 +59,11 @@ class LabelsFinder < UnionFinder
     items.where(title: title)
   end
 
-  def group
-    strong_memoize(:group) do
+  def group_ids
+    strong_memoize(:group_ids) do
       group = Group.find(params[:group_id])
-      authorized_to_read_labels?(group) && group
+      groups = params[:include_ancestor_groups].present? ? group.self_and_ancestors : [group]
+      groups_user_can_read_labels(groups).map(&:id)
     end
   end
 
@@ -120,4 +121,10 @@ class LabelsFinder < UnionFinder
 
     Ability.allowed?(current_user, :read_label, label_parent)
   end
+
+  def groups_user_can_read_labels(groups)
+    DeclarativePolicy.user_scope do
+      groups.select { |group| authorized_to_read_labels?(group) }
+    end
+  end
 end

app/services/issuable_base_service.rb

@@ -79,8 +79,12 @@ class IssuableBaseService < BaseService
     return unless labels
 
     params[:label_ids] = labels.split(",").map do |label_name|
-      service = Labels::FindOrCreateService.new(current_user, project, title: label_name.strip)
-      label   = service.execute
+      label = Labels::FindOrCreateService.new(
+        current_user,
+        parent,
+        title: label_name.strip,
+        available_labels: available_labels
+      ).execute
 
       label.try(:id)
     end.compact
@@ -104,7 +108,7 @@ class IssuableBaseService < BaseService
   end
 
   def available_labels
-    LabelsFinder.new(current_user, project_id: @project.id).execute
+    @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id).execute
   end
 
   def merge_quick_actions_into_params!(issuable)
@@ -305,4 +309,8 @@ class IssuableBaseService < BaseService
   def update_project_counter_caches?(issuable)
     issuable.state_changed?
   end
+
+  def parent
+    project
+  end
 end

app/services/labels/find_or_create_service.rb

 module Labels
   class FindOrCreateService
-    def initialize(current_user, project, params = {})
+    def initialize(current_user, parent, params = {})
       @current_user = current_user
-      @project = project
+      @parent = parent
+      @available_labels = params.delete(:available_labels)
       @params = params.dup.with_indifferent_access
     end
 
@@ -13,12 +14,13 @@ module Labels
 
     private
 
-    attr_reader :current_user, :project, :params, :skip_authorization
+    attr_reader :current_user, :parent, :params, :skip_authorization
 
     def available_labels
       @available_labels ||= LabelsFinder.new(
         current_user,
-        project_id: project.id
+        "#{parent_type}_id".to_sym => parent.id,
+        only_group_labels: parent_is_group?
       ).execute(skip_authorization: skip_authorization)
     end
 
@@ -27,8 +29,8 @@ module Labels
     def find_or_create_label
       new_label = available_labels.find_by(title: title)
 
-      if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, project))
-        new_label = Labels::CreateService.new(params).execute(project: project)
+      if new_label.nil? && (skip_authorization || Ability.allowed?(current_user, :admin_label, parent))
+        new_label = Labels::CreateService.new(params).execute(parent_type.to_sym => parent)
       end
 
       new_label
@@ -37,5 +39,13 @@ module Labels
     def title
       params[:title] || params[:name]
     end
+
+    def parent_type
+      parent.model_name.param_key
+    end
+
+    def parent_is_group?
+      parent_type == "group"
+    end
   end
 end

doc/api/epics.md

@@ -17,12 +17,14 @@ Gets all epics of the requested group and its subgroups.
 ```
 GET /groups/:id/-/epics
 GET /groups/:id/-/epics?author_id=5
+GET /groups/:id/-/epics?labels=bug,reproduced
 ```
 
 | Attribute           | Type             | Required   | Description                                                                            |
 | ------------------- | ---------------- | ---------- | ---------------------------------------------------------------------------------------|
 | `id`                | integer/string   | yes        | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user                |
 | `author_id`         | integer          | no         | Return epics created by the given user `id`                                                                                                        |
+| `labels`            | string           | no         | Return epics matching a comma separated list of labels names. Label names from the epic group or a parent group can be used                |
 | `order_by`          | string           | no         | Return epics ordered by `created_at` or `updated_at` fields. Default is `created_at`                                                               |
 | `sort`              | string           | no         | Return epics sorted in `asc` or `desc` order. Default is `desc`                                                                                    |
 | `search`            | string           | no         | Search epics against their `title` and `description`                                                                                               |
@@ -49,6 +51,7 @@ Example response:
     "avatar_url": "http://www.gravatar.com/avatar/018729e129a6f31c80a6327a30196823?s=80&d=identicon",
     "web_url": "http://localhost:3001/kam"
   },
+  "labels": [],
   "start_date": null,
   "end_date": null,
   "created_at": "2018-01-21T06:21:13.165Z",
@@ -110,6 +113,7 @@ POST /groups/:id/-/epics
 | ------------------- | ---------------- | ---------- | ---------------------------------------------------------------------------------------|
 | `id`                | integer/string   | yes        | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user                |
 | `title`             | string           | yes        | The title of the epic |
+| `labels`            | string           | no         | The comma separated list of labels |
 | `description`       | string           | no         | The description of the epic  |
 | `start_date`        | string           | no         | The start date of the epic  |
 | `end_date`          | string.          | no         | The end date of the epic |
@@ -135,6 +139,7 @@ Example response:
     "id" : 18,
     "username" : "eileen.lowe"
   },
+  "labels": [],
   "start_date": null,
   "end_date": null,
   "created_at": "2018-01-21T06:21:13.165Z",
@@ -156,6 +161,7 @@ PUT /groups/:id/-/epics/:epic_iid
 | `epic_iid`          | integer/string   | yes        | The internal ID  of the epic  |
 | `title`             | string           | no         | The title of an epic |
 | `description`       | string           | no         | The description of an epic  |
+| `labels`            | string           | no         | The comma separated list of labels |
 | `start_date`        | string           | no         | The start date of an epic  |
 | `end_date`          | string.          | no         | The end date of an epic |
 
@@ -180,6 +186,7 @@ Example response:
     "id" : 18,
     "username" : "eileen.lowe"
   },
+  "labels": [],
   "start_date": null,
   "end_date": null,
   "created_at": "2018-01-21T06:21:13.165Z",

ee/app/controllers/groups/epics_controller.rb

@@ -54,11 +54,12 @@ class Groups::EpicsController < Groups::ApplicationController
   end
 
   def epic_params_attributes
-    %i[
-      title
-      description
-      start_date
-      end_date
+    [
+      :title,
+      :description,
+      :start_date,
+      :end_date,
+      label_ids: []
     ]
   end
 
@@ -67,7 +68,7 @@ class Groups::EpicsController < Groups::ApplicationController
   end
 
   def update_service
-    Epics::UpdateService.new(nil, current_user, epic_params)
+    Epics::UpdateService.new(@group, current_user, epic_params)
   end
 
   def finder_type

ee/app/finders/epics_finder.rb

@@ -11,6 +11,7 @@ class EpicsFinder < IssuableFinder
     items = by_search(items)
     items = by_author(items)
     items = by_timeframe(items)
+    items = by_label(items)
 
     sort(items)
   end

ee/app/serializers/epic_entity.rb

@@ -11,4 +11,5 @@ class EpicEntity < IssuableEntity
   expose :web_url do |epic|
     group_epic_path(epic.group, epic)
   end
+  expose :labels, using: LabelEntity
 end

ee/app/services/epics/base_service.rb

+module Epics
+  class BaseService < IssuableBaseService
+    attr_reader :group
+
+    def initialize(group, current_user, params)
+      @group, @current_user, @params = group, current_user, params
+    end
+
+    private
+
+    def available_labels
+      @available_labels ||= LabelsFinder.new(
+        current_user,
+        group_id: group.id,
+        only_group_labels: true,
+        include_ancestor_groups: true
+      ).execute
+    end
+
+    def parent
+      group
+    end
+  end
+end

ee/app/services/epics/create_service.rb

 module Epics
-  class CreateService < IssuableBaseService
-    attr_reader :group
-
-    def initialize(group, current_user, params)
-      @group, @current_user, @params = group, current_user, params
-    end
-
+  class CreateService < Epics::BaseService
     def execute
       @epic = group.epics.new(whitelisted_epic_params)
       create(@epic)

ee/app/services/epics/update_service.rb

 module Epics
-  class UpdateService < ::IssuableBaseService
+  class UpdateService < Epics::BaseService
     def execute(epic)
       update(epic)
     end

ee/changelogs/unreleased/epic-add-labels.yml

+---
+title: Allow adding or removing labels from epics and filter epics by labels
+merge_request:
+author:
+type: added

ee/lib/api/epics.rb

@@ -32,8 +32,9 @@ module API
 
       def find_epics(args = {})
         args = declared_params.merge(args)
+        args[:label_name] = args.delete(:labels)
 
-        epics = EpicsFinder.new(current_user, args).execute
+        epics = EpicsFinder.new(current_user, args).execute.preload(:labels)
 
         epics.reorder(args[:order_by] => args[:sort])
       end
@@ -54,6 +55,7 @@ module API
                         desc: 'Return epics sorted in `asc` or `desc` order.'
         optional :search, type: String, desc: 'Search epics for text present in the title or description'
         optional :author_id, type: Integer, desc: 'Return epics which are authored by the user with the given ID'
+        optional :labels, type: String, desc: 'Comma-separated list of label names'
       end
       get ':id/-/epics' do
         present find_epics(group_id: user_group.id), with: Entities::Epic
@@ -79,6 +81,7 @@ module API
         optional :description, type: String, desc: 'The description of an epic'
         optional :start_date, type: String, desc: 'The start date of an epic'
         optional :end_date, type: String, desc: 'The end date of an epic'
+        optional :labels, type: String, desc: 'Comma-separated list of label names'
       end
       post ':id/-/epics' do
         authorize_can_create!
@@ -100,14 +103,15 @@ module API
         optional :description, type: String, desc: 'The description of an epic'
         optional :start_date, type: String, desc: 'The start date of an epic'
         optional :end_date, type: String, desc: 'The end date of an epic'
-        at_least_one_of :title, :description, :start_date, :end_date
+        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        at_least_one_of :title, :description, :start_date, :end_date, :labels
       end
       put ':id/-/epics/:epic_iid' do
         authorize_can_admin!
         update_params = declared_params(include_missing: false)
         update_params.delete(:epic_iid)
 
-        result = ::Epics::UpdateService.new(nil, current_user, update_params).execute(epic)
+        result = ::Epics::UpdateService.new(user_group, current_user, update_params).execute(epic)
 
         if result.valid?
           present result, with: Entities::Epic

ee/spec/controllers/groups/epics_controller_spec.rb

@@ -4,6 +4,7 @@ describe Groups::EpicsController do
   let(:group) { create(:group, :private) }
   let(:epic) { create(:epic, group: group) }
   let(:user)  { create(:user) }
+  let(:label) { create(:group_label, group: group, title: 'Bug') }
 
   before do
     sign_in(user)
@@ -170,7 +171,7 @@ describe Groups::EpicsController do
     describe 'PUT #update' do
       before do
         group.add_developer(user)
-        put :update, group_id: group, id: epic.to_param, epic: { title: 'New title' }, format: :json
+        put :update, group_id: group, id: epic.to_param, epic: { title: 'New title', label_ids: [label.id] }, format: :json
       end
 
       it 'returns status 200' do
@@ -178,7 +179,10 @@ describe Groups::EpicsController do
       end
 
       it 'updates the epic correctly' do
-        expect(epic.reload.title).to eq('New title')
+        epic.reload
+
+        expect(epic.title).to eq('New title')
+        expect(epic.labels).to eq([label])
       end
     end
 
@@ -210,7 +214,7 @@ describe Groups::EpicsController do
 
     describe '#create' do
       subject do
-        post :create, group_id: group, epic: { title: 'new epic', description: 'some descripition' }
+        post :create, group_id: group, epic: { title: 'new epic', description: 'some descripition', label_ids: [label.id] }
       end
 
       context 'when user has permissions to create an epic' do
@@ -229,6 +233,10 @@ describe Groups::EpicsController do
             expect { subject }.to change { Epic.count }.from(0).to(1)
           end
 
+          it 'assigns labels to the new epic' do
+            expect { subject }.to change { LabelLink.count }.from(0).to(1)
+          end
+
           it 'returns the correct json' do
             subject
 

ee/spec/factories/epics.rb

@@ -3,5 +3,15 @@ FactoryBot.define do
     title { generate(:title) }
     group
     author
+
+    factory :labeled_epic do
+      transient do
+        labels []
+      end
+
+      after(:create) do |epic, evaluator|
+        epic.update_attributes(labels: evaluator.labels)
+      end
+    end
   end
 end

ee/spec/finders/epics_finder_spec.rb

@@ -79,6 +79,15 @@ describe EpicsFinder do
           end
         end
 
+        context 'by label' do
+          let(:label) { create(:label) }
+          let!(:labeled_epic) { create(:labeled_epic, group: group, labels: [label]) }
+
+          it 'returns all epics with given label' do
+            expect(epics(label_name: label.title)).to contain_exactly(labeled_epic)
+          end
+        end
+
         context 'when subgroups are supported', :nested_groups do
           let(:subgroup) { create(:group, :private, parent: group) }
           let(:subgroup2) { create(:group, :private, parent: subgroup) }

ee/spec/fixtures/api/schemas/public_api/v4/epic.json

@@ -18,6 +18,12 @@
       },
       "additionalProperties": false
     },
+    "labels": {
+      "type": "array",
+      "items": {
+        "type": "string"
+      }
+    },
     "start_date": { "type": ["string", "null"] },
     "end_date": { "type": ["string", "null"] },
     "created_at": { "type": ["string", "null"] },

ee/spec/fixtures/api/schemas/public_api/v4/epic_issue_link.json

@@ -21,7 +21,13 @@
         "start_date": { "type": ["string", "null"] },
         "end_date": { "type": ["string", "null"] },
         "created_at": { "type": ["string", "null"] },
-        "updated_at": { "type": ["string", "null"] }
+        "updated_at": { "type": ["string", "null"] },
+        "labels": {
+          "type": "array",
+          "items": {
+            "type": "string"
+          }
+        }
       },
       "additionalProperties": false
     },

ee/spec/requests/api/epics_spec.rb

@@ -78,6 +78,8 @@ describe API::Epics do
                created_at: 2.days.ago,
                updated_at: 3.days.ago)
       end
+      let!(:label) { create(:group_label, title: 'a-test', group: group) }
+      let!(:label_link) { create(:label_link, label: label, target: epic2) }
 
       before do
         stub_licensed_features(epics: true)
@@ -130,6 +132,12 @@ describe API::Epics do
 
         expect_array_response([epic2.id, epic.id])
       end
+
+      it 'returns an array of labeled epics' do
+        get api(url, user), labels: label.title
+
+        expect_array_response([epic2.id])
+      end
     end
   end
 
@@ -157,7 +165,7 @@ describe API::Epics do
 
   describe 'POST /groups/:id/-/epics' do
     let(:url) { "/groups/#{group.path}/-/epics" }
-    let(:params) { { title: 'new epic', description: 'epic description' } }
+    let(:params) { { title: 'new epic', description: 'epic description', labels: 'label1' } }
 
     it_behaves_like 'error requests'
 
@@ -196,6 +204,7 @@ describe API::Epics do
 
           expect(epic.title).to eq('new epic')
           expect(epic.description).to eq('epic description')
+          expect(epic.labels.first.title).to eq('label1')
         end
       end
     end
@@ -203,7 +212,7 @@ describe API::Epics do
 
   describe 'PUT /groups/:id/-/epics/:epic_iid' do
     let(:url) { "/groups/#{group.path}/-/epics/#{epic.iid}" }
-    let(:params) { { title: 'new title', description: 'new description' } }
+    let(:params) { { title: 'new title', description: 'new description', labels: 'label2' } }
 
     it_behaves_like 'error requests'
 
@@ -250,6 +259,7 @@ describe API::Epics do
 
           expect(result.title).to eq('new title')
           expect(result.description).to eq('new description')
+          expect(result.labels.first.title).to eq('label2')
         end
       end
     end

ee/spec/serializers/epic_entity_spec.rb

@@ -10,7 +10,7 @@ describe EpicEntity do
   subject { described_class.new(resource, request: request).as_json }
 
   it 'has Issuable attributes' do
-    expect(subject).to include(:id, :iid, :description,  :title)
+    expect(subject).to include(:id, :iid, :description,  :title, :labels)
   end
 
   it 'has epic specific attributes' do

ee/spec/services/epics/update_service_spec.rb

@@ -7,7 +7,7 @@ describe Epics::UpdateService do
 
   describe '#execute' do
     def update_epic(opts)
-      described_class.new(nil, user, opts).execute(epic)
+      described_class.new(group, user, opts).execute(epic)
     end
 
     context 'multiple values update' do

lib/api/entities.rb

@@ -508,6 +508,10 @@ module API
       expose :end_date
       expose :created_at
       expose :updated_at
+      expose :labels do |epic, options|
+        # Avoids an N+1 query since labels are preloaded
+        epic.labels.map(&:title).sort
+      end
     end
 
     class EpicIssue < Issue

spec/finders/labels_finder_spec.rb

@@ -5,6 +5,8 @@ describe LabelsFinder do
     let(:group_1) { create(:group) }
     let(:group_2) { create(:group) }
     let(:group_3) { create(:group) }
+    let(:private_group_1) { create(:group, :private) }
+    let(:private_subgroup_1) { create(:group, :private, parent: private_group_1) }
 
     let(:project_1) { create(:project, namespace: group_1) }
     let(:project_2) { create(:project, namespace: group_2) }
@@ -20,6 +22,8 @@ describe LabelsFinder do
     let!(:group_label_1) { create(:group_label, group: group_1, title: 'Label 1 (group)') }
     let!(:group_label_2) { create(:group_label, group: group_1, title: 'Group Label 2') }
     let!(:group_label_3) { create(:group_label, group: group_2, title: 'Group Label 3') }
+    let!(:private_group_label_1) { create(:group_label, group: private_group_1, title: 'Private Group Label 1') }
+    let!(:private_subgroup_label_1) { create(:group_label, group: private_subgroup_1, title: 'Private Sub Group Label 1') }
 
     let(:user) { create(:user) }
 
@@ -66,6 +70,25 @@ describe LabelsFinder do
           expect(finder.execute).to eq [group_label_2, group_label_1]
         end
       end
+
+      context 'when including labels from group ancestors', :nested_groups do
+        it 'returns labels from group and its ancestors' do
+          private_group_1.add_developer(user)
+          private_subgroup_1.add_developer(user)
+
+          finder = described_class.new(user, group_id: private_subgroup_1.id, only_group_labels: true, include_ancestor_groups: true)
+
+          expect(finder.execute).to eq [private_group_label_1, private_subgroup_label_1]
+        end
+
+        it 'ignores labels from groups which user can not read' do
+          private_subgroup_1.add_developer(user)
+
+          finder = described_class.new(user, group_id: private_subgroup_1.id, only_group_labels: true, include_ancestor_groups: true)
+
+          expect(finder.execute).to eq [private_subgroup_label_1]
+        end
+      end
     end
 
     context 'filtering by project_id' do

spec/services/labels/find_or_create_service_spec.rb

@@ -15,47 +15,79 @@ describe Labels::FindOrCreateService do
 
     context 'when acting on behalf of a specific user' do
       let(:user) { create(:user) }
-      subject(:service) { described_class.new(user, project, params) }
-      before do
-        project.add_developer(user)
-      end
 
-      context 'when label does not exist at group level' do
-        it 'creates a new label at project level' do
-          expect { service.execute }.to change(project.labels, :count).by(1)
+      context 'when finding labels on project level' do
+        subject(:service) { described_class.new(user, project, params) }
+
+        before do
+          project.add_developer(user)
         end
-      end
 
-      context 'when label exists at group level' do
-        it 'returns the group label' do
-          group_label = create(:group_label, group: group, title: 'Security')
+        context 'when label does not exist at group level' do
+          it 'creates a new label at project level' do
+            expect { service.execute }.to change(project.labels, :count).by(1)
+          end
+        end
 
-          expect(service.execute).to eq group_label
+        context 'when label exists at group level' do
+          it 'returns the group label' do
+            group_label = create(:group_label, group: group, title: 'Security')
+
+            expect(service.execute).to eq group_label
+          end
+        end
+
+        context 'when label exists at project level' do
+          it 'returns the project label' do
+            project_label = create(:label, project: project, title: 'Security')
+
+            expect(service.execute).to eq project_label
+          end
         end
       end
 
-      context 'when label does not exist at group level' do
-        it 'creates a new label at project leve' do
-          expect { service.execute }.to change(project.labels, :count).by(1)
+      context 'when finding labels on group level' do
+        subject(:service) { described_class.new(user, group, params) }
+
+        before do
+          group.add_developer(user)
+        end
+
+        context 'when label does not exist at group level' do
+          it 'creates a new label at group level' do
+            expect { service.execute }.to change(group.labels, :count).by(1)
+          end
+        end
+
+        context 'when label exists at group level' do
+          it 'returns the group label' do
+            group_label = create(:group_label, group: group, title: 'Security')
+
+            expect(service.execute).to eq group_label
+          end
         end
       end
+    end
+
+    context 'when authorization is not required' do
+      context 'when finding labels on project level' do
+        subject(:service) { described_class.new(nil, project, params) }
 
-      context 'when label exists at project level' do
         it 'returns the project label' do
           project_label = create(:label, project: project, title: 'Security')
 
-          expect(service.execute).to eq project_label
+          expect(service.execute(skip_authorization: true)).to eq project_label
         end
       end
-    end
 
-    context 'when authorization is not required' do
-      subject(:service) { described_class.new(nil, project, params) }
+      context 'when finding labels on group level' do
+        subject(:service) { described_class.new(nil, group, params) }
 
-      it 'returns the project label' do
-        project_label = create(:label, project: project, title: 'Security')
+        it 'returns the group label' do
+          group_label = create(:group_label, group: group, title: 'Security')
 
-        expect(service.execute(skip_authorization: true)).to eq project_label
+          expect(service.execute(skip_authorization: true)).to eq group_label
+        end
       end
     end
   end