-
Notifications
You must be signed in to change notification settings - Fork 210
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
Add conditional api #390
base: master
Are you sure you want to change the base?
Add conditional api #390
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 |
---|---|---|
|
@@ -9,7 +9,25 @@ def initialize(model, relation, options) | |
@model = model | ||
@relation = relation.is_a?(Enumerable) ? relation : [relation] | ||
|
||
@counter_cache_name = options.fetch(:column_name, "#{model.name.demodulize.tableize}_count") | ||
@counter_cache_name = proc do |model| | ||
conditions = proc do |condition| | ||
case condition | ||
when Symbol | ||
model.public_send(condition) | ||
when Proc | ||
model.instance_exec(&condition) | ||
else | ||
raise ArgumentError, "Condition must be a symbol or a proc" | ||
end | ||
end | ||
|
||
conditions_allow_change? = Array.wrap(options[:if]).all?(&conditions) && | ||
Array.wrap(options[:unless]).none?(&conditions) | ||
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. Looks pretty neat! |
||
return unless conditions_allow_change? | ||
|
||
options.fetch(:column_name, "#{model.name.demodulize.tableize}_count") | ||
end | ||
|
||
@column_names = options[:column_names] | ||
@delta_column = options[:delta_column] | ||
@foreign_key_values = options[:foreign_key_values] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
require 'models/simple_dependent' | ||
require 'models/conditional_main' | ||
require 'models/conditional_dependent' | ||
require 'models/conditional_dependent_shorthand' | ||
require 'models/post' | ||
require 'models/post_comment' | ||
require 'models/post_like' | ||
|
@@ -1711,6 +1712,24 @@ def yaml_load(yaml) | |
ConditionalMain.find_each { |main| expect(main.conditional_dependents_count).to eq(main.id % 2 == 0 ? 3 : 0) } | ||
end | ||
|
||
it "should correctly fix the counter caches for thousands of records when counter is conditional using :if/:unless" do | ||
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. We'd need a test for the normal updating on insert / update / delete as well, right? I don't even think we change 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. Yeah I agree with sidestepping the fix counts logic since that's orthogonal. I tried to think of a way to have fix_count work well out of the box with this approach, but it would require counting in ruby instead of sql. |
||
# first, clean up | ||
ConditionalDependentShorthand.delete_all | ||
ConditionalMain.delete_all | ||
|
||
MANY.times do |i| | ||
main = ConditionalMain.create | ||
3.times { main.conditional_dependent_shorthands.create(:condition => main.id % 2 == 0) } | ||
end | ||
|
||
ConditionalMain.find_each { |main| expect(main.conditional_dependent_shorthands_count).to eq(main.id % 2 == 0 ? 3 : 0) } | ||
|
||
ConditionalMain.order(db_random).limit(A_FEW).update_all :conditional_dependent_shorthands_count => 1 | ||
ConditionalDependentShorthand.counter_culture_fix_counts :batch_size => A_BATCH | ||
|
||
ConditionalMain.find_each { |main| expect(main.conditional_dependent_shorthands_count).to eq(main.id % 2 == 0 ? 3 : 0) } | ||
end | ||
|
||
it "should correctly fix the counter caches when no dependent record exists for some of main records" do | ||
# first, clean up | ||
SimpleDependent.delete_all | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
class ConditionalDependentShorthand < ActiveRecord::Base | ||
belongs_to :conditional_main | ||
scope :condition, -> { where(condition: true) } | ||
|
||
counter_culture :conditional_main, if: :condition?, column_names: -> { { | ||
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. We'd need a test for 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. Yep, and when passing an array of symbols for if / unless, and when using if and unless together. |
||
ConditionalDependentShorthand.condition => :conditional_dependent_shorthands_count, | ||
} } | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
class ConditionalMain < ActiveRecord::Base | ||
has_many :conditional_dependents | ||
has_many :conditional_dependent_shorthands | ||
end |
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 think we should have an example with
unless
as well.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.
Agreed. Should I continue this pattern to expand out the variations?
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.
Specifically, symbol, proc, array of symbols, array of procs
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.
Yeah, I think so