Add missing support for modulo operations on durations

Rails 5.1 introduce an `ActiveSupport::Duration::Scalar` class as
a wrapper around a numeric value as a way of ensuring a duration
was the outcome of an expression. However the implementation was
missing support for modulo operations. This commit adds support
for those operations and should result in a duration being
returned from expressions involving them.

Fixes #29603 and #29743.

activesupport/CHANGELOG.md

@@ -1,3 +1,25 @@
+*   Fix modulo operations involving durations
+
+    Rails 5.1 introduce an `ActiveSupport::Duration::Scalar` class as a wrapper
+    around a numeric value as a way of ensuring a duration was the outcome of
+    an expression. However the implementation was missing support for modulo
+    operations. This support has now been added and should result in a duration
+    being returned from expressions involving modulo operations.
+
+    Prior to Rails 5.1:
+
+        5.minutes % 2.minutes
+        => 60
+
+    Now:
+
+        5.minutes % 2.minutes
+        => 1 minute
+
+    Fixes #29603 and #29743.
+
+    *Sayan Chakraborty*, *Andrew White*
+
 *   Fix division where a duration is the denominator
 
     PR #29163 introduced a change in behavior when a duration was the denominator

activesupport/lib/active_support/duration.rb

@@ -80,6 +80,14 @@ def /(other)
         end
       end
 
+      def %(other)
+        if Duration === other
+          Duration.build(value % other.value)
+        else
+          calculate(:%, other)
+        end
+      end
+
       private
         def calculate(op, other)
           if Scalar === other
@@ -113,6 +121,8 @@ def raise_type_error(other)
       years:   SECONDS_PER_YEAR
     }.freeze
 
+    PARTS = [:years, :months, :weeks, :days, :hours, :minutes, :seconds].freeze
+
     attr_accessor :value, :parts
 
     autoload :ISO8601Parser,     "active_support/duration/iso8601_parser"
@@ -163,6 +173,30 @@ def years(value) #:nodoc:
         new(value * SECONDS_PER_YEAR, [[:years, value]])
       end
 
+      # Creates a new Duration from a seconds value that is converted
+      # to the individual parts:
+      #
+      #   ActiveSupport::Duration.build(31556952).parts # => {:years=>1}
+      #   ActiveSupport::Duration.build(2716146).parts  # => {:months=>1, :days=>1}
+      #
+      def build(value)
+        parts = {}
+        remainder = value.to_f
+
+        PARTS.each do |part|
+          unless part == :seconds
+            part_in_seconds = PARTS_IN_SECONDS[part]
+            parts[part] = remainder.div(part_in_seconds)
+            remainder = (remainder % part_in_seconds).round(9)
+          end
+        end
+
+        parts[:seconds] = remainder
+        parts.reject! { |k, v| v.zero? }
+
+        new(value, parts)
+      end
+
       private
 
         def calculate_total_seconds(parts)
@@ -240,6 +274,18 @@ def /(other)
       end
     end
 
+    # Returns the modulo of this Duration by another Duration or Numeric.
+    # Numeric values are treated as seconds.
+    def %(other)
+      if Duration === other || Scalar === other
+        Duration.build(value % other.value)
+      elsif Numeric === other
+        Duration.build(value % other)
+      else
+        raise_type_error(other)
+      end
+    end
+
     def -@ #:nodoc:
       Duration.new(-value, parts.map { |type, number| [type, -number] })
     end
@@ -324,7 +370,7 @@ def ago(time = ::Time.current)
     def inspect #:nodoc:
       parts.
         reduce(::Hash.new(0)) { |h, (l, r)| h[l] += r; h }.
-        sort_by { |unit,  _ | [:years, :months, :weeks, :days, :hours, :minutes, :seconds].index(unit) }.
+        sort_by { |unit,  _ | PARTS.index(unit) }.
         map     { |unit, val| "#{val} #{val == 1 ? unit.to_s.chop : unit.to_s}" }.
         to_sentence(locale: ::I18n.default_locale)
     end

activesupport/test/core_ext/duration_test.rb

@@ -109,7 +109,44 @@ def test_multiply
   def test_divide
     assert_equal 1.day, 7.days / 7
     assert_instance_of ActiveSupport::Duration, 7.days / 7
+
+    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
+
     assert_equal 1, 1.day / 1.day
+    assert_kind_of Integer, 1.day / 1.hour
+  end
+
+  def test_modulo
+    assert_equal 1.minute, 5.minutes % 120
+    assert_instance_of ActiveSupport::Duration, 5.minutes % 120
+
+    assert_equal 1.minute, 5.minutes % 2.minutes
+    assert_instance_of ActiveSupport::Duration, 5.minutes % 2.minutes
+
+    assert_equal 1.minute, 5.minutes % 120.seconds
+    assert_instance_of ActiveSupport::Duration, 5.minutes % 120.seconds
+
+    assert_equal 5.minutes, 5.minutes % 1.hour
+    assert_instance_of ActiveSupport::Duration, 5.minutes % 1.hour
+
+    assert_equal 1.day, 36.days % 604800
+    assert_instance_of ActiveSupport::Duration, 36.days % 604800
+
+    assert_equal 1.day, 36.days % 7.days
+    assert_instance_of ActiveSupport::Duration, 36.days % 7.days
+
+    assert_equal 800.seconds, 8000 % 1.hour
+    assert_instance_of ActiveSupport::Duration, 8000 % 1.hour
+
+    assert_equal 1.month, 13.months % 1.year
+    assert_instance_of ActiveSupport::Duration, 13.months % 1.year
   end
 
   def test_date_added_with_multiplied_duration
@@ -409,8 +446,6 @@ def test_scalar_divide
     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_kind_of Integer, scalar / 2.seconds
 
@@ -421,6 +456,31 @@ def test_scalar_divide
     assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
   end
 
+  def test_scalar_modulo
+    scalar = ActiveSupport::Duration::Scalar.new(10)
+
+    assert_equal 1, 31 % scalar
+    assert_instance_of ActiveSupport::Duration::Scalar, 31 % scalar
+    assert_equal 1, scalar % 3
+    assert_instance_of ActiveSupport::Duration::Scalar, scalar % 3
+    assert_equal 1, 31.seconds % scalar
+    assert_instance_of ActiveSupport::Duration, 31.seconds % scalar
+    assert_equal 1, scalar % 3.seconds
+    assert_instance_of ActiveSupport::Duration, scalar % 3.seconds
+
+    exception = assert_raises(TypeError) do
+      scalar % "foo"
+    end
+
+    assert_equal "no implicit conversion of String into ActiveSupport::Duration::Scalar", exception.message
+  end
+
+  def test_scalar_modulo_parts
+    scalar = ActiveSupport::Duration::Scalar.new(82800)
+    assert_equal({ hours: 1 }, (scalar % 2.hours).parts)
+    assert_equal(3600, (scalar % 2.hours).value)
+  end
+
   def test_twelve_months_equals_one_year
     assert_equal 12.months, 1.year
   end
@@ -429,17 +489,6 @@ 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|