user_for_paper_trail should return an id instead of a user

[dnagir]

The user_for_paper_trail on the controller returns the the current_user when available.

I'm not sure why the user (rather than an id) should be returned from there because the whodunnit column is a string.

This became a problem when I changed the type of whodunnit from a string to an integer (so that I can join on the whodunnit in SQL queries). As a result, the whodunnit column is no longer being saved.

The workaround was to override user_for_paper_trail:

# application_controller.rb
#...
  def user_for_paper_trail
    current_user.try(:id)
  end
# ...

Either way, user_for_paper_trail needs to correspond to whatever type of whodunnit is.

Does that make sense?

[batter]

As it says in the API section of the README, you should feel free to override these methods as you see fit. You can define it on your ApplicationController and it should become the default method for any controller, because all other controllers inherit from ApplicationController in Rails.

Here is what I put in the ApplicationController of one of my apps where I was trying to do something like you are:

def current_user_id
  @current_user_id ||= current_user.try(:id)
end
alias_method :user_for_paper_trail, :current_user_id

[dnagir]

@batter yeah, that's what I've done.

But I am just curious about:

1. why paper_trail is setting whodunnit (by default) to the value returned from current_user (which is more than likely to be an ActiveRecord instance, not an id)?
2. why it wasn't an issue when whodunnit column was a string (it was storing correct id converted to string) and why it became an issue when whodunnit column was changed to an integer?

Thanks.

[batter]

In my case, we were using UUID's, so it made more sense to keep it as a String type column for our purposes. I think if you left it as a String type column, it could still work, you just might not be able to do joins agains your Versions tables with ID's. But Rails is smart enough to cast a string to an Integer if you try something like User.find("2") before it makes the query against the database.

I'm not sure what you mean by your 2nd question. I believe the reason why it wouldn't be an issue when you're whodunnit column was a String, is that Rails is smart enough to cast an Fixnum (or any other object) to a String (by invoking to_s on said object) prior to inserting into a String column, but it my not always know how to do the other way around. You could always do something like this current_user.to_param which is usually the same as current_user.id.

You could be right, it might make sense to have the default value for user_for_paper_trail to be current_user.try(:id) or something, but the default was current_user before I took over as maintainer of the library, and it is customizable enough that we never really saw a need to change it. @airblade - any thoughts on this?

[airblade]

I think my thought process at the time went like this:

• paper_trail needs to store a reference to the current 'actor', in whodunnit;
• some apps use current_user, some use current_person, etc (return values assumed to be ActiveRecord instances), so abstract the current_whatever pattern with a user_for_paper_trail method which calls current_user by default and is overridable for other cases;
• when setting whodunnit, paper_trail should call id on the ActiveRecord instance and store that.

However I checked the application from which I extracted paper_trail in the first place. The application controller has this method:

def user_for_paper_trail
  current_user.try :id
end

So it looks like I made a mistake during the extraction and had the :id message sent within the user_for_paper_trail method rather than the whodunnit= method.

Oops.

In the end it possibly worked out ok that way because not all apps would want whodunnit= to send :id to the user_for_paper_trail result, e.g. apps using UUIDs.

Anyway, I think it was a bit of a blunder. Sorry about that ;)

[dnagir]

In the end it possibly worked out ok that way because not all apps would want whodunnit= to send :id to the user_for_paper_trail result

That is very interesting because I didn't override user_for_paper_trail and the whodunnit was correctly set to the id of the current_user (coming from Devise).
Really wonder why it was working...

So by the sound of it user_for_paper_trail does indeed need to return an id instead of the actual user model by default. Is it right?

[batter]

I don't think it's extremely important what the return value defaults to for user_for_paper_trail, as many users will still need to customize it to their needs, but I suppose it does make some sense to default ti to an id as opposed to an instance. Will definitely take this into consideration.

[dwbutler]

I did run into an issue with this when attempting to use prepared statements with the JDBC mysql driver in JRuby. The driver simply calls to_s on the object that maps to the VARCHAR whodunnit column. In my app, User#to_s was overriden to output a nicer object. I had to add an override to my controllers to fix this:

def user_for_paper_trail
  current_user.andand.id
end

So I would argue that it is actually important for user_for_paper_trail to return the correct type of data by default.

[dnagir]

Yes, what's the point returning object at all if the gem is expecting an id. That isn't a sensitive default IMO. The default should indeed be current_user.try(:id)

[sjobe]

I believe the documentation should be updated to cover this. It currently says If your ApplicationController has a current_user method, PaperTrail will store the value it returns in the version's whodunnit column.. Going by what the documentation said, we ended up having a versions table with the to_s representation of the user in some versions' whodunnit value and the user's id in others.

[batter]

You're right, looks like the docs need to be updated. I will try to give them an update this afternoon, thanks for pointing that out.

[sjobe]

👍 :-)

[batter]

@sjobe - I just pushed b106aef, is that a better / more accurate summary of the behavior? Here is the the default method definition for user_for_paper_trail to populate it for the record.

[dnagir]

@batter in this commit why are you returning id "normally" but the current_user again in case of NoMethodError?

When do you expect a NoMethodError there at all?

That looks even more inconsistent and confusing :(

Seriously, just return current_user && current_user.id and let the gem user override the behaviour.

This will raise explicit and obvious error if there's no current_user or user has no id method.
100% obvious, normal Ruby error without the hacks for AS 3-4.

Why use try and try! plus the hack of checking the AS version if it's as simple as current_user && current_user.id?

It's much easier to look at the method:

      def user_for_paper_trail
        return nil unless defined?(current_user)
        current_user && current_user.id
      end

than the one in the commit.

And there's no benefit for the complexity of a method.

Also I can understand the check for the existance of current_user (defined?(current_user)) and that's how it always worked.

But think about this.

99% of use cases we want the versions with whodunnit.
So when there's no current_user method, I'd rather have an error raised rather than silently NOT storing the current user details with the versions.

Sometimes there's just too much magic for no benefit :)

Please have a thought about it and consider this.

Just my $0.02.

[batter]

@dnagir - simply put, not all current_user methods always return an object that will respond to an id method, and in those cases current_user && current_user.id will throw an error by default with this out of the box implementation for those users, which should be avoided at all costs. This is a simple avoidable issue for a user if they read the documentation in the README regarding how this works.

Also, that is not a recent commit, that has been the implementation since v3.0.1. See #346 for historical context as to why it works the way it does.

[sjobe]

@batter yes, the update definitely is a better description of what's happening. thanks!

[dnagir]

Ben, curious where did you see the user object that doesn't respond to id?

On Fri, 10 Apr 2015 7:39 am Serign Jobe [email protected] wrote:

@batter https://github.com/batter yes, the update definitely is a
better description of what's happening. thanks!

—
Reply to this email directly or view it on GitHub
#316 (comment)
.