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

Wrong charges on Stripe when using three digits subunit. #3725

Open
coorasse opened this issue Jul 31, 2020 · 11 comments
Open

Wrong charges on Stripe when using three digits subunit. #3725

coorasse opened this issue Jul 31, 2020 · 11 comments
Labels
type:bug Error, flaw or fault

Comments

@coorasse
Copy link
Contributor

coorasse commented Jul 31, 2020

In our system the prices are defined with three digits subunit. and the charges on Stripe are 10 times higher.

Solidus Version:
2.10.0

To Reproduce
Our Money configuration has the following:

Spree.config do |config|
  config.currency = 'CHF'
end

Money::Currency.table[:chf].merge!(subunit_to_unit: 1000, smallest_denomination: 50)

to override the default behaviour.

Given an amount of 60.20 CHF, when a Payment is captured or authorized (see

amount ||= money.money.cents
and
response = payment_method.send(action, money.money.cents,
) the method cents is called which returns 60200 instead of the expected 6020, necessary for the Stripe charge.

Additional context

Our current patch consists of:

  • defining a new method in Spree::Money
def two_digits_cents
  ((cents / currency.subunit_to_unit.to_f) * 100).to_i
end
  • overriding the methods listed above to use money.two_digits_cents instead of money.money.cents.

I'd love to open a PR with a fix, but I am not sure if this approach is correct. If you confirm me that it is, I'll proceed with a PR.

@aldesantis
Copy link
Member

aldesantis commented Jul 31, 2020

@coorasse I'm not very familiar with this aspect of Solidus so I may be wrong, but I suspect the problem stems from the subunit_to_unit configuration, which represents the proportion between the subunit (cents) and the unit (franc). When you set it to 1000, you tell Money that a franc is made of 1000 cents, which means 60.20 CHF will be (incorrectly) multiplied by 1000 and converted into 60200 cents.

Can you clarify why you set that value to 1000? Perhaps there's a better way to achieve the same goal.

@coorasse
Copy link
Contributor Author

We configured it like that because the shop administrator wants to set prices using 3 digits.

@aldesantis
Copy link
Member

@coorasse do you know the business requirement behind that choice? As far as I can see, for a product to cost 60.203 CHF doesn't mean anything to a customer if the smallest unit is 0.01 CHF.

In fact, look at what happens with your fix:

cents = 60203 # 60.203 CHF
subunit_to_unit = 1000

((cents / subunit_to_unit.to_f) * 100).to_i # => 6020 (60.20 CHF)

This means that, even though the price of the product is 60.203 CHF, you're charging the customer 60.20 CHF (which, off the top of my head, may also cause accounting problems in the long run).

I think that's the root of the problem here. Increasing subunit_to_unit to 1000 allows you to specify such granular prices, but it's a hack that I suspect may create weird problems such as this.

If you could share a bit behind why you need such precision in the prices, we can probably find another solution.

@coorasse
Copy link
Contributor Author

The client needs to set them with three digits. I cannot share too much information, but since the amounts bought are high, it really makes a difference for them to also be able to change the third digit. I can think about the gas price for example.

Next to that, in this business case, we have also the option smallest_denomination: 50, which rounds the total always to 0.05 cents since you cannot invoice with a higher detail (as you also correctly pointed out).

So the example you reported, is absolutely true in general, but it does not affect us because you cannot have a total of 60.203 CHF: only single item prices. If you buy an item for 60.203, the cart total will display 60.20 anyway.

That was also my doubt: can we change it in the Payment model? Is there a case where the payment needs to be with cents precision? (so 60203?)
In the case of Stripe, specifically, you can only submit two digits, so 6020 => 60.20CHF would be any way better than 60203 => 602.30 CHF 😆

Still, at the moment, if you configure Solidus with any currency which has more than two digits for cents, Stripe charges are wrong. https://github.com/RubyMoney/money/blob/master/config/currency_iso.json#L223
So maybe we can ignore our specific case and generalise the issue?

@aldesantis
Copy link
Member

aldesantis commented Jul 31, 2020

Got it, the use case makes sense!

With that said, I don't know if this is a general issue: I think if you create a Stripe charge in a currency that has more than two digits for cents, Stripe will compute the final amount correctly. For instance, I expect that a charge with { "amount": 12345, "currency": "BHD" } would be converted into BHD 12.345. The reason it's not working in your case is that Swiss francs use two digits, but you're sending three.

In your specific situation, one problem I see is that Solidus may not mark the order as paid if the order total is smaller than the payment total because of the rounding that takes place in your custom #two_digit_cents method.

Have you tried the Money.infinite_precision option? I wasn't familiar with it, but it looks like it will allow you to specify as many digits as you want after the decimal separator without impacting the conversion between unit and subunit. I would try that or some other solution that has to do with the rounding configuration rather than the actual currency configuration.

Keep me posted on how it goes!

@spaghetticode
Copy link
Member

Still, at the moment, if you configure Solidus with any currency which has more than two digits for cents, Stripe charges are wrong. https://github.com/RubyMoney/money/blob/master/config/currency_iso.json#L223

@coorasse with Payment Intents at the moment the number if digits is hardcoded, see https://github.com/solidusio/solidus_stripe/blob/master/app/controllers/solidus_stripe/intents_controller.rb#L54. I guess this may not a problem for your specific scenario, as you said that the order will be charged up to 2 decimals anyways, but it may need to be addressed for other situations.

@aldesantis
Copy link
Member

@spaghetticode yeah, I think we need to fix that to use current_order.currency.subunit_to_unit, otherwise it won't work for currencies where the unit is made up by more or fewer than 100 sub-units.

@coorasse
Copy link
Contributor Author

coorasse commented Aug 3, 2020

Thanks for the hint @aldesantis! Here are my findings so far:

  • configuring Money.infinite_precision = true seems a better option than playing around with subunit_to_unit and smallest_denomination options.
  • 🆕 The partial to render the input for the money always use precision: 2 by default https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/shared/_number_with_currency.html.erb#L10 (that was the same also before. I just noticed that now,maybe it should be a different issue)
  • The calls to money.money.cents I mentioned above returns now a BigDecimal instead of an Integer, therefore I had to explicitly do money.cents.to_i to avoid errors when calling Stripe. (by the way: the call money.money.cents should be replaced with money.cents anyway.
  • You are right, the payment gateway takes care already of 2 or 3 digits precision currencies.

So, to conclude, I think:

  • Money.infinite_precision = true is the configuration I was looking for. Thanks!
  • When configuring it, I still had to patch Solidus and explicitly call cents.to_i (PR needed?)
  • The partial I mentioned above does not support more digits. PR needed?

Thanks for the help so far!

@aldesantis
Copy link
Member

@coorasse thanks for the tests, I think the issues you found should be addressed — I'm pretty sure you're not the only store needing greater-than-usual precision for product prices.

Would you have some time to submit your patches as PRs to the core? If not, I can also try to find some time this week!

@coorasse
Copy link
Contributor Author

coorasse commented Aug 3, 2020

If it is not urgent, yes, I'd be glad to open a PR. Not this week, though. 👍

@waiting-for-dev waiting-for-dev added the type:bug Error, flaw or fault label Sep 6, 2022
@elia
Copy link
Member

elia commented Apr 5, 2023

@coorasse I think this might be fixed by the release of solidus_stripe v5, which features and uses a conversion utility module: https://github.com/solidusio/solidus_stripe/blob/be652e65b3f9d892c9e462bf7cda0dbcb0f8f6a0/lib/solidus_stripe/money_to_stripe_amount_converter.rb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error, flaw or fault
Projects
None yet
Development

No branches or pull requests

5 participants