Merge branch 'ajk-epic-tree-node-factory' into 'master'

Detect problems with polymorphic epic trees

See merge request gitlab-org/gitlab!42083

app/models/concerns/relative_positioning.rb

@@ -200,4 +200,10 @@ module RelativePositioning
   # Override if you want to be notified of failures to move
   def could_not_move(exception)
   end
+
+  # Override if the implementing class is not a simple application record, for
+  # example if the record is loaded from a union.
+  def reset_relative_position
+    reset.relative_position
+  end
 end

ee/app/models/concerns/epic_tree_sorting.rb

@@ -6,21 +6,66 @@ module EpicTreeSorting
   include RelativePositioning
 
   class_methods do
+    extend ::Gitlab::Utils::Override
+
     def relative_positioning_query_base(object)
+      # Only non-root nodes are sortable.
+      return none if object.root_epic_tree_node?
+
+      issue_type = EpicIssue.underscore
+      epic_type = Epic.underscore
+
+      issue_selection = <<~SELECT_LIST
+        id, relative_position, epic_id as parent_id, epic_id, '#{issue_type}' as object_type
+      SELECT_LIST
+      epic_selection = <<~SELECT_LIST
+        id, relative_position, parent_id, parent_id as epic_id, '#{epic_type}' as object_type
+      SELECT_LIST
+
       from_union([
-        EpicIssue.select("id, relative_position, epic_id as parent_id, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
-        Epic.select("id, relative_position, parent_id, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
+        EpicIssue.select(issue_selection).in_epic(object.parent_ids),
+        Epic.select(epic_selection).in_parents(object.parent_ids)
       ])
     end
 
     def relative_positioning_parent_column
       :epic_id
     end
+
+    override :move_nulls
+    def move_nulls(objects, **args)
+      super(objects&.reject(&:root_epic_tree_node?), **args)
+    end
   end
 
   included do
     extend ::Gitlab::Utils::Override
 
+    override :move_between
+    def move_between(*)
+      super unless root_epic_tree_node?
+    end
+
+    override :move_after
+    def move_after(*)
+      super unless root_epic_tree_node?
+    end
+
+    override :move_before
+    def move_before(*)
+      super unless root_epic_tree_node?
+    end
+
+    override :move_to_end
+    def move_to_end
+      super unless root_epic_tree_node?
+    end
+
+    override :move_to_start
+    def move_to_start
+      super unless root_epic_tree_node?
+    end
+
     override :update_relative_siblings
     def update_relative_siblings(relation, range, delta)
       items_to_update = relation
@@ -38,9 +83,27 @@ module EpicTreeSorting
     def exclude_self(relation, excluded: self)
       return relation unless excluded&.id.present?
 
-      object_type = excluded.try(:object_type) || excluded.class.table_name.singularize
+      relation.where.not(*excluded.epic_tree_node_filter_condition)
+    end
+
+    override :reset_relative_position
+    def reset_relative_position
+      current = self.class.relative_positioning_query_base(self)
+        .where(*epic_tree_node_filter_condition)
+        .pluck(:relative_position)
+        .first
+
+      self.relative_position = current
+    end
+
+    def epic_tree_node_filter_condition
+      ['object_type = ? AND id = ?', *epic_tree_node_identity]
+    end
+
+    def epic_tree_node_identity
+      type = try(:object_type) || self.class.underscore
 
-      relation.where.not('object_type = ? AND id = ?', object_type, excluded.id)
+      [type, id]
     end
   end
 end

ee/app/models/ee/epic.rb

@@ -123,6 +123,10 @@ module EE
       before_save :set_fixed_start_date, if: :start_date_is_fixed?
       before_save :set_fixed_due_date, if: :due_date_is_fixed?
 
+      def root_epic_tree_node?
+        parent_id.nil?
+      end
+
       private
 
       def set_fixed_start_date

ee/app/models/epic_issue.rb

@@ -18,6 +18,10 @@ class EpicIssue < ApplicationRecord
 
   validate :validate_confidential_epic
 
+  def root_epic_tree_node?
+    false
+  end
+
   private
 
   def validate_confidential_epic

ee/changelogs/unreleased/ajk-epic-tree-node-factory.yml

+---
+title: Allow epic tree nodes to reset correctly
+merge_request: 42083
+author:
+type: fixed

ee/spec/factories/epic_tree_nodes.rb

+# frozen_string_literal: true
+
+# Factory that builds either an epic or an epic issue, depending
+# on the value of :object_type
+FactoryBot.define do
+  factory :epic_tree_node, class: 'Object' do
+    association :parent, factory: :epic
+    sequence(:object_type) { |n| n.even? ? :epic_issue : :epic }
+
+    relative_position { RelativePositioning::START_POSITION }
+
+    group { parent.group }
+
+    initialize_with do
+      g = group # Need to call so it does not get assigned
+      key = object_type == :epic ? :parent : :epic
+      extras = object_type == :epic ? { group: g } : {}
+
+      obj = FactoryBot.build(object_type,
+                       **extras,
+                       key => parent,
+                       relative_position: relative_position)
+      obj
+    end
+  end
+end

ee/spec/models/concerns/epic_tree_sorting_spec.rb

@@ -59,14 +59,17 @@ RSpec.describe EpicTreeSorting 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
+      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)
-      expect(epic_issue3.reload.relative_position).to be > epic3.reload.relative_position
+      expect(epic_issue2.relative_position)
+        .to be_between(epic2.reload.relative_position, epic3.reload.relative_position).exclusive
+      expect(epic_issue3.reload.relative_position)
+        .to be > epic3.reload.relative_position
     end
   end
 

ee/spec/models/epic_issue_spec.rb

@@ -29,8 +29,12 @@ RSpec.describe EpicIssue do
   context "relative positioning" do
     it_behaves_like "a class that supports relative positioning" do
       let_it_be(:epic) { create(:epic) }
-      let(:factory) { :epic_issue }
-      let(:default_params) { { epic: epic } }
+      let(:factory) { :epic_tree_node }
+      let(:default_params) { { parent: epic, group: epic.group } }
+
+      def as_item(item)
+        item.epic_tree_node_identity
+      end
     end
 
     context 'with a mixed tree level' do

ee/spec/models/epic_spec.rb

@@ -617,18 +617,24 @@ RSpec.describe Epic do
   end
 
   context "relative positioning" do
+    let_it_be(:parent) { create(:epic) }
+    let_it_be(:group) { create(:group) }
+
     context 'there is no parent' do
-      it_behaves_like "a class that supports relative positioning" do
-        let(:factory) { :epic }
-        let(:default_params) { {} }
-      end
+      let_it_be(:factory) { :epic }
+      let_it_be(:default_params) { { group: group } }
+
+      it_behaves_like "no-op relative positioning"
     end
 
     context 'there is a parent' do
       it_behaves_like "a class that supports relative positioning" do
-        let_it_be(:parent) { create(:epic) }
-        let(:factory) { :epic }
+        let(:factory) { :epic_tree_node }
         let(:default_params) { { parent: parent, group: parent.group } }
+
+        def as_item(item)
+          item.epic_tree_node_identity
+        end
       end
     end
   end

lib/gitlab/relative_positioning/item_context.rb

@@ -131,12 +131,12 @@ module Gitlab
 
       def shift_left
         move_sequence_before(true)
-        object.reset
+        object.reset_relative_position
       end
 
       def shift_right
         move_sequence_after(true)
-        object.reset
+        object.reset_relative_position
       end
 
       def create_space_left

spec/support/shared_examples/models/relative_positioning_shared_examples.rb

@@ -31,6 +31,41 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     end
   end
 
+  def as_item(item)
+    item # Override to perform a transformation, if necessary
+  end
+
+  def as_items(items)
+    items.map { |item| as_item(item) }
+  end
+
+  describe '#scoped_items' do
+    it 'includes all items with the same scope' do
+      scope = as_items([item1, item2, new_item, create_item])
+      irrelevant = create(factory, {}) # This should not share the scope
+      context = RelativePositioning.mover.context(item1)
+
+      same_scope = as_items(context.scoped_items)
+
+      expect(same_scope).to include(*scope)
+      expect(same_scope).not_to include(as_item(irrelevant))
+    end
+  end
+
+  describe '#relative_siblings' do
+    it 'includes all items with the same scope, except self' do
+      scope = as_items([item2, new_item, create_item])
+      irrelevant = create(factory, {}) # This should not share the scope
+      context = RelativePositioning.mover.context(item1)
+
+      siblings = as_items(context.relative_siblings)
+
+      expect(siblings).to include(*scope)
+      expect(siblings).not_to include(as_item(item1))
+      expect(siblings).not_to include(as_item(irrelevant))
+    end
+  end
+
   describe '.move_nulls_to_end' do
     let(:item3) { create_item }
     let(:sibling_query) { item1.class.relative_positioning_query_base(item1) }
@@ -47,7 +82,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
       expect(item1.relative_position).to be(1000)
 
       expect(sibling_query.where(relative_position: nil)).not_to exist
-      expect(sibling_query.reorder(:relative_position, :id)).to eq([item1, item2, item3])
+      expect(as_items(sibling_query.reorder(:relative_position, :id))).to eq(as_items([item1, item2, item3]))
     end
 
     it 'preserves relative position' do
@@ -120,16 +155,16 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     it 'does not have an N+1 issue' do
       create_items_with_positions(10..12)
 
-      a, b, c, d, e, f = create_items_with_positions([nil, nil, nil, nil, nil, nil])
+      a, b, c, d, e, f, *xs = create_items_with_positions([nil] * 10)
 
       baseline = ActiveRecord::QueryRecorder.new do
-        described_class.move_nulls_to_end([a, e])
+        described_class.move_nulls_to_end([a, b])
       end
 
-      expect { described_class.move_nulls_to_end([b, c, d]) }
+      expect { described_class.move_nulls_to_end([c, d, e, f]) }
         .not_to exceed_query_limit(baseline)
 
-      expect { described_class.move_nulls_to_end([f]) }
+      expect { described_class.move_nulls_to_end(xs) }
         .not_to exceed_query_limit(baseline.count)
     end
   end
@@ -149,7 +184,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
 
       expect(items.sort_by(&:relative_position)).to eq(items)
       expect(sibling_query.where(relative_position: nil)).not_to exist
-      expect(sibling_query.reorder(:relative_position, :id)).to eq(items)
+      expect(as_items(sibling_query.reorder(:relative_position, :id))).to eq(as_items(items))
       expect(item3.relative_position).to be(1000)
     end
 
@@ -652,3 +687,119 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     (RelativePositioning::MIN_POSITION..).take(size)
   end
 end
+
+RSpec.shared_examples 'no-op relative positioning' do
+  def create_item(**params)
+    create(factory, params.merge(default_params))
+  end
+
+  let_it_be(:item1) { create_item }
+  let_it_be(:item2) { create_item }
+  let_it_be(:new_item) { create_item(relative_position: nil) }
+
+  def any_relative_positions
+    new_item.class.reorder(:relative_position, :id).pluck(:id, :relative_position)
+  end
+
+  shared_examples 'a no-op method' do
+    it 'does not raise errors' do
+      expect { perform }.not_to raise_error
+    end
+
+    it 'does not perform any DB queries' do
+      expect { perform }.not_to exceed_query_limit(0)
+    end
+
+    it 'does not change any relative_position' do
+      expect { perform }.not_to change { any_relative_positions }
+    end
+  end
+
+  describe '.scoped_items' do
+    subject { RelativePositioning.mover.context(item1).scoped_items }
+
+    it 'is empty' do
+      expect(subject).to be_empty
+    end
+  end
+
+  describe '.relative_siblings' do
+    subject { RelativePositioning.mover.context(item1).relative_siblings }
+
+    it 'is empty' do
+      expect(subject).to be_empty
+    end
+  end
+
+  describe '.move_nulls_to_end' do
+    subject { item1.class.move_nulls_to_end([new_item, item1]) }
+
+    it_behaves_like 'a no-op method' do
+      def perform
+        subject
+      end
+    end
+
+    it 'does not move any items' do
+      expect(subject).to eq(0)
+    end
+  end
+
+  describe '.move_nulls_to_start' do
+    subject { item1.class.move_nulls_to_start([new_item, item1]) }
+
+    it_behaves_like 'a no-op method' do
+      def perform
+        subject
+      end
+    end
+
+    it 'does not move any items' do
+      expect(subject).to eq(0)
+    end
+  end
+
+  describe 'instance methods' do
+    subject { new_item }
+
+    describe '#move_to_start' do
+      it_behaves_like 'a no-op method' do
+        def perform
+          subject.move_to_start
+        end
+      end
+    end
+
+    describe '#move_to_end' do
+      it_behaves_like 'a no-op method' do
+        def perform
+          subject.move_to_end
+        end
+      end
+    end
+
+    describe '#move_between' do
+      it_behaves_like 'a no-op method' do
+        def perform
+          subject.move_between(item1, item2)
+        end
+      end
+    end
+
+    describe '#move_before' do
+      it_behaves_like 'a no-op method' do
+        def perform
+          subject.move_before(item1)
+        end
+      end
+    end
+
+    describe '#move_after' do
+      it_behaves_like 'a no-op method' do
+        def perform
+          subject.move_after(item1)
+        end
+      end
+    end
+  end
+end