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.
rails · Jun 28, 2017 · a54e13bd2e8fb4d6aa0aebe59271699a2d62567b · a54e13b
1 parent c6a28b2
commit a54e13bd2e8fb4d6aa0aebe59271699a2d62567b
Unified Split

Showing with 131 additions and 14 deletions.

+22 −0 activesupport/CHANGELOG.md

+47 −1 activesupport/lib/active_support/duration.rb

+62 −13 activesupport/test/core_ext/duration_test.rb
diff --git a/activesupport/CHANGELOG.md b/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
diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb
@@ -82,6 +82,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
@@ -115,6 +123,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"
@@ -165,6 +175,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)
@@ -242,6 +276,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
@@ -326,7 +372,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
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
@@ -111,7 +111,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
@@ -411,8 +448,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

@@ -423,6 +458,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
@@ -431,17 +491,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|