Unexpected behaviour change between 1.11.0 and 1.12.0

[wjessop]

I'm seeing an unexpected behaviour change between money-rails 1.11.0 and 1.12.0 (the behaviour introduced in 1.12.0 is present in the latest version, but I narrowed it down to this version jump). I'm not sure if this issue is in money-rails, money or monetize so opening an issue here.

These two Rails console sessions sum up the issue:

v1.11.0

Loading development environment (Rails 5.1.7)
[1] pry(main)> BigDecimal('0.2525e1').to_money('EUR')
=> #<Money fractional:253 currency:EUR>
[2] pry(main)> MoneyRails::VERSION
=> "1.11.0"

v1.12.0

Loading development environment (Rails 5.1.7)
[1] pry(main)> BigDecimal('0.2525e1').to_money('EUR')
=> #<Money fractional:252 currency:EUR>
[2] pry(main)> MoneyRails::VERSION
=> "1.12.0"

Basically, the to_money method does additional rounding in v.1.12.0, which unfortunately throws calculations out in our app. This is the config we're currently using:

MoneyRails.configure do |config|
  config.default_currency = :eur
  config.rounding_mode = BigDecimal::ROUND_HALF_EVEN
  config.default_format = {
    format: "%n %u"
  }
end

[wjessop]

Further investigation, if I monkey patch the Monetize module with these two methods taken from 1.7.0 the previous behaviour is restored, and I can upgrade money-rails 1.13.3.

module Monetize
  def self.from_numeric(value, currency = Money.default_currency)
    case value
    when Integer
      from_fixnum(value, currency)
    when Numeric
      value = BigDecimal.new(value.to_s)
      from_bigdecimal(value, currency)
    else
      fail ArgumentError, "'value' should be a type of Numeric"
    end
  end

  def self.from_bigdecimal(value, currency = Money.default_currency)
    currency = Money::Currency.wrap(currency)
    value *= currency.subunit_to_unit
    value = value.round unless Money.infinite_precision
    Money.new(value, currency)
  end
end

[antstorm]

@wjessop while this might be a change in behaviour, the value should actually be 252:

>> BigDecimal('0.2525e1').round(2, BigDecimal::ROUND_HALF_EVEN)
=> 0.252e1

The reason your patch worked is due to value.round, which uses the default rounding mode, which I believe is BigDecimal::ROUND_HALF_UP:

>> BigDecimal('0.2525e1').round(2, BigDecimal::ROUND_HALF_UP)
=> 0.253e1
>> BigDecimal('0.2525e1').round(2)
=> 0.253e1

It was probably a bug fix that resulted in correct behaviour in 1.12.0. Try setting your rounding mode to BigDecimal::ROUND_HALF_UP and I think you'll get your expected behaviour back without the monkey patch

[wjessop]

@antstorm I don't think we can realistically ask that the behaviour is changed back because a) the new behaviour is the right behaviour and b) even if it wasn't it's been around long enough that the behaviour is "baked in" and people will have written their applications expecting the new behaviour.

What I would say is that this change definitely deserved a changelog entry and a major version increment as it changed the behaviour of an existing method (and I'd consider putting this in the changelog retrospectively, it would have saved me hours of work).

From the deprecation notice it seems that the default is ROUND_HALF_EVEN:

[WARNING] The default rounding mode will change from `ROUND_HALF_EVEN` to
  `ROUND_HALF_UP` in the next major release. Set it explicitly using
  `Money.rounding_mode=` to avoid potential problems.

We can't change the default to BigDecimal::ROUND_HALF_UP unfortunately, our code (and many thousands of payments) currently relies on the weird split in behaviour (ROUND_HALF_UP for raw Money objects, ROUND_HALF_EVEN when ActiveRecord objects are saved), there isn't a single Bigdecimal rounding method we can use that doesn't cause some expectation to break in our application (I've tried them all).

I guess we will have to roll with the monkey patch until we have worked out a way to transition to the new behaviour.

[semmons99]

sorry. it is always difficult to decide on versioning when a bug is squashed but people used the bug without notice. closing for now. would happily accept a changelog or similar update if people feel it is warranted.

[wjessop]

Thanks for the reply @semmons99. I definitely thing it's worthy of a changelog entry, it's a breaking change introduced in a minor version.