Add a factory for either an epic or an epic issue

This is useful when we need to test behaviour in epic trees,
where an item might be either an epic or an epic issue. Failure to test
this assumption means we have code that is silently buggy or can fail at
runtime.

Some of these issues are discovered here and fixed (notably, reloading
the relative position requires custom code for these two classes, since
they are queried for in a union).

To do this, a new public method is added to RelativePositioning classes:
`#reset_relative_position`, which is is called from outside self during
`.move_nulls`.

Testing improvements:

Tests are added for scoped_items and relative_siblings

The RelativePositioning system relies on `scoped_items` and
`relative_siblings` for many operations. These methods rely on some
guarantees about how they include and exclude items. These assumptions
are tested here.

These changes fix tests that are designed to test for correct behaviour
of the relative positioning sibling queries, in particular excluding
irrelevant items and the current item.

Other changes:

- Rename filter_epic_tree_node to epic_tree_node_filter_condition
  This represents its nature rather better.
- Use `Class#underscore` rather than using the table-name, since
  this is both simpler, and also more resistant to re-naming either of
  Epic or EpicIssue.
- Make all relative positioning methods no-ops on root nodes. Root nodes
  are not in fact sortable, as they have no relative siblings.
  Epics at the root node are not sortable, but those contained within
  the tree are. This change makes it so that all operations on epics at
  the root node are no-ops.
- Restore an exclusivity test that was removed in
  304f7212,
  added after master was broken by a test failure. See:
  https://gitlab.com/gitlab-org/gitlab/-/issues/247120

Since this is a ~bug fix, we add changelog entry.

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