Skip to content

Change HUF denomination#742

Closed
roshats wants to merge 1 commit intoRubyMoney:masterfrom
SoftSwiss:huf-denomination-patch
Closed

Change HUF denomination#742
roshats wants to merge 1 commit intoRubyMoney:masterfrom
SoftSwiss:huf-denomination-patch

Conversation

@roshats
Copy link

@roshats roshats commented Jan 15, 2018

subunit_to_unit for HUF was changed from 100 to 1 in #679. According to ISO (https://www.currency-iso.org/dam/downloads/lists/list_one.xml) and Wikipedia subunit_to_unit for HUF is 100. It is important to have HUF configured according to ISO because usually cents is used in communication between services. Also, https://github.com/RubyMoney/money-rails gem recommends to store amount in cents.

@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage increased (+0.04%) to 99.315% when pulling 74af9da on SoftSwiss:huf-denomination-patch into 2626d76 on RubyMoney:master.

In RubyMoney#679 `subunit_to_unit` for HUF was change from 100 to 1. According to [ISO](https://www.iso.org/iso-4217-currency-codes.html) (https://www.currency-iso.org/dam/downloads/lists/list_one.xml) and [wiki](https://en.wikipedia.org/wiki/Hungarian_forint) `subunit_to_unit` for HUF is 100. It is important to have HUF configured according to ISO because usually cents is used in communication between services. Also, https://github.com/RubyMoney/money-rails gem recommends to store amount in cents.
@roshats roshats force-pushed the huf-denomination-patch branch from 74af9da to 0dcb55e Compare January 15, 2018 13:58
@coveralls
Copy link

coveralls commented Jan 15, 2018

Coverage Status

Coverage increased (+0.04%) to 99.315% when pulling 0dcb55e on SoftSwiss:huf-denomination-patch into 2626d76 on RubyMoney:master.

@antstorm
Copy link
Contributor

@Azdaroth can you please comment on this one? Since I haven't used HUF in any of the projects I worked on, I'm intended to go with the most popular approach.

@ct-clearhaus
Copy link
Contributor

I think it is indicated that ISO 4217 is followed, e.g. https://github.com/RubyMoney/money#currency-exponent.

It doesn't say Money#iso_subunit_to_unit though, but I think it would be "least surprise" that ISO 4217 is followed also wrt. the exponent/subunit_to_unit.

Exponent changes can be quite fundamental when you are handling money 🙂

@Azdaroth
Copy link
Contributor

As far as I remember, filler (a subunit) has been deprecated long time ago and is no longer used anywhere, so not sure if this is a valid approach to roll back that change.

Might be a good idea to ask someone from Hungary about it, @kaizencodes, what's your take on that?

@kaizencodes
Copy link

There is no subunit for HUF anymore. so the correct value is 1 also the smallest unit is 5 everything gets rounded to it.

@ct-clearhaus
Copy link
Contributor

The central bank of Hungary seems to state FX rates using exponent 2: https://www.mnb.hu/en/arfolyamok

Although never used, the exponent may still officially match that of ISO 4217 which states the exponent to be 2 (https://www.iso.org/iso-4217-currency-codes.html).

As of #679 we've "manually" overridden HUF to have exponent 2 because that's what Visa and Mastercard thinks.

@Azdaroth
Copy link
Contributor

Hmm, interesting fact that this is what Visa and MasterCard. Honestly, I don't know what the the "right" decision should be here. Bringing back subunits sounds like something that doesn't really model the real world, but apparently there are some legacy issues that might be worth to consider.

@ct-clearhaus
Copy link
Contributor

Speaking about legacy, Visa and Mastercard can be decades behind ISO 4217; for instance wrt. CLP:

I am missing the "tz database of currencies" ...

@ideasasylum
Copy link

Possibly related annoyance: I'm adding HUF support to a checkout using money v6.10.1 and Stripe. I tried to make a 4000 Ft charge through Stripe and received the following error:

Amount must convert to at least 50 cents. 40.00 Ft converts to approximately €0.12.

Stripe is expecting an exponent of 2 for HUF (i.e, 400000 not 4000).

I can't say which is technically/legally correct or whether money or Stripe is wrong here. But I definitely think this can cause massive problems integrating with other systems. If I'd tested the charge with a higher amount that was above Stripe's minimum amount, I probably wouldn't have noticed it was charging them 1/100th of the intended price!

@antstorm
Copy link
Contributor

I like the idea of following the path of least surprise and making money compatible with VISA and MasterCard. But this raises a question of whether that configuration makes sense when displaying the values to users?

Maybe a solution here is allowing the override of subunit_to_unit when formatting?

@JanStevens
Copy link

We also got hit by this issue, but we would rather have the money gem follow the ISO standard and not some odd banking quirk.

For example Stripe does follow the ISO standard and HUF should have subunit to unit of 100.

I think everyones life is easier if the money gem follows the standard (so the display to end user is always correct) and developers are responsible for implementing various quirks depending on the external source they need to connect with.

Can we get this merged and released? Another option would be to have some sort of switch in place that loads in defaults for Stripe / Visa / Mastercard / ISO standard and then everyone can pick what they want to work with

@pavlad
Copy link

pavlad commented Jul 17, 2019

Instead of waiting for the outcome of this decision or forking I would recommend those who use MoneyRails to override the default HUF currency in an initializer:

# config/initializers/money.rb

MoneyRails.configure do |config|
  # Overwrite HUF to match ISO 4217
  # Money gem currently deviates from the ISO standards (by default subunit_to_unit is 1)
  # https://en.wikipedia.org/wiki/ISO_4217
  #
  config.register_currency = {
    'priority': 100,
    'iso_code': 'HUF',
    'name': 'Hungarian Forint',
    'symbol': 'Ft',
    'alternate_symbols': [],
    'subunit': '',
    'subunit_to_unit': 100,
    'symbol_first': false,
    'html_entity': '',
    'decimal_mark': ',',
    'thousands_separator': ' ',
    'iso_numeric': '348',
    'smallest_denomination': 5
  }
end

@antstorm
Copy link
Contributor

@JanStevens as I mentioned it makes sense to follow the ISO standard. I need to figure out how to safely roll this out as it might negatively impact some users.

Also, when displaying this to the customers would you just / 100?

@JanStevens
Copy link

@antstorm I would suggest major version and let's go through the list again and ensure all currencies follow the ISO standard.

I would display the currency following the standard and not go dived it myself

@antstorm
Copy link
Contributor

@JanStevens We still need to figure out a good migration path as hiding this behind a major version might mean that some apps will get stuck behind forever. I'm thinking of making the list immutable and introducing changes by adding "diffs", then you set the version you're on and we'll tell you exactly what have changed allowing you to migrate or ignore.

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.

10 participants