Rails Ruby on Rails

Comments and changes to this ticket

• 

  Santiago Pastorino September 8th, 2010 @ 12:38 PM

  • Importance changed from “” to “Low”

  Can you provide a failing test case and/or a patch?
  Thanks.

• You flagged this item as spam.
  

  Alexis Toulotte September 8th, 2010 @ 01:27 PM

  Sure, I will soon :)

• You flagged this item as spam.
  

  Marcelo Giorgi September 8th, 2010 @ 01:32 PM

  Hi,

  I've been working on this and came up with this test and patch.

  • set-attributes-properly-for-model-build-from-assoc-with-conditions.diff 7.9 KB

• You flagged this item as spam.
  

  Marcelo Giorgi September 8th, 2010 @ 01:44 PM

  • Tag changed from “associations, scope” to “associations, patch, scope”

• You flagged this item as spam.
  

  xds2000 September 14th, 2010 @ 11:32 AM

  +1 i have test Marcelo Giorgi patch,pass
  my env:
  rvm 1.0.7
  ruby 1.8.7 (2009-12-24 patchlevel 248) [i686-linux], MBARI 0x8770, Ruby Enterprise Edition 2010.01

• You flagged this item as spam.
  

  xds2000 September 14th, 2010 @ 11:50 AM

  more test result:
  [xds2000@localhost activerecord] (build_model_from_association %)$ rake test_mysql TEST=test/cases/method_scoping_test.rb (in /home/xds2000/workspace/rails/activerecord) /home/xds2000/.rvm/rubies/ree-1.8.7-2010.01/bin/ruby -w -I"lib:test:test/connections/native_mysql" "/home/xds2000/.rvm/gems/ree-1.8.7-2010.01/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/cases/method_scoping_test.rb" /home/xds2000/.rvm/rubies/ree-1.8.7-2010.01/lib/ruby/1.8/pathname.rb:263: warning: *' interpreted as argument prefix Using native MySQL<br/> Loaded suite /home/xds2000/.rvm/gems/ree-1.8.7-2010.01/gems/rake-0.8.7/lib/rake/rake_test_loader<br/> Started<br/> ..................................................... Finished in 1.12336 seconds.

  53 tests, 131 assertions, 0 failures, 0 errors

• You flagged this item as spam.
  

  Lake September 15th, 2010 @ 06:42 AM

  Hey, nice addition. I'm on 1.9.2 and the tests pass for me as well.

  However, I notice that there two test method definitions with typos. The word "through" should be used, but "thorugh" is used instead. You can find the typos in "activerecord/test/cases/associations/has_many_through_associations_test.rb"

  Also, I noticed that this block of code is duplicated.

    if scope = self.class.send(:current_scoped_methods)
      create_with = scope.scope_for_create
      create_with.each { |att,value| self.respond_to?(:"#{att}=") && self.send("#{att}=", value) } if create_with
    end
  

  It is written once in the initialize method and once in the initialize_copy method. What steps can be taken to reduce the duplication? I attempted to pull out the code into an instance method. That seemed to work, though I had difficulties coming up with a proper name and home for the method. Maybe you have a better idea?

• You flagged this item as spam.
  

  Marcelo Giorgi September 15th, 2010 @ 01:41 PM

  • Assigned user set to “Santiago Pastorino”

  You see, that's what I love about our community, great feedback!

  I agree with your recommendation, so I've created a method called populate_with_scope_attributes, to avoid that duplication. I've updated the patch with this fix, and made the correspondent adjustments for the test typos.

  Please let me know if this changes are appropriate for what you suggested.

  BTW: Patch still applies cleanly on master

  Thank you very much

  • set-attributes-properly-for-model-build-from-assoc-with-conditions-update.diff 8.2 KB

• You flagged this item as spam.
  

  Marcelo Giorgi September 15th, 2010 @ 04:38 PM

  Just noticed that Lake was right and the entire block can be wrapped into a separated method.

  This is the updated patch

  • set-attributes-properly-for-model-build-from-assoc-with-conditions-after-lake-suggestion.diff 8.4 KB

• You flagged this item as spam.
  

  Lake September 15th, 2010 @ 11:57 PM

  Thanks for updating the patch! Your modifications look great.

  This patch applies cleanly to master and the "rake test_mysql" suite is passing.

  Nice work! +1

• You flagged this item as spam.
  

  Alvaro Campodonico September 16th, 2010 @ 10:52 PM

  +1

  This patch applies cleanly on master now and test are passing.

  I like the last patch as it is more DRY (as Lake mentioned).

  About my environment, I'm using ree-1.8.7-2010.01

• You flagged this item as spam.
  

  Lake September 16th, 2010 @ 10:58 PM

  • Tag changed from “associations, patch, scope” to “associations, patch, scope, verified”
• 

  Repository September 28th, 2010 @ 07:23 PM

  • State changed from “new” to “resolved”

  (from [cdfd013dd7953fc87038c73ecddbaa8cfe29a301]) Set attributes properly for model built from association with conditions [#5562 state:resolved]

  Signed-off-by: Santiago Pastorino santiago@wyeworks.com
  http://github.com/rails/rails/commit/cdfd013dd7953fc87038c73ecddbaa...

• 

  Repository September 28th, 2010 @ 07:23 PM

  (from [9e5545cd662a7ffdf93eb176c25f83170a6a388e]) Set attributes properly for model built from association with conditions [#5562 state:resolved]

  Signed-off-by: Santiago Pastorino santiago@wyeworks.com
  http://github.com/rails/rails/commit/9e5545cd662a7ffdf93eb176c25f83...