Merge pull request #29971 from rails/fix-duration-division

Fix division where a duration is the denominator

activesupport/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   Fix division where a duration is the denominator
+
+    PR #29163 introduced a change in behavior when a duration was the denominator
+    in a calculation - this was incorrect as dividing by a duration should always
+    return a `Numeric`. The behavior of previous versions of Rails has been restored.
+
+    Fixes #29592.
+
+    *Andrew White*
+
+
 ## Rails 5.1.3.rc2 (July 25, 2017) ##
 
 *   No changes.

activesupport/lib/active_support/duration.rb

@@ -74,10 +74,7 @@ def *(other)
 
       def /(other)
         if Duration === other
-          new_parts = other.parts.map { |part, other_value| [part, value / other_value] }.to_h
-          new_value = new_parts.inject(0) { |total, (part, value)| total + value * Duration::PARTS_IN_SECONDS[part] }
-
-          Duration.new(new_value, new_parts)
+          value / other.value
         else
           calculate(:/, other)
         end
@@ -232,8 +229,10 @@ def *(other)
 
     # Divides this Duration by a Numeric and returns a new Duration.
     def /(other)
-      if Scalar === other || Duration === other
+      if Scalar === other
         Duration.new(value / other.value, parts.map { |type, number| [type, number / other.value] })
+      elsif Duration === other
+        value / other.value
       elsif Numeric === other
         Duration.new(value / other, parts.map { |type, number| [type, number / other] })
       else

activesupport/test/core_ext/duration_test.rb

@@ -408,9 +408,11 @@ def test_scalar_divide
     assert_equal 5, scalar / 2
     assert_instance_of ActiveSupport::Duration::Scalar, scalar / 2
     assert_equal 10, 100.seconds / scalar
+    assert_instance_of ActiveSupport::Duration, 100.seconds / scalar
+    assert_equal 20, 2.seconds * scalar
     assert_instance_of ActiveSupport::Duration, 2.seconds * scalar
     assert_equal 5, scalar / 2.seconds
-    assert_instance_of ActiveSupport::Duration, scalar / 2.seconds
+    assert_kind_of Integer, scalar / 2.seconds
 
     exception = assert_raises(TypeError) do
       scalar / "foo"
@@ -419,15 +421,6 @@ def test_scalar_divide
     assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
   end
 
-  def test_scalar_divide_parts
-    scalar = ActiveSupport::Duration::Scalar.new(10)
-
-    assert_equal({ days: 2 }, (scalar / 5.days).parts)
-    assert_equal(172800, (scalar / 5.days).value)
-    assert_equal({ days: -2 }, (scalar / -5.days).parts)
-    assert_equal(-172800, (scalar / -5.days).value)
-  end
-
   def test_twelve_months_equals_one_year
     assert_equal 12.months, 1.year
   end
@@ -436,6 +429,17 @@ def test_thirty_days_does_not_equal_one_month
     assert_not_equal 30.days, 1.month
   end
 
+  def test_division
+    assert_equal 1.hour, 1.day / 24
+    assert_instance_of ActiveSupport::Duration, 1.day / 24
+
+    assert_equal 24, 86400 / 1.hour
+    assert_kind_of Integer, 86400 / 1.hour
+
+    assert_equal 24, 1.day / 1.hour
+    assert_kind_of Integer, 1.day / 1.hour
+  end
+
   def test_adding_one_month_maintains_day_of_month
     (1..11).each do |month|
       [1, 14, 28].each do |day|