ErwinM/acts_as_tenant

Upgrade to master branch, now current_user error

philsmy opened this issue · 12 comments

philsmy commented

Was running on 0.5.1 but we upgraded to the branch (because we wanted conditional require_tenant, which doesn't work right in that version) and now we get

23:06:18 web.1    | NameError (undefined local variable or method `current_user' for main:Object
23:06:18 web.1    | 
23:06:18 web.1    |   config.require_tenant = lambda { !current_user.admin? }
23:06:18 web.1    |                                     ^^^^^^^^^^^^):
23:06:18 web.1    |   
23:06:18 web.1    | config/initializers/acts_as_tenant.rb:2:in `block (2 levels) in <main>'
23:06:18 web.1    | app/controllers/dashboard_controller.rb:3:in `index'

config/initializers/acts_as_tenant.rb looks like this:

ActsAsTenant.configure do |config|
  config.require_tenant = lambda { !current_user.admin? }
end

Did upgrading mean we should have changed something else?

Running with devise 4.8.1

excid3 commented

The lambda doesn't run in the context of a request, so you wouldn't have access to current_user from Devise. And it wouldn't make sense to either because this applies in the Rails console too which is outside of a request.

You could use CurrentAttributes to assign a user if you wanted to access it inside this lambda.

See b55da4a

cc @cmer

philsmy commented

I appreciate what you are saying, my issue is more with the fact that 'upgrading' from 0.5.1 to master causes our app to break. Also, we are doing nothing that is not stated in https://github.com/ErwinM/acts_as_tenant#configuration-options

If that doesn't work it shouldn't be there anymore.

excid3 commented

You are using it in a way that doesn't make sense. You cannot access current_user in the lambda because it does not run in the context of a Rails request.

excid3 commented

Just to better explain:

Before this commit, ActsAsTenant would not evalulate the lambda, so yes there is a difference in how it works. Your lambda would have been ignored previously, but I don't think I would consider that a regression since you were expected to assign a boolean to the config option.

chrise86 commented

@excid3 sorry, I'm confused reading this, as it seems that @philsmy has followed what is written in the readme. Is this not correct?

  • 👍1
excid3 commented

The way it's implemented, I don't believe that example will work.

philsmy commented

IGNORE THIS

Rightly or wrongly, I fixed it like this. It works for both rails_admin and activeadmin:

# confing/initializers/acts_as_tenant.rb
ActsAsTenant.configure do |config|
  config.require_tenant = lambda do
    if defined?(current_user)
      !current_user.admin?
    else
      defined?(current_admin_user) ? !current_admin_user.admin? : false
    end
  end
end

At least our app runs without getting an exception and as far as I can tell the behavior is correct.

cmer commented

I think @excid3 is right, it doesn't seem to work the way I put it in the README. @philsmy are you sure current_user is ever defined? It doesn't seem to ever be available for me.

philsmy commented

Ah buggers. You are right. I am going to delete that comment of mine as it is misleading. So basically we just set the thing to false all the time? What is the point of this setting?

cmer commented

See my PR from tonight. You can set it based on other factors. ie: path if you have an admin dashboard, for example.

philsmy commented

@cmer I tried your code and same thing - it seems $request_env is never set.

cmer commented

That's just an example. You'd have to set that up in a middleware. That's outside of the scope of the readme.