Only apply journal for current index when resetting #434
Conversation
|
Hey, awesome patch, will review it soon, thanks! |
| @@ -48,15 +48,19 @@ def any_records? | |||
| @records.any? | |||
| end | |||
|
|
|||
| def apply_changes_from(time) | |||
There was a problem hiding this comment.
Hm, I'm not sure this interface makes sense, it will be never used, we are using the journal instance only for filling it in now.
| private | ||
|
|
||
| def identify(objects) | ||
| @index.adapter.identify(objects) | ||
| end | ||
|
|
||
| class << self | ||
| def apply_changes_from(time) | ||
| group(entries_from(time)).each do |entry| | ||
| def apply_changes_from(time, options = {}) |
There was a problem hiding this comment.
Does it make sense to support multiple indexes passing?
| comparator => time.to_i | ||
| def query(time, comparator, index = nil, use_filter = true) | ||
| filter_query = | ||
| if use_filter |
There was a problem hiding this comment.
maybe it makes sense to move some logic to separate methods? as for me, there are too many if-else statements in this method. Also, I would move this part to a separate method instead of just assigning it to filter_query
| expect(CountryIndex.all.to_a.length).to eq 1 | ||
|
|
||
| # Replay on specific index | ||
| Chewy::Journal.new(CityIndex).apply_changes_from(time) |
There was a problem hiding this comment.
yeah, I would change the interface, to not pass an index name to the journal instance. I think it doesn't make sense. I would just pass it as :only parameter toapply_changes_from class method. Like there: Chewy::Journal.apply_changes_from(time, only: [CityIndex])
|
Updated it, let me know what you guys think. |
|
Awesome, thanks! |
When resetting an index that has journaling, the current code actually replays all the journal entries, no matter which index has just been reset.
This PR only applies journal entries for the given index. I find my code a bit clunky (the new
querymethod).Any thoughts?