Parse currency correctly if found from existing hash

[kahshing96]

• Fix parsing issue for certain currency symbols like Malaysia Ringgit (RM) as addressed in Version 1.11.0 broke parsing for Malaysian ringgit when using the symbol 'RM' #153 by checking that
  • the computed_currency parsed from the given input from a match ISO currency code regex is a correct ISO code from existing Monetize::Parser::CURRENCY_SYMBOLS, then only assigned it with value
  • else fallback to use compute_currency which match against currency symbol regex
  • eg:

  "RM100".to_money    # #<Money fractional:10000 currency:MYR>
  "S$100".to_money    # #<Money fractional:10000 currency:SGD>
  "NT$100".to_money   # #<Money fractional:10000 currency:TWD>
  "HK$100".to_money   # #<Money fractional:10000 currency:HKD>
  "$100 CAD".to_money # #<Money fractional:10000 currency:CAD>

• Add & fix related rspecs

[kahshing96]

Hi @semmons99 , would you able to take a look at this PR?

[semmons99]

ty. merged!

[kahshing96]

Hi @semmons99 thanks for merging, is it possible to bump the version for this gem in both rubygems and create a new version tag in Github ?

[semmons99]

done. release 1.13.0 @kahshing96

[Calyhre]

@kahshing96 @semmons99 That's a massive breaking change considering the limited set of CURRENCY_SYMBOLS.

# monetize 0.12.0
'100 NOK'.to_money => #<Money fractional:10000 currency:NOK>

# monetize 0.13.0 (with EUR as default)
'100 NOK'.to_money => #<Money fractional:10000 currency:EUR>

[tagliala]

Just noticed this change when writing 1.13.0 changelog

Yes, this is a breaking change and should not belong to 1.x, however, I'm wondering why this is not using ISO codes from Money (Money::Currency.all.map(&:iso_code))

[weizheheng]

Yes, this is breaking our production because Monetize::Parser::CURRENCY_SYMBOLS do not have the full list of currency. Now when we try to do:

fee = "AUD 10.00"

# Our default currency is "USD"
Monetize.parse(fee)
# => #<Money fractional:1000 currency:USD>

# The result is in USD instead of AUD

[dramalho]

(time passed, a legacy project upgraded the gem, weirdness ensued 😄 )

ok, this line is kinda weird as it's nullifying the computed_currency based on a very tiny list of known currencies and not, say, the entire Money::Currency list

for example, we had a bunch of tests that specifically tested Australian Dollars parsing, as in

Monetize.parse('100 AUD')

AUD is known to Money, so that should be fine

 Money::Currency.new('AUD')
=> #<Money::Currency id: aud, priority: 4, symbol_first: true, thousands_separator: ,, html_entity: $, decimal_mark: ., name: Australian Dollar, symbol: $, subunit_to_unit: 100, exponent: 2, iso_code: AUD, iso_numeric: 036, subunit: Cent, smallest_denomination: 5, format: >

but because of this line and because it's not on the subset of currencies Monetize knows about, it's now defaulting to USD and I'm kinda struggling with a reason why this would be good, especially when the result is used in Money::Currency.wrap(parse_currency)

What's worse - in my case - is that there's no way around it, if there's a chance that we'll use a valid currency that unfortunately is not on the subset of Monetize known currencies, it'll end up being nillified

is there any context to this?

[semmons99]

I think it is likely simply an oversight. Because there is a smaller subset of known symbols than known currencies, it is unfortunately gating them down too far. I think the better solution would be to try and match on symbol from this hash and then use the much larger known currencies iso codes to figure out the rest. we would be happy to accept and merge a PR that did it that way.

[dramalho]

Yeah, I understand the need to have a list for "our pick of currency for a given symbol" -- there are many currencies with dollar signs in them, so that one I can follow, the other way around I couldn't understand , but your explanation makes sense :)

I might take a stab at a PR for this, keeping the lookup for symbols as is but relying on Money::Currency to figure out if the named currency is known (and nullifying if not, which I think was the real intention here)