-
Notifications
You must be signed in to change notification settings - Fork 463
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
base: master
Are you sure you want to change the base?
Scope for leaves #246
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks like "children" to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
end | ||||||
|
||||||
# Orphan strategy writer | ||||||
def orphan_strategy= orphan_strategy | ||||||
# Check value of orphan strategy, only rootify, adopt, restrict or destroy is allowed | ||||||
|
@@ -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 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): |
||
|
||
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 | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.