Merge branch '7331-child-epic-creation' into 'master'

Support creating a child epic from API

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

doc/api/epic_links.md

@@ -113,6 +113,40 @@ Example response:
 }
 ```
 
+## Create and assign a child epic
+
+Creates a a new epic and associates it with provided parent epic. The response is LinkedEpic object.
+
+```
+POST /groups/:id/epics/:epic_iid/epics
+```
+
+| 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      |
+| `epic_iid`      | integer        | yes      | The internal ID of the (future parent) epic.                                                                                       |
+| `title` | integer        | yes      | The global ID of the child epic. Internal ID can't be used because they can conflict with epics from other groups. |
+
+```bash
+curl --header POST "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/groups/1/epics/5/epics?title=Newpic
+```
+
+Example response:
+
+```json
+{
+  "id": 24,
+  "iid": 2,
+  "title": "child epic",
+  "group_id": 49,
+  "parent_id": 23,
+  "has_children": false,
+  "reference":  "&2",
+  "url": "http://localhost/groups/group16/-/epics/2",
+  "relation_url": "http://localhost/groups/group16/-/epics/1/links/24"
+}
+```
+
 ## Re-order a child epic
 
 ```

doc/api/epics.md

@@ -166,6 +166,7 @@ POST /groups/:id/epics
 | `start_date_fixed`  | string           | no         | The fixed start date of an epic (since 11.3) |
 | `due_date_is_fixed` | boolean          | no         | Whether due date should be sourced from `due_date_fixed` or from milestones (since 11.3) |
 | `due_date_fixed`    | string           | no         | The fixed due date of an epic (since 11.3) |
+| `parent_id`         | integer/string   | no         | The id of a parent epic (since 11.11) |
 
 ```bash
 curl --header POST "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/groups/1/epics?title=Epic&description=Epic%20description

ee/app/models/ee/epic.rb

@@ -44,6 +44,7 @@ module EE
       has_many :issues, through: :epic_issues
 
       validates :group, presence: true
+      validate :validate_parent, on: :create
 
       alias_attribute :parent_ids, :parent_id
 
@@ -79,6 +80,8 @@ module EE
 
       scope :with_api_entity_associations, -> { preload(:author, :labels, :group) }
 
+      MAX_HIERARCHY_DEPTH = 5
+
       def etag_caching_enabled?
         true
       end
@@ -264,6 +267,10 @@ module EE
       hierarchy.ancestors(hierarchy_order: :asc)
     end
 
+    def max_hierarchy_depth_achieved?
+      base_and_ancestors.count >= MAX_HIERARCHY_DEPTH
+    end
+
     def descendants
       hierarchy.descendants
     end
@@ -272,6 +279,10 @@ module EE
       ancestors.exists?(epic.id)
     end
 
+    def has_children?
+      issues.any? || descendants.any?
+    end
+
     def hierarchy
       ::Gitlab::ObjectHierarchy.new(self.class.where(id: id))
     end
@@ -280,6 +291,26 @@ module EE
     def update_project_counter_caches
     end
 
+    # we call this when creating a new epic (Epics::CreateService) or linking an existing one (EpicLinks::CreateService)
+    # when called from EpicLinks::CreateService we pass
+    #   parent_epic - because we don't have parent attribute set on epic
+    #   parent_group_descendants - we have preloaded them in the service and we want to prevent performance problems
+    #     when linking a lot of issues
+    def valid_parent?(parent_epic: nil, parent_group_descendants: nil)
+      parent_epic ||= parent
+
+      return true unless parent_epic
+
+      parent_group_descendants ||= parent_epic.group.self_and_descendants
+
+      return false if self == parent_epic
+      return false if level_depth_exceeded?(parent_epic)
+      return false if parent_epic.has_ancestor?(self)
+      return false if parent_epic.children.to_a.include?(self)
+
+      parent_group_descendants.include?(group)
+    end
+
     def issues_readable_by(current_user, preload: nil)
       related_issues = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position')
         .joins(:epic_issue)
@@ -301,5 +332,24 @@ module EE
     def banzai_render_context(field)
       super.merge(label_url_method: :group_epics_url)
     end
+
+    def validate_parent
+      return true if valid_parent?
+
+      errors.add :parent, 'The parent is not valid'
+    end
+    private :validate_parent
+
+    def level_depth_exceeded?(parent_epic)
+      hierarchy.max_descendants_depth.to_i + parent_epic.ancestors.count >= MAX_HIERARCHY_DEPTH
+    end
+    private :level_depth_exceeded?
+
+    def base_and_ancestors
+      return self.class.none unless parent_id
+
+      hierarchy.base_and_ancestors(hierarchy_order: :asc)
+    end
+    private :base_and_ancestors
   end
 end

ee/app/services/epic_links/create_service.rb

@@ -3,7 +3,9 @@
 module EpicLinks
   class CreateService < IssuableLinks::CreateService
     def execute
-      return error('Epic hierarchy level too deep', 409) if parent_ancestors_count >= 4
+      if issuable.max_hierarchy_depth_achieved?
+        return error('Epic hierarchy level too deep', 409)
+      end
 
       super
     end
@@ -36,12 +38,10 @@ module EpicLinks
     end
 
     def linkable_epic?(epic)
-      return false if epic == issuable
-      return false if previous_related_issuables.include?(epic)
-      return false if level_depth_exceeded?(epic)
-      return false if issuable.has_ancestor?(epic)
-
-      issuable_group_descendants.include?(epic.group)
+      epic.valid_parent?(
+        parent_epic: issuable,
+        parent_group_descendants: issuable_group_descendants
+      )
     end
 
     def references(extractor)
@@ -60,14 +60,6 @@ module EpicLinks
       @descendants ||= issuable.group.self_and_descendants
     end
 
-    def level_depth_exceeded?(epic)
-      epic.hierarchy.max_descendants_depth + parent_ancestors_count >= 5
-    end
-
-    def parent_ancestors_count
-      @parent_ancestors_count ||= issuable.ancestors.count
-    end
-
     def issuables_assigned_message
       'Epic(s) already assigned'
     end

ee/app/services/epics/create_service.rb

@@ -4,6 +4,8 @@ module Epics
   class CreateService < Epics::BaseService
     def execute
       @epic = group.epics.new(whitelisted_epic_params)
+      @epic.move_to_start if @epic.parent
+
       create(@epic)
     end
 
@@ -18,7 +20,7 @@ module Epics
     end
 
     def whitelisted_epic_params
-      params.slice(:title, :description, :start_date, :end_date, :milestone, :label_ids)
+      params.slice(:title, :description, :start_date, :end_date, :milestone, :label_ids, :parent_id)
     end
   end
 end

ee/changelogs/unreleased/7331-child-epic-creation.yml

+---
+title: Support creating a new child epic from the API
+merge_request:
+author:
+type: added

ee/lib/api/epic_links.rb

@@ -68,6 +68,26 @@ module API
         end
       end
 
+      desc 'Create and relate epic to a parent' do
+        success EE::API::Entities::Epic
+      end
+      params do
+        requires :title, type: String, desc: 'The title of a child epic'
+      end
+      post ':id/(-/)epics/:epic_iid/epics' do
+        authorize_can_admin!
+
+        create_params = { parent_id: epic.id, title: params[:title] }
+
+        child_epic = ::Epics::CreateService.new(user_group, current_user, create_params).execute
+
+        if child_epic.valid?
+          present child_epic, with: EE::API::Entities::LinkedEpic, user: current_user
+        else
+          render_validation_error!(epic)
+        end
+      end
+
       desc 'Remove epics relation'
       params do
         use :child_epic_id

ee/lib/api/epics.rb

@@ -66,6 +66,7 @@ module API
         optional :end_date, as: :due_date_fixed, type: String, desc: 'The due date of an epic'
         optional :due_date_is_fixed, type: Boolean, desc: 'Indicates due date should be sourced from due_date_fixed field not the issue milestones'
         optional :labels, type: Array[String], coerce_with: Validations::Types::LabelsList.coerce, desc: 'Comma-separated list of label names'
+        optional :parent_id, type: Integer, desc: 'The id of a parent epic'
       end
       post ':id/(-/)epics' do
         authorize_can_create!

ee/lib/ee/api/entities.rb

@@ -201,6 +201,26 @@ module EE
         expose :issue_link_id
       end
 
+      class LinkedEpic < Grape::Entity
+        expose :id
+        expose :iid
+        expose :title
+        expose :group_id
+        expose :parent_id
+        expose :has_children?, as: :has_children
+        expose :reference do |epic|
+          epic.to_reference(epic.parent.group)
+        end
+
+        expose :url do |epic|
+          ::Gitlab::Routing.url_helpers.group_epic_url(epic.group, epic)
+        end
+
+        expose :relation_url do |epic|
+          ::Gitlab::Routing.url_helpers.group_epic_link_url(epic.parent.group, epic.parent.iid, epic.id)
+        end
+      end
+
       class Epic < Grape::Entity
         can_admin_epic = ->(epic, opts) { Ability.allowed?(opts[:user], :admin_epic, epic) }
 
@@ -221,7 +241,7 @@ module EE
         expose :state
         expose :created_at
         expose :updated_at
-        expose :labels do |epic, options|
+        expose :labels do |epic|
           # Avoids an N+1 query since labels are preloaded
           epic.labels.map(&:title).sort
         end

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

+{
+  "type": "object",
+  "properties": {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "title": { "type": "string" },
+    "group_id": { "type": "integer" },
+    "parent_id": { "type": ["integer", "null"] },
+    "has_children": { "type": "boolean" },
+    "reference": { "type": "string" },
+    "url": { "type": "string" },
+    "relation_url": { "type": "string" }
+  },
+  "required": [
+    "id", "iid", "group_id", "title", "parent_id", "has_children", "reference", "url", "relation_url"
+  ],
+  "additionalProperties": false
+}

ee/spec/models/epic_spec.rb

@@ -21,6 +21,18 @@ describe Epic do
     it { is_expected.to validate_presence_of(:group) }
     it { is_expected.to validate_presence_of(:author) }
     it { is_expected.to validate_presence_of(:title) }
+
+    it 'is valid with a valid parent' do
+      epic = build(:epic, group: group, parent: create(:epic, group: group))
+
+      expect(epic).to be_valid
+    end
+
+    it 'is not valid with invalid parent' do
+      epic = build(:epic, group: group, parent: create(:epic))
+
+      expect(epic).not_to be_valid
+    end
   end
 
   describe 'modules' do
@@ -86,6 +98,110 @@ describe Epic do
     end
   end
 
+  describe '#valid_parent?' do
+    context 'basic checks' do
+      let(:epic) { build(:epic, group: group) }
+
+      it 'returns true without parent' do
+        expect(epic.valid_parent?).to be_truthy
+      end
+
+      it 'returns true with a valid parent' do
+        epic.parent = create(:epic, group: group)
+
+        expect(epic.valid_parent?).to be_truthy
+      end
+
+      it 'returns false with a parent from different group' do
+        epic.parent = create(:epic)
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+
+      it 'returns false when level is too deep' do
+        epic1 = create(:epic, group: group)
+        epic2 = create(:epic, group: group, parent: epic1)
+        epic3 = create(:epic, group: group, parent: epic2)
+        epic4 = create(:epic, group: group, parent: epic3)
+        epic5 = create(:epic, group: group, parent: epic4)
+        epic6 = create(:epic, group: group, parent: epic5)
+        epic.parent = epic6
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+    end
+
+    context 'when adding an Epic that has existing children' do
+      let(:parent_epic) { create(:epic, group: group) }
+      let(:epic) { build(:epic, group: group) }
+      let(:child_epic1) { create(:epic, group: group, parent: epic)}
+
+      it 'returns true when total depth after adding will not exceed limit' do
+        epic.parent = parent_epic
+
+        expect(epic.valid_parent?).to be_truthy
+      end
+
+      it 'returns false when total depth after adding would exceed limit' do
+        child_epic2 = create(:epic, group: group, parent: child_epic1)
+        child_epic3 = create(:epic, group: group, parent: child_epic2)
+        child_epic4 = create(:epic, group: group, parent: child_epic3)
+        create(:epic, group: group, parent: child_epic4)
+
+        epic.parent = parent_epic
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+    end
+
+    context 'when parent has ancestors and epic has children' do
+      let(:root_epic) { create(:epic, group: group) }
+      let(:parent_epic) { create(:epic, group: group, parent: root_epic) }
+      let(:epic) { build(:epic, group: group) }
+      let(:child_epic1) { create(:epic, group: group, parent: epic)}
+
+      it 'returns true when total depth after adding will not exceed limit' do
+        epic.parent = parent_epic
+
+        expect(epic.valid_parent?).to be_truthy
+      end
+
+      it 'returns false when total depth after adding would exceed limit' do
+        root_epic.update(parent: create(:epic, group: group))
+        create(:epic, group: group, parent: child_epic1)
+
+        epic.parent = parent_epic
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+    end
+
+    context 'when hierarchy is cyclic' do
+      let(:epic) { create(:epic, group: group) }
+
+      it 'returns false when parent is same as the epic' do
+        epic.parent = epic
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+
+      it 'returns false when child epic is parent of the given parent' do
+        epic1 = create(:epic, group: group, parent: epic)
+        epic.parent = epic1
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+
+      it 'returns false when child epic is an ancestor of the given parent' do
+        epic1 = create(:epic, group: group, parent: epic)
+        epic2 = create(:epic, group: group, parent: epic1)
+        epic.parent = epic2
+
+        expect(epic.valid_parent?).to be_falsey
+      end
+    end
+  end
+
   context 'hierarchy' do
     let(:epic1) { create(:epic, group: group) }
     let(:epic2) { create(:epic, group: group, parent: epic1) }

ee/spec/requests/api/epic_links_spec.rb

@@ -62,7 +62,7 @@ describe API::EpicLinks do
     end
   end
 
-  describe 'POST /groups/:id/epics/:epic_iid/epics' do
+  describe 'POST /groups/:id/epics/:epic_iid/epics/child_epic_id' do
     let(:child_epic) { create(:epic, group: group) }
     let(:url) { "/groups/#{group.path}/epics/#{epic.iid}/epics/#{child_epic.id}" }
 
@@ -112,6 +112,42 @@ describe API::EpicLinks do
     end
   end
 
+  describe 'POST /groups/:id/epics/:epic_iid/epics' do
+    let(:url) { "/groups/#{group.path}/-/epics/#{epic.iid}/epics" }
+
+    subject { post api(url, user), params: { title: 'child epic' } }
+
+    it_behaves_like 'user does not have access'
+
+    context 'when epics feature is enabled' do
+      before do
+        stub_licensed_features(epics: true)
+      end
+
+      context 'when user is guest' do
+        it 'returns 403' do
+          group.add_guest(user)
+
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+
+      context 'when user is developer' do
+        it 'returns 201 status' do
+          group.add_developer(user)
+
+          subject
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(response).to match_response_schema('public_api/v4/linked_epic', dir: 'ee')
+          expect(epic.reload.children).to include(Epic.last)
+        end
+      end
+    end
+  end
+
   describe 'PUT /groups/:id/epics/:epic_iid/epics/:child_epic_id' do
     let!(:child_epic) { create(:epic, group: group, parent: epic, relative_position: 100) }
     let!(:sibling_1) { create(:epic, group: group, parent: epic, relative_position: 200) }

ee/spec/requests/api/epics_spec.rb

@@ -406,13 +406,15 @@ describe API::Epics do
 
   describe 'POST /groups/:id/epics' do
     let(:url) { "/groups/#{group.path}/epics" }
+    let(:parent_epic) { create(:epic, group: group) }
     let(:params) do
       {
         title: 'new epic',
         description: 'epic description',
         labels: 'label1',
         due_date_fixed: '2018-07-17',
-        due_date_is_fixed: true
+        due_date_is_fixed: true,
+        parent_id: parent_epic.id
       }
     end
 
@@ -455,6 +457,8 @@ describe API::Epics do
           expect(epic.due_date_fixed).to eq(Date.new(2018, 7, 17))
           expect(epic.due_date_is_fixed).to eq(true)
           expect(epic.labels.first.title).to eq('label1')
+          expect(epic.parent).to eq(parent_epic)
+          expect(epic.relative_position).not_to be_nil
         end
 
         context 'when deprecated start_date and end_date params are present' do

ee/spec/services/epic_links/create_service_spec.rb

@@ -148,6 +148,25 @@ describe EpicLinks::CreateService, :postgresql do
           it 'returns success status' do
             expect(subject).to eq(status: :success)
           end
+
+          it 'avoids un-necessary database queries' do
+            epic1 = create(:epic, group: group)
+
+            # Establish baseline
+            add_epic([valid_reference])
+
+            control = ActiveRecord::QueryRecorder.new { add_epic([epic1.to_reference(full: true)]) }
+
+            new_epics = [create(:epic, group: group), create(:epic, group: group)]
+
+            # threshold is 6 because
+            # 1. we need to check hierarchy for each child epic (3 queries)
+            # 2. we have to update the  record (2 including releasing savepoint)
+            # 3. we have to update start and due dates for all updated epics
+            expect do
+              ActiveRecord::QueryRecorder.new { add_epic(new_epics.map { |epic| epic.to_reference(full: true) }) }
+            end.not_to exceed_query_limit(control).with_threshold(6)
+          end
         end
 
         context 'when at least one epic is still not assigned to the parent epic' do
@@ -190,55 +209,6 @@ describe EpicLinks::CreateService, :postgresql do
           end
         end
 
-        context 'when adding to an Epic that is already at maximum depth' do
-          before do
-            epic1 = create(:epic, group: group)
-            epic2 = create(:epic, group: group, parent: epic1)
-            epic3 = create(:epic, group: group, parent: epic2)
-            epic4 = create(:epic, group: group, parent: epic3)
-
-            epic.update(parent: epic4)
-          end
-
-          subject { add_epic([valid_reference]) }
-
-          it 'returns an error' do
-            expect(subject).to eq(message: 'Epic hierarchy level too deep', status: :error, http_status: 409)
-          end
-
-          it 'no relationship is created' do
-            expect { subject }.not_to change { epic.children.count }
-          end
-        end
-
-        context 'when adding an Epic that has existing children' do
-          subject { add_epic([valid_reference]) }
-
-          context 'when total depth after adding would exceed limit' do
-            before do
-              epic1 = create(:epic, group: group)
-
-              epic.update(parent: epic1) # epic is on level 2
-
-              # epic_to_add has 3 children (level 4 including epic_to_add)
-              # that would mean level 6 after relating epic_to_add on epic
-              epic2 = create(:epic, group: group, parent: epic_to_add)
-              epic3 = create(:epic, group: group, parent: epic2)
-              create(:epic, group: group, parent: epic3)
-            end
-
-            include_examples 'returns not found error'
-          end
-
-          context 'when Epic to add has more than 5 children' do
-            before do
-              create_list(:epic, 8, group: group, parent: epic_to_add)
-            end
-
-            include_examples 'returns success'
-          end
-        end
-
         context 'when an epic is already assigned to another epic' do
           let(:another_epic) { create(:epic, group: group) }
 

ee/spec/services/epics/create_service_spec.rb

@@ -3,7 +3,8 @@ require 'spec_helper'
 describe Epics::CreateService do
   let(:group) { create(:group, :internal)}
   let(:user) { create(:user) }
-  let(:params) { { title: 'new epic', description: 'epic description' } }
+  let!(:parent_epic) { create(:epic, group: group) }
+  let(:params) { { title: 'new epic', description: 'epic description', parent_id: parent_epic.id } }
 
   subject { described_class.new(group, user, params).execute }
 
@@ -11,17 +12,15 @@ describe Epics::CreateService do
     it 'creates one epic correctly' do
       allow(NewEpicWorker).to receive(:perform_async)
 
-      expect { subject }.to change { Epic.count }.from(0).to(1)
+      expect { subject }.to change { Epic.count }.by(1)
 
       epic = Epic.last
       expect(epic).to be_persisted
       expect(epic.title).to eq('new epic')
       expect(epic.description).to eq('epic description')
+      expect(epic.parent).to eq(parent_epic)
+      expect(epic.relative_position).not_to be_nil
       expect(NewEpicWorker).to have_received(:perform_async).with(epic.id, user.id)
     end
-
-    it_behaves_like 'new issuable with scoped labels' do
-      let(:parent) { group }
-    end
   end
 end