ryanb / cancan

Authorization Gem for Ruby on Rails.
MIT License
6.27k stars 784 forks source link

namespace problem after version 1.6.8 #663

Closed diegorv closed 12 years ago

diegorv commented 12 years ago

After 1.6.8, things like that, stopped working

can :manage, CallCenter::Prospect

the error:

Expected /Users/diegorv/work/theERP/app/models/call_center/prospect.rb to define Prospect
andhapp commented 12 years ago

@diegorv: Thanks for reporting the bug. Apart from this, did you see any thing else?

diegorv commented 12 years ago

Not yet!

andhapp commented 12 years ago

Cool. I'll look at this one. There were few pull requests related to namespace models (or controllers), which I guess might have introduced this regression.

andhapp commented 12 years ago

@diegorv: Good news is that I could reproduce the error but to confirm that we have the same setup, just wanted to confirm one thing. You have the following model class:

class CallCenter::Prospect < ActiveRecord::Base
end

and this is in a file called app/models/prospect.rb. Please confirm.

Also, I'm guessing that when you upgraded CanCan, you also upgraded Rails version, because I could reproduce the same error with CanCan 1.6.7 and Rails 3.2.6, 3.2.5, 3.1.6, 3.1.5 and 3.1.4.

Just trying to establish where the bug is coming from and I suspect it's Rails. Please post your comments.

Update: Summarised my effort.

diegorv commented 12 years ago

Thank you for your work!

This is my complete info about my project

ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-darwin11.3.0]
gem 'rails', '3.2.6'
gem 'cancan'

file: app/models/call_center/prospect.rb

class CallCenter::Prospect < ActiveRecord::Base
end

file: app/models/call_center.rb

module CallCenter
 def self.table_name_prefix
  'call_center_'
 end
end

file: app/models/users/ability.rb

class Ability
  include CanCan::Ability

  def initialize(user)
    @user = user || User.new
    @user.roles.each { |role| send(role.url) }
  end

  def telemarketing
    can :manage, CallCenter::Prospect
  end
end

Before problem: rails 3.2.6 cancan 1.6.7

After problem rails 3.2.6 cancan 1.6.8

no update in others gems

andhapp commented 12 years ago

Thanks. I need to do more investigation, but I'm on the case.

diegorv commented 12 years ago

I found the bug

in file: cancan/lib/cancan/controller_resource.rb, line 222

def namespace
  @params[:controller].split("::")[0..-2]
end

to debug, i changed it to:

def namespace
  puts "controller: #{@params[:controller]}"
  @params[:controller].split("::")[0..-2]
end

the output:

 controller: call_center/prospects

Change

@params[:controller].split("::")[0..-2]

To

@params[:controller].split("/")[0..-2]

Fixed this bug!

But not sure how that affects everything

diegorv commented 12 years ago

Look this commit, this caused the bug

https://github.com/ryanb/cancan/commit/d5baed6281b4628071f8737090eaaec5168eacbf (9 months ago and merge NOW?! WTF?! too old guys)

Before this commit (working good)

def namespaced_name
 @name || @params[:controller].sub("Controller", "").singularize.camelize.constantize

 rescue NameError
   name
 end

After this commit (bug)

def namespace
  @params[:controller].split("::")[0..-2]
end

def namespaced_name
  [namespace, name.camelize].join('::').singularize.camelize.constantize
rescue NameError
  name
end

in older versions of Rails (like 9 months ago HAHAHAH), should work, but namespace is now "/" instead "::"

Regression for me.. :/

andhapp commented 12 years ago

@diegorv: Nice work! Would you like to create a pull request for it please?

The commit that you pointed to was meant to fix the nested resource loading, so not sure why it caused this regression.

Like any other open source project, the main creator of this library @ryanb was busy with other assignments, I guess, and this kind of went on the back burner. On the positive side, I've been working through the pull requests and managing the issues.

Thanks again for finding the cause of this issue.

diegorv commented 12 years ago

@andhapp I understand completely, so I'm trying to help too

I`ll make the pull request, but i don't have ruby 1.8.7, could u run the specs?

andhapp commented 12 years ago

Okay. I will. Will close this issue as you've opened a related pull request. Thanks, again.

andhapp commented 12 years ago

@diegorv : Hello, just looking at this again and I can't seem to replicate this issue anymore with the setup that you've mentioned in one of the comments above. So, I created a test application for the same and it'd be great if you could look at it and try replicating it at your end. Here's the repository (https://github.com/andhapp/cancan-issue-663).

This is what I do to test:

  1. Run the rails console
  2. Assuming there is a user in the database if not, please create one with User.create(username: 'test', password: 'test'
  3. In the console, do
    user = User.first # the user we just created
    ability = Ability.new(user) 
    ability.telemarketing # This should throw errors but for me it doesn't
    

Please test and let us know the outcome.

diegorv commented 12 years ago

@andhapp, your example is wrong, because without user role "telemarketing" like that

def roles
  ['telemarketing']
end

This

def initialize(user)
  @user = user || User.new
  @user.roles.each { |role| send(role) }
end

Will never call

def telemarketing
  can :manage, CallCenter::Prospect
end

Take a look in my fork: https://github.com/diegorv/cancan-issue-663, read "README"

The same problem, with different fix in https://github.com/ryanb/cancan/pull/668 (I prefer this instead of my)

andhapp commented 12 years ago

@diegrov: Ok gotcha. That pull request is against 2.x branch...so we need backport it for 1.x branch. I'll close this issue and close your pull request and then port the #668 to 1.x branch. Thanks for taking the time to look at my app.

diegorv commented 12 years ago

@andhapp you're welcome! keep rocking :)

Alexander-Senko commented 11 years ago

WTF?! The bug is still present in last version I can see (1.6.9) and in the master branch: https://github.com/ryanb/cancan/blob/master/lib/cancan/controller_resource.rb. Does anybody care?

diegorv commented 11 years ago

@andhapp ? @ryanb ?

:/

jaredbeck commented 11 years ago

This issue has been fixed in 1.6.10 by #675 (https://github.com/ryanb/cancan/commit/60cf6a67ef59c0c9b63bc27ea0101125c4193ea6)