Merge pull request #659 from kbrock/alt_ancestry_column_suite

Alt ancestry column suite

.github/workflows/run_test_suite.yml

@@ -68,6 +68,7 @@ jobs:
       MYSQL_PWD: password
       # for rails tests (from matrix)
       BUNDLE_GEMFILE: gemfiles/gemfile_${{ matrix.activerecord }}.gemfile
+      FORMAT: ${{ matrix.format }}
     steps:
       - name: checkout code
         uses: actions/checkout@v3
@@ -84,25 +85,39 @@ jobs:
         env:
           DB: sqlite3
         run: |
-          FORMAT=${{ matrix.format }} bundle exec rake
+          bundle exec rake
       - name: run pg tests
         env:
           DB: pg
         run: |
-          FORMAT=${{ matrix.format }} bundle exec rake
+          bundle exec rake
       - name: run pg tests (UPDATE_STRATEGY=sql)
         env:
           DB: pg
+          UPDATE_STRATEGY: sql
         run: |
-          FORMAT=${{ matrix.format }} UPDATE_STRATEGY=sql bundle exec rake
-
+          bundle exec rake
+      - name: run pg tests (ANCESTRY_COLUMN=ancestry_alt)
+        env:
+          DB: pg
+          ANCESTRY_COLUMN: ancestry_alt
+        run: |
+          bundle exec rake
+      - name: run pg tests (UPDATE_STRATEGY=sql ANCESTRY_COLUMN=ancestry_alt)
+        env:
+          DB: pg
+          ANCESTRY_COLUMN: ancestry_alt
+          UPDATE_STRATEGY: sql
+        run: |
+          bundle exec rake
       - name: run mysql tests
         env:
           DB: mysql2
         run: |
-          FORMAT=${{ matrix.format }} bundle exec rake
+          bundle exec rake
       - name: run mysql tests (ANCESTRY_COLUMN_TYPE=binary)
         env:
           DB: mysql2
+          ANCESTRY_COLUMN_TYPE: binary
         run: |
-          FORMAT=${{ matrix.format }} ANCESTRY_COLUMN_TYPE=binary bundle exec rake
+          bundle exec rake

CHANGELOG.md

@@ -41,6 +41,10 @@ jobs. if you need to do this in the ui, use the depth_caching.
 * These are seen as internal and may go away:
   - `apply_orphan_strategy` (TODO: use `orphan_strategy => none` and define `before_destory`)
 
+## Version [4.3.3] <sub><sup>2023-04-01</sub></sup>
+
+* Fix: sort_by_ancesty with custom ancestry_column [#656](https://github.com/stefankroes/ancestry/pull/656) (thx @mitsuru)
+
 ## Version [4.3.2] <sub><sup>2023-03-25</sub></sup>
 
 * Fix: added back fields that were removed in #589 [#647](https://github.com/stefankroes/ancestry/pull/647) (thx @rastamhadi)

lib/ancestry/version.rb

@@ -1,3 +1,3 @@
 module Ancestry
-  VERSION = '4.3.1'
+  VERSION = '5.0.0'
 end

test/concerns/arrangement_test.rb

@@ -129,29 +129,22 @@ def test_ancestors_arrange_leaf_node
 
   def test_arrange_serializable
     AncestryTestDatabase.with_model :depth => 2, :width => 2 do |model, _roots|
-      if AncestryTestDatabase.materialized_path2?
-        result = [{"ancestry"=>'/',
-            "id"=>4,
-            "children"=>
-            [{"ancestry"=>"/4/", "id"=>6, "children"=>[]},
-              {"ancestry"=>"/4/", "id"=>5, "children"=>[]}]},
-          {"ancestry"=>'/',
-            "id"=>1,
-            "children"=>
-            [{"ancestry"=>"/1/", "id"=>3, "children"=>[]},
-              {"ancestry"=>"/1/", "id"=>2, "children"=>[]}]}]
-      else
-        result = [{"ancestry"=>nil,
-            "id"=>4,
-            "children"=>
-            [{"ancestry"=>"4", "id"=>6, "children"=>[]},
-              {"ancestry"=>"4", "id"=>5, "children"=>[]}]},
-          {"ancestry"=>nil,
-            "id"=>1,
-            "children"=>
-            [{"ancestry"=>"1", "id"=>3, "children"=>[]},
-              {"ancestry"=>"1", "id"=>2, "children"=>[]}]}]
-      end
+      col = model.ancestry_column
+      # materialized path 2 has a slash at the beginning and end
+      fmt = AncestryTestDatabase.materialized_path2? ? -> (a) { a ? "/#{a}/" : "/" } : -> (a) {a}
+      result = [{
+        col=>fmt[nil], "id"=>4, "children"=> [{
+          col=>fmt["4"], "id"=>6, "children" => []
+        }, {
+          col=>fmt["4"], "id"=>5, "children" => []
+        }]
+      }, {
+        col=>fmt[nil], "id"=>1, "children"=> [{
+          col=>fmt["1"], "id"=>3, "children"=>[]
+        }, {
+          col=>fmt["1"], "id"=>2, "children"=>[]
+        }]
+      }]
 
       assert_equal model.arrange_serializable(order: "id desc"), result
     end
@@ -160,15 +153,18 @@ def test_arrange_serializable
   def test_arrange_serializable_with_block
     AncestryTestDatabase.with_model :depth => 2, :width => 2 do |model, _roots|
       expected_result = [{
-          "id"=>4,
-          "children"=>
-           [{"id"=>6},
-            {"id"=>5}]},
-         {
-          "id"=>1,
-          "children"=>
-           [{"id"=>3},
-            {"id"=>2}]}]
+        "id"=>4, "children"=>[{
+          "id"=>6
+        }, {
+          "id"=>5
+        }]
+      }, {
+        "id"=>1, "children"=> [{
+          "id"=>3
+        }, {
+          "id"=>2
+        }]
+      }]
       result = model.arrange_serializable(order: "id desc") do |parent, children|
         out = {}
         out["id"] = parent.id

test/concerns/build_ancestry_test.rb

@@ -2,6 +2,8 @@
 
 class BuildAncestryTest < ActiveSupport::TestCase
   def test_build_ancestry_from_parent_ids
+    ancestry_column = AncestryTestDatabase.ancestry_column
+
     AncestryTestDatabase.with_model :skip_ancestry => true, :extra_columns => {:parent_id => :integer} do |model|
       [model.create!].each do |parent1|
         (Array.new(5) { model.create! :parent_id => parent1.id }).each do |parent2|
@@ -14,7 +16,7 @@ def test_build_ancestry_from_parent_ids
       # Assert all nodes where created
       assert_equal (0..3).map { |n| 5 ** n }.sum, model.count
 
-      model.has_ancestry
+      model.has_ancestry ancestry_column: ancestry_column
       model.build_ancestry_from_parent_ids!
 
       # Assert ancestry integrity
@@ -43,6 +45,8 @@ def test_build_ancestry_from_parent_ids
   end
 
   def test_build_ancestry_from_other_ids
+    ancestry_column = AncestryTestDatabase.ancestry_column
+
     AncestryTestDatabase.with_model :skip_ancestry => true, :extra_columns => {:misc_id => :integer} do |model|
       [model.create!].each do |parent1|
         (Array.new(5) { model.create! :misc_id => parent1.id }).each do |parent2|
@@ -55,7 +59,7 @@ def test_build_ancestry_from_other_ids
       # Assert all nodes where created
       assert_equal (0..3).map { |n| 5 ** n }.sum, model.count
 
-      model.has_ancestry
+      model.has_ancestry ancestry_column: ancestry_column
       model.build_ancestry_from_parent_ids! :misc_id
 
       # Assert ancestry integrity

test/concerns/has_ancestry_test.rb

@@ -2,7 +2,11 @@
 
 class HasAncestryTreeTest < ActiveSupport::TestCase
   def test_default_ancestry_column
-    AncestryTestDatabase.with_model do |model|
+    AncestryTestDatabase.with_model skip_ancestry: true, ancestry_column: :ancestry do |model|
+      model.class_eval do
+        # explicitly calling has_ancestry so we can be sure no args passed
+        has_ancestry
+      end
       assert_equal :ancestry, model.ancestry_column
     end
   end
@@ -57,18 +61,6 @@ def test_modified_parents_set_ancestry_properly
     end
   end
 
-  def test_set_parent_with_non_default_ancestry_column
-    AncestryTestDatabase.with_model :depth => 3, :width => 3, :ancestry_column => :alternative_ancestry do |_model, roots|
-      root1, root2, _root3 = roots.map(&:first)
-      assert_no_difference 'root1.descendants.size' do
-        assert_difference 'root2.descendants.size', root1.subtree.size do
-          root1.parent = root2
-          root1.save!
-        end
-      end
-    end
-  end
-
   def test_set_parent_id
     AncestryTestDatabase.with_model :depth => 3, :width => 3 do |_model, roots|
       root1, root2, _root3 = roots.map(&:first)
@@ -81,18 +73,6 @@ def test_set_parent_id
     end
   end
 
-  def test_set_parent_id_with_non_default_ancestry_column
-    AncestryTestDatabase.with_model :depth => 3, :width => 3, :ancestry_column => :alternative_ancestry do |_model, roots|
-      root1, root2, _root3 = roots.map(&:first)
-      assert_no_difference 'root1.descendants.size' do
-        assert_difference 'root2.descendants.size', root1.subtree.size do
-          root1.parent_id = root2.id
-          root1.save!
-        end
-      end
-    end
-  end
-
   def test_setup_test_nodes
     AncestryTestDatabase.with_model :depth => 3, :width => 3 do |model, roots|
       assert_equal Array, roots.class

test/concerns/materialized_path2_test.rb

@@ -97,27 +97,4 @@ def test_ancestry_validation_exclude_self
       end
     end
   end
-
-  private
-
-  def assert_ancestry(node, value, child: :skip, db: :value)
-    if value.nil?
-      assert_nil node.ancestry
-    else
-      assert_equal value, node.ancestry
-    end
-
-    db = value if db == :value
-    if db.nil?
-      assert_nil node.ancestry_in_database
-    else
-      assert_equal db, node.ancestry_in_database
-    end
-
-    if child.nil?
-      assert_nil node.child_ancestry
-    elsif child != :skip
-      assert_equal child, node.child_ancestry
-    end
-  end
 end

test/concerns/materialized_path_test.rb

@@ -99,27 +99,4 @@ def test_ancestry_validation_exclude_self
       end
     end
   end
-
-  private
-
-  def assert_ancestry(node, value, child: :skip, db: :value)
-    if value.nil?
-      assert_nil node.ancestry
-    else
-      assert_equal value, node.ancestry
-    end
-
-    db = value if db == :value
-    if db.nil?
-      assert_nil node.ancestry_in_database
-    else
-      assert_equal db, node.ancestry_in_database
-    end
-
-    if child.nil?
-      assert_nil node.child_ancestry
-    elsif child != :skip
-      assert_equal child, node.child_ancestry
-    end
-  end
 end

test/concerns/orphan_strategies_test.rb

@@ -55,17 +55,16 @@ def test_orphan_adopt_strategy
       n4 = model.create!(:parent => n2)   #create child with parent=n2, depth = 2
       n5 = model.create!(:parent => n4)   #create child with parent=n4, depth = 3
       n2.destroy                          # delete a node with desecendants
-      assert_equal(model.find(n3.id).parent,n1, "orphan's not parentified" )
-      assert_equal(model.find(n5.id).ancestor_ids, [n1.id,n4.id], "ancestry integrity not maintained")
+      n3.reload
+      n5.reload
+      assert_equal n3.parent_id, n1.id, "orphan's not parentified"
+      assert_equal n5.ancestor_ids, [n1.id, n4.id], "ancestry integrity not maintained"
       n1.destroy                          # delete a root node with desecendants
-      if AncestryTestDatabase.materialized_path2?
-        assert_equal(model.find(n3.id).ancestry, model.ancestry_root, " new root node has no root ancestry string")
-      else
-        assert_nil(model.find(n3.id).ancestry," new root node has no empty ancestry string")
-      end
-      assert_equal(model.find(n3.id).valid?, true, " new root node is not valid")
-      assert_nil(model.find(n3.id).parent_id, " Children of the deleted root not rootfied")
-      assert_equal(model.find(n5.id).ancestor_ids, [n4.id], "ancestry integrity not maintained")
+      n3.reload
+      n5.reload
+      assert_nil n3.parent_id, " new root node should have no parent"
+      assert n3.valid?, " new root node is not valid"
+      assert_equal n5.ancestor_ids, [n4.id], "ancestry integrity not maintained"
     end
   end
 

test/concerns/sort_by_ancestry_test.rb

@@ -233,17 +233,4 @@ def test_sort_by_ancestry_with_block_paginated_missing_parents_and_children
       end
     end
   end
-
-  def test_sort_by_ancestry_with_ancestry_column
-    AncestryTestDatabase.with_model :ancestry_column => :t1, :extra_columns => {:rank => :integer} do |model|
-      _, n2, n3, n4, n5, _ = build_ranked_tree(model)
-
-      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).offset(1).take(4), &RANK_SORT)
-      if CORRECT
-        assert_equal [n3, n2, n4, n5].map(&:id), records.map(&:id)
-      else
-        assert_equal [n2, n4, n5, n3].map(&:id), records.map(&:id)
-      end
-    end
-  end
 end

test/environment.rb

@@ -11,8 +11,9 @@
 
 require 'active_support'
 require 'active_support/test_case'
+require 'test_helpers'
 ActiveSupport.test_order = :random if ActiveSupport.respond_to?(:test_order=)
-
+ActiveSupport::TestCase.include(TestHelpers)
 require 'active_record'
 require 'logger'
 
@@ -56,7 +57,7 @@ def self.setup
       Ancestry.default_update_strategy = ENV["UPDATE_STRATEGY"] == "sql" ? :sql : :ruby
       Ancestry.default_ancestry_format = ENV["FORMAT"].to_sym if ENV["FORMAT"].present?
 
-      puts "testing #{db_type} #{Ancestry.default_update_strategy == :sql ? "(sql) " : ""}(with #{column_type} column)"
+      puts "testing #{db_type} #{Ancestry.default_update_strategy == :sql ? "(sql) " : ""}(with #{column_type} #{ancestry_column})"
       puts "column format: #{Ancestry.default_ancestry_format} options: #{column_options.inspect}"
 
     rescue => err
@@ -71,7 +72,11 @@ def self.setup
   end
 
   def self.column_type
-    @column_type ||= ENV["COLUMN_TYPE"].presence || "string"
+    @column_type ||= ENV["ANCESTRY_COLUMN_TYPE"].presence || "string"
+  end
+
+  def self.ancestry_column
+    @ancestry_column ||= ENV["ANCESTRY_COLUMN"].presence || "ancestry"
   end
 
   def self.ancestry_collation
@@ -111,16 +116,16 @@ def self.column_options(force_allow_nil: false)
   def self.with_model options = {}
     depth                = options.delete(:depth) || 0
     width                = options.delete(:width) || 0
-    ancestry_column      = options[:ancestry_column] || :ancestry
     skip_ancestry        = options.delete(:skip_ancestry)
     extra_columns        = options.delete(:extra_columns)
     default_scope_params = options.delete(:default_scope_params)
 
+    options[:ancestry_column] ||= ancestry_column
     table_options={}
     table_options[:id] = options.delete(:id) if options.key?(:id)
 
     ActiveRecord::Base.connection.create_table 'test_nodes', **table_options do |table|
-      table.send(column_type, ancestry_column, **column_options(force_allow_nil: skip_ancestry))
+      table.send(column_type, options[:ancestry_column], **column_options(force_allow_nil: skip_ancestry))
       table.integer options[:cache_depth] == true ? :ancestry_depth : options[:cache_depth] if options[:cache_depth]
       if options[:counter_cache]
         counter_cache_column = options[:counter_cache] == true ? :children_count : options[:counter_cache]