Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scope for leaves #246

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@ services:
- postgresql

before_script:
- mysql -e 'create database ancestry_test;' || true
- psql -c 'create database ancestry_test;' -U postgres || true
- mysql -e 'CREATE DATABASE ancestry_test CHARACTER SET utf8 COLLATE utf8_general_ci;' || true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @antstorm

I think this line is significant. Could you please explain more about this?

We may want to add this to the general documentation to help people.
Also if this is not changed as the database collation, then I wonder what users can do to get this behavior.

I'm also thinking postgres on mac has a similar issue with this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe this line should be collation on just the column if they don't do this to the whole database?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added collation to the column and not to the whole database.
Thanks for putting this on my radar.

- psql -c 'CREATE DATABASE ancestry_test;' -U postgres || true
31 changes: 31 additions & 0 deletions lib/ancestry/class_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ def scope_depth depth_options, depth
end
end

# Scope that returns all the leaves
def leaves
id_column = "#{table_name}.id"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
id_column = "#{table_name}.id"
id_column = "#{table_name}.#{self.class.primary_key}"

id_column_as_text = sql_cast_as_text(id_column)
parent_ancestry = sql_concat("#{table_name}.#{ancestry_column}", "'/'", id_column_as_text)

joins("LEFT JOIN #{table_name} AS c ON c.#{ancestry_column} = #{id_column_as_text} OR c.#{ancestry_column} = #{parent_ancestry}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like "children" to me.
Think we may want "subtree" instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it seems to be working.

Could you just give your opinion on what you think about this line and multiple levels of nested members?

Thnx

.group(id_column)
.having('COUNT(c.id) = 0')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.having('COUNT(c.id) = 0')
.having("COUNT(c.#{self.class.primary_key}) = 0")

end

# Orphan strategy writer
def orphan_strategy= orphan_strategy
# Check value of orphan strategy, only rootify, adopt, restrict or destroy is allowed
Expand Down Expand Up @@ -225,5 +236,25 @@ def primary_key_is_an_integer?
end
end
end

private

def sql_concat *parts
if %w(sqlite sqlite3).include?(connection.adapter_name.downcase)
parts.join(' || ')
else
"CONCAT(#{parts.join(', ')})"
end
end

def sql_cast_as_text column
text_type = if %w(mysql mysql2).include?(connection.adapter_name.downcase)
'CHAR'
else
'TEXT'
end

"CAST(#{column} AS #{text_type})"
end
end
end
33 changes: 33 additions & 0 deletions test/concerns/class_methods_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
require_relative '../environment'

class ClassMethodsTest < ActiveSupport::TestCase
def test_sql_concat
AncestryTestDatabase.with_model do |model|
result = model.send(:sql_concat, 'table_name.id', "'/'")

case ActiveRecord::Base.connection.adapter_name.downcase.to_sym
when :sqlite
assert_equal result, "table_name.id || '/'"
when :mysql
assert_equal result, "CONCAT(table_name.id, '/')"
when :postgresql
assert_equal result, "CONCAT(table_name.id, '/')"
end
end
end

def text_sql_cast_as_text
AncestryTestDatabase.with_model do |model|
result = model.send(:sql_cast_as_text, 'table_name.id')

case ActiveRecord::Base.connection.adapter_name.downcase.to_sym
when :sqlite
assert_equal result, 'CAST(table_name.id AS TEXT)'
when :mysql
assert_equal result, 'CAST(table_name.id AS CHAR)'
when :postgresql
assert_equal result, 'CAST(table_name.id AS TEXT)'
end
end
end
end
3 changes: 3 additions & 0 deletions test/concerns/scopes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ def test_scopes
# Roots assertion
assert_equal roots.map(&:first), model.roots.to_a

# Leaves assertion
assert_equal model.all.select(&:is_childless?), model.leaves.order(:id).to_a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, this does look like it is bringing back nodes from 2 levels deep.

ASIDE (nothing to do with you or this PR):
The way the tests are organized, it sure makes it hard to know where different elements are tested.


model.all.each do |test_node|
# Assertions for ancestors_of named scope
assert_equal test_node.ancestors.to_a, model.ancestors_of(test_node).to_a
Expand Down