Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug introduced by PR#167 not covering all available currencies #181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thai-truong
Copy link

@thai-truong thai-truong commented Jun 6, 2024

Currently, there is an existing bug in monetize v1.13.0 introduced by #167 that attempted to implement additional logic for currency parsing discussed in #153.

Example of the bug:

  • Expected:
"RM100".to_money    # #<Money fractional:10000 currency:MYR>
Monetize.parse("RM100")   # #<Money fractional:10000 currency:MYR>
Monetize.parse("100 RM")   # #<Money fractional:10000 currency:MYR>
  • Reality:
[1] pry(main)> Monetize.parse("RM100")
=> #<Money fractional:10000 currency:USD>

[8] pry(main)> "RM100".to_money
=> #<Money fractional:10000 currency:USD>

[14] pry(main)> Monetize.parse("100 RM")
=> #<Money fractional:10000 currency:USD>

This PR aims to:

@jaredbeck
Copy link

Currently, there is an existing bug in monetize v1.13.0 ..

Does this bug only affect the Malaysian ringgit (MYR) or are other currencies affected? Thanks.

@thai-truong
Copy link
Author

thai-truong commented Jul 30, 2024

Currently, there is an existing bug in monetize v1.13.0 ..

Does this bug only affect the Malaysian ringgit (MYR) or are other currencies affected? Thanks.

This affects any currency that is not in Monetize::Parser::CURRENCY_SYMBOLS.values, according to this line in the code.

I've personally run into the aforementioned issue with Malaysian Ringgit (as "RM", not "MYR") and Australian Dollar (AUD) since neither of them are in Monetize::Parser::CURRENCY_SYMBOLS.values.

Screenshot from 2024-07-30 13-49-35

Monetize::Parser::CURRENCY_SYMBOLS's main purpose seems to be to map non-ISO currency symbols to their respective ISO-4217 currency code. That's why I think it is unsuitable to be used in the manner implemented in #167

What do you think? And thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants