Skip to content

Commit

Permalink
Refactoring default :user_for_paper_trail method so that 'current_use…
Browse files Browse the repository at this point in the history 
…r' only gets invoked if it is defined. Close #228
  • Loading branch information
Ben Atkins committed May 22, 2013
1 parent e6ba772 commit 9da4930
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## 2.7.2 (Unreleased)

- [#228](/~https://github.com/airblade/paper_trail/issues/228) - Refactored default `user_for_paper_trail` method implementation
so that `current_user` only gets invoked if it is defined.
- [#219](/~https://github.com/airblade/paper_trail/pull/219) - Fixed issue where attributes stored with `nil` value might not get
reified properly depending on the way the serializer worked.
- [#187](/~https://github.com/airblade/paper_trail/pull/187) - Confirmed JRuby support.
Expand Down
2 changes: 1 addition & 1 deletion lib/paper_trail/controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def self.included(base)
# Override this method in your controller to call a different
# method, e.g. `current_person`, or anything you like.
def user_for_paper_trail
current_user rescue nil
current_user if defined?(current_user)
end

# Returns any information about the controller or request that you
Expand Down

6 comments on commit 9da4930

@bradleypriest
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit breaks my konacha test suite with devise

NoMethodError: undefined method `authenticate' for nil:NilClass
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/devise-2.2.4/lib/devise/controllers/helpers.rb:56:in `current_user'       
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/paper_trail-2.7.2/lib/paper_trail/controller.rb:17:in `user_for_paper_trail'
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/paper_trail-2.7.2/lib/paper_trail/controller.rb:59:in `set_paper_trail_whodunnit'
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/activesupport-3.2.13/lib/active_support/callbacks.rb:429:in `_run__54299882614051597__process_action__1959988355013902496__callbacks'
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/activesupport-3.2.13/lib/active_support/callbacks.rb:405:in `__run_callback'
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/activesupport-3.2.13/lib/active_support/callbacks.rb:385:in `_run_process_action_callbacks'
  /Users/bradley/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/activesupport-3.2.13/lib/active_support/callbacks.rb:81:in `run_callbacks'

I know it's not really your responsibility to make sure it plays well with other gems, but this was working fine up until this commit.

The nil object it's trying to call authenticate on is request.env['warden']

@batter
Copy link
Collaborator

@batter batter commented on 9da4930 May 30, 2013

Choose a reason for hiding this comment

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

Hmm... Ironically this commit was made in an effort to be more compatible with Warden (see #228). Perhaps the implementation could be changed to this:

current_user if defined?(current_user) rescue nil

Seems a little overkill haha. Alternatively you can overwrite the user_for_paper_trail method in your ApplicationController and put whatever you want / need in there.

@bradleypriest
Copy link
Contributor

Choose a reason for hiding this comment

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

I've gone with the following, but if I hadn't been following along with the commits, this may have really thrown me for a while

module KonachaPaperTrailOverride
  def paper_trail_enabled_for_controller; end
end
Konacha::SpecsController.send(:include, KonachaPaperTrailOverride)

@batter
Copy link
Collaborator

@batter batter commented on 9da4930 May 30, 2013

Choose a reason for hiding this comment

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

So is this just a matter of you not disabling PaperTrail in the test suite when you intended to or do you have some sort of a suggestion for a modification to the code?

@bradleypriest
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the answer is in this situation, previously it was swallowing an error that it probably shouldn't have, so I guess this is the correct behaviour. I'm just trying to think if there's a better way to fail. I guess we just hope that this makes it into Google

@daviscabral
Copy link

Choose a reason for hiding this comment

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

👍

Please sign in to comment.