Fix bug when using multiple trackers. #196
Conversation
|
Looks legit. Need to fix the build, changelog, etc., maybe document what happens with multiple trackers? |
CHANGELOG.md
Outdated
| @@ -1,6 +1,6 @@ | |||
| ### 0.6.2 (Next) | |||
|
|
|||
| * Your contribution here. | |||
| * Fix bug when using multiple trackers | |||
There was a problem hiding this comment.
Describe the actual bug. Conform to other lines. Put back the contributing message.
lib/mongoid/history/tracker.rb
Outdated
| @@ -18,8 +18,6 @@ module Tracker | |||
|
|
|||
| index(scope: 1) | |||
| index(association_chain: 1) | |||
|
|
|||
| Mongoid::History.tracker_class_name = name.tableize.singularize.to_sym | |||
There was a problem hiding this comment.
Is anything changing for existing users because of this?
There was a problem hiding this comment.
I'm guessing this line was originally there so that you don't have to configure a default tracker. It would dynamically set the loaded tracker as default. When multiple trackers got supported, this line no longer works as expected.
So i'm guessing the ones that will be affected are the ones that don't have a default tracker configured and do not have a tracker class set in the trackable options.
CHANGELOG.md
Outdated
|
|
||
| * Your contribution here. | ||
|
|
||
| ### 0.6.2 (2017/10/31) |
There was a problem hiding this comment.
This isn't released, put this back, we update it when releasing, separately.
|
|
||
| If you are using multiple trackers and the `tracker_class_name` parameter is | ||
| not specified, Mongoid::History will use the default tracker configured in the | ||
| initializer file or whatever the first tracker was loaded. |
There was a problem hiding this comment.
Feels like this is saying that the first tracker loaded is going to be load-order dependent. That's not what's happening here, no? What's the default tracker?
There was a problem hiding this comment.
I just went by what the original readme said: "Mongoid::History will use the first loaded class to include Mongoid::History::Tracker as the default history tracker"
That's if there's no initializer. It looks to me that line 22 in tracker.rb would just set it to whatever gets loaded first.
There was a problem hiding this comment.
Got it. Lets leave it as is and maybe open an issue about that?
There was a problem hiding this comment.
Yeah maybe the default tracker shouldn't be optional but should be a required in the configuration.
| before :all do | ||
| Mongoid::History.tracker_class_name = nil | ||
|
|
||
| class First |
There was a problem hiding this comment.
Nitpick, these are global classes despite what you may think. I would put them into a module like module MultipleHistoryTrackersSpec to make sure there's no conflict, and make sure to fix up anything in other specs the same way as well.
There was a problem hiding this comment.
Oh i see. Yeah I was having conflict when I was using Foo. I'll do what you're suggesting.
There was a problem hiding this comment.
So I did what you've asked but the tracker classes can't be put in a module as there's no way to specify the module when passing a tracker_class to #track_history. I still moved the trackables into their own modules though.
I also tried doing the same thing for the other specs but even that wasn't working because those specs are using a modifier and there's no way to specify a module in the modifier.
I think we should leave in a separate issue since I don't have time right now to work on these. Let me know if you want me to revert the changes to the spec that I've added.
|
Thanks for changing the specs. Open an issue for the module limitations, seems like something we should fix. You still have to change CHANGELOG here not to say 0.6.3, you're not releasing with this PR. |
Problem: when using multiple trackers for different classes, the history gets mixed up between the different trackers. Solution: only set Mongoid::History.tracker_class_name if it's not nil.
Problem: when using multiple trackers for different classes, the history gets mixed up between the different trackers.
Solution: remove the line on tracker.rb that sets the global history tracker everytime a tracker is loaded.