Merge branch '7328-reorder-epics-in-an-epic' into 'master'

Add reordering of child epics

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

db/schema.rb

@@ -1116,6 +1116,7 @@ ActiveRecord::Schema.define(version: 20190220150130) do
     t.integer "closed_by_id"
     t.datetime "closed_at"
     t.integer "parent_id"
+    t.integer "relative_position"
     t.index ["assignee_id"], name: "index_epics_on_assignee_id", using: :btree
     t.index ["author_id"], name: "index_epics_on_author_id", using: :btree
     t.index ["closed_by_id"], name: "index_epics_on_closed_by_id", using: :btree

doc/user/group/epics/index.md

@@ -73,9 +73,9 @@ These are dynamic dates in that if milestones are re-assigned to the issues, if
 milestone dates change, or if issues are added or removed from the epic, then
 the re-calculation will happen immediately to set a new dynamic date.
 
-## Reordering issues in an epic
+## Reordering issues and child epics
 
-Drag and drop to reorder issues in an epic. New issues added to an epic appear at the top of the list.
+Drag and drop to reorder issues and child epics. New issues and child epics added to an epic appear at the top of the list.
 
 ## Deleting an epic
 

ee/app/assets/javascripts/related_issues/components/related_issues_block.vue

@@ -124,8 +124,8 @@ export default {
       const nextItemEl = itemEl.nextElementSibling;
 
       return {
-        beforeId: prevItemEl && parseInt(prevItemEl.dataset.epicIssueId, 0),
-        afterId: nextItemEl && parseInt(nextItemEl.dataset.epicIssueId, 0),
+        beforeId: prevItemEl && parseInt(prevItemEl.dataset.orderingId, 0),
+        afterId: nextItemEl && parseInt(nextItemEl.dataset.orderingId, 0),
       };
     },
     reordered(event) {
@@ -147,6 +147,9 @@ export default {
     removeDraggingCursor() {
       document.body.classList.remove('is-dragging');
     },
+    issuableOrderingId({ epic_issue_id: epicIssueId, id }) {
+      return this.issuableType === 'issue' ? epicIssueId : id;
+    },
   },
 };
 </script>
@@ -231,7 +234,7 @@ export default {
               'card-slim': canReorder,
             }"
             :data-key="issue.id"
-            :data-epic-issue-id="issue.epic_issue_id"
+            :data-ordering-id="issuableOrderingId(issue)"
             class="js-related-issues-token-list-item list-item pt-0 pb-0"
           >
             <issue-item

ee/app/controllers/groups/epic_links_controller.rb

@@ -5,6 +5,12 @@ class Groups::EpicLinksController < Groups::EpicsController
 
   before_action :check_nested_support!
 
+  def update
+    result = EpicLinks::UpdateService.new(child_epic, current_user, params[:epic]).execute
+
+    render json: { message: result[:message] }, status: result[:http_status]
+  end
+
   def destroy
     result = ::Epics::UpdateService.new(group, current_user, { parent: nil }).execute(child_epic)
 

ee/app/models/ee/epic.rb

@@ -13,6 +13,7 @@ module EE
       include Awardable
       include LabelEventable
       include Descendant
+      include RelativePositioning
 
       enum state: { opened: 1, closed: 2 }
 
@@ -44,6 +45,10 @@ module EE
 
       validates :group, presence: true
 
+      alias_attribute :parent_ids, :parent_id
+
+      scope :in_parents, -> (parent_ids) { where(parent_id: parent_ids) }
+
       scope :order_start_or_end_date_asc, -> do
         # mysql returns null values first in opposite to postgres which
         # returns them last by default
@@ -67,6 +72,10 @@ module EE
         reorder(::Gitlab::Database.nulls_last_order('start_date', 'DESC'), 'id DESC')
       end
 
+      scope :order_relative_position, -> do
+        reorder('relative_position ASC', 'id DESC')
+      end
+
       def etag_caching_enabled?
         true
       end
@@ -118,6 +127,7 @@ module EE
         when 'start_date_desc' then order_start_date_desc
         when 'end_date_asc' then order_end_date_asc
         when 'end_date_desc' then order_end_date_desc
+        when 'relative_position' then order_relative_position
         else
           super
         end
@@ -127,6 +137,11 @@ module EE
         ::Group
       end
 
+      # Column name used by RelativePositioning for scoping. This is not related to `parent_class` above.
+      def parent_column
+        :parent_id
+      end
+
       # Return the deepest relation level for an epic.
       # Example 1:
       # epic1 - parent: nil

ee/app/services/epic_links/create_service.rb

@@ -14,10 +14,17 @@ module EpicLinks
       affected_epics = [issuable]
       affected_epics << referenced_epic if referenced_epic.parent
 
-      referenced_epic.update(parent: issuable)
+      set_child_epic!(referenced_epic)
+
       affected_epics.each(&:update_start_and_due_dates)
     end
 
+    def set_child_epic!(child_epic)
+      child_epic.parent = issuable
+      child_epic.move_to_start
+      child_epic.save!
+    end
+
     def linkable_issuables(epics)
       @linkable_issuables ||= begin
         return [] unless can?(current_user, :admin_epic, issuable.group)

ee/app/services/epic_links/list_service.rb

@@ -9,7 +9,7 @@ module EpicLinks
     def child_issuables
       return [] unless issuable&.group&.feature_available?(:epics)
 
-      EpicsFinder.new(current_user, parent_id: issuable.id, group_id: issuable.group.id).execute
+      EpicsFinder.new(current_user, parent_id: issuable.id, group_id: issuable.group.id, sort: 'relative_position').execute
     end
 
     override :serializer

ee/app/services/epic_links/update_service.rb

+# frozen_string_literal: true
+
+module EpicLinks
+  class UpdateService < BaseService
+    attr_reader :epic
+    private :epic
+
+    def initialize(epic, user, params)
+      @epic = epic
+      @current_user = user
+      @params = params
+    end
+
+    def execute
+      move_epic!
+      success
+    rescue ActiveRecord::RecordNotFound
+      error('Epic not found for given params', 404)
+    end
+
+    private
+
+    def move_epic!
+      return unless params[:move_after_id] || params[:move_before_id]
+
+      before_epic = Epic.in_parents(epic.parent_id).find(params[:move_before_id]) if params[:move_before_id]
+      after_epic = Epic.in_parents(epic.parent_id).find(params[:move_after_id]) if params[:move_after_id]
+
+      epic.move_between(before_epic, after_epic)
+      epic.save!
+    end
+  end
+end

ee/changelogs/unreleased/7328-epic-in-epic.yml

+---
+title: Add reordering of child epics
+merge_request: 9283
+author:
+type: added

ee/db/migrate/20190128104236_add_relative_position_to_epics.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRelativePositionToEpics < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    add_column :epics, :relative_position, :integer
+  end
+
+  def down
+    remove_column :epics, :relative_position
+  end
+end

ee/db/migrate/20190218031401_set_default_position_for_child_epics.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class SetDefaultPositionForChildEpics < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    current_position = nil
+    current_parent_id = nil
+
+    connection.exec_query(existing_child_epics_query.to_sql).rows.each do |id, parent_id|
+      if parent_id != current_parent_id
+        current_position = Gitlab::Database::MAX_INT_VALUE / 2
+        current_parent_id = parent_id
+      else
+        current_position += 500
+      end
+
+      update_position(id, current_position)
+    end
+  end
+
+  def down
+  end
+
+  private
+
+  def epics_table
+    @epics_table ||= Arel::Table.new(:epics)
+  end
+
+  def existing_child_epics_query
+    epics_table.project(epics_table[:id], epics_table[:parent_id])
+      .where(epics_table[:parent_id].not_eq(nil))
+      .order(epics_table[:parent_id], epics_table[:id].desc)
+  end
+
+  def update_position(epic_id, position)
+    execute "UPDATE epics SET relative_position = #{position} WHERE id = #{epic_id}"
+  end
+end

ee/spec/controllers/groups/epic_links_controller_spec.rb

@@ -98,6 +98,61 @@ describe Groups::EpicLinksController, :postgresql do
     end
   end
 
+  describe 'PUT #update' do
+    before do
+      epic1.update(parent: parent_epic)
+      epic2.update(parent: parent_epic)
+    end
+
+    let(:move_before_epic) { epic2 }
+
+    subject do
+      put :update, params: { group_id: group, epic_id: parent_epic.to_param, id: epic1.id, epic: { move_before_id: move_before_epic.id } }
+    end
+
+    it_behaves_like 'unlicensed epics action'
+
+    context 'when epics are enabled' do
+      before do
+        stub_licensed_features(epics: true)
+      end
+
+      context 'when user has permissions to reorder epics' do
+        before do
+          group.add_developer(user)
+        end
+
+        it 'returns status 200' do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+
+        it 'updates the epic position' do
+          expect { subject }.to change { epic1.reload.relative_position }
+        end
+
+        context 'when move_before_id is not a sibling epic' do
+          let(:move_before_epic) { create(:epic, group: group) }
+
+          it 'returns status 404' do
+            subject
+
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
+      end
+
+      context 'when user does not have permissions to reorder epics' do
+        it 'returns status 403' do
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+  end
+
   describe 'DELETE #destroy' do
     before do
       epic1.update(parent: parent_epic)

ee/spec/features/epics/epic_issues_spec.rb

@@ -18,6 +18,13 @@ describe 'Epic Issues', :js do
     ]
   end
 
+  let!(:nested_epics) do
+    [
+      create(:epic, group: group, parent_id: epic.id, relative_position: 1),
+      create(:epic, group: group, parent_id: epic.id, relative_position: 2)
+    ]
+  end
+
   def visit_epic
     stub_licensed_features(epics: true)
 
@@ -43,14 +50,23 @@ describe 'Epic Issues', :js do
       expect(page).not_to have_selector('.js-related-issues-block h3.card-title button')
     end
 
+    it 'user cannot add new epics to the epic', :postgresql do
+      expect(page).not_to have_selector('.js-related-epics-block h3.card-title button')
+    end
+
     it 'user cannot reorder issues in epic' do
       expect(page).not_to have_selector('.js-related-issues-block .js-related-issues-token-list-item.user-can-drag')
     end
+
+    it 'user cannot reorder epics in epic', :postgresql do
+      expect(page).not_to have_selector('.js-related-epics-block .js-related-epics-token-list-item.user-can-drag')
+    end
   end
 
   context 'when user is a group member' do
     let(:issue_to_add) { create(:issue, project: private_project) }
     let(:issue_invalid) { create(:issue) }
+    let(:epic_to_add) { create(:epic, group: group) }
 
     def add_issues(references)
       find('.js-related-issues-block h3.card-title button').click
@@ -64,6 +80,16 @@ describe 'Epic Issues', :js do
       wait_for_requests
     end
 
+    def add_epics(references)
+      find('.js-related-epics-block h3.card-title button').click
+      find('.js-related-epics-block .js-add-issuable-form-input').set(references)
+
+      find('.js-related-epics-block .js-add-issuable-form-input').send_keys(:tab)
+      find('.js-related-epics-block .js-add-issuable-form-add-button').click
+
+      wait_for_requests
+    end
+
     before do
       group.add_developer(user)
       visit_epic
@@ -85,6 +111,22 @@ describe 'Epic Issues', :js do
       end
     end
 
+    it 'user can see all epics of the group and delete the associations', :postgresql do
+      within('.js-related-epics-block ul.related-items-list') do
+        expect(page).to have_selector('li', count: 2)
+        expect(page).to have_content(nested_epics[0].title)
+        expect(page).to have_content(nested_epics[1].title)
+
+        first('li button.js-issue-item-remove-button').click
+      end
+
+      wait_for_requests
+
+      within('.js-related-epics-block ul.related-items-list') do
+        expect(page).to have_selector('li', count: 1)
+      end
+    end
+
     it 'user cannot add new issues to the epic from another group' do
       add_issues("#{issue_invalid.to_reference(full: true)}")
 
@@ -106,6 +148,19 @@ describe 'Epic Issues', :js do
       end
     end
 
+    it 'user can add new epics to the epic', :postgresql do
+      references = "#{epic_to_add.to_reference(full: true)}"
+      add_epics(references)
+
+      expect(page).not_to have_selector('.content-wrapper .alert-wrapper .flash-text')
+      expect(page).not_to have_content('No Epic found for given params')
+
+      within('.js-related-epics-block ul.related-items-list') do
+        expect(page).to have_selector('li', count: 3)
+        expect(page).to have_content(epic_to_add.title)
+      end
+    end
+
     it 'user can reorder issues in epic' do
       expect(first('.js-related-issues-block .js-related-issues-token-list-item')).to have_content(public_issue.title)
       expect(page.all('.js-related-issues-block .js-related-issues-token-list-item').last).to have_content(private_issue.title)
@@ -115,5 +170,15 @@ describe 'Epic Issues', :js do
       expect(first('.js-related-issues-block .js-related-issues-token-list-item')).to have_content(private_issue.title)
       expect(page.all('.js-related-issues-block .js-related-issues-token-list-item').last).to have_content(public_issue.title)
     end
+
+    it 'user can reorder epics in epic', :postgresql do
+      expect(first('.js-related-epics-block .js-related-issues-token-list-item')).to have_content(nested_epics[0].title)
+      expect(page.all('.js-related-epics-block .js-related-issues-token-list-item').last).to have_content(nested_epics[1].title)
+
+      drag_to(selector: '.js-related-epics-block .related-items-list', to_index: 1)
+
+      expect(first('.js-related-epics-block .js-related-issues-token-list-item')).to have_content(nested_epics[1].title)
+      expect(page.all('.js-related-epics-block .js-related-issues-token-list-item').last).to have_content(nested_epics[0].title)
+    end
   end
 end

ee/spec/javascripts/issuable/related_issues/components/related_issues_block_spec.js

@@ -173,4 +173,72 @@ describe('RelatedIssuesBlock', () => {
       });
     });
   });
+
+  describe('issuableOrderingId returns correct issuable order id when', () => {
+    it('issuableType is epic', () => {
+      vm = new RelatedIssuesBlock({
+        propsData: {
+          pathIdSeparator: '#',
+          issuableType: 'issue',
+        },
+      }).$mount();
+
+      const orderId = vm.issuableOrderingId(issuable1);
+
+      expect(orderId).toBe(issuable1.epic_issue_id);
+    });
+
+    it('issuableType is issue', () => {
+      vm = new RelatedIssuesBlock({
+        propsData: {
+          pathIdSeparator: '#',
+          issuableType: 'epic',
+        },
+      }).$mount();
+
+      const orderId = vm.issuableOrderingId(issuable1);
+
+      expect(orderId).toBe(issuable1.id);
+    });
+  });
+
+  describe('renders correct ordering id when', () => {
+    let relatedIssues;
+
+    beforeAll(() => {
+      relatedIssues = [issuable1, issuable2, issuable3, issuable4, issuable5];
+    });
+
+    it('issuableType is epic', () => {
+      vm = new RelatedIssuesBlock({
+        propsData: {
+          pathIdSeparator: '#',
+          issuableType: 'epic',
+          relatedIssues,
+        },
+      }).$mount();
+
+      const listItems = vm.$el.querySelectorAll('.list-item');
+
+      Array.from(listItems).forEach((item, index) => {
+        expect(Number(item.dataset.orderingId)).toBe(relatedIssues[index].id);
+      });
+    });
+
+    it('issuableType is issue', () => {
+      vm = new RelatedIssuesBlock({
+        propsData: {
+          pathIdSeparator: '#',
+          issuableType: 'issue',
+          relatedIssues,
+        },
+      }).$mount();
+
+      const listItems = vm.$el.querySelectorAll('.list-item');
+
+      Array.from(listItems).forEach((item, index) => {
+        expect(Number(item.dataset.orderingId)).toBe(relatedIssues[index].epic_issue_id);
+      });
+    });
+  });
 });

ee/spec/models/epic_spec.rb

@@ -33,10 +33,10 @@ describe Epic do
   end
 
   describe 'ordering' do
-    let!(:epic1) { create(:epic, start_date: 7.days.ago, end_date: 3.days.ago, updated_at: 3.days.ago, created_at: 7.days.ago) }
-    let!(:epic2) { create(:epic, start_date: 3.days.ago, updated_at: 10.days.ago, created_at: 12.days.ago) }
-    let!(:epic3) { create(:epic, end_date: 5.days.ago, updated_at: 5.days.ago, created_at: 6.days.ago) }
-    let!(:epic4) { create(:epic) }
+    let!(:epic1) { create(:epic, start_date: 7.days.ago, end_date: 3.days.ago, updated_at: 3.days.ago, created_at: 7.days.ago, relative_position: 3) }
+    let!(:epic2) { create(:epic, start_date: 3.days.ago, updated_at: 10.days.ago, created_at: 12.days.ago, relative_position: 1) }
+    let!(:epic3) { create(:epic, end_date: 5.days.ago, updated_at: 5.days.ago, created_at: 6.days.ago, relative_position: 2) }
+    let!(:epic4) { create(:epic, relative_position: 4) }
 
     def epics(order_by)
       described_class.order_by(order_by)
@@ -77,6 +77,10 @@ describe Epic do
     it 'orders by created_at DESC' do
       expect(epics(:created_desc)).to eq([epic4, epic3, epic1, epic2])
     end
+
+    it 'orders by relative_position ASC' do
+      expect(epics(:relative_position)).to eq([epic2, epic3, epic1, epic4])
+    end
   end
 
   describe '#ancestors', :nested_groups do

ee/spec/services/epic_links/create_service_spec.rb

@@ -18,6 +18,14 @@ describe EpicLinks::CreateService, :postgresql do
         expect(epic.reload.children).to include(epic_to_add)
       end
 
+      it 'moves the new child epic to the top and moves the existing ones down' do
+        existing_child_epic = create(:epic, group: group, parent: epic, relative_position: 1000)
+
+        subject
+
+        expect(epic_to_add.reload.relative_position).to be < existing_child_epic.reload.relative_position
+      end
+
       it 'returns success status' do
         expect(subject).to eq(status: :success)
       end

ee/spec/services/epic_links/list_service_spec.rb

@@ -7,8 +7,8 @@ describe EpicLinks::ListService, :postgresql do
   let(:group) { create(:group, :public) }
   let(:parent_epic) { create(:epic, group: group) }
 
-  let!(:epic1) { create :epic, group: group, parent: parent_epic }
-  let!(:epic2) { create :epic, group: group, parent: parent_epic }
+  let!(:epic1) { create :epic, group: group, parent: parent_epic, relative_position: 1 }
+  let!(:epic2) { create :epic, group: group, parent: parent_epic, relative_position: 2 }
 
   def epics_to_results(epics)
     epics.map do |epic|
@@ -47,22 +47,22 @@ describe EpicLinks::ListService, :postgresql do
         it 'returns related issues JSON' do
           expected_result = epics_to_results([epic1, epic2])
 
-          expect(subject).to match_array(expected_result)
+          expect(subject).to eq(expected_result)
         end
       end
 
       context 'with nested groups' do
         let(:subgroup1) { create(:group, :private, parent: group) }
         let(:subgroup2) { create(:group, :private, parent: group) }
-        let!(:epic_subgroup1) { create :epic, group: subgroup1, parent: parent_epic }
-        let!(:epic_subgroup2) { create :epic, group: subgroup2, parent: parent_epic }
+        let!(:epic_subgroup1) { create :epic, group: subgroup1, parent: parent_epic, relative_position: 10 }
+        let!(:epic_subgroup2) { create :epic, group: subgroup2, parent: parent_epic, relative_position: 5 }
 
         it 'returns all child epics for a group member' do
           group.add_developer(user)
 
-          expected_result = epics_to_results([epic1, epic2, epic_subgroup1, epic_subgroup2])
+          expected_result = epics_to_results([epic1, epic2, epic_subgroup2, epic_subgroup1])
 
-          expect(subject).to match_array(expected_result)
+          expect(subject).to eq(expected_result)
         end
 
         it 'returns only some child epics for a subgroup member' do
@@ -70,7 +70,7 @@ describe EpicLinks::ListService, :postgresql do
 
           expected_result = epics_to_results([epic1, epic2, epic_subgroup2])
 
-          expect(subject).to match_array(expected_result)
+          expect(subject).to eq(expected_result)
         end
       end
     end

ee/spec/services/epic_links/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EpicLinks::UpdateService do
+  let(:user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:parent_epic) { create(:epic, group: group) }
+
+  let(:child_epic1) { create(:epic, group: group, parent: parent_epic, relative_position: 1) }
+  let(:child_epic2) { create(:epic, group: group, parent: parent_epic, relative_position: 2) }
+  let(:child_epic3) { create(:epic, group: group, parent: parent_epic, relative_position: 300) }
+  let(:child_epic4) { create(:epic, group: group, parent: parent_epic, relative_position: 400) }
+
+  let(:epic_to_move) { child_epic3 }
+
+  subject do
+    described_class.new(epic_to_move, user, params).execute
+  end
+
+  def ordered_epics
+    Epic.where(parent_id: parent_epic.id).order('relative_position, id DESC')
+  end
+
+  describe '#execute' do
+    context 'when params are nil' do
+      let(:params) { { move_before_id: nil, move_after_id: nil } }
+
+      it 'does not change order of child epics' do
+        expect(subject).to include(status: :success)
+        expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic3, child_epic4])
+      end
+    end
+
+    context 'when moving to start' do
+      let(:params) { { move_before_id: nil, move_after_id: child_epic1.id } }
+
+      it 'reorders child epics' do
+        expect(subject).to include(status: :success)
+        expect(ordered_epics).to eq([child_epic3, child_epic1, child_epic2, child_epic4])
+      end
+    end
+
+    context 'when moving to end' do
+      let(:params) { { move_before_id: child_epic4.id, move_after_id: nil } }
+
+      it 'reorders child epics' do
+        expect(subject).to include(status: :success)
+        expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic4, child_epic3])
+      end
+    end
+
+    context 'when moving between siblings' do
+      let(:params) { { move_before_id: child_epic1.id, move_after_id: child_epic2.id } }
+
+      it 'reorders child epics' do
+        expect(subject).to include(status: :success)
+        expect(ordered_epics).to eq([child_epic1, child_epic3, child_epic2, child_epic4])
+      end
+    end
+
+    context 'when params are invalid' do
+      let(:other_epic) { create(:epic, group: group) }
+
+      shared_examples 'returns error' do
+        it 'does not change order of child epics and returns error' do
+          expect(subject).to include(message: 'Epic not found for given params', status: :error, http_status: 404)
+          expect(ordered_epics).to eq([child_epic1, child_epic2, child_epic3, child_epic4])
+        end
+      end
+
+      context 'when move_before_id is not a child of parent epic' do
+        let(:params) { { move_before_id: other_epic.id, move_after_id: child_epic2.id } }
+
+        it_behaves_like 'returns error'
+      end
+
+      context 'when move_after_id is not a child of parent epic' do
+        let(:params) { { move_before_id: child_epic1.id, move_after_id: other_epic.id } }
+
+        it_behaves_like 'returns error'
+      end
+    end
+  end
+end