Don't build through association on new action #128

Open
ryanb opened this Issue Aug 20, 2010 · 13 comments

Projects

None yet

8 participants

@ryanb
Owner
ryanb commented Aug 20, 2010

Currently if you have a nested resource, the new action will build through the association in load_resource before filter.

# new action
@task = @project.tasks.build

The problem with this is that it adds the new (empty) task to the @project.tasks association. What if one wants to list the @project.tasks in the side bar? The new task will show up there.

Instead It should do this.

@task = Task.new
@task.project = @project

The reason it's not done through mass assignment is so it's compatible with attr_accessible.

@lhoBas
lhoBas commented Aug 24, 2010

This sounds like a good change, had to work around this a couple of times in my app now.

@ryanb
Owner
ryanb commented Aug 31, 2010

This will cause some problems with polymorphic associations. Let's say you have articles and photos and both have nested comments. Currently the CommentsController would be made like this.

load_and_authorize_resource :article
load_and_authorize_resource :photo
load_and_authorize_resource :comment, :through => [:article, :photo]

The problem is @article.comments.build works but @comment.article = @article won't work. Instead it should be @comment.commentable = @article or whatever the name of the polymorphic association is. Therefore we need some way to customize both the name of the association and the instance variable(s) used. I'm considering changing the :through option to :belongs_to so it is apparent this should match the name of the association.

load_and_authorize_resource :article
load_and_authorize_resource :photo
load_and_authorize_resource :comment, :belongs_to => :commentable, :belongs_to_instance => [:article, :photo]

I don't care for the :belongs_to_instance option but can't think of anything better at the moment..

@nandalopes

This can help?

load_and_authorize_resource :article, :instance_name => :commentable
load_and_authorize_resource :photo, :instance_name => :commentable
load_and_authorize_resource :comment, :through => :commentable

So we can do this:

@comment = Comment.new
@comment.commentable = @commentable
@greenplastik

nandalopes approach seems like a decent approach. though i'm not sure i would change the normal polymorphic wording. Maybe something for polymorphic controllers, you'd do something like:

load_and_authorize_resource :article, :as => :commentable
load_and_authorize_resource :photo, :as => :commentable
load_and_authorize_resource :comment, :belongs_to => :commentable, :polymorphic => true

Then if it's not polymorphic you could leave off the polymorphic attribute. So for a ArticlesController, where an article belongs to a user, you'd go with something like:

load_and_authorize_resource :user
load_and_authorize_resource :article, :belongs_to => :user

Thoughts?

@clyfe
clyfe commented Mar 19, 2011

Note that sometimes (all times?) assigning to a relation saves to database!

vhochstein/active_scaffold#92

@szimek
szimek commented Mar 24, 2011

Why not simply @task = @project.tasks.new instead of @task = @project.tasks.build?

@ryanb
Owner
ryanb commented Mar 24, 2011

@szimek both new and build have the same behavior when calling on an association (AFAIK).

@szimek
szimek commented Mar 24, 2011

@ryanb I've recently changed few can? calls in my app to use new instead of build and I was sure that the behavior of #new and #build is different, but I checked it again and:

class User < ActiveRecord::Base
  has_many :friendships
  has_many :friends, :through => :friendships
end

user.friendships # => []
user.friendships.new
user.friendships # => []
user.friendships.build
user.friendships # => [#<Friendship id: nil, user_id: 1, friend_id: nil>]

# But
user.friends # => []
user.friends.new
user.friends # => [#<User id: nil...>]

Got to love Rails consistency :)

@clyfe
clyfe commented Mar 24, 2011

put this in lighthouse

@ryanb
Owner
ryanb commented Mar 25, 2011

@szimek, thanks for researching this. Seeing this makes me think it's an even better idea to not go through associations each time.

I wonder how something like InheritedResource handles nested resources. Might be worth investigating.

@ellmo
ellmo commented Jun 23, 2012

I see this is an old issue and it has it's milestone set to 2.0, but I've recently stumbled upon a discrepancy when using the recommended solution:

if can? :read, @project => Task

So in my application there are Projects, which have Maps, now I set up my scopes and abilities and I think they are okay, but I get two different responses (copying output from console, a is an Ability instance created for a specific user):

a.can? :create, Project.find(8).maps.build
  #Project Load (0.6ms)  SELECT `projects`.* FROM `projects` WHERE `projects`.`id` = 8 LIMIT 1
#=> false

a.can? :create, Project.find(8).maps.new
  #Project Load (0.5ms)  SELECT `projects`.* FROM `projects` WHERE `projects`.`id` = 8 LIMIT 1
#=> false

a.can? :create, project => Map
#=> true

Two first behaviors are correct (or at least they are what I'm aiming for). Any idea what I'm missing?

@sh-ft sh-ft referenced this issue in inossidabile/heimdallr-resource Dec 2, 2012
Merged

Proper specs, refactoring and a couple of new features #4

@xhoy
xhoy commented Jul 1, 2014

Thanks for your submission! The ryanb/cancan repository has been inactive since Sep 06, 2013.
Since only Ryan himself has commit permissions, the CanCan project is on a standstill.

CanCan has many open issues, including missing support for Rails 4. To keep CanCan alive, an active fork exists at cancancommunity/cancancan. The new gem is cancancan. More info is available at #994.

If your pull request or issue is still applicable, it would be really appreciated if you resubmit it to CanCanCan.

We hope to see you on the other side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment