Add support for relative ordering between classes

- add support for ordering of objects
from 2 different classes

db/post_migrate/20190926180443_move_epic_issues_after_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 MoveEpicIssuesAfterEpics < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 1_000
+
+  disable_ddl_transaction!
+
+  class Epic < ActiveRecord::Base
+    self.table_name = 'epics'
+  end
+
+  class EpicIssue < ActiveRecord::Base
+    self.table_name = 'epic_issues'
+
+    include ::EachBatch
+  end
+
+  def up
+    maximum_epic_position = Epic.maximum(:relative_position)
+
+    return unless maximum_epic_position
+
+    max_position = Gitlab::Database::MAX_INT_VALUE
+
+    delta = ((maximum_epic_position - max_position) / 2.0).abs.ceil
+
+    EpicIssue.where('relative_position < ?', maximum_epic_position).each_batch(of: BATCH_SIZE) do |batch, _|
+      batch.update_all("relative_position = relative_position + #{delta}")
+    end
+  end
+
+  def down
+    # no need
+  end
+end

ee/app/models/concerns/epic_tree_sorting.rb

+# frozen_string_literal: true
+
+module EpicTreeSorting
+  extend ActiveSupport::Concern
+  include FromUnion
+  include RelativePositioning
+
+  class_methods do
+    def relative_positioning_query_base(object)
+      from_union([
+        EpicIssue.select("id, relative_position, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
+        Epic.select("id, relative_position, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
+      ])
+    end
+
+    def relative_positioning_parent_column
+      :epic_id
+    end
+  end
+
+  included do
+    def move_sequence(start_pos, end_pos, delta)
+      items_to_update = scoped_items
+        .where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
+
+      items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
+        items = type.camelcase.constantize.where(id: group_items.map(&:id))
+        items = items.where.not(id: self.id) if type == self.class.underscore
+        items.update_all("relative_position = relative_position + #{delta}")
+      end
+    end
+  end
+end

ee/app/models/ee/epic.rb

@@ -12,9 +12,9 @@ module EE
       include Referable
       include Awardable
       include LabelEventable
-      include RelativePositioning
       include UsageStatistics
       include FromUnion
+      include EpicTreeSorting
 
       enum state_id: {
         opened: ::Epic.available_states[:opened],
@@ -177,14 +177,6 @@ module EE
         ::Group
       end
 
-      def relative_positioning_query_base(epic)
-        in_parents(epic.parent_ids)
-      end
-
-      def relative_positioning_parent_column
-        :parent_id
-      end
-
       # Return the deepest relation level for an epic.
       # Example 1:
       # epic1 - parent: nil

ee/app/models/epic_issue.rb

 # frozen_string_literal: true
 
 class EpicIssue < ApplicationRecord
-  include RelativePositioning
+  include EpicTreeSorting
 
   validates :epic, :issue, presence: true
   validates :issue, uniqueness: true
@@ -12,12 +12,4 @@ class EpicIssue < ApplicationRecord
   alias_attribute :parent_ids, :epic_id
 
   scope :in_epic, ->(epic_id) { where(epic_id: epic_id) }
-
-  def self.relative_positioning_query_base(epic_issue)
-    in_epic(epic_issue.parent_ids)
-  end
-
-  def self.relative_positioning_parent_column
-    :epic_id
-  end
 end

ee/app/services/epics/tree_reorder_service.rb

@@ -11,40 +11,39 @@ module Epics
     end
 
     def execute
-      klass = case moving_object
-              when EpicIssue
-                EpicIssues::UpdateService
-              when Epic
-                EpicLinks::UpdateService
-              end
-
-      return error('Only epics and epic_issues are supported.') unless klass
-
       error_message = validate_objects
 
       return error(error_message) if error_message.present?
 
-      klass.new(moving_object, current_user, moving_params).execute
+      move!
+      success
     end
 
     private
 
-    def moving_params
-      key = case params[:relative_position].to_sym
-            when :after
-              :move_after_id
-            when :before
-              :move_before_id
+    def move!
+      moving_object.move_between(before_object, after_object)
+      moving_object.save!(touch: false)
     end
 
-      {}.tap { |p| p[key] = adjacent_reference.id }
+    def before_object
+      return unless params[:relative_position].to_sym == :before
+
+      adjacent_reference
+    end
+
+    def after_object
+      return unless params[:relative_position].to_sym == :after
+
+      adjacent_reference
     end
 
-    # for now we support only ordering within the same type
-    # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
     def validate_objects
+      unless moving_object.is_a?(EpicIssue) || moving_object.is_a?(Epic)
+        return 'Only epics and epic_issues are supported.'
+      end
+
       return 'You don\'t have permissions to move the objects.' unless authorized?
-      return 'Provided objects are not the same type.' if moving_object.class != adjacent_reference.class
     end
 
     def authorized?

ee/spec/models/concerns/epic_tree_sorting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EpicTreeSorting do
+  let(:group) { create(:group) }
+  let(:base_epic) { create(:epic, group: group) }
+  let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 10) }
+  let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 500) }
+  let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1002) }
+  let!(:epic_issue4) { create(:epic_issue, epic: base_epic, relative_position: 1500) }
+  let!(:epic1) { create(:epic, parent: base_epic, group: group, relative_position: 100) }
+  let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1000) }
+  let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1001) }
+
+  context '#move_after' do
+    it 'moves an epic' do
+      epic1.move_after(epic_issue2)
+
+      expect(epic1.relative_position).to be_between(epic_issue2.reload.relative_position, epic2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue2.move_after(epic2)
+
+      expect(epic_issue2.relative_position).to be_between(epic2.reload.relative_position, epic3.reload.relative_position).exclusive
+    end
+  end
+
+  context '#move_before' do
+    it 'moves an epic' do
+      epic2.move_before(epic_issue2)
+
+      expect(epic2.relative_position).to be_between(epic_issue1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue3.move_before(epic2)
+
+      expect(epic_issue3.relative_position).to be_between(epic_issue2.reload.relative_position, epic2.reload.relative_position).exclusive
+    end
+  end
+
+  context '#move_between' do
+    it 'moves an epic' do
+      epic1.move_between(epic_issue1, epic_issue2)
+
+      expect(epic1.relative_position).to be_between(epic_issue1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+
+    it 'moves an epic_issue' do
+      epic_issue3.move_between(epic1, epic_issue2)
+
+      expect(epic_issue3.relative_position).to be_between(epic1.reload.relative_position, epic_issue2.reload.relative_position).exclusive
+    end
+  end
+end

ee/spec/models/epic_issue_spec.rb

@@ -10,4 +10,31 @@ describe EpicIssue do
       let(:default_params) { { epic: epic } }
     end
   end
+
+  context 'relative positioning with 2 classes' do
+    let(:group) { create(:group) }
+    let(:base_epic) { create(:epic, group: group) }
+    let!(:epic_issue1) { create(:epic_issue, epic: base_epic, relative_position: 10) }
+    let!(:epic_issue2) { create(:epic_issue, epic: base_epic, relative_position: 50) }
+    let!(:epic_issue3) { create(:epic_issue, epic: base_epic, relative_position: 1500) }
+    let!(:epic1) { create(:epic, parent: base_epic, group: group, relative_position: 1000) }
+    let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 2000) }
+
+    context '#move_after' do
+      it 'moves the epic after an epic_issue' do
+        epic1.move_after(epic_issue3)
+
+        expect(epic1.relative_position).to be > epic_issue3.relative_position
+      end
+    end
+
+    context '#move_between' do
+      it 'moves the epic between epic_issues' do
+        epic1.move_between(epic_issue1, epic_issue2)
+
+        expect(epic1.relative_position).to be > epic_issue1.relative_position
+        expect(epic1.relative_position).to be < epic_issue2.relative_position
+      end
+    end
+  end
 end

ee/spec/services/epics/tree_reorder_service_spec.rb

@@ -12,8 +12,8 @@ describe Epics::TreeReorderService do
     let(:issue2) { create(:issue, project: project) }
     let(:epic1) { create(:epic, group: group, parent: epic, relative_position: 10) }
     let(:epic2) { create(:epic, group: group, parent: epic, relative_position: 20) }
-    let(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue1, relative_position: 10) }
-    let(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 20) }
+    let(:epic_issue1) { create(:epic_issue, epic: epic, issue: issue1, relative_position: 30) }
+    let(:epic_issue2) { create(:epic_issue, epic: epic, issue: issue2, relative_position: 40) }
 
     let(:relative_position) { :after }
     let!(:tree_object_1) { epic1 }
@@ -32,10 +32,8 @@ describe Epics::TreeReorderService do
 
     shared_examples 'error for the tree update' do |expected_error|
       it 'does not change relative_positions' do
-        subject
-
-        expect(tree_object_1.relative_position).to eq(10)
-        expect(tree_object_2.relative_position).to eq(20)
+        expect { subject }.not_to change { tree_object_1.relative_position }
+        expect { subject }.not_to change { tree_object_2.relative_position }
       end
 
       it 'returns error status' do
@@ -69,12 +67,16 @@ describe Epics::TreeReorderService do
           let!(:tree_object_1) { epic_issue1 }
           let!(:tree_object_2) { epic_issue2 }
 
-          # for now we support only ordering within the same type
-          # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
           context 'when object being moved is not the same type as the switched object' do
+            let!(:tree_object_3) { epic1 }
+            let!(:tree_object_4) { epic2 }
             let(:adjacent_reference_id) { GitlabSchema.id_from_object(epic2) }
 
-            it_behaves_like 'error for the tree update', 'Provided objects are not the same type.'
+            it 'reorders the objects' do
+              subject
+
+              expect(epic2.reload.relative_position).to be > tree_object_2.reload.relative_position
+            end
           end
 
           context 'when no object to switch is provided' do
@@ -117,14 +119,6 @@ describe Epics::TreeReorderService do
           let!(:tree_object_1) { epic1 }
           let!(:tree_object_2) { epic2 }
 
-          # for now we support only ordering within the same type
-          # Follow-up issue: https://gitlab.com/gitlab-org/gitlab/issues/13633
-          context 'when object being moved is not the same type as the switched object' do
-            let(:adjacent_reference_id) { GitlabSchema.id_from_object(epic_issue2) }
-
-            it_behaves_like 'error for the tree update', 'Provided objects are not the same type.'
-          end
-
           context 'when the reordered epics are not subepics of the base epic' do
             let(:another_group) { create(:group) }
             let(:another_epic) { create(:epic, group: another_group) }