Use correct order when repositioning existing designs

Unpositioned designs are sorted by ID, so we need to explictly specify
that order when calling `move_nulls_to_end`.

We also always need to reload the design objects in case their position
was changed in the DB by the `move_nulls_to_end` call.

app/models/concerns/relative_positioning.rb

@@ -137,9 +137,10 @@ module RelativePositioning
     #                          If `true`, then all objects with `null` positions are placed _after_
     #                          all siblings with positions. If `false`, all objects with `null`
     #                          positions are placed _before_ all siblings with positions.
+    # @returns [Number] The number of moved records.
     def move_nulls(objects, at_end:)
       objects = objects.reject(&:relative_position)
-      return if objects.empty?
+      return 0 if objects.empty?
 
       representative = objects.first
       number_of_gaps = objects.size + 1 # 1 at left, one between each, and one at right
@@ -186,6 +187,8 @@ module RelativePositioning
           end
         end
       end
+
+      objects.size
     end
   end
 

app/models/design_management/design.rb

@@ -87,10 +87,12 @@ module DesignManagement
         # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/17788/diffs#note_230875678
         order(:relative_position, :id)
       else
-        order(:id)
+        in_creation_order
       end
     end
 
+    scope :in_creation_order, -> { reorder(:id) }
+
     scope :with_filename, -> (filenames) { where(filename: filenames) }
     scope :on_issue, ->(issue) { where(issue_id: issue) }
 

app/services/design_management/move_designs_service.rb

@@ -39,9 +39,12 @@ module DesignManagement
     delegate :issue, :project, to: :current_design
 
     def move_nulls_to_end
-      current_design.class.move_nulls_to_end(issue.designs)
-      next_design.reset if next_design && next_design.relative_position.nil?
-      previous_design.reset if previous_design && previous_design.relative_position.nil?
+      moved_records = current_design.class.move_nulls_to_end(issue.designs.in_creation_order)
+      return if moved_records == 0
+
+      current_design.reset
+      next_design&.reset
+      previous_design&.reset
     end
 
     def neighbors

changelogs/unreleased/fix-move-null-designs.yml

+---
+title: Use correct order when repositioning existing designs
+merge_request: 39826
+author:
+type: fixed

spec/models/design_management/design_spec.rb

@@ -185,6 +185,12 @@ RSpec.describe DesignManagement::Design do
       end
     end
 
+    describe '.in_creation_order' do
+      it 'sorts by ID in ascending order' do
+        expect(described_class.in_creation_order).to eq([design1, design2, design3, deleted_design])
+      end
+    end
+
     describe '.with_filename' do
       it 'returns correct design when passed a single filename' do
         expect(described_class.with_filename(design1.filename)).to eq([design1])

spec/services/design_management/move_designs_service_spec.rb

@@ -117,9 +117,30 @@ RSpec.describe DesignManagement::MoveDesignsService do
       let(:previous_design) { designs.second }
       let(:next_design) { designs.third }
 
-      it 'calls move_between and is successful' do
-        expect(current_design).to receive(:move_between).with(previous_design, next_design)
+      it 'repositions existing designs and correctly places the given design' do
+        other_design1 = create(:design, issue: issue, relative_position: 10)
+        other_design2 = create(:design, issue: issue, relative_position: 20)
+        other_design3, other_design4 = create_list(:design, 2, issue: issue)
+
         expect(subject).to be_success
+
+        expect(issue.designs.ordered(issue.project)).to eq([
+          # Existing designs which already had a relative_position set.
+          # These should stay at the beginning, in the same order.
+          other_design1,
+          other_design2,
+
+          # The designs we're passing into the service.
+          # These should be placed between the existing designs, in the correct order.
+          previous_design,
+          current_design,
+          next_design,
+
+          # Existing designs which didn't have a relative_position set.
+          # These should be placed at the end, in the order of their IDs.
+          other_design3,
+          other_design4
+        ])
       end
     end
   end

spec/support/shared_examples/models/relative_positioning_shared_examples.rb

@@ -24,7 +24,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
       item3.update!(relative_position: nil)
 
       items = [item1, item2, item3]
-      described_class.move_nulls_to_end(items)
+      expect(described_class.move_nulls_to_end(items)).to be(2)
 
       expect(items.sort_by(&:relative_position)).to eq(items)
       expect(item1.relative_position).to be(1000)
@@ -53,9 +53,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     it 'does not perform any moves if all items have their relative_position set' do
       item1.update!(relative_position: 1)
 
-      expect do
-        described_class.move_nulls_to_end([item1])
-      end.not_to change { item1.reset.relative_position }
+      expect(described_class.move_nulls_to_start([item1])).to be(0)
+      expect(item1.reload.relative_position).to be(1)
     end
 
     it 'manages to move nulls to the end even if there is a sequence at the end' do
@@ -97,7 +96,7 @@ RSpec.shared_examples 'a class that supports relative positioning' do
       item3.update!(relative_position: 1000)
 
       items = [item1, item2, item3]
-      described_class.move_nulls_to_start(items)
+      expect(described_class.move_nulls_to_start(items)).to be(2)
       items.map(&:reload)
 
       expect(items.sort_by(&:relative_position)).to eq(items)
@@ -128,10 +127,8 @@ RSpec.shared_examples 'a class that supports relative positioning' do
     it 'does not perform any moves if all items have their relative_position set' do
       item1.update!(relative_position: 1)
 
-      described_class.move_nulls_to_start([item1])
-      item1.reload
-
-      expect(item1.relative_position).to be(1)
+      expect(described_class.move_nulls_to_start([item1])).to be(0)
+      expect(item1.reload.relative_position).to be(1)
     end
   end