Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider undeprecating attributes_for #274

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

Consider undeprecating attributes_for #274

subelsky opened this issue Jan 18, 2012 · 19 comments

Comments

@subelsky
Copy link
Contributor

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.

@jferris
Copy link
Member

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" }?

@stevenharman
Copy link

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).

@masterkain
Copy link

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

@subelsky
Copy link
Contributor Author

@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.

@ruckus-matte
Copy link

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

@masterkain
Copy link

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
Copy link

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
Copy link

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.

@rawongithub
Copy link

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

@itspriddle
Copy link

+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
Copy link

DouweM commented Jan 19, 2012

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

@justinko
Copy link
Contributor

+1

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

@saks
Copy link

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}

@justinko
Copy link
Contributor

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

@joshuaclayton
Copy link
Contributor

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
Copy link

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)

@ruckus-matte
Copy link

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

@saks
Copy link

saks commented Jan 21, 2012

Thanks guys!

@subelsky
Copy link
Contributor Author

awesome thank you @thoughtbot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests