Add conditional api #390
Add conditional api #390
Conversation
Adds a conditional API similar to [ActiveRecord's Callbacks](https://guides.rubyonrails.org/active_record_callbacks.html#conditional-callbacks). Resolves magnusvk#389.
|
Here's a first pass at #389. LMK what tweaks you'd like |
lib/counter_culture/counter.rb
Outdated
| quoted_column = if klass.connection.adapter_name == 'Mysql2' | ||
| "#{klass.quoted_table_name}.#{model.connection.quote_column_name(change_counter_column)}" | ||
| return unless conditions_allow_change?(obj) | ||
| return unless id_to_change && change_counter_column |
There was a problem hiding this comment.
lots of noise here but the only change is switching to a guard clause since there are two now
| ```ruby | ||
| class Product < ActiveRecord::Base | ||
| belongs_to :category | ||
| counter_culture :category, column_name: :special_count, if: :special? |
There was a problem hiding this comment.
I think we should have an example with unless as well.
There was a problem hiding this comment.
Agreed. Should I continue this pattern to expand out the variations?
There was a problem hiding this comment.
Specifically, symbol, proc, array of symbols, array of procs
lib/counter_culture/counter.rb
Outdated
| @@ -17,6 +17,8 @@ def initialize(model, relation, options) | |||
| @delta_magnitude = options[:delta_magnitude] || 1 | |||
| @with_papertrail = options.fetch(:with_papertrail, false) | |||
| @execute_after_commit = options.fetch(:execute_after_commit, false) | |||
| @if_conditions = Array.wrap(options[:if]) | |||
| @unless_conditions = Array.wrap(options[:unless]) | |||
There was a problem hiding this comment.
Here's a different idea, I'd love to discuss whether that's a better approach: What if we took what's passed into options[:if] or options[:unless] and "compile" that into a Proc to pass to use for column_name? This is pseudo-code, but something like
if options[:if].present?
@counter_cache_name = proc do |model|
Array.wrap(options[:if]).inject(:&&) { |method| model.public_send(method) }
end
end
The advantage I see of that is that it simplifies the code paths—there's still only one code path to consider and if and unless truly just become shorthand.
Is that better? Or does that make the code more complicated?
There was a problem hiding this comment.
Yeah that was the approach I considered originally, but then we can't specify a custom column name when using if/ unless.
There was a problem hiding this comment.
Can you say a bit more about that? I don't think I follow why that doesn't work.
There was a problem hiding this comment.
Sure, if I’m understanding you correctly that we “compile” into a proc passed internally to column_name, then we can’t do the following:
column_name: :foo_count, if: :special?
Because our compiled proc for if would overwrite the :foo_count symbol
There was a problem hiding this comment.
Couldn't we use the value passed as the name in the proc we compile the if / unless into?
There was a problem hiding this comment.
Yeah, you’re right. Nice, I’ll take a look at refactoring to that.
There was a problem hiding this comment.
Updated, LMK what you think. Then I'll look at updating the tests
| belongs_to :conditional_main | ||
| scope :condition, -> { where(condition: true) } | ||
|
|
||
| counter_culture :conditional_main, if: :condition?, column_names: -> { { |
There was a problem hiding this comment.
We'd need a test for unless and for the case of passing a Proc as well.
There was a problem hiding this comment.
Yep, and when passing an array of symbols for if / unless, and when using if and unless together.
| @@ -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.
We'd need a test for the normal updating on insert / update / delete as well, right? I don't even think we change fix_counts in this PR, that's just based on column_names so I don't even think this test here adds any test coverage we don't already have.
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| conditions_allow_change? = Array.wrap(options[:if]).all?(&conditions) && | ||
| Array.wrap(options[:unless]).none?(&conditions) |
Adds a conditional API similar to ActiveRecord's Callbacks.
Resolves #389.