Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

jaredbeck
Copy link
Member

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.

Also,

- add Railtie config option: config.paper_trail.enabled
- deprecate PaperTrail.config.enabled, use enabled? instead
@seanlinsley
Copy link
Member

Pinging @stormsilver since they originally made the change in #541, and may be able to provide more context.

@stormsilver
Copy link
Contributor

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.

@jaredbeck
Copy link
Member Author

Thanks for the quick reply, Eric. I think if we merged this (#720) you could still use PaperTrail.enabled_for_controller= for your use case (background jobs) is that not correct? Note that it is poorly named. A better name would be PaperTrail.enabled_for_current_thread=.

@stormsilver
Copy link
Contributor

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!

@jaredbeck
Copy link
Member Author

Eric, Can you please try using enabled_for_controller= instead of enabled= and confirm that it works for your background jobs? Thanks.

@@ -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
Copy link
Collaborator

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.

Copy link
Collaborator

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?

@jaredbeck
Copy link
Member Author

.. not sure it belongs as part of this PR, seems unrelated ..

Yeah, this and #717 are based on #695 which really does two things. To make this easier to review I've created #723, a subset of this. Let's start there and then come back to this.

@jaredbeck
Copy link
Member Author

Closed by #723, a subset of this PR; it fixes #635 but not #584. I'll fix the conflicts in this and open a new PR with Herman's work.

@jaredbeck jaredbeck closed this Mar 5, 2016
@jaredbeck jaredbeck deleted the a_simpler_off_switch branch July 9, 2017 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants