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

Some ApplePay amounts may be wrong in some locales. #84

Open
kristoffer-zliide opened this issue Aug 25, 2021 · 5 comments
Open

Some ApplePay amounts may be wrong in some locales. #84

kristoffer-zliide opened this issue Aug 25, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@kristoffer-zliide
Copy link

Regarding the use of NSDecimalNumber in https://github.com/google-pay/flutter-plugin/blob/main/pay_ios/ios/Classes/PaymentHandler.swift#L125, please read the Discussion section in https://developer.apple.com/documentation/foundation/nsdecimalnumber/1409902-init.

Should amounts really be strings? What are the odds of users of this plugin will convert their amounts to string correctly? It should always be with a . as decimal separator, right?

@JlUgia JlUgia self-assigned this Aug 31, 2021
@JlUgia JlUgia added the bug Something isn't working label Aug 31, 2021
@JlUgia
Copy link
Member

JlUgia commented Sep 10, 2021

ACK @kristoffer-zliide. Thank you for bringing it up.
My suggestion, for consistency reasons would be to use the decimal point element for all prices. It seems that Double does not account for the locale at instantiation.
I'm suggesting the following:

NSDecimalNumber(value: Double(item["amount"] as! String)!)

There's the more explicit alternative to use a number formatter, though I don't think the extra overhead justifies the marginal added clarity.
Did you have other ideas in mind?

@kristoffer-zliide
Copy link
Author

I don't really know anything about Apple's APIs, sorry. I just thought that since you're representing decimal numbers as strings, there must be bugs in there, and, lo and behold, there was.

Next up are all the bugs people using this plugin will make converting their decimal amounts to strings to pass them to this plugin. How to fix those? I can't help thinking the right way to go is using double for amounts in the API. Then pass it as-is to Apple Pay and take care of using the correct formatting that Google Pay expects (. and exactly two decimal places?)

Note that these bugs probably won't be caught during regular QA, as this issue also demonstrates, since all test devices use a locale where it works, so bugs that cause payment to fail sometimes for some people will be released to the public.

@JlUgia
Copy link
Member

JlUgia commented Sep 14, 2021

This is indeed a situation where there's going to be discrepancies between platforms (Apple Pay using NSDecimalNumber and Google Pay using a String), and hence, there's the opportunity to pick what's more convenient. I personally see reasons for and against both approaches:

  • String: The price is used for display purposes only on both platforms, so the numeric value is irrelevant. Strings are univocal (when conversions aren't required).
  • Number: It's more intuitive to think of a price as a number. Said so, types holding numbers have limitations in size, precision and rounding.

Looking at it from the eyes of this plugin, choosing either of them would require converting to/from the types on one of the platforms, so another question to ask is whether it's preferable to convert from a number to a string on Android, or from a string to a number on iOS. They seem equivalent to me.

All in all, it mostly comes down to dev preference on whether to work with numbers or strings when defining a price amount for an item. To me, both options are equally reasonable.

For now, I'll add a PR to take localization out of the equation when converting amounts on iOS. In the meantime, it's worthwhile considering opening up a new discussion on the topic to gather general thoughts/preferences from users of the library.

@kristoffer-zliide
Copy link
Author

Just to be clear, the String used in the API is not the string used for display right? The API string needs to be English, whereas the display strings are in the phone's locale. That might lead to more confusion.

@JlUgia
Copy link
Member

JlUgia commented Jan 31, 2022

Your assumption is correct.
For now, the API has been updated to remove ambiguity and unexpected results.
With that, I feel that the API can benefit from using univocal representations of amounts like cents, or base 10 decimal numbers, so I suggest leaving this thread open for now, and considering taking it to a formal discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants