Fixed that projects are not ordered alphabetically after renaming project (#11508).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@10187 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/project.rb

@@ -81,6 +81,7 @@ class Project < ActiveRecord::Base
   # reserved words
   validates_exclusion_of :identifier, :in => %w( new )
 
+  after_save :update_position_under_parent, :if => Proc.new {|project| project.name_changed?}
   before_destroy :delete_all_members
 
   scope :has_module, lambda { |mod| { :conditions => ["#{Project.table_name}.id IN (SELECT em.project_id FROM #{EnabledModule.table_name} em WHERE em.name=?)", mod.to_s] } }
@@ -383,22 +384,7 @@ class Project < ActiveRecord::Base
       # Nothing to do
       true
     elsif p.nil? || (p.active? && move_possible?(p))
-      # Insert the project so that target's children or root projects stay alphabetically sorted
+      set_or_update_position_under(p)
-      sibs = (p.nil? ? self.class.roots : p.children)
-      to_be_inserted_before = sibs.detect {|c| c.name.to_s.downcase > name.to_s.downcase }
-      if to_be_inserted_before
-        move_to_left_of(to_be_inserted_before)
-      elsif p.nil?
-        if sibs.empty?
-          # move_to_root adds the project in first (ie. left) position
-          move_to_root
-        else
-          move_to_right_of(sibs.last) unless self == sibs.last
-        end
-      else
-        # move_to_child_of adds the project in last (ie.right) position
-        move_to_child_of(p)
-      end
       Issue.update_versions_from_hierarchy_change(self)
       true
     else
@@ -943,4 +929,28 @@ class Project < ActiveRecord::Base
     end
     update_attribute :status, STATUS_ARCHIVED
   end
+
+  def update_position_under_parent
+    set_or_update_position_under(parent)
+  end
+
+  # Inserts/moves the project so that target's children or root projects stay alphabetically sorted
+  def set_or_update_position_under(target_parent)
+    sibs = (target_parent.nil? ? self.class.roots : target_parent.children)
+    to_be_inserted_before = sibs.sort_by {|c| c.name.to_s.downcase}.detect {|c| c.name.to_s.downcase > name.to_s.downcase }
+
+    if to_be_inserted_before
+      move_to_left_of(to_be_inserted_before)
+    elsif target_parent.nil?
+      if sibs.empty?
+        # move_to_root adds the project in first (ie. left) position
+        move_to_root
+      else
+        move_to_right_of(sibs.last) unless self == sibs.last
+      end
+    else
+      # move_to_child_of adds the project in last (ie.right) position
+      move_to_child_of(target_parent)
+    end
+  end
 end

test/unit/project_nested_set_test.rb

@@ -19,95 +19,108 @@ require File.expand_path('../../test_helper', __FILE__)
 
 class ProjectNestedSetTest < ActiveSupport::TestCase
 
-  context "nested set" do
+  def setup
-    setup do
+    Project.delete_all
-      Project.delete_all
 
-      @a = Project.create!(:name => 'Project A', :identifier => 'projecta')
+    @a = Project.create!(:name => 'A', :identifier => 'projecta')
-      @a1 = Project.create!(:name => 'Project A1', :identifier => 'projecta1')
+    @a1 = Project.create!(:name => 'A1', :identifier => 'projecta1')
-      @a1.set_parent!(@a)
+    @a1.set_parent!(@a)
-      @a2 = Project.create!(:name => 'Project A2', :identifier => 'projecta2')
+    @a2 = Project.create!(:name => 'A2', :identifier => 'projecta2')
-      @a2.set_parent!(@a)
+    @a2.set_parent!(@a)
 
-      @b = Project.create!(:name => 'Project B', :identifier => 'projectb')
+    @b = Project.create!(:name => 'B', :identifier => 'projectb')
-      @b1 = Project.create!(:name => 'Project B1', :identifier => 'projectb1')
+    @b1 = Project.create!(:name => 'B1', :identifier => 'projectb1')
-      @b1.set_parent!(@b)
+    @b1.set_parent!(@b)
-      @b11 = Project.create!(:name => 'Project B11', :identifier => 'projectb11')
+    @b11 = Project.create!(:name => 'B11', :identifier => 'projectb11')
-      @b11.set_parent!(@b1)
+    @b11.set_parent!(@b1)
-      @b2 = Project.create!(:name => 'Project B2', :identifier => 'projectb2')
+    @b2 = Project.create!(:name => 'B2', :identifier => 'projectb2')
-      @b2.set_parent!(@b)
+    @b2.set_parent!(@b)
 
-      @c = Project.create!(:name => 'Project C', :identifier => 'projectc')
+    @c = Project.create!(:name => 'C', :identifier => 'projectc')
-      @c1 = Project.create!(:name => 'Project C1', :identifier => 'projectc1')
+    @c1 = Project.create!(:name => 'C1', :identifier => 'projectc1')
-      @c1.set_parent!(@c)
+    @c1.set_parent!(@c)
 
-      [@a, @a1, @a2, @b, @b1, @b11, @b2, @c, @c1].each(&:reload)
+    @a, @a1, @a2, @b, @b1, @b11, @b2, @c, @c1 = *(Project.all.sort_by(&:name))
-    end
-
-    context "#create" do
-      should "build valid tree" do
-        assert_nested_set_values({
-          @a   => [nil,   1,  6],
-          @a1  => [@a.id, 2,  3],
-          @a2  => [@a.id, 4,  5],
-          @b   => [nil,   7, 14],
-          @b1  => [@b.id, 8, 11],
-          @b11 => [@b1.id,9, 10],
-          @b2  => [@b.id,12, 13],
-          @c   => [nil,  15, 18],
-          @c1  => [@c.id,16, 17]
-        })
-      end
-    end
-
-    context "#set_parent!" do
-      should "keep valid tree" do
-        assert_no_difference 'Project.count' do
-          Project.find_by_name('Project B1').set_parent!(Project.find_by_name('Project A2'))
-        end
-        assert_nested_set_values({
-          @a   => [nil,   1, 10],
-          @a2  => [@a.id, 4,  9],
-          @b1  => [@a2.id,5,  8],
-          @b11 => [@b1.id,6,  7],
-          @b   => [nil,  11, 14],
-          @c   => [nil,  15, 18]
-        })
-      end
-    end
-
-    context "#destroy" do
-      context "a root with children" do
-        should "not mess up the tree" do
-          assert_difference 'Project.count', -4 do
-            Project.find_by_name('Project B').destroy
-          end
-          assert_nested_set_values({
-            @a  => [nil,   1,  6],
-            @a1 => [@a.id, 2,  3],
-            @a2 => [@a.id, 4,  5],
-            @c  => [nil,   7, 10],
-            @c1 => [@c.id, 8,  9]
-          })
-        end
-      end
-
-      context "a child with children" do
-        should "not mess up the tree" do
-          assert_difference 'Project.count', -2 do
-            Project.find_by_name('Project B1').destroy
-          end
-          assert_nested_set_values({
-            @a  => [nil,   1,  6],
-            @b  => [nil,   7, 10],
-            @b2 => [@b.id, 8,  9],
-            @c  => [nil,  11, 14]
-          })
-        end
-      end
-    end
   end
 
+  def test_valid_tree
+    assert_valid_nested_set
+  end
+
+  def test_moving_a_child_to_a_different_parent_should_keep_valid_tree
+    assert_no_difference 'Project.count' do
+      Project.find_by_name('B1').set_parent!(Project.find_by_name('A2'))
+    end
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_root_to_first_position_should_update_nested_set_order
+    @c.name = '1'
+    @c.save!
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_root_to_middle_position_should_update_nested_set_order
+    @a.name = 'BA'
+    @a.save!
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_root_to_last_position_should_update_nested_set_order
+    @a.name = 'D'
+    @a.save!
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_root_to_same_position_should_update_nested_set_order
+    @c.name = 'D'
+    @c.save!
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_child_should_update_nested_set_order
+    @a1.name = 'A3'
+    @a1.save!
+    assert_valid_nested_set
+  end
+
+  def test_renaming_a_child_with_child_should_update_nested_set_order
+    @b1.name = 'B3'
+    @b1.save!
+    assert_valid_nested_set
+  end
+
+  def test_adding_a_root_to_first_position_should_update_nested_set_order
+    project = Project.create!(:name => '1', :identifier => 'projectba')
+    assert_valid_nested_set
+  end
+
+  def test_adding_a_root_to_middle_position_should_update_nested_set_order
+    project = Project.create!(:name => 'BA', :identifier => 'projectba')
+    assert_valid_nested_set
+  end
+
+  def test_adding_a_root_to_last_position_should_update_nested_set_order
+    project = Project.create!(:name => 'Z', :identifier => 'projectba')
+    assert_valid_nested_set
+  end
+
+  def test_destroying_a_root_with_children_should_keep_valid_tree
+    assert_difference 'Project.count', -4 do
+      Project.find_by_name('B').destroy
+    end
+    assert_valid_nested_set
+  end
+
+  def test_destroying_a_child_with_children_should_keep_valid_tree
+    assert_difference 'Project.count', -2 do
+      Project.find_by_name('B1').destroy
+    end
+    assert_valid_nested_set
+  end
+
+  private
+
   def assert_nested_set_values(h)
     assert Project.valid?
     h.each do |project, expected|
@@ -115,4 +128,40 @@ class ProjectNestedSetTest < ActiveSupport::TestCase
       assert_equal expected, [project.parent_id, project.lft, project.rgt], "Unexpected nested set values for #{project.name}"
     end
   end
+
+  def assert_valid_nested_set
+    projects = Project.all
+    lft_rgt = projects.map {|p| [p.lft, p.rgt]}.flatten
+    assert_equal projects.size * 2, lft_rgt.uniq.size
+    assert_equal 1, lft_rgt.min
+    assert_equal projects.size * 2, lft_rgt.max
+
+    projects.each do |project|
+      # lft should always be < rgt
+      assert project.lft < project.rgt, "lft=#{project.lft} was not < rgt=#{project.rgt} for project #{project.name}"
+      if project.parent_id
+        # child lft/rgt values must be greater/lower
+        assert_not_nil project.parent, "parent was nil for project #{project.name}"
+        assert project.lft > project.parent.lft, "lft=#{project.lft} was not > parent.lft=#{project.parent.lft} for project #{project.name}"
+        assert project.rgt < project.parent.rgt, "rgt=#{project.rgt} was not < parent.rgt=#{project.parent.rgt} for project #{project.name}"
+      end
+      # no overlapping lft/rgt values
+      overlapping = projects.detect {|other| 
+        other != project && (
+          (other.lft > project.lft && other.lft < project.rgt && other.rgt > project.rgt) ||
+          (other.rgt > project.lft && other.rgt < project.rgt && other.lft < project.lft)
+        )
+      }
+      assert_nil overlapping, (overlapping && "Project #{overlapping.name} (#{overlapping.lft}/#{overlapping.rgt}) overlapped #{project.name} (#{project.lft}/#{project.rgt})")
+    end
+
+    # root projects sorted alphabetically
+    assert_equal Project.roots.map(&:name).sort, Project.roots.sort_by(&:lft).map(&:name), "Root projects were not properly sorted"
+    projects.each do |project|
+      if project.children.any?
+        # sibling projects sorted alphabetically
+        assert_equal project.children.map(&:name).sort, project.children.order('lft').map(&:name), "Project #{project.name}'s children were not properly sorted"
+      end
+    end
+  end
 end