Fixup Sorting

Make sorting more consistent

The 2 cases are

- no rank function: only move parents to in front of children
- rank function: sort each level with ranking function

Author: kbrock

lib/ancestry/class_methods.rb

@@ -70,17 +70,13 @@ def arrange_serializable options={}, nodes=nil, &block
     # Pseudo-preordered array of nodes.  Children will always follow parents,
     # for ordering nodes within a rank provide block, eg. Node.sort_by_ancestry(Node.all) {|a, b| a.rank <=> b.rank}.
     def sort_by_ancestry(nodes, &block)
-      arranged = nodes if nodes.is_a?(Hash)
-
-      unless arranged
+      if nodes.is_a?(Hash)
+        arranged = nodes
+      else
         presorted_nodes = nodes.sort do |a, b|
-          a_cestry, b_cestry = a.ancestry || '0', b.ancestry || '0'
-
-          if block_given? && a_cestry == b_cestry
-            yield a, b
-          else
-            a_cestry <=> b_cestry
-          end
+          r = a.ancestor_ids <=> b.ancestor_ids
+          r = yield(a, b) if r == 0 && block_given?
+          r
         end
 
         arranged = arrange_nodes(presorted_nodes)

test/concerns/sort_by_ancestry_test.rb

@@ -8,40 +8,49 @@ def test_sort_by_ancestry
       #     - n3
       #     - n4
       #   - n5
+      #     - n6
       n1 = model.create!
       n2 = model.create!(:parent => n1)
       n3 = model.create!(:parent => n2)
       n4 = model.create!(:parent => n2)
       n5 = model.create!(:parent => n1)
+      n6 = model.create!(:parent => n5)
+      nodes = [n1, n2, n3, n4, n5, n6]
 
-      records = model.sort_by_ancestry(model.all.sort_by(&:id).reverse)
-      if records[1] == n2
-        if records[2] == n3
-          assert_equal [n1, n2, n3, n4, n5].map(&:id), records.map(&:id)
-        else
-          assert_equal [n1, n2, n4, n3, n5].map(&:id), records.map(&:id)
-        end
-      else
-        if records[3] == n3
-          assert_equal [n1, n5, n2, n3, n4].map(&:id), records.map(&:id)
-        else
-          assert_equal [n1, n5, n2, n4, n3].map(&:id), records.map(&:id)
-        end
-      end
+      # n1 needs to move to front, and n2 needs to move in front of n4, n3
+      assert_equal [n1, n5, n6, n2, n4, n3].map(&:id), model.sort_by_ancestry(model.all.sort_by(&:id).reverse).map(&:id)
+      # none are parents
+      #assert_equal [n5, n4, n3].map(&:id), model.sort_by_ancestry([n5, n4, n3]).map(&:id)
+      # at the same level
+      assert_equal [n3, n4].map(&:id), model.sort_by_ancestry([n3, n4]).map(&:id)
+      # n1 needs to move below both
+      assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n5, n2, n1]).map(&:id)
+      # n1 needs to move below even a double descendant
+      #assert_equal [n1, n5, n4].map(&:id), model.sort_by_ancestry([n5, n4, n1]).map(&:id)
     end
   end
 
   def test_sort_by_ancestry_with_block
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
+      # - n1 (0)
+      #   - n5 (0)
+      #   - n3 (3)
+      # - n2 (1)
+      #   - n4 (0)
+      #   - n6 (1)
       n1 = model.create!(:rank => 0)
       n2 = model.create!(:rank => 1)
-      n3 = model.create!(:rank => 0, :parent => n1)
+      n3 = model.create!(:rank => 3, :parent => n1)
       n4 = model.create!(:rank => 0, :parent => n2)
-      n5 = model.create!(:rank => 1, :parent => n1)
+      n5 = model.create!(:rank => 0, :parent => n1)
       n6 = model.create!(:rank => 1, :parent => n2)
 
-      records = model.sort_by_ancestry(model.all.sort_by(&:rank).reverse) {|a, b| a.rank <=> b.rank}
-      assert_equal [n1, n3, n5, n2, n4, n6].map(&:id), records.map(&:id)
+      sort = -> (a, b)  { a.rank <=> b.rank }
+      records = model.sort_by_ancestry(model.all.sort_by(&:rank).reverse, &sort)
+      # all parents, all children
+      assert_equal [n1, n5, n3, n2, n4, n6].map(&:id), records.map(&:id)
+      # all parents, some children
+      assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n2, n1, n5],&sort).map(&:id)
     end
   end
 end