Merge pull request #31483 from yhirano55/fix_validation_callbacks_on_multiple_context_in_active_model

Bugfix: validation callbacks on multiple context

3 files changed, 60 insertions, 9 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index 2dfde11707..b67a803b9d 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Fix to working before/after validation callbacks on multiple contexts.
+
+    *Yoshiyuki Hirano*
+
 ## Rails 5.2.0.beta2 (November 28, 2017) ##
 
 *   No changes.
diff --git a/activemodel/lib/active_model/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb
index 4d0ab2a2fe..11a8b2b229 100644
--- a/activemodel/lib/active_model/validations/callbacks.rb
+++ b/activemodel/lib/active_model/validations/callbacks.rb
@@ -54,14 +54,16 @@ module ActiveModel
         #   person.valid? # => true
         #   person.name   # => "bob"
         def before_validation(*args, &block)
-          options = args.last
-          if options.is_a?(Hash) && options[:on]
-            options[:if] = Array(options[:if])
-            options[:on] = Array(options[:on])
+          options = args.extract_options!
+          options[:if] = Array(options[:if])
+
+          if options.key?(:on)
             options[:if].unshift ->(o) {
-              options[:on].include? o.validation_context
+              !(Array(options[:on]) & Array(o.validation_context)).empty?
             }
           end
+
+          args << options
           set_callback(:validation, :before, *args, &block)
         end
 
@@ -95,13 +97,15 @@ module ActiveModel
           options = args.extract_options!
           options[:prepend] = true
           options[:if] = Array(options[:if])
-          if options[:on]
-            options[:on] = Array(options[:on])
+
+          if options.key?(:on)
             options[:if].unshift ->(o) {
-              options[:on].include? o.validation_context
+              !(Array(options[:on]) & Array(o.validation_context)).empty?
             }
           end
-          set_callback(:validation, :after, *(args << options), &block)
+
+          args << options
+          set_callback(:validation, :after, *args, &block)
         end
       end
 
diff --git a/activemodel/test/cases/validations/callbacks_test.rb b/activemodel/test/cases/validations/callbacks_test.rb
index d3a9a17a05..ff3cf61746 100644
--- a/activemodel/test/cases/validations/callbacks_test.rb
+++ b/activemodel/test/cases/validations/callbacks_test.rb
@@ -59,6 +59,18 @@ class DogValidatorWithOnCondition < Dog
   def set_after_validation_marker;  history << "after_validation_marker" ; end
 end
 
+class DogValidatorWithOnMultipleCondition < Dog
+  before_validation :set_before_validation_marker_on_context_a, on: :context_a
+  before_validation :set_before_validation_marker_on_context_b, on: :context_b
+  after_validation :set_after_validation_marker_on_context_a, on: :context_a
+  after_validation :set_after_validation_marker_on_context_b, on: :context_b
+
+  def set_before_validation_marker_on_context_a; history << "before_validation_marker on context_a"; end
+  def set_before_validation_marker_on_context_b; history << "before_validation_marker on context_b"; end
+  def set_after_validation_marker_on_context_a;  history << "after_validation_marker on context_a" ; end
+  def set_after_validation_marker_on_context_b;  history << "after_validation_marker on context_b" ; end
+end
+
 class DogValidatorWithIfCondition < Dog
   before_validation :set_before_validation_marker1, if: -> { true }
   before_validation :set_before_validation_marker2, if: -> { false }
@@ -98,6 +110,37 @@ class CallbacksWithMethodNamesShouldBeCalled < ActiveModel::TestCase
     assert_equal [], d.history
   end
 
+  def test_on_multiple_condition_is_respected_for_validation_with_matching_context
+    d = DogValidatorWithOnMultipleCondition.new
+    d.valid?(:context_a)
+    assert_equal ["before_validation_marker on context_a", "after_validation_marker on context_a"], d.history
+
+    d = DogValidatorWithOnMultipleCondition.new
+    d.valid?(:context_b)
+    assert_equal ["before_validation_marker on context_b", "after_validation_marker on context_b"], d.history
+
+    d = DogValidatorWithOnMultipleCondition.new
+    d.valid?([:context_a, :context_b])
+    assert_equal([
+      "before_validation_marker on context_a",
+      "before_validation_marker on context_b",
+      "after_validation_marker on context_a",
+      "after_validation_marker on context_b"
+    ], d.history)
+  end
+
+  def test_on_multiple_condition_is_respected_for_validation_without_matching_context
+    d = DogValidatorWithOnMultipleCondition.new
+    d.valid?(:save)
+    assert_equal [], d.history
+  end
+
+  def test_on_multiple_condition_is_respected_for_validation_without_context
+    d = DogValidatorWithOnMultipleCondition.new
+    d.valid?
+    assert_equal [], d.history
+  end
+
   def test_before_validation_and_after_validation_callbacks_should_be_called
     d = DogWithMethodCallbacks.new
     d.valid?