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

Allow whodunnit to be a proc #976

Merged
merged 8 commits into from Jul 8, 2017
Merged

Conversation

rangerscience
Copy link
Contributor

Basically, I've got a circumstance where I'm trying to figure out what is changing one of my models (it's not Sidekiq jobs, and it's not going through the controllers...).

Right now I've got:

class PapertrailSidekiqMiddleware
  def call(worker_class, job, queue)
    if(worker_class.class == Sidekiq::Extensions::DelayedClass)
      PaperTrail.whodunnit = YAML.load(job["args"].first).first.to_s
    else
      PaperTrail.whodunnit = worker_class.class.to_s
    end
    yield
  end
end

Sidekiq.configure_server do |config|
  config.server_middleware do |chain|
    chain.add PapertrailSidekiqMiddleware
  end
end

This works, but it also only covers Sidekiq; if there's no place to stick the whodunnit initialization like this, there's no way to make the whodunnit dynamic and record the actual situation.

However, with a change like this, you could set a whodunnit during the creation of the paper trail, so you could evaluate anything you like (inspecting the callstack, the originally run Ruby command, etc etc).

PS - Honestly, I was 100% convinced I wanted this feature as part of tracking down this issue, but in this moment I'm honestly no remembering what the plan was that needed it.... :P

PPS - I totally don't expect this PR to get accepted, but I wanted to start the conversation and figured this'd be more illustrative than making an issue. LMK how I should proceed!

PPPS - I also ran into a couple of issues when first running rspec on Papertrail, so there's a couple of Gemfile commits to fix that.

Copy link
Member

@jaredbeck jaredbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will not be a commonly used feature, but it adds so little complexity that I'm happy to add it.

Please add an entry to the changelog under "Unreleased"

Gemfile Outdated
@@ -1,2 +1,5 @@
source "https://rubygems.org"
gemspec

gem "rubocop-rspec", "~> 1.15.1"
gem "rails-controller-testing"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not edit the Gemfile. Please edit Appraisals instead. But, I don't think any changes to gems are necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kk. I'm not familiar with Appraisals, but I can go figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, when run via appraisal, everything is fine.

paper_trail_store[:whodunnit].call
else
paper_trail_store[:whodunnit]
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the conditional check respond_to?(:call) instead of == Proc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, that makes a lot of sense; sharp knives and all that.

expect(described_class.whodunnit).to eq(1)
expect(described_class.whodunnit).to eq(2)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D

@rangerscience
Copy link
Contributor Author

Updated as per review and failed tests; rewrote Git history of this branch to keep it super clean.

I added a bit to the documentation where I (as a new and inexperienced contributor) was confused.

@jaredbeck jaredbeck merged commit b5500f1 into paper-trail-gem:master Jul 8, 2017
@jaredbeck
Copy link
Member

Merged. Thanks for your contribution!

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020
* Allow whodunnit to be a proc
* Update contribution quidelines
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.

None yet

2 participants