3

I know the dogma says to not access current_user in a model but I don't fully agree with it. For example, I want to write a set of logging functions when an action happens via a rails callback. Or simply writing who wrote a change when an object can have multiple people write to it (not like a message which has a single owner). In many ways, I see current_user more as config for an application - in other words make this app respond to this user. I would rather have my logging via the model DSL rather than in the action where it seems REALLY out of place. What am I missing?

This idea seems rather inelegant Access current_user in model

as does this: http://rails-bestpractices.com/posts/47-fetch-current-user-in-models

thx

edit #1 So my question isn't if there are gems that can do auditing / logging. I currently use paper_trail (although moving away from it because I can do same functionality in approx 10 lines of ruby code); it is more about whether current_user should never be accessed in the model - I essentially want to REDUCE my controller code and push down logic to models where it should be. Part of this might be due to the history of ActiveRecord which is essentially a wrapper around database tables for which RoR has added a lot of functionality over the years.

Community
  • 1
  • 1
timpone
  • 19,235
  • 36
  • 121
  • 211

2 Answers2

1

You've given several examples that you'd like to accomplish, I'll go through the solution to each one separately:

I want to write a set of logging functions when an action happens via a rails callback

Depending on how you want to log (DB vs writing to the logger). If you want to log to the DB, you should have a separate logging model which is given the appropriate information from the controller, or simply with a belongs_to :user type setup. If you want to write to the logger, you should create a method in your application controller which you can call from your create and update methods (or whatever other actions you wanted to have a callback on.)

Or simply writing who wrote a change when an object can have multiple people write to it 

class Foo < ActiveRecord::Base
  belongs_to :user, as: :edited_by
end

class FooController < ApplicationController
  def update
    @foo = Foo.find(params[:id])
    @foo.attributes = params[:foo]
    @foo.edited_by = current_user
  end
end

I think you're misunderstanding what the model in Rails does. Its scope is the database. The reason it can't access current_user, is because the current user is not stored in the database, it is a session variable. This has absolutely nothing to do with the model, as this is something that can not exist without a browser.

ActiveRecord::Base is not a class that is designed to work with the browser, it is something that works with the database and only the database. You are using the browser as an interface to that model, but that layer is what needs to access browser specific things such as session variables, as your model is extending a class that is literally incapable of doing so.

This is not a dogma or style choice. This is a fact of the limitations of the class your model is extending from. That means your options basically boil down to extending from something else, handling it in your controller layer, or passing it to the model from your controller layer. ActiveRecord will not do what you want in this case.

sgrif
  • 3,702
  • 24
  • 30
  • hmm... so I think we might be agreeing to disagree. Clearly, I can monkey patch ActiveRecord::Base to put the current_user in there. The issue is do I really want to couple my User model to activities such as logging or auditing. While on the surface, it might seem like a good idea, it's actually a really bad idea. For example, the paper_trail gem, has a column called whodunnit which is basically the current_user.id. I guess that's the use case where it seems most pronounced. You don't really want to couple models to logging in a relational sense but you do want to have the DSL flexibility – timpone Aug 16 '12 at 18:33
  • I didn't claim that you couldn't force this behavior in by extending it with a module with some ugly code. You could do the same thing to handle outputting markup to display and determining what the request format was. My statement was that ActiveRecord will not do this nicely for you, and it will be easier to simply pass it as a method parameter. You wouldn't add a module to ActionController to have it start formatting markup, either. To give you a use case, what happens when you decide this needs to be a long running process and move it to a bg task or cron job where there is no session? – sgrif Aug 16 '12 at 23:12
  • Another way to put it is you've got 3 classes that handle 3 interfaces. ActiveRecord handles databases, ActionController handles HTTP, ActionView handles browser display. current_user is an HTTP session variable. There is a reason that these three pieces can communicate with each other. – sgrif Aug 16 '12 at 23:15
0

The two links you show (each showing imho the same approach) is very similar to a approach I still use. I store the current_user somewhere (indeed thread-context is the safest), and in an observer I can then create a kind of audit-log of all changes to the watched models, and still log the user.

This is imho a really clean approach.

An alternative method, which is more explicit, less clean but more MVC, is that you let the controller create the audit-log, effectively logging the actions of the users, and less the effects on different models. This might also be useful, and in one website we did both. In a controller you know the current-user, and you know the action, but it is more verbose.

I believe your concerns are that somehow this proposed solution is not good enough, or not MVC enough, or ... what?

Another related question: How to create a full Audit log in Rails for every table?

Also check out the audited gem, which solves this problem as well very cleanly.

Hope this helps.

Community
  • 1
  • 1
nathanvda
  • 49,707
  • 13
  • 117
  • 139
  • thx for the response. I agree with your first few paragraphs as it being a clean approach (kinda); honestly, I think it would be better to have a single accessor for all levels. I kinda think of the current_user as config which is obviously available to the model. I kinda disagree with the concept of MVC enough. I guess I'm more curious why this is intentionally not allowed more than anything else. – timpone Aug 15 '12 at 04:17
  • MVC is a very clean and powerful concept, but sometimes you do need cross-cutting information. So I believe MVC is very powerful and needed, to keep all the concerns separated, and to make your code clean. But for the cross-cutting behaviour we need to use a "hack". If we use observers, we don't even infect (?) our models with this, but we create a separate layer. The `audited` gem does the same. MVC in itself is very needed and powerful, and just because it does not solve this problem well, does not mean it is entirely bad. And there is perfectly working and compatible solution, imo. – nathanvda Aug 15 '12 at 09:24
  • Right, you're more agreeing than disagreeing. I feel like the inittial idea of MVC is fine but I don't understand the inability to access session variables or current user. I actually don't think MVC ever specifies that session variables shouldn't be accessed. I consider this to be config for an app not some violation of some sacred principle. More than willing to be shown why this is such a bad idea. I would argue that the notion of a different database in the config (like test or prod) is the same as the config that I'm discussing here. – timpone Aug 16 '12 at 18:23