diff --git a/app/models/project.rb b/app/models/project.rb index 425d61d..e9525ec 100644 --- a/app/models/project.rb +++ b/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 - 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 + set_or_update_position_under(p) 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 diff --git a/test/unit/project_nested_set_test.rb b/test/unit/project_nested_set_test.rb index 0ccac20..07d1a8f 100644 --- a/test/unit/project_nested_set_test.rb +++ b/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 - setup do - Project.delete_all - - @a = Project.create!(:name => 'Project A', :identifier => 'projecta') - @a1 = Project.create!(:name => 'Project A1', :identifier => 'projecta1') - @a1.set_parent!(@a) - @a2 = Project.create!(:name => 'Project A2', :identifier => 'projecta2') - @a2.set_parent!(@a) - - @b = Project.create!(:name => 'Project B', :identifier => 'projectb') - @b1 = Project.create!(:name => 'Project B1', :identifier => 'projectb1') - @b1.set_parent!(@b) - @b11 = Project.create!(:name => 'Project B11', :identifier => 'projectb11') - @b11.set_parent!(@b1) - @b2 = Project.create!(:name => 'Project B2', :identifier => 'projectb2') - @b2.set_parent!(@b) - - @c = Project.create!(:name => 'Project C', :identifier => 'projectc') - @c1 = Project.create!(:name => 'Project C1', :identifier => 'projectc1') - @c1.set_parent!(@c) - - [@a, @a1, @a2, @b, @b1, @b11, @b2, @c, @c1].each(&:reload) - end + def setup + Project.delete_all - 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 + @a = Project.create!(:name => 'A', :identifier => 'projecta') + @a1 = Project.create!(:name => 'A1', :identifier => 'projecta1') + @a1.set_parent!(@a) + @a2 = Project.create!(:name => 'A2', :identifier => 'projecta2') + @a2.set_parent!(@a) - 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 + @b = Project.create!(:name => 'B', :identifier => 'projectb') + @b1 = Project.create!(:name => 'B1', :identifier => 'projectb1') + @b1.set_parent!(@b) + @b11 = Project.create!(:name => 'B11', :identifier => 'projectb11') + @b11.set_parent!(@b1) + @b2 = Project.create!(:name => 'B2', :identifier => 'projectb2') + @b2.set_parent!(@b) + + @c = Project.create!(:name => 'C', :identifier => 'projectc') + @c1 = Project.create!(:name => 'C1', :identifier => 'projectc1') + @c1.set_parent!(@c) + + @a, @a1, @a2, @b, @b1, @b11, @b2, @c, @c1 = *(Project.all.sort_by(&:name)) + 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 - 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 + def test_renaming_a_root_to_first_position_should_update_nested_set_order + @c.name = '1' + @c.save! + assert_valid_nested_set + 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 + 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