Introduce Money.from_amount for compatibility with RubyMoney #44
Conversation
…xtension offsite_payments
| it "raises ArgumentError with unsupported argument" do | ||
| expect { Money.from_amount(Object.new) }.to raise_error(ArgumentError) | ||
| end | ||
| end |
There was a problem hiding this comment.
rubymoney money tests (for reference)
lib/money/money.rb
Outdated
| @@ -6,6 +6,10 @@ class Money | |||
|
|
|||
| attr_reader :value, :cents | |||
|
|
|||
| def self.from_amount(value) | |||
| new(value) | |||
| end | |||
There was a problem hiding this comment.
what do we expect to happen when someone passes a currency to from_amount? Example: https://github.com/activemerchant/offsite_payments/blob/71f6cdcdfaeb7a35acd5e525d8f86565234778e8/lib/offsite_payments/integrations/easy_pay.rb#L104
There was a problem hiding this comment.
Good catch. That'll work for Money.new though because offiste_payment has this line just for us: https://github.com/activemerchant/offsite_payments/blob/e90298b1801552070ca9cee83f649363567cb8a9/lib/offsite_payments/notification.rb#L37
There was a problem hiding this comment.
Nit: Might as well
class << self
alias_method :new, :from_amount
end
|
Mind linking to the big write-up you guys did on what's the problem we're encountering and how it solves it? |
| @@ -485,4 +485,16 @@ | |||
| end | |||
|
|
|||
| end | |||
|
|
|||
| describe "from_amount quacks like RubyMoney" do | |||
|
Fixed all the things, ready for re-review! |
| alias_method :from_amount, :new | ||
| end | ||
|
|
||
| def initialize(value = 0, _currency = nil) |
lib/money/money.rb
Outdated
| @@ -266,8 +270,10 @@ def all_rational?(splits) | |||
| def value_to_decimal(num) | |||
| if num.respond_to?(:to_d) | |||
| num.is_a?(Rational) ? num.to_d(16) : num.to_d | |||
| else | |||
| elsif num.is_a?(Money) || num.is_a?(Numeric) || num.is_a?(String) | |||
There was a problem hiding this comment.
Where did the need for this come from?
There was a problem hiding this comment.
In order for the "raises ArgumentError with unsupported argument" spec from RubyMoney to pass we can't just call num#to_s and pass it to BigDecimal like we did. I couldn't think of a way to duck(type) myself out of this one 🤔
There was a problem hiding this comment.
When would we have num.is_a?(Numeric) || num.is_a?(String)? Both respond to the .to_d in theory, so would do the first if
There was a problem hiding this comment.
Derp, you're right!
…ney, fix `value_to_decimal` to match RubyMoney behaviour This is an interim solution to unbblock ourselves from upgrading offsite_payments. See https://github.com/Shopify/shopify/issues/95949 for a write-up.
…cation:0x000000029b82d0>` from `bundle exec rake` that is run by CI by upgrading RSpec and fixing some spec warnings along the way
Introduce Money.from_amount for compatibility with RubyMoney and by extension offsite_payments.
We use this gem in Shopify instead of
moneyon Rubygems. The API's were similar enough until activemerchant/offsite_payments#217 introduced a change which broke upgrading offsite_payments in Shopify. This PR aims to restore compatibility between ShopifyMoney and RubyMoney to unblock ourselves until a better solution is found.ShopifyMoney.new calls
#value_to_decimalto either cast a Numeric object to BigDecimal, or have BigDecimal parse the input value. RubyMoney does something similar for .from_amount.