Fixed that to_param should be used and honored instead of hardcoding the id (closes #11406) [gspiers]

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

3 files changed, 112 insertions, 32 deletions

diff --git a/activeresource/CHANGELOG b/activeresource/CHANGELOG
index 1e1459df67..6048a6aafc 100644
--- a/activeresource/CHANGELOG
+++ b/activeresource/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Fixed that to_param should be used and honored instead of hardcoding the id #11406 [gspiers]
+
 * Improve documentation. [Radar, Jan De Poorter, chuyeow, xaviershay, danger, miloops, Xavier Noria,  Sunny Ripert]
 
 * Use HEAD instead of GET in exists? [bscofield]
diff --git a/activeresource/lib/active_resource/base.rb b/activeresource/lib/active_resource/base.rb
index 4d43a800eb..37e77d4f93 100644
--- a/activeresource/lib/active_resource/base.rb
+++ b/activeresource/lib/active_resource/base.rb
@@ -350,7 +350,7 @@ module ActiveResource
       #
       def element_path(id, prefix_options = {}, query_options = nil)
         prefix_options, query_options = split_options(prefix_options) if query_options.nil?
-        "#{prefix(prefix_options)}#{collection_name}/#{id}.#{format.extension}#{query_string(query_options)}"
+        "#{prefix(prefix_options)}#{collection_name}/#{id}.#{format.extension}#{query_string(query_options)}"        
       end
 
       # Gets the collection path for the REST resources.  If the +query_options+ parameter is omitted, Rails
@@ -760,7 +760,7 @@ module ActiveResource
     #   that_guy.exists?
     #   # => false
     def exists?
-      !new? && self.class.exists?(id, :params => prefix_options)
+      !new? && self.class.exists?(to_param, :params => prefix_options)      
     end
 
     # A method to convert the the resource to an XML string.
@@ -807,7 +807,7 @@ module ActiveResource
     #   my_branch.name
     #   # => Wilson Road
     def reload
-      self.load(self.class.find(id, :params => @prefix_options).attributes)
+      self.load(self.class.find(to_param, :params => @prefix_options).attributes)
     end
 
     # A method to manually load attributes from a hash. Recursively loads collections of
@@ -903,7 +903,7 @@ module ActiveResource
       end
 
       def element_path(options = nil)
-        self.class.element_path(id, options || prefix_options)
+        self.class.element_path(to_param, options || prefix_options)
       end
 
       def collection_path(options = nil)
diff --git a/activeresource/test/base_test.rb b/activeresource/test/base_test.rb
index c85d40f8fa..13709f1ca1 100644
--- a/activeresource/test/base_test.rb
+++ b/activeresource/test/base_test.rb
@@ -7,40 +7,45 @@ class BaseTest < Test::Unit::TestCase
   def setup
     @matz  = { :id => 1, :name => 'Matz' }.to_xml(:root => 'person')
     @david = { :id => 2, :name => 'David' }.to_xml(:root => 'person')
+    @greg  = { :id => 3, :name => 'Greg' }.to_xml(:root => 'person')    
     @addy  = { :id => 1, :street => '12345 Street' }.to_xml(:root => 'address')
     @default_request_headers = { 'Content-Type' => 'application/xml' }
     @rick = { :name => "Rick", :age => 25 }.to_xml(:root => "person")
     @people = [{ :id => 1, :name => 'Matz' }, { :id => 2, :name => 'David' }].to_xml(:root => 'people')
     @people_david = [{ :id => 2, :name => 'David' }].to_xml(:root => 'people')
     @addresses = [{ :id => 1, :street => '12345 Street' }].to_xml(:root => 'addresses')
-    
+
     ActiveResource::HttpMock.respond_to do |mock|
-      mock.get    "/people/1.xml",             {}, @matz
-      mock.get    "/people/2.xml",             {}, @david
-      mock.get    "/people/3.xml",             {'key' => 'value'}, nil, 404
-      mock.put    "/people/1.xml",             {}, nil, 204
-      mock.delete "/people/1.xml",             {}, nil, 200
-      mock.delete "/people/2.xml",             {}, nil, 400
-      mock.get    "/people/99.xml",            {}, nil, 404
-      mock.post   "/people.xml",               {}, @rick, 201, 'Location' => '/people/5.xml'
-      mock.get    "/people.xml",               {}, @people
-      mock.get    "/people/1/addresses.xml",   {}, @addresses
-      mock.get    "/people/1/addresses/1.xml", {}, @addy
-      mock.get    "/people/1/addresses/2.xml", {}, nil, 404
-      mock.get    "/people/2/addresses/1.xml", {}, nil, 404
-      mock.put    "/people/1/addresses/1.xml", {}, nil, 204
-      mock.delete "/people/1/addresses/1.xml", {}, nil, 200
-      mock.post   "/people/1/addresses.xml",   {}, nil, 201, 'Location' => '/people/1/addresses/5'
-      mock.get    "/people//addresses.xml",    {}, nil, 404
-      mock.get    "/people//addresses/1.xml",  {}, nil, 404
-      mock.put    "/people//addresses/1.xml",  {}, nil, 404
-      mock.delete "/people//addresses/1.xml",  {}, nil, 404
-      mock.post   "/people//addresses.xml",    {}, nil, 404
-      mock.head   "/people/1.xml",             {}, nil, 200
-      mock.head   "/people/99.xml",            {}, nil, 404
-      mock.head   "/people/1/addresses/1.xml", {}, nil, 200
-      mock.head   "/people/1/addresses/2.xml", {}, nil, 404
-      mock.head   "/people/2/addresses/1.xml", {}, nil, 404
+      mock.get    "/people/1.xml",                {}, @matz
+      mock.get    "/people/2.xml",                {}, @david
+      mock.get    "/people/Greg.xml",             {}, @greg      
+      mock.get    "/people/4.xml",                {'key' => 'value'}, nil, 404
+      mock.put    "/people/1.xml",                {}, nil, 204
+      mock.delete "/people/1.xml",                {}, nil, 200
+      mock.delete "/people/2.xml",                {}, nil, 400
+      mock.get    "/people/99.xml",               {}, nil, 404
+      mock.post   "/people.xml",                  {}, @rick, 201, 'Location' => '/people/5.xml'
+      mock.get    "/people.xml",                  {}, @people
+      mock.get    "/people/1/addresses.xml",      {}, @addresses
+      mock.get    "/people/1/addresses/1.xml",    {}, @addy
+      mock.get    "/people/1/addresses/2.xml",    {}, nil, 404
+      mock.get    "/people/2/addresses/1.xml",    {}, nil, 404
+      mock.get    "/people/Greg/addresses/1.xml", {}, @addy      
+      mock.put    "/people/1/addresses/1.xml",    {}, nil, 204
+      mock.delete "/people/1/addresses/1.xml",    {}, nil, 200
+      mock.post   "/people/1/addresses.xml",      {}, nil, 201, 'Location' => '/people/1/addresses/5'
+      mock.get    "/people//addresses.xml",       {}, nil, 404
+      mock.get    "/people//addresses/1.xml",     {}, nil, 404
+      mock.put    "/people//addresses/1.xml",     {}, nil, 404
+      mock.delete "/people//addresses/1.xml",     {}, nil, 404
+      mock.post   "/people//addresses.xml",       {}, nil, 404
+      mock.head   "/people/1.xml",                {}, nil, 200
+      mock.head   "/people/Greg.xml",             {}, nil, 200
+      mock.head   "/people/99.xml",               {}, nil, 404
+      mock.head   "/people/1/addresses/1.xml",    {}, nil, 200
+      mock.head   "/people/1/addresses/2.xml",    {}, nil, 404
+      mock.head   "/people/2/addresses/1.xml",    {}, nil, 404
+      mock.head   "/people/Greg/addresses/1.xml", {}, nil, 200
     end
 
     Person.user = nil
@@ -302,6 +307,30 @@ class BaseTest < Test::Unit::TestCase
   def test_custom_element_path
     assert_equal '/people/1/addresses/1.xml', StreetAddress.element_path(1, :person_id => 1)
     assert_equal '/people/1/addresses/1.xml', StreetAddress.element_path(1, 'person_id' => 1)
+    assert_equal '/people/Greg/addresses/1.xml', StreetAddress.element_path(1, 'person_id' => 'Greg')    
+  end
+  
+  def test_custom_element_path_with_redefined_to_param
+    Person.module_eval do
+      alias_method :original_to_param_element_path, :to_param
+       def to_param  
+         name
+       end
+    end
+
+    # Class method.
+    assert_equal '/people/Greg.xml', Person.element_path('Greg')
+    
+    # Protected Instance method.
+    assert_equal '/people/Greg.xml', Person.find('Greg').send(:element_path)
+
+    ensure
+      # revert back to original
+      Person.module_eval do
+        # save the 'new' to_param so we don't get a warning about discarding the method
+        alias_method :element_path_to_param, :to_param
+        alias_method :to_param, :original_to_param_element_path
+      end
   end
 
   def test_custom_element_path_with_parameters
@@ -406,7 +435,7 @@ class BaseTest < Test::Unit::TestCase
 
   def test_custom_header
     Person.headers['key'] = 'value'
-    assert_raises(ActiveResource::ResourceNotFound) { Person.find(3) }
+    assert_raises(ActiveResource::ResourceNotFound) { Person.find(4) }
   ensure
     Person.headers.delete('key')
   end
@@ -487,6 +516,26 @@ class BaseTest < Test::Unit::TestCase
     assert_equal address, address.reload
   end
   
+  def test_reload_with_redefined_to_param
+    Person.module_eval do
+      alias_method :original_to_param_reload, :to_param
+       def to_param  
+         name
+       end
+    end
+
+    person = Person.find('Greg')
+    assert_equal person, person.reload
+
+    ensure
+      # revert back to original
+      Person.module_eval do
+        # save the 'new' to_param so we don't get a warning about discarding the method
+        alias_method :reload_to_param, :to_param
+        alias_method :to_param, :original_to_param_reload
+      end
+  end  
+  
   def test_reload_works_without_prefix_options    
     person = Person.find(:first)
     assert_equal person, person.reload
@@ -595,6 +644,35 @@ class BaseTest < Test::Unit::TestCase
     assert !StreetAddress.new({:id => 2, :person_id => 1}).exists?
   end
   
+  def test_exists_with_redefined_to_param
+    Person.module_eval do
+      alias_method :original_to_param_exists, :to_param
+       def to_param  
+         name
+       end
+    end
+
+    # Class method.
+    assert Person.exists?('Greg')    
+
+    # Instance method.
+    assert Person.find('Greg').exists?    
+
+    # Nested class method.
+    assert StreetAddress.exists?(1,  :params => { :person_id => Person.find('Greg').to_param })    
+
+    # Nested instance method.
+    assert StreetAddress.find(1, :params => { :person_id => Person.find('Greg').to_param }).exists?
+
+    ensure
+      # revert back to original
+      Person.module_eval do
+        # save the 'new' to_param so we don't get a warning about discarding the method
+        alias_method :exists_to_param, :to_param
+        alias_method :to_param, :original_to_param_exists
+      end
+  end  
+  
   def test_to_xml
     matz = Person.find(1)
     xml = matz.to_xml