Permalink
Browse files

better fix for #9994

fixes rails/rails#10304

Fix for #9994 is already applied but it is causing problems.

Previous fix took the approach of applying checking if the
`CollectionProxy` responds to a method or not. If it does
then invoke the method on `CollectionProxy` instance. This was
needed so that the association record could be created for
hm:t .

However this approach is causing problem. If someone defines a
method called `find_by_anything` on base class then this method
is invoked on `CollectionProxy` first. And that's wrong.

In this PR the approach is to send the method to base class and
if the association is of type hm:t and if the method is
`find_or_create` or `find_or_creat!` then invoke.
`save_through_record` on the `proxy_association`.
  • Loading branch information...
Neeraj Singh
Neeraj Singh committed Apr 27, 2013
1 parent 5ed930c commit 22f14f908572fa3c6da701dc8bc424a5838047b0
Showing with 27 additions and 6 deletions.
  1. +8 −6 lib/active_record/deprecated_finders/collection_proxy.rb
  2. +16 −0 test/associations_test.rb
  3. +3 −0 test/helper.rb
@@ -4,15 +4,17 @@ class CollectionProxy
module InterceptDynamicInstantiators
def method_missing(method, *args, &block)
match = DynamicMatchers::Method.match(klass, method)
- sanitized_method = match.class.prefix + match.class.suffix if match
- if match && self.respond_to?(sanitized_method) && proxy_association.reflection.options[:through].present?
- self.send(sanitized_method, Hash[match.attribute_names.zip(args)])
-
- elsif match && match.is_a?(DynamicMatchers::Instantiator)
+ if match && match.is_a?(DynamicMatchers::Instantiator)
scoping do
klass.send(method, *args) do |record|
- proxy_association.add_to_target(record)
+
+ sanitized_method = match.class.prefix + match.class.suffix
+ if %w(find_or_create_by find_or_create_by!).include?(sanitized_method) && proxy_association.reflection.options[:through].present?
+ proxy_association.send(:save_through_record, record)
+ else
+ proxy_association.add_to_target(record)
+ end
yield record if block_given?
end
end
View
@@ -8,6 +8,14 @@ def @klass.name; 'Post'; end
Appointment.delete_all
end
+ it 'should respect find_by_anything method defined on the base class' do
+ physician = Physician.create!
+
+ ActiveSupport::Deprecation.silence do
+ assert_equal [], physician.patients.find_by_custom_name
+ end
+ end
+
it 'find_or_create_by on has_many through should work' do
physician = Physician.create!
ActiveSupport::Deprecation.silence do
@@ -16,6 +24,14 @@ def @klass.name; 'Post'; end
assert_equal 1, Appointment.count
end
+ it 'find_or_create_by with bang on has_many through should work' do
+ physician = Physician.create!
+ ActiveSupport::Deprecation.silence do
+ physician.patients.find_or_create_by_name!('Tim')
+ end
+ assert_equal 1, Appointment.count
+ end
+
it 'find_or_create_by on has_many should work' do
physician = Physician.create!
ActiveSupport::Deprecation.silence do
View
@@ -50,6 +50,9 @@ class Appointment < ActiveRecord::Base
end
class Patient < ActiveRecord::Base
+ def self.find_by_custom_name
+ []
+ end
end
class Physician < ActiveRecord::Base

0 comments on commit 22f14f9

Please sign in to comment.