-
Notifications
You must be signed in to change notification settings - Fork 901
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
PaperTrail.enabled now affects all threads, again #720
Conversation
Also, - add Railtie config option: config.paper_trail.enabled - deprecate PaperTrail.config.enabled, use enabled? instead
Pinging @stormsilver since they originally made the change in #541, and may be able to provide more context. |
Okay, I think I'm all caught up on the background here. :) The context for #541 is when you are using PaperTrail in a multi-threaded environment and you would like to turn it off for some threads but not all. In our case, we are using Sidekiq. We have some jobs that need to run with PaperTrail, but some jobs (large data imports) that need to run without PaperTrail. So the goal is to be able to disable PaperTrail on a thread-by-thread basis without affecting whether it's enabled for other threads. I can't remember all of the vagaries of instance variables and threads, but it looks like this patch would remove the ability to selectively disable PaperTrail for some threads. To that end, the modifications in #695 and #717 seem best - you can set an "overall" disabled status which then becomes the default for the per-thread status. |
Thanks for the quick reply, Eric. I think if we merged this (#720) you could still use |
Sure - it seems like some people want a way to globally disable PaperTrail and I have a use case for doing it per-thread. If we can come up with mechanisms for accomplishing both of those goals then that seems like a good solution! |
Eric, Can you please try using |
@@ -2,3 +2,7 @@ | |||
# since otherwise the model(s) will get loaded in via the `Rails::Engine`. | |||
require "paper_trail/frameworks/active_record/models/paper_trail/version_association" | |||
require "paper_trail/frameworks/active_record/models/paper_trail/version" | |||
|
|||
ActiveSupport.on_load(:active_record) do |
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 this needs to remain within lib/paper_trail.rb
because this file only conditionally gets loaded when ActiveRecord
is utilized outside of the context of rails. Or perhaps there should be an lib/paper_trail/frameworks/active_model.rb
. Regardless not sure it belongs as part of this PR, seems unrelated. Happy to make a PR for that piece separately if you'd like.
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.
Oh I see now that you also added a similar block inside of the Rails::Engine
. Did this have issues too related to thread safety?
This is a simpler alternative to #717 based on conversation with @seanlinsley in #635
As with #717, this expands on @Hermanverschooten's suggestion in #695, particularly re: the
Railtie
.