Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$250] Individual expense amount doesn't match amount in split details #42870

Closed m-natarajan closed 4 months ago

m-natarajan commented 5 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: n/a Reproducible in staging?: needs reproduction Reproducible in production?: needs reproduction If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @neil-marcellini Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1717082762084089

Action Performed:

  1. Have another account create a group chat with you and others (8 people total, one of those is split@expensify.com)
  2. Another account scans a receipt and creates a split with the group
  3. Open the split in the group to see the details and view the amount requested from you
  4. Open the individual expense sent to you

Expected Result:

The individual expense and the amount shown in the split details match

Also, somewhat related, half of us should have our amount set to $44.14, and half $44.15. $353.16 / 8 = $44.145. Floor that to $44.14, then times 8 = $353.12. That leaves a remainder of $0.04 to spread across 4 participants. That’s the expected result, and maybe what actually happened, but in the details a different strategy was used.

Actual Result:

The individual expense amount is one cent lower than the amount shown in the split details

For reference/debugging, the reportID of the group chat is 787074187111862

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

New Expensify 2024-05-30 08 19 40

New Expensify 2024-05-30 08 20 05

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d36641ac794340f1
  • Upwork Job ID: 1797370126203723776
  • Last Price Increase: 2024-06-23
melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

MelvinBot commented 5 months ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01d36641ac794340f1

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

badeggg commented 5 months ago

Hi, @m-natarajan can you explain more about the actual result? This seems fine to me: 353.16 = 44.15 * 7 + 44.11 image

ShridharGoel commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Individual expense amount doesn't match amount in split details.

What is the root cause of that problem?

As of now, the below calculation takes place:

totalInCurrencySubunit

totalInCurrencySubunit = Math.round((353.16 / 100) 100) = Math.round(353.16) = 35316 (Assuming total currencyUnit directly)

Total Participants:

totalParticipants = numberOfParticipants + 1 = 7 + 1 = 8

Amount Per Person:

amountPerPerson = Math.round(totalInCurrencySubunit / totalParticipants) = Math.round(35316 / 8) = Math.round(4414.5) = 4415

Final Amount Calculation:

If isDefaultUser is true: sumAmount = amountPerPerson totalParticipants = 4415 8 = 35320 difference = totalInCurrencySubunit - sumAmount = 35316 - 35320 = -4 finalAmount = amountPerPerson + difference = 4415 + (-4) = 4411

If isDefaultUser is false: finalAmount = amountPerPerson = 4415

Converting back to main currency:

Math.round((finalAmount * 100) / currencyUnit)

Results: If isDefaultUser is false:

finalAmount = 4415 Converted back: Math.round((4415 * 100) / 100) = 4415 Amount per person: 4415 subunits = 44.15 main currency units If isDefaultUser is true:

finalAmount = 4411 Converted back: Math.round((4411 * 100) / 100) = 4411 Amount for default user: 4411 subunits = 44.11 main currency units

So, the calculations would yield:

Each participant would pay 44.15 main currency units. The payer would pay 44.11.

What changes do you think we should make in order to solve the problem?

Use the below logic to ensure a better distribution of the split amounts:

function calculateAmount(numberOfParticipants: number, total: number, currency: string, isDefaultUser = false, index: number): number {
    // Since the backend can maximum store 2 decimal places, any currency with more than 2 decimals
    // has to be capped to 2 decimal places
    const currencyUnit = Math.min(100, CurrencyUtils.getCurrencyUnit(currency));
    const totalInCurrencySubunit = Math.round((total / 100) * currencyUnit);
    const totalParticipants = numberOfParticipants + 1;
    const baseAmountPerPerson = Math.floor(totalInCurrencySubunit / totalParticipants);
    const remainder = totalInCurrencySubunit % totalParticipants;

    let finalAmount = baseAmountPerPerson;
    // Distribute the remainder evenly among participants
    const additionalAmount = (index < remainder) ? 1 : 0;
    finalAmount += additionalAmount;
    return Math.round((finalAmount * 100) / currencyUnit);
}

Using the above logic:

Half the participants (4 people) will pay 4415 (44.15 units). The other half (4 people) will pay 4414 (44.14 units).

arjunnepal commented 5 months ago

Contributor details Your Expensify account email: arjunnepal123@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0146ece8c60f682588

As highlighted in numerous comments, the pricing structure initially divides the total cost into two categories: one for oneself and another for friends. This intentional decision by the developer aims to avoid the necessity of a random selection process between paying $44.14 or $44.15 when dividing the cost in half. Additionally, it's crucial to consider that dividing the cost using a strict half-and-half logic could result in varying bills if repeated, thus introducing inconsistencies.

If there's a genuine need to divide the cost among four payers, a fairer solution could entail randomly selecting four users for a 'lucky group' who pay a slightly reduced amount, while the remaining four form the 'unlucky group' and pay a slightly higher sum.

melvin-bot[bot] commented 5 months ago

📣 @arjunnepal! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
goldenbear101 commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue. Individual expense amount doesn't match amount in split details.

What is the root cause of that problem?

I believe the issue is coming from the way the split calculation is being done, causing the discrepancies of one cent:

Current Calculation:

1, Total amount: $353.16. Total participants: 8. Amount per person calculated as: Math.round(35316 / 8) = Math.round(4414.5) = 4415 subunits. This leads to a discrepancy when re-aggregating the amounts, causing some participants to have a one-cent difference.

Proposed Calculation:

2, Split the total amount into subunits. Floor the amount per person to handle even distribution. Distribute any remainder evenly across participants.

Current cal:

Conversion to Subunits:

const totalInCurrencySubunit = Math.round((353.16 / 100) * 100) = 35316;

Participants Calculation:

const totalParticipants = numberOfParticipants + 1 = 7 + 1 = 8;

Amount Per Person:

const amountPerPerson = Math.round(totalInCurrencySubunit / totalParticipants) = Math.round(35316 / 8) = 4415;
Final Amount Calculation for Default User:
if (isDefaultUser) {
    const sumAmount = amountPerPerson * totalParticipants = 4415 * 8 = 35320;
    const difference = totalInCurrencySubunit - sumAmount = 35316 - 35320 = -4;
    finalAmount = amountPerPerson + difference = 4415 + (-4) = 4411;
} else {
    finalAmount = amountPerPerson = 4415;
}

Proposed Logic: Implementation

function calculateAmount(numberOfParticipants, total, currency, isDefaultUser = false, index) {
    // Ensure the currency unit caps at 2 decimal places
    const currencyUnit = Math.min(100, CurrencyUtils.getCurrencyUnit(currency));
    const totalInCurrencySubunit = Math.round((total / 100) * currencyUnit);
    const totalParticipants = numberOfParticipants + 1;
    const baseAmountPerPerson = Math.floor(totalInCurrencySubunit / totalParticipants);
    const remainder = totalInCurrencySubunit % totalParticipants;

    // Calculate the final amount by distributing the remainder
    let finalAmount = baseAmountPerPerson;
    const additionalAmount = (index < remainder) ? 1 : 0;
    finalAmount += additionalAmount;

    return Math.round((finalAmount * 100) / currencyUnit);
}

Converting the subunits back to the main currency makes the final amount accurate.

melvin-bot[bot] commented 5 months ago

@eVoloshchak, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MitchExpensify commented 5 months ago

@eVoloshchak What do you think about the proposals?

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 5 months ago

@eVoloshchak, @MitchExpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

MitchExpensify commented 5 months ago

Bump @eVoloshchak, thanks

MitchExpensify commented 5 months ago

Not overdue, Melvin! The ball is in @eVoloshchak 's court

eVoloshchak commented 5 months ago

@ShridharGoel, @goldenbear101, I get the following error when testing both proposals (split 353.16 between 8 participants)

Screenshot 2024-06-11 at 22 27 39

Could you double-check this please? There might have been recent changes to main

goldenbear101 commented 5 months ago

Not sure why but I would do some rechecking shortly

ShridharGoel commented 5 months ago

@eVoloshchak index also needs to be be passed (like on line 6710 in IOU.ts):

        const splitAmount = IOUUtils.calculateAmount(participantsLength, amount, currency, isPayer, index);
melvin-bot[bot] commented 5 months ago

@eVoloshchak @MitchExpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

MitchExpensify commented 5 months ago

Looks like the ball is in your court to keep this moving @eVoloshchak

melvin-bot[bot] commented 5 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 5 months ago

@eVoloshchak, @MitchExpensify Eep! 4 days overdue now. Issues have feelings too...

MitchExpensify commented 5 months ago

Friendly bump @eVoloshchak

MitchExpensify commented 5 months ago

Do you have time to check this out @eVoloshchak ? I can reassign if you do not 👍

eVoloshchak commented 5 months ago

@ShridharGoel, thank you, can test the proposal and verify there is an equal distribution. However, as @arjunnepal has pointed out in https://github.com/Expensify/App/issues/42870#issuecomment-2145710614

This intentional decision by the developer aims to avoid the necessity of a random selection process between paying $44.14 or $44.15 when dividing the cost in half. Additionally, it's crucial to consider that dividing the cost using a strict half-and-half logic could result in varying bills if repeated, thus introducing inconsistencies

If you split an expense between an odd number of participants, the first participant (you) always gets the bigger amount. I think we need to randomize the distribution between participants

MitchExpensify commented 5 months ago

I think the first participant (you) always getting the higher amount is fine.

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 4 months ago

@eVoloshchak, @MitchExpensify Eep! 4 days overdue now. Issues have feelings too...

MitchExpensify commented 4 months ago

@arielgreen @youssef-lr tagging you as authors of https://docs.google.com/document/d/1wOB61kuktAKbvMsg8dAm9FaO_bhZepXe8agQtvvpgk4/edit

What is currently expected behavior here?

MitchExpensify commented 4 months ago

No overdue, Melvin

ShridharGoel commented 4 months ago

Any updates on this?

ShridharGoel commented 4 months ago

If you split an expense between an odd number of participants, the first participant (you) always gets the bigger amount.

@eVoloshchak Do you think this is always going to be true?

MitchExpensify commented 4 months ago

@arielgreen @youssef-lr I'm hoping you can let us know what is expected here as I assume this came up in the split project already

youssef-lr commented 4 months ago

This issue was created before we implemented uneven splits, I don't think this is reproducible anymore because of the way we save individual splits now. Closing!

MitchExpensify commented 4 months ago

Awesome!