App: [HOLD for payment 2023-04-27] [$2000] Split - 'Split bill' incorrectly divides bills for some currencies

I think we landed here:

we shouldn’t split in cents if the currency doesn’t show cents in the UI. For example, for JPY, if we split 4 JPY between 3 people, the splits should be 1, 1 and 2, and not 1.33, 1.33 and 1.34.

@aldo-expensify should we hold this until backend update deploy to staging?

I think as soon as the backend PR is merged (no need to wait for deploy), @tienifr can update the PR to use the decimals in Onyx. I’m waiting because I don’t want to ask @tienifr to update the PR to use this and then, if it doesn’t get approved for some reason, @tienifr has to rollback.

Actually, currencyList is also stored in Onyx. However this data is provided by the back-end, specifically on OpenApp API response. Therefore, I believe we should go with @aldo-expensify’s suggestion on using Intl.formatNumber.

Lucky for me, the PR I’ve been working on already follows the second approach 😆😆 So I think it’s ready for your review!

10 JPY between 4 attendees should be split like 3, 3, 3 and 1 (remainder: 10 - 3 - 3 - 3)

@aldo-expensify Sorry, but it sounds incorrect right?

it may be, but my intention is to keep the scope of this issue small and that current split logic can be discussed/fixed in an another issue. I had a small chat with @JmillsExpensify about this and splitting these tiny amounts into many participants is just an edge case we may not care about.

Here we are just dealing with the cents appearing in currencies that don’t support it.

Here’s summary for PR merge timeline:

• Contributor assigned on Apr 4
• Issue put on hold while doing backend work
• Backend hit staging on Apr 13 and PR resumed - this can be actual assign date
• PR merged on Apr 18

There was weekend between Apr 13 - Apr 18 So I can say PR merged within 3 business days

No PRs caused regression. The issue existed from the beginning. Before, we treated all currencies same as USD which has 2 digits. So 1-3 items can be checked off. For regression test steps, we already have automated tests with good examples with edge cases so I don’t think it’s required.

@tienifr the backend PRs has been merged, but not deployed yet. Can you update your PR so it gets the decimal numbers from currencyList in Onyx? 🙏

I’ll remove the HOLD once the backend PR is deployed.

Either way, can we hold this until we get confirmation? Because PR cannot be merged until then and it affects timeline.

The currency list we send from the backend already contains a property call “decimals” for some of them. This property usually is 0 or missing. If it is missing, we are supposed to assume that there are 2 decimals.

This list is a bit outdated and there is a good number of currencies with 0 decimals that are missing decimals: 0. I’m updating this in the backend. After this is updated, @tienifr you can use the currency list from onyx to figure out the number of decimals.

ok I also suggest to do nothing in split logic. So leave negative value as is now for such extreme edge case. That being said, we can go with @tienifr’s solution Let’s make sure that we are not missing any currencies that don’t have 2 digits and also not exist in formatjs currency digits data

🎀 👀 🎀 C+ reviewed cc: @aldo-expensify

currency: BHD split 10, 0.05 to 3 people (including current user) Expected result: (3.334, 3.333, 3.333), (0.016, 0.017, 0.017)

@aimane-chnaif here’s the demo video you requested for this case

https://user-images.githubusercontent.com/113963320/228623640-c9b846f0-b4a7-4e94-ac98-947743cc141d.mp4

@aimane-chnaif I think it was marked “do nothing” by another contributor rather than the team, so the solution hasn’t been made yet if it should be a GH issue 😁 Maybe @aldo-expensify can help decide here whether to fix it as part of this issue.

@tienifr thanks for following discussions which made scope of this issue more clear.

Some feedback from your proposal:

Get the currency minor units data. It can be taken from the intl-numberformat that we’re using here https://github.com/formatjs/formatjs/blob/62ec0b3338dacc54340c559a11128a50e02d95a2/packages/intl-numberformat/src/currency-digits.generated.ts.

I am not sure if we should use data from this library. i.e. even main currencies (USD, EUR, GBP) don’t exist. And we can’t confirm that missing currencies always have 2 decimal digits.

We can use another library that deals specifically with number like https://dinerojs.com/ to split the bill/, then we’ll have the correct currency handling out of the box.

Using another 3rd party is the last option. Let’s try to find solution with currently imported libraries as much as possible.

Agreed, I think we currently use Intl.NumberFormat to format currency? it would be ideal to use the same and not a 3rd party library so we don’t get inconsistencies whereIntl doesn’t use decimals from some currency and the 3rd party library does.

I completed an example with JPY: 10 JPY between 4 attendees should be split like 3, 3, 2 and 2, and the request to the backend should have the amounts 300, 300, 200 and 200.

@aldo-expensify it looks like you want to change how we split the amount as well rather than just dealing with the special currencies. For example for regular currencies like USD, if we split 0.1 USD (10 cents) to 4 people, it will be split to 0.01, 0.03, 0.03, 0.03 (current staging/prod is like this, the user who splits will get the 0.01 and the rest will have the same split). While you’re expecting it will be split to 0.03, 0.03, 0.02, 0.02.

This is different from the original issue here which only relates to some special currencies and whether they have cents or not. Can you confirm we want to do this (change how we split even for regular currencies like USD) as part of this ticket or create another bug for this?

Sorry, that was my mistake, it is not the intention to change how it works for USD. I’ll update my example to follow the same pattern. Splitting 10 JPY between 4 people should be 3, 3, 3 and 1 if that is more consistent with the current split in USD.

FYI the solution for that new split logic (aside from the above proposals, if we choose to implement it) will be:

1. Split the amount equally first using Math.floor (eg. Math.floor(10 / 4) = 2 -> we get an array of [2, 2, 2, 2])
2. Get the remainder (eg. 10 % 4 = 2)
3. Add 1 from the remainder to each split until the remainder is exhausted (eg. the array now becomes [3, 3, 2, 2])

A short snippet for the logic:

const allocateAmount = (amount, portionCount) => {
    let splits = Array(portionCount).fill(Math.floor(amount/portionCount));
    let remainder = amount % portionCount;
    if (remainder > 0) {
        for (let i = 0; i < remainder; i++) {
            splits[i] = splits[i] + 1;
        }
    } 
    return splits;
}

Compared to the current split logic that we have:

1. Split the amount equally first using Math.round (eg. Math.round(10 / 4) = 3 -> we get an array of [3, 3, 3, 3])
2. Get what’s the difference between the total and the splits (10 - 3 * 4 = -2)
3. Add the difference to the current user’s split (3 - 2 = 1, now we have the splits of [1, 3, 3, 3])

This is the current behavior.

@aimane-chnaif friendly bump for reviewing proposals

I know that we can do cents when we split the amount equally, but I’m thinking that we shouldn’t split in cents if the currency doesn’t show cents in the UI. For example, for JPY, if we split 4 JPY between 3 people, the splits should be 1, 1 and 2, and not 1.33, 1.33 and 1.34.

Do you agree @aimane-chnaif @hayata-suenaga ?

reviewing proposals shortly