melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
aldo-expensify
commented
1 year ago @luacmartins it looks like we have different rounding strategies in the front vs the server. I just wanted to confirm that the expected would have been 37.33 in the front instead of 33, right?
Also, I know you are working on a PR for split bill, is that PR going to fix this?
luacmartins
commented
1 year ago I just wanted to confirm that the expected would have been 37.33 in the front instead of 33, right?
Do you mean 37.33 instead of 37? I think that's correct and it should show 37.33.
Also, I know you are working on a PR for split bill, is that PR going to fix this?
My PR is not fixing this.
aldo-expensify
commented
1 year ago Do you mean 37.33 instead of 37?
yes :P
ok! thanks
abekkala
commented
1 year ago @aldo-expensify I see that you wanted this to be Internal. Based on this SO - should this also be Demolition?
aldo-expensify
commented
1 year ago Based on this SO - should this also be Demolition?
It is not really needed, I'll work in a solution. If you add demolition, it will be assigned to me anyway :P
luacmartins
commented
1 year ago I think that we merged the CME and Demolition teams into the engineering teams. Is that label still needed?
aldo-expensify
commented
1 year ago I think that we merged the CME and Demolition teams into the engineering teams
I think your are right about this (checked in Salt/www-whitelist)
Is that label still needed?
Shouldn't be
aldo-expensify
commented
1 year ago From this result:
It seems that ALL (Qindarka) is a currency that doesn't do "cents" (there are lots of currencies like this). Since the split has 33 "cents" of ALL, the front end rounds it to 0. I think this is correct. The problem is the backend not considering that this currency doesn't have "cents" and it should be rounding to the unit.
luacmartins
commented
1 year ago Nice find! I agree that this is an issue with the server then!
aldo-expensify
commented
1 year ago The backend stores the number of decimals for each currency here: https://github.com/Expensify/PHP-Libs/blob/main/src/CurrencyInfo.php
I'm thinking about running a small JS snipped like this:
const currencySymbols = ['AED', 'AFN', 'ALL', 'AMD', 'ANG', 'AOA', 'ARS', 'AUD', 'AWG', 'AZN', 'BAM', 'BBD', 'BDT', 'BGN', 'BHD', 'BIF', 'BMD', 'BND', 'BOB', 'BRL', 'BSD', 'BTN', 'BWP', 'BYN', 'BYR', 'BZD', 'CAD', 'CDF', 'CHF', 'CLP', 'CNY', 'COP', 'CRC', 'CUC', 'CUP', 'CVE', 'CZK', 'DJF', 'DKK', 'DOP', 'DZD', 'EEK', 'EGP', 'ERN', 'ETB', 'EUR', 'FJD', 'FKP', 'GBP', 'GEL', 'GHS', 'GIP', 'GMD', 'GNF', 'GTQ', 'GYD', 'HKD', 'HNL', 'HRK', 'HTG', 'HUF', 'IDR', 'ILS', 'INR', 'IQD', 'IRR', 'ISK', 'JMD', 'JOD', 'JPY', 'KES', 'KGS', 'KHR', 'KMF', 'KPW', 'KRW', 'KWD', 'KYD', 'KZT', 'LAK', 'LBP', 'LKR', 'LRD', 'LSL', 'LTL', 'LVL', 'LYD', 'MAD', 'MDL', 'MGA', 'MKD', 'MMK', 'MNT', 'MOP', 'MRO', 'MRU', 'MUR', 'MVR', 'MWK', 'MXN', 'MYR', 'MZN', 'NAD', 'NGN', 'NIO', 'NOK', 'NPR', 'NZD', 'OMR', 'PAB', 'PEN', 'PGK', 'PHP', 'PKR', 'PLN', 'PYG', 'QAR', 'RON', 'RSD', 'RUB', 'RWF', 'SAR', 'SBD', 'SCR', 'SDG', 'SEK', 'SGD', 'SHP', 'SLL', 'SOS', 'SRD', 'STD', 'STN', 'SVC', 'SYP', 'SZL', 'THB', 'TJS', 'TMT', 'TND', 'TOP', 'TRY', 'TTD', 'TWD', 'TZS', 'UAH', 'UGX', 'USD', 'UYU', 'UZS', 'VEB', 'VEF', 'VES', 'VND', 'VUV', 'WST', 'XAF', 'XCD', 'XOF', 'XPF', 'YER', 'ZAR', 'ZMK', 'ZMW'];
const decimals = [0.1, 0.01, 0.001, 0.0001, 0.00001, 0.000001];
currencySymbols.map(currencySymbol => {
let decimalPlaces = 0;
let decimalPlacesTested = {};
decimals.forEach((value, index) => {
decimalPlacesTested[value] = Intl.NumberFormat('en', {style: 'currency', currency: currencySymbol}).format(value);
if (decimalPlacesTested[value].indexOf(`${value}`) !== -1) {
decimalPlaces = index + 1;
}
});
return {
currencySymbol,
decimalPlaces,
decimalPlacesTested,
};
});Hmm, the ISO4217 specifies the number for decimals (see https://en.wikipedia.org/wiki/ISO_4217), and it doesn't match with what Intl.NumberFormat('en', {style: 'currency', currency: currencySymbol}).format(value) produces. We use ISO4217 i nthe backend. I think we have two options:
iouDecimals that matches Intl.NumberFormat. A new key instead of updating decimals because I'm worried about breaking something 😬 aldo-expensify
commented
1 year ago Hmm, ok, I think we can do it in the App passing maximumFractionDigits: <decimalsAccordingToISO4217> in the options given to Intl.NumberFormat. For example:
Intl.NumberFormat('es', {style: 'currency', currency: 'ALL', maximumFractionDigits: 3}).format(0.001)
'0,001 ALL'Some weird cases:
COP (Colombian Peso)
ALL (Albanian Lek)
I'll try to get some opinions in slack about:
aldo-expensify
commented
1 year ago Following the slack discussion...
It seems that the best solution for now is using the php NumberFormatter library in the backend because the results produced align very well with what Intl produces.
To compare the results between php NumberFormatter and js Intl, I ran the following snippets:
const currencySymbols = ['AED', 'AFN', 'ALL', 'AMD', 'ANG', 'AOA', 'ARS', 'AUD', 'AWG', 'AZN', 'BAM', 'BBD', 'BDT', 'BGN', 'BHD', 'BIF', 'BMD', 'BND', 'BOB', 'BRL', 'BSD', 'BTN', 'BWP', 'BYN', 'BYR', 'BZD', 'CAD', 'CDF', 'CHF', 'CLP', 'CNY', 'COP', 'CRC', 'CUC', 'CUP', 'CVE', 'CZK', 'DJF', 'DKK', 'DOP', 'DZD', 'EEK', 'EGP', 'ERN', 'ETB', 'EUR', 'FJD', 'FKP', 'GBP', 'GEL', 'GHS', 'GIP', 'GMD', 'GNF', 'GTQ', 'GYD', 'HKD', 'HNL', 'HRK', 'HTG', 'HUF', 'IDR', 'ILS', 'INR', 'IQD', 'IRR', 'ISK', 'JMD', 'JOD', 'JPY', 'KES', 'KGS', 'KHR', 'KMF', 'KPW', 'KRW', 'KWD', 'KYD', 'KZT', 'LAK', 'LBP', 'LKR', 'LRD', 'LSL', 'LTL', 'LVL', 'LYD', 'MAD', 'MDL', 'MGA', 'MKD', 'MMK', 'MNT', 'MOP', 'MRO', 'MRU', 'MUR', 'MVR', 'MWK', 'MXN', 'MYR', 'MZN', 'NAD', 'NGN', 'NIO', 'NOK', 'NPR', 'NZD', 'OMR', 'PAB', 'PEN', 'PGK', 'PHP', 'PKR', 'PLN', 'PYG', 'QAR', 'RON', 'RSD', 'RUB', 'RWF', 'SAR', 'SBD', 'SCR', 'SDG', 'SEK', 'SGD', 'SHP', 'SLL', 'SOS', 'SRD', 'STD', 'STN', 'SVC', 'SYP', 'SZL', 'THB', 'TJS', 'TMT', 'TND', 'TOP', 'TRY', 'TTD', 'TWD', 'TZS', 'UAH', 'UGX', 'USD', 'UYU', 'UZS', 'VEB', 'VEF', 'VES', 'VND', 'VUV', 'WST', 'XAF', 'XCD', 'XOF', 'XPF', 'YER', 'ZAR', 'ZMK', 'ZMW'];
const decimals = [0.1, 0.01, 0.001, 0.0001, 0.00001, 0.000001];
currencySymbols.map(currencySymbol => {
let decimalPlaces = 0;
let decimalPlacesTested = {};
decimals.forEach((value, index) => {
const en = Intl.NumberFormat('en', {style: 'currency', currency: currencySymbol}).format(value);
const es = Intl.NumberFormat('es', {style: 'currency', currency: currencySymbol}).format(value);
decimalPlacesTested[value] = {en, es};
if (en.indexOf(`${value}`) !== -1) {
decimalPlaces = index + 1;
}
});
return {
currencySymbol,
decimalPlaces,
decimalPlacesTested,
};
});<?
$currencySymbols = ['AED', 'AFN', 'ALL', 'AMD', 'ANG', 'AOA', 'ARS', 'AUD', 'AWG', 'AZN', 'BAM', 'BBD', 'BDT', 'BGN', 'BHD', 'BIF', 'BMD', 'BND', 'BOB', 'BRL', 'BSD', 'BTN', 'BWP', 'BYN', 'BYR', 'BZD', 'CAD', 'CDF', 'CHF', 'CLP', 'CNY', 'COP', 'CRC', 'CUC', 'CUP', 'CVE', 'CZK', 'DJF', 'DKK', 'DOP', 'DZD', 'EEK', 'EGP', 'ERN', 'ETB', 'EUR', 'FJD', 'FKP', 'GBP', 'GEL', 'GHS', 'GIP', 'GMD', 'GNF', 'GTQ', 'GYD', 'HKD', 'HNL', 'HRK', 'HTG', 'HUF', 'IDR', 'ILS', 'INR', 'IQD', 'IRR', 'ISK', 'JMD', 'JOD', 'JPY', 'KES', 'KGS', 'KHR', 'KMF', 'KPW', 'KRW', 'KWD', 'KYD', 'KZT', 'LAK', 'LBP', 'LKR', 'LRD', 'LSL', 'LTL', 'LVL', 'LYD', 'MAD', 'MDL', 'MGA', 'MKD', 'MMK', 'MNT', 'MOP', 'MRO', 'MRU', 'MUR', 'MVR', 'MWK', 'MXN', 'MYR', 'MZN', 'NAD', 'NGN', 'NIO', 'NOK', 'NPR', 'NZD', 'OMR', 'PAB', 'PEN', 'PGK', 'PHP', 'PKR', 'PLN', 'PYG', 'QAR', 'RON', 'RSD', 'RUB', 'RWF', 'SAR', 'SBD', 'SCR', 'SDG', 'SEK', 'SGD', 'SHP', 'SLL', 'SOS', 'SRD', 'STD', 'STN', 'SVC', 'SYP', 'SZL', 'THB', 'TJS', 'TMT', 'TND', 'TOP', 'TRY', 'TTD', 'TWD', 'TZS', 'UAH', 'UGX', 'USD', 'UYU', 'UZS', 'VEB', 'VEF', 'VES', 'VND', 'VUV', 'WST', 'XAF', 'XCD', 'XOF', 'XPF', 'YER', 'ZAR', 'ZMK', 'ZMW'];
$decimals = [0.1, 0.01, 0.001, 0.0001, 0.00001, 0.000001];
$result = [];
foreach ($currencySymbols as $currencySymbol) {
$decimalPlaces = 0;
$decimalPlacesTested = [];
foreach ($decimals as $index => $value) {
$index2 = $index+1;
$stringValue = sprintf("%.${index2}f", $value);
$en = (new NumberFormatter("en", NumberFormatter::CURRENCY))->formatCurrency($value, $currencySymbol);
$es = (new NumberFormatter("es", NumberFormatter::CURRENCY))->formatCurrency($value, $currencySymbol);
$decimalPlacesTested[$stringValue] = ['en' => $en, 'es' => $es];
if (strpos($en, $stringValue) !== false) {
$decimalPlaces = $index2;
}
}
$result[] = [
"currencySymbol" => $currencySymbol,
"decimalPlaces" => $decimalPlaces,
"decimalPlacesTested" => $decimalPlacesTested,
];
}
file_put_contents("currencies-php.json", json_encode($result, JSON_UNESCAPED_UNICODE));Put the results in two json files and compare them agains each other
There were differences in only 3 currency symbols:
| locale | php NumberFormatter | js Intl |
| es | CA$ | CAD |
| en | PHP | ₱ |
| en | CFA | F CFA |
The numbers of decimal places in both libraries matched
aldo-expensify
commented
1 year ago 
deetergp
commented
1 year ago https://github.com/Expensify/Web-Expensify/pull/35845 was reverted here due to this deploy blocker.
abekkala
commented
1 year ago I'm now ooo and will need to reassign. It looks like PR 35845 for fix was all reviewed internally as well. So, I don't believe that there would be an contributor payment due with this one. But maybe @aldo-expensify can confirm.
Most likely just the BZ checklist for this one.
JmillsExpensify
commented
1 year ago Yeah, no C/C+ on this issue.
JmillsExpensify
commented
1 year ago That said, don't forget to compensate @bernhardoj for reporting.
lschurr
commented
1 year ago Are we just waiting to see if there are any regressions on this one? Shall I post the job to pay @bernhardoj for reporting?
lschurr
commented
1 year ago @bernhardoj - Could you apply to the job here? https://www.upwork.com/jobs/~01936a4e7a1ece5487
JmillsExpensify
commented
1 year ago Given that you're probably headed out on vacay after tomorrow, I'd go ahead and try to settle up before you leave. The PR is internal and we chose to fix it, so fair game to pay out on.
lschurr
commented
1 year ago Thanks @JmillsExpensify - I'll keep an eye on it. When @bernhardoj applies/accepts in Upwork, I'll pay!
aldo-expensify
commented
1 year ago About the resolution state of this bug:
My first PR caused a regression because we were missing some php library in production/staging. The PR was reverted
I'm waiting for this Salt PR to be merge/salted to have the library available, after that I can re-do my first PR
aldo-expensify
commented
1 year ago Salt PR was salted.
New PR up redoing the change in the php API: https://github.com/Expensify/Web-Expensify/pull/35908
lschurr
commented
1 year ago Just noting from the BZ side that the payment was already made to the bug reporter.
lschurr
commented
1 year ago @aldo-expensify is this one resolved?
aldo-expensify
commented
1 year ago @aldo-expensify is this one resolved?
I believe so. The PR https://github.com/Expensify/Web-Expensify/pull/35908 is in production and was QAed.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Amount is not rounded up/down
Actual Result:
Amount is rounded up/down
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.38-5 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/207428947-3d50dac5-4543-4254-8fcd-4ea621639511.mp4
https://user-images.githubusercontent.com/43996225/207428976-01eb9be4-3399-4e15-8a5d-c205451534f3.mp4
Expensify/Expensify Issue URL: Issue reported by: @bernhardoj Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1670937919835729
View all open jobs on GitHub