Make Dynamic Finders on Association Collections Respect Association :order [patrick.t.joyce, technoweenie]. Closes #10211

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8174 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

5 files changed, 86 insertions, 3 deletions

diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index 13c99455c0..0a03efa5e4 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -133,7 +133,7 @@ module ActiveRecord
         end
 
         def construct_scope
-          { :find => { :conditions => @finder_sql, :joins => @join_sql, :readonly => false } }
+          { :find => { :conditions => @finder_sql, :joins => @join_sql, :readonly => false, :order => @reflection.options[:order] } }
         end
 
         # Join tables with additional columns on top of the two foreign keys must be considered ambiguous unless a select
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 2880bde3a4..972745ee31 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -167,7 +167,7 @@ module ActiveRecord
         def construct_scope
           create_scoping = {}
           set_belongs_to_association_for(create_scoping)
-          { :find => { :conditions => @finder_sql, :readonly => false }, :create => create_scoping }
+          { :find => { :conditions => @finder_sql, :readonly => false, :order => @reflection.options[:order] }, :create => create_scoping }
         end
     end
   end
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 8bff88e839..a9cacb09c8 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -235,7 +235,8 @@ module ActiveRecord
             :find   => { :from        => construct_from,
                          :conditions  => construct_conditions,
                          :joins       => construct_joins,
-                         :select      => construct_select } }
+                         :select      => construct_select,
+                         :order       => @reflection.options[:order] } }
         end
 
         def construct_sql
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index 842d8e453a..4969d65a99 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -463,6 +463,26 @@ class HasManyAssociationsTest < Test::Unit::TestCase
     assert_equal 2, companies(:first_firm).limited_clients.find(:all, :limit => nil).size
   end
 
+  def test_dynamic_find_should_respect_association_order
+    assert_equal companies(:second_client), companies(:first_firm).clients_sorted_desc.find(:first, :conditions => "type = 'Client'")
+    assert_equal companies(:second_client), companies(:first_firm).clients_sorted_desc.find_by_type('Client')
+  end
+
+  def test_dynamic_find_order_should_override_association_order
+    assert_equal companies(:first_client), companies(:first_firm).clients_sorted_desc.find(:first, :conditions => "type = 'Client'", :order => 'id')
+    assert_equal companies(:first_client), companies(:first_firm).clients_sorted_desc.find_by_type('Client', :order => 'id')
+  end
+
+  def test_dynamic_find_all_should_respect_association_order
+    assert_equal [companies(:second_client), companies(:first_client)], companies(:first_firm).clients_sorted_desc.find(:all, :conditions => "type = 'Client'")
+    assert_equal [companies(:second_client), companies(:first_client)], companies(:first_firm).clients_sorted_desc.find_all_by_type('Client')
+  end
+
+  def test_dynamic_find_all_order_should_override_association_order
+    assert_equal [companies(:first_client), companies(:second_client)], companies(:first_firm).clients_sorted_desc.find(:all, :conditions => "type = 'Client'", :order => 'id')
+    assert_equal [companies(:first_client), companies(:second_client)], companies(:first_firm).clients_sorted_desc.find_all_by_type('Client', :order => 'id')
+  end
+
   def test_triple_equality
     assert !(Array === Firm.find(:first).clients)
     assert Firm.find(:first).clients === Array
@@ -1079,6 +1099,27 @@ class HasManyAssociationsTest < Test::Unit::TestCase
   def test_assign_ids_for_through
     assert_raise(NoMethodError) { authors(:mary).comment_ids = [123] }
   end
+
+  def test_dynamic_find_should_respect_association_order_for_through
+    assert_equal Comment.find(10), authors(:david).comments_desc.find(:first, :conditions => "comments.type = 'SpecialComment'")
+    assert_equal Comment.find(10), authors(:david).comments_desc.find_by_type('SpecialComment')
+  end
+
+  def test_dynamic_find_order_should_override_association_order_for_through
+    assert_equal Comment.find(3), authors(:david).comments_desc.find(:first, :conditions => "comments.type = 'SpecialComment'", :order => 'comments.id')
+    assert_equal Comment.find(3), authors(:david).comments_desc.find_by_type('SpecialComment', :order => 'comments.id')
+  end
+
+  def test_dynamic_find_all_should_respect_association_order_for_through
+    assert_equal [Comment.find(10), Comment.find(7), Comment.find(6), Comment.find(3)], authors(:david).comments_desc.find(:all, :conditions => "comments.type = 'SpecialComment'")
+    assert_equal [Comment.find(10), Comment.find(7), Comment.find(6), Comment.find(3)], authors(:david).comments_desc.find_all_by_type('SpecialComment')
+  end
+
+  def test_dynamic_find_all_order_should_override_association_order_for_through
+    assert_equal [Comment.find(3), Comment.find(6), Comment.find(7), Comment.find(10)], authors(:david).comments_desc.find(:all, :conditions => "comments.type = 'SpecialComment'", :order => 'comments.id')
+    assert_equal [Comment.find(3), Comment.find(6), Comment.find(7), Comment.find(10)], authors(:david).comments_desc.find_all_by_type('SpecialComment', :order => 'comments.id')
+  end
+
 end
 
 class BelongsToAssociationsTest < Test::Unit::TestCase
@@ -1783,6 +1824,46 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
     assert_equal 3, projects(:active_record).limited_developers.find(:all, :limit => nil).size
   end
 
+  def test_dynamic_find_should_respect_association_order
+    # Developers are ordered 'name DESC, id DESC'
+    low_id_jamis = developers(:jamis)
+    middle_id_jamis = developers(:poor_jamis)
+    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
+
+    assert_equal high_id_jamis, projects(:active_record).developers.find(:first, :conditions => "name = 'Jamis'")
+    assert_equal high_id_jamis, projects(:active_record).developers.find_by_name('Jamis')
+  end
+
+  def test_dynamic_find_order_should_override_association_order
+    # Developers are ordered 'name DESC, id DESC'
+    low_id_jamis = developers(:jamis)
+    middle_id_jamis = developers(:poor_jamis)
+    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
+
+    assert_equal low_id_jamis, projects(:active_record).developers.find(:first, :conditions => "name = 'Jamis'", :order => 'id')
+    assert_equal low_id_jamis, projects(:active_record).developers.find_by_name('Jamis', :order => 'id')
+  end
+
+  def test_dynamic_find_all_should_respect_association_order
+    # Developers are ordered 'name DESC, id DESC'
+    low_id_jamis = developers(:jamis)
+    middle_id_jamis = developers(:poor_jamis)
+    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
+
+    assert_equal [high_id_jamis, middle_id_jamis, low_id_jamis], projects(:active_record).developers.find(:all, :conditions => "name = 'Jamis'")
+    assert_equal [high_id_jamis, middle_id_jamis, low_id_jamis], projects(:active_record).developers.find_all_by_name('Jamis')
+  end
+
+  def test_dynamic_find_all_order_should_override_association_order
+    # Developers are ordered 'name DESC, id DESC'
+    low_id_jamis = developers(:jamis)
+    middle_id_jamis = developers(:poor_jamis)
+    high_id_jamis = projects(:active_record).developers.create(:name => 'Jamis')
+
+    assert_equal [low_id_jamis, middle_id_jamis, high_id_jamis], projects(:active_record).developers.find(:all, :conditions => "name = 'Jamis'", :order => 'id')
+    assert_equal [low_id_jamis, middle_id_jamis, high_id_jamis], projects(:active_record).developers.find_all_by_name('Jamis', :order => 'id')
+  end
+
   def test_new_with_values_in_collection
     jamis = DeveloperForProjectWithAfterCreateHook.find_by_name('Jamis')
     david = DeveloperForProjectWithAfterCreateHook.find_by_name('David')
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index 979c72ec63..782a7d608f 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -15,6 +15,7 @@ class Author < ActiveRecord::Base
     end
   end
   has_many :comments, :through => :posts
+  has_many :comments_desc, :through => :posts, :source => :comments, :order => 'comments.id DESC'
   has_many :funky_comments, :through => :posts, :source => :comments
 
   has_many :special_posts