Ensure nested with_scope merges conditions inside out [#2193 state:resolved]

Signed-off-by: NPratik Naik <pratiknaik@gmail.com>

activerecord/lib/active_record/base.rb

@@ -416,7 +416,7 @@ def self.reset_subclasses #:nodoc:
     end
 
     @@subclasses = {}
-    
+
     ##
     # :singleton-method:
     # Contains the database configuration - as is typically stored in config/database.yml -
@@ -661,7 +661,6 @@ def find_by_sql(sql)
         connection.select_all(sanitize_sql(sql), "#{name} Load").collect! { |record| instantiate(record) }
       end
 
-
       # Returns true if a record exists in the table that matches the +id+ or
       # conditions given, or false otherwise. The argument can take five forms:
       #
@@ -1003,7 +1002,6 @@ def decrement_counter(counter_name, id)
         update_counters(id, counter_name => -1)
       end
 
-
       # Attributes named in this macro are protected from mass-assignment,
       # such as <tt>new(attributes)</tt>,
       # <tt>update_attributes(attributes)</tt>, or
@@ -1104,7 +1102,6 @@ def serialized_attributes
         read_inheritable_attribute(:attr_serialized) or write_inheritable_attribute(:attr_serialized, {})
       end
 
-
       # Guesses the table name (in forced lower-case) based on the name of the class in the inheritance hierarchy descending
       # directly from ActiveRecord::Base. So if the hierarchy looks like: Reply < Message < ActiveRecord::Base, then Message is used
       # to guess the table name even when called on Reply. The rules used to do the guess are handled by the Inflector class
@@ -1417,7 +1414,6 @@ def inspect
         end
       end
 
-
       def quote_value(value, column = nil) #:nodoc:
         connection.quote(value,column)
       end
@@ -1486,7 +1482,7 @@ def respond_to?(method_id, include_private = false)
         elsif match = DynamicScopeMatch.match(method_id)
           return true if all_attributes_exists?(match.attribute_names)
         end
-        
+
         super
       end
 
@@ -2014,7 +2010,6 @@ def expand_id_conditions(id_or_conditions)
           end
         end
 
-
         # Defines an "attribute" method (like +inheritance_column+ or
         # +table_name+). A new (class) method will be created with the
         # given name. If a value is specified, the new method will
@@ -2111,7 +2106,7 @@ def with_scope(method_scoping = {}, action = :merge, &block)
           end
 
           # Merge scopings
-          if action == :merge && current_scoped_methods
+          if [:merge, :reverse_merge].include?(action) && current_scoped_methods
             method_scoping = current_scoped_methods.inject(method_scoping) do |hash, (method, params)|
               case hash[method]
                 when Hash
@@ -2133,7 +2128,11 @@ def with_scope(method_scoping = {}, action = :merge, &block)
                       end
                     end
                   else
-                    hash[method] = hash[method].merge(params)
+                    if action == :reverse_merge
+                      hash[method] = hash[method].merge(params)
+                    else
+                      hash[method] = params.merge(hash[method])
+                    end
                   end
                 else
                   hash[method] = params
@@ -2143,7 +2142,6 @@ def with_scope(method_scoping = {}, action = :merge, &block)
           end
 
           self.scoped_methods << method_scoping
-
           begin
             yield
           ensure
@@ -2749,7 +2747,6 @@ def attributes=(new_attributes, guard_protected_attributes = true)
         assign_multiparameter_attributes(multi_parameter_attributes)
       end
 
-
       # Returns a hash of all the attributes with their names as keys and the values of the attributes as values.
       def attributes
         self.attribute_names.inject({}) do |attrs, name|

activerecord/lib/active_record/named_scope.rb

@@ -104,7 +104,7 @@ def named_scope(name, options = {}, &block)
         end
       end
     end
-    
+
     class Scope
       attr_reader :proxy_scope, :proxy_options, :current_scoped_methods_when_defined
       NON_DELEGATE_METHODS = %w(nil? send object_id class extend find size count sum average maximum minimum paginate first last empty? any? respond_to?).to_set
@@ -175,7 +175,7 @@ def method_missing(method, *args, &block)
         if scopes.include?(method)
           scopes[method].call(self, *args)
         else
-          with_scope :find => proxy_options, :create => proxy_options[:conditions].is_a?(Hash) ?  proxy_options[:conditions] : {} do
+          with_scope({:find => proxy_options, :create => proxy_options[:conditions].is_a?(Hash) ?  proxy_options[:conditions] : {}}, :reverse_merge) do
             method = :new if method == :build
             if current_scoped_methods_when_defined
               with_scope current_scoped_methods_when_defined do

activerecord/test/cases/method_scoping_test.rb

@@ -262,6 +262,15 @@ def test_merge_options
     end
   end
 
+  def test_merge_inner_scope_has_priority
+    Developer.with_scope(:find => { :limit => 5 }) do
+      Developer.with_scope(:find => { :limit => 10 }) do
+        merged_option = Developer.instance_eval('current_scoped_methods')[:find]
+        assert_equal({ :limit => 10 }, merged_option)
+      end
+    end
+  end
+
   def test_replace_options
     Developer.with_scope(:find => { :conditions => "name = 'David'" }) do
       Developer.with_exclusive_scope(:find => { :conditions => "name = 'Jamis'" }) do
@@ -400,6 +409,29 @@ def test_merged_scoped_find_combines_and_sanitizes_conditions
     end
   end
 
+  def test_nested_scoped_create
+    comment = nil
+    Comment.with_scope(:create => { :post_id => 1}) do
+      Comment.with_scope(:create => { :post_id => 2}) do
+        assert_equal({ :post_id => 2 }, Comment.send(:current_scoped_methods)[:create])
+        comment = Comment.create :body => "Hey guys, nested scopes are broken. Please fix!"
+      end
+    end
+    assert_equal 2, comment.post_id
+  end
+
+  def test_nested_exclusive_scope_for_create
+    comment = nil
+    Comment.with_scope(:create => { :body => "Hey guys, nested scopes are broken. Please fix!" }) do
+      Comment.with_exclusive_scope(:create => { :post_id => 1 }) do
+        assert_equal({ :post_id => 1 }, Comment.send(:current_scoped_methods)[:create])
+        comment = Comment.create :body => "Hey guys"
+      end
+    end
+    assert_equal 1, comment.post_id
+    assert_equal 'Hey guys', comment.body
+  end
+
   def test_merged_scoped_find_on_blank_conditions
     [nil, " ", [], {}].each do |blank|
       Developer.with_scope(:find => {:conditions => blank}) do
@@ -523,7 +555,6 @@ def test_nested_scope
   end
 end
 
-
 class HasAndBelongsToManyScopingTest< ActiveRecord::TestCase
   fixtures :posts, :categories, :categories_posts
 
@@ -549,7 +580,6 @@ def test_nested_scope
   end
 end
 
-
 class DefaultScopingTest < ActiveRecord::TestCase
   fixtures :developers
 
@@ -577,7 +607,7 @@ def test_default_scoping_with_inheritance
     # Scopes added on children should append to parent scope
     expected_klass_scope = [{ :create => {}, :find => { :order => 'salary DESC' }}, { :create => {}, :find => {} }]
     assert_equal expected_klass_scope, klass.send(:scoped_methods)
-    
+
     # Parent should still have the original scope
     assert_equal scope, DeveloperOrderedBySalary.send(:scoped_methods)
   end
@@ -620,7 +650,6 @@ def test_overwriting_default_scope
 =begin
 # We disabled the scoping for has_one and belongs_to as we can't think of a proper use case
 
-
 class BelongsToScopingTest< ActiveRecord::TestCase
   fixtures :comments, :posts
 
@@ -640,7 +669,6 @@ def test_forwarding_to_dynamic_finders
 
 end
 
-
 class HasOneScopingTest< ActiveRecord::TestCase
   fixtures :comments, :posts
 

activerecord/test/cases/named_scope_test.rb

@@ -247,7 +247,7 @@ def test_should_create_with_bang_with_proxy_options
     topic = Topic.approved.create!({})
     assert topic.approved
   end
-  
+
   def test_should_build_with_proxy_options_chained
     topic = Topic.approved.by_lifo.build({})
     assert topic.approved
@@ -287,15 +287,21 @@ def test_chaining_with_duplicate_joins
     assert_equal post.comments.size, Post.scoped(:joins => join).scoped(:joins => join, :conditions => "posts.id = #{post.id}").size
   end
 
-  def test_chanining_should_use_latest_conditions_when_creating
-    post1 = Topic.rejected.approved.new
-    assert post1.approved?
+  def test_chaining_should_use_latest_conditions_when_creating
+    post = Topic.rejected.new
+    assert !post.approved?
+
+    post = Topic.rejected.approved.new
+    assert post.approved?
 
-    post2 = Topic.approved.rejected.new
-    assert ! post2.approved?
+    post = Topic.approved.rejected.new
+    assert !post.approved?
+
+    post = Topic.approved.rejected.approved.new
+    assert post.approved?
   end
 
-  def test_chanining_should_use_latest_conditions_when_searching
+  def test_chaining_should_use_latest_conditions_when_searching
     # Normal hash conditions
     assert_equal Topic.all(:conditions => {:approved => true}), Topic.rejected.approved.all
     assert_equal Topic.all(:conditions => {:approved => false}), Topic.approved.rejected.all
@@ -306,7 +312,7 @@ def test_chanining_should_use_latest_conditions_when_searching
     # Nested hash conditions with different keys
     assert_equal [posts(:sti_comments)], Post.with_special_comments.with_post(4).all.uniq
   end
-  
+
   def test_methods_invoked_within_scopes_should_respect_scope
     assert_equal [], Topic.approved.by_rejected_ids.proxy_options[:conditions][:id]
   end