Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Consider undeprecating attributes_for #274

Closed
subelsky opened this Issue Jan 18, 2012 · 19 comments

Comments

Projects
None yet
Contributor

subelsky commented Jan 18, 2012

Please undeprecate attributes_for? This syntax is extremely useful to me, and your suggested hack (Factory.build(:user).attributes) does not work for me. I have a controller spec roughly like this:

MyClass.should_not_receive(:new)
post :create, my_class: Factory.build(:my_class,params).attributes 
# fails because you're forcing me to call MyClass.new to setup the test

Besides this problem, attributes returns a bunch of stuff not needed in a scenario like that (such as created_at, updated_at, etc).

I love FactoryGirl so please don't take this as a criticism. I just really rely on attributes_for and I would probably have to create a private fork if you remove this essential feature.

Owner

jferris commented Jan 18, 2012

If you're using stubs, why do you need actual attributes? Couldn't you just
post with a hash like my_class: { "attribute" => "value" }?

One issue with deprecating FactoryGirl.attributes_for is when using it to create a new user, with password.

That is, FactoryGirl.create(:user).attributes[:password] is, in most cases, some hashed version of the password, not suitable for submitting to a User.new/User.create. You can work around this in several ways - including merging in a known attribute when you care, or by not using a factory in those cases (I've done the later).

I too agree to not deprecate this method, see my comment here: a883315#commitcomment-878888

Contributor

subelsky commented Jan 18, 2012

@jferris yes, I could do a hash like that but then I'm repeating information that I'm hoping to keep DRY in the factory definitions. That particular code involves some decision making based on the contents of the hash so I can't just pass a random hash. This was also just an isolated example, I use attributes_for in several other scenarios across various projects. @stevenharman gave another good one.

@ghost

ghost commented Jan 18, 2012

+1 I got hit by the user issue too. attributes_for is too valuable.

It's nice to have a attribute hash from factories that contains only the valid attributes necessary to successfully create a valid record. Please do not deprecate.

tehtorq commented Jan 19, 2012

Also in favor of not deprecating this method. I use attributes_for to obtain random valid attributes which are passed as part of a larger hash to a request class which delegates those attributes to the related class. The workaround seems messier and leaves an unwanted model as a side-effect.

Odaeus commented Jan 19, 2012

Please restore this method as I have the user issue as well. There are valid reasons that make attributes_for useful.

+1 for UNdeprecate this useful method. As it was explained in previous posts, build().attributes does NOT functionally replace attributes_for().

+1 here too. An important difference between the two methods is that attributes_for skips over any ActiveRecord validations and associations. It can be useful to use attributes_for to get a hash of attributes on a model without needing validations or FactoryGirl's associations.

DouweM commented Jan 19, 2012

Another +1! My argument is exactly the same as @itspriddle's.

Contributor

justinko commented Jan 20, 2012

+1

I'm curious, what was the reason to deprecate it in the first place?

saks commented Jan 20, 2012

+1

i have troubles with that, because new syntax (FactoryGirl.build(:basic_user).attributes) and old way (Factory.attributes_for :basic_user) returns different results:

Factory.attributes_for :basic_user:
{:first_name=>"John", :last_name=>"Doe", :email=>"user1.1327095603.430233@example.com", :birthdate=>Mon, 20 Jan 1997, :password=>"password", :password_confirmation=>"password", :password_current=>"password", :admin=>false, :email_confirmed=>true, :license_agreement=>true, :time_zone=>"Bratislava", :facebook_uri=>"http://www.facebook.com/...", :twitter_username=>"...", :website_uri=>"http://....com/", :biography=>"All your base\n\nare belong to us.", :current_login_ip=>"0.0.0.0", :account_type=>"regular"}

FactoryGirl.build(:basic_user).attributes:
{"id"=>nil, "email"=>nil, "birthdate"=>nil, "activation_code"=>nil, "created_at"=>nil, "updated_at"=>nil, "single_access_token"=>nil, "login_count"=>0, "failed_login_count"=>0, "last_request_at"=>nil, "current_login_at"=>nil, "last_login_at"=>nil, "current_login_ip"=>"0.0.0.0", "last_login_ip"=>nil, "rss_token"=>nil, "license_agreement"=>true, "under13ok"=>false, "can_review_academic"=>true, "sponsor_id"=>nil, "cached_slug"=>nil, "active"=>true, "type_of_view"=>0, "is_dashboard_visited"=>false, "auto_smarthinking"=>false, "gear_notification"=>false, "onetime_sso_token"=>nil, "onetime_sso_token_created_at"=>nil, "super_admin"=>false, "user_views_count"=>0, "invited_by_id"=>nil, "inviter_bonus_given"=>nil, "fb_profile_id"=>nil, "use_fb_photo"=>nil, "birthdate_from_fb"=>false, "facebook_uri"=>"http://www.facebook.com/...", "twitter_username"=>"...", "website_uri"=>"http://...", "censored"=>false}

FactoryGirl.create(:basic_user).attributes:
{"id"=>1, "email"=>nil, "birthdate"=>nil, "activation_code"=>nil, "created_at"=>Fri, 20 Jan 2012 15:47:25 CST -06:00, "updated_at"=>Fri, 20 Jan 2012 15:47:25 CST -06:00, "single_access_token"=>"MoqP0HX8DVRzF26BZXRB", "login_count"=>0, "failed_login_count"=>0, "last_request_at"=>nil, "current_login_at"=>nil, "last_login_at"=>nil, "current_login_ip"=>"0.0.0.0", "last_login_ip"=>nil, "rss_token"=>nil, "license_agreement"=>true, "under13ok"=>false, "can_review_academic"=>true, "sponsor_id"=>nil, "cached_slug"=>nil, "active"=>true, "type_of_view"=>0, "is_dashboard_visited"=>false, "auto_smarthinking"=>false, "gear_notification"=>false, "onetime_sso_token"=>nil, "onetime_sso_token_created_at"=>nil, "super_admin"=>false, "user_views_count"=>0, "invited_by_id"=>nil, "inviter_bonus_given"=>nil, "fb_profile_id"=>nil, "use_fb_photo"=>nil, "birthdate_from_fb"=>false, "facebook_uri"=>"http://www.facebook.com/...", "twitter_username"=>"...", "website_uri"=>"http://.....com/", "censored"=>false}
Contributor

justinko commented Jan 20, 2012

@saks - I was just about to report that. Thanks!

Owner

joshuaclayton commented Jan 20, 2012

Hey everyone!

We ended up un-deprecating it (along with build_stubbed) here: 2d6adfd

We've been trying to simplify the various build strategies and the goal was to introduce a way to register different build strategies (so developers could write their own). While this may be delayed, it's still a long-term goal. We've definitely heard you all loud and clear; attributes_for is going to stick around without any change to its implementation.

Thanks for voicing your concerns about the removal of attributes_for - we appreciate the feedback!

jmazzi commented Jan 20, 2012

Thank you for listening to us!
On Jan 20, 2012 5:49 PM, "Joshua Clayton" <
reply@reply.github.com>
wrote:

Hey everyone!

We ended up un-deprecating it (along with build_stubbed) here:
2d6adfd

We've been trying to simplify the various build strategies and the goal
was to introduce a way to register different build strategies (so
developers could write their own). While this may be delayed, it's still a
long-term goal. We've definitely heard you all loud and clear;
attributes_for is going to stick around without any change to its
implementation.

Thanks for voicing your concerns about the removal of attributes_for -
we appreciate the feedback!


Reply to this email directly or view it on GitHub:
#274 (comment)

@ghost

ghost commented Jan 20, 2012

and we didn't have to get Google and Wikipedia to go dark. :)

saks commented Jan 21, 2012

Thanks guys!

Contributor

subelsky commented Jan 22, 2012

awesome thank you @thoughtbot!!

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