Also reorder project roots and sort by lft only

Previously, we only reordered children when a project gets saved,
this obviously does not sort the project roots.

To fix this, the reorder job is extended to resort all project roots.
When saving a project, all its siblings are being resorted.

app/models/projects/hierarchy.rb

@@ -30,7 +30,7 @@ module Projects::Hierarchy
   extend ActiveSupport::Concern
 
   included do
-    acts_as_nested_set order_column: :name, dependent: :destroy
+    acts_as_nested_set order_column: :lft, dependent: :destroy
 
     # Keep the siblings sorted after naming changes to ensure lft sort includes name sorting
     before_save :remember_reorder
@@ -40,20 +40,32 @@ module Projects::Hierarchy
       @reorder_nested_set = nil
       return unless siblings.any?
 
-      left_neighbor = find_left_neighbor(parent, :name, true)
+      left_neighbor = left_neighbor_by_name_order
 
       if left_neighbor
         move_to_right_of(left_neighbor)
-      elsif self != parent.children.first
-        move_to_left_of(parent.children[0])
+      elsif self != self_and_siblings.first
+        move_to_left_of(self_and_siblings.first)
       end
     end
 
+    ##
+    # Find the sibling for which the current project's name is smaller.
+    # Since we sort ascending, start from the back.
+    # Returns:
+    #   - nil, if the current project does not have a left neighbor (should be added as first)
+    #   - the project sibling for which the project should be appended to the right to
+    def left_neighbor_by_name_order
+      siblings
+        .reverse_each
+        .detect { |project| project.name.casecmp(name) == -1 }
+    end
+
     # We need to remember if we want to reorder as nested_set
     # will perform another save directly in +after_save+ if a parent was set
     # and that clear new_record? as well as previous_new_record?
     def remember_reorder
-      @reorder_nested_set = parent_id.present? && (new_record? || name_changed?)
+      @reorder_nested_set = new_record? || name_changed?
     end
   end
 end

app/models/queries/projects/orders/typeahead_order.rb

@@ -34,6 +34,6 @@ class Queries::Projects::Orders::TypeaheadOrder < Queries::Projects::Orders::Def
   end
 
   def order
-    model.order(lft: :asc, name: :asc)
+    model.order(lft: :asc)
   end
 end

app/workers/projects/reorder_hierarchy_job.rb

@@ -27,7 +27,7 @@
 #++
 
 module Projects
-  class ReorderChildrenJob < ApplicationJob
+  class ReorderHierarchyJob < ApplicationJob
     def perform
       Rails.logger.info { "Resorting siblings by name in the project's nested set." }
       Project.transaction { reorder! }
@@ -36,9 +36,13 @@ module Projects
     private
 
     def reorder!
+      # Reorder the project roots
+      reorder_siblings Project.roots
+
+      # Reorder every project hierarchy
       Project
         .where(id: unique_parent_ids)
-        .find_each(&method(:reorder_children))
+        .find_each { |project| reorder_siblings(project.children) }
     end
 
     def unique_parent_ids
@@ -48,14 +52,14 @@ module Projects
         .distinct
     end
 
-    def reorder_children(parent)
-      return unless parent.children.many?
+    def reorder_siblings(siblings)
+      return unless siblings.many?
 
       # Resort children manually
-      sorted = parent.children.sort_by(&:name)
+      sorted = siblings.sort_by { |project| project.name.downcase }
 
       # Get the current first child
-      first = parent.children.first
+      first = siblings.first
 
       sorted.each_with_index do |child, i|
         if i == 0

db/migrate/20220202140507_reorder_project_children.rb

@@ -1,5 +1,5 @@
 class ReorderProjectChildren < ActiveRecord::Migration[6.1]
   def up
-    ::Projects::ReorderChildrenJob.perform_later
+    ::Projects::ReorderHierarchyJob.perform_later
   end
 end

spec/models/projects/reorder_nested_set_spec.rb

@@ -29,26 +29,41 @@
 require 'spec_helper'
 
 describe Project, 'reordering of nested set', type: :model do
-  shared_let(:parent_project) { create :project, name: 'Parent' }
+  # Create some parents in non-alphabetical order
+  shared_let(:parent_project_b) { create(:project, name: 'ParentB') }
+  shared_let(:parent_project_a) { create(:project, name: 'ParentA') }
 
   # Create some children in non-alphabetical order
-  shared_let(:child_a) { create :project, name: 'A', parent: parent_project }
-  shared_let(:child_f) { create :project, name: 'F', parent: parent_project }
-  shared_let(:child_b) { create :project, name: 'B', parent: parent_project }
+  # including lower case to test case insensitivity
+  shared_let(:child_e) { create :project, name: 'e', parent: parent_project_a }
+  shared_let(:child_c) { create :project, name: 'C', parent: parent_project_a }
+  shared_let(:child_f) { create :project, name: 'F', parent: parent_project_a }
+  shared_let(:child_d) { create :project, name: 'D', parent: parent_project_b }
+  shared_let(:child_b) { create :project, name: 'B', parent: parent_project_b }
 
-  subject { parent_project.children.reorder(:lft) }
+  subject { described_class.all.reorder(:lft) }
 
   it 'has the correct sort' do
-    expect(subject.reload.pluck(:name)).to eq %w[A B F]
+    expect(subject.reload.pluck(:name)).to eq %w[ParentA C e F ParentB B D]
   end
 
   context 'when renaming a child' do
     before do
-      child_a.update! name: 'Z'
+      child_b.update! name: 'Z'
     end
 
     it 'updates that order' do
-      expect(subject.reload.pluck(:name)).to eq %w[B F Z]
+      expect(subject.reload.pluck(:name)).to eq %w[ParentA C e F ParentB D Z]
+    end
+  end
+
+  context 'when adding a new first child to the parent (Regression #40930)' do
+    it 'still resorts them' do
+      expect(subject.reload.pluck(:name)).to eq %w[ParentA C e F ParentB B D]
+
+      described_class.create!(parent: parent_project_a, name: 'A')
+
+      expect(subject.reload.pluck(:name)).to eq %w[ParentA A C e F ParentB B D]
     end
   end
 end

spec/models/queries/projects/project_query_spec.rb

@@ -149,7 +149,7 @@ describe Queries::Projects::ProjectQuery, type: :model do
     describe '#results' do
       it 'returns all visible projects ordered by lft asc' do
         expect(instance.results.to_sql)
-          .to eql base_scope.except(:order).order(lft: :asc, name: :asc, id: :desc).to_sql
+          .to eql base_scope.except(:order).order(lft: :asc, id: :desc).to_sql
       end
     end
   end

spec/workers/projects/reorder_children_job_integration_spec.rb

@@ -28,29 +28,38 @@
 
 require 'spec_helper'
 
-describe Projects::ReorderChildrenJob, type: :model do
+describe Projects::ReorderHierarchyJob, type: :model do
   subject(:job) { described_class.perform_now }
 
-  shared_let(:parent_project) { create(:project, name: 'Parent') }
+  shared_let(:parent_project_a) { create(:project, name: 'ParentA') }
+  shared_let(:parent_project_b) { create(:project, name: 'ParentB') }
 
-  shared_let(:child_a) { create :project, name: 'A', parent: parent_project }
-  shared_let(:child_b) { create :project, name: 'B', parent: parent_project }
-  shared_let(:child_c) { create :project, name: 'C', parent: parent_project }
+  shared_let(:child_a) { create :project, name: 'A', parent: parent_project_a }
+  shared_let(:child_b) { create :project, name: 'B', parent: parent_project_b }
+  shared_let(:child_c) { create :project, name: 'C', parent: parent_project_a }
+  shared_let(:child_d) { create :project, name: 'D', parent: parent_project_b }
+  shared_let(:child_f) { create :project, name: 'F', parent: parent_project_a }
 
-  let(:ordered) { parent_project.children.reorder(:lft) }
+  let(:ordered) { Project.all.reorder(:lft) }
 
   before do
-    # Update the names
-    child_a.update_column(:name, 'Second')
-    child_b.update_column(:name, 'Third')
-    child_c.update_column(:name, 'First')
+    # Update the names, including lower case to test case insensitivity
+    parent_project_a.update_column(:name, 'ParentLast')
+    parent_project_b.update_column(:name, 'ParentFirst')
+    child_a.update_column(:name, 'SecondChild')
+    child_b.update_column(:name, 'ThirdChild')
+    child_c.update_column(:name, 'firstChild')
+    child_d.update_column(:name, 'FourthChild')
+    child_f.update_column(:name, 'FifthChild')
   end
 
   it 'corrects the order' do
-    expect(ordered.pluck(:name)).to eq %w[Second Third First]
+    expect(ordered.pluck(:name)).to eq %w[ParentLast SecondChild firstChild FifthChild
+                                          ParentFirst ThirdChild FourthChild]
 
     subject
 
-    expect(ordered.reload.pluck(:name)).to eq %w[First Second Third]
+    expect(ordered.reload.pluck(:name)).to eq %w[ParentFirst FourthChild ThirdChild
+                                                 ParentLast FifthChild firstChild SecondChild]
   end
 end