[$250] [P2P Distance] - Split amount is not recalculated when entering 0 as input & editing distance

IuliiaHerets commented 1 month 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: v9.0.26-2 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to group chat.
Click + > Split expense > Distance.
Enter waypoints > Next.
Manually enter 0 on the split input field next to any participant.
Click RHP back button.
Edit the waypoints.
Click Next.

Expected Result:

The individual split amount will be recalculated and equal split will be populated in the input field for each participant.

Actual Result:

The input fields for the other participants become 0, while random amount is calculated for the user that has 0 in input field in Step 5. Reset button also disappears.

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/a637a37d-e9d4-4ed9-9381-0478ede90013

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021831341350520719367
Upwork Job ID: 1831341350520719367
Last Price Increase: 2024-09-04
Automatic offers:

nkdengineer | Contributor | 103887821

Issue Owner

Current Issue Owner: @rushatgabhane

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @johncschuster (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.

IuliiaHerets commented 1 month ago

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

Krishna2323 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-29 22:23:23 UTC.

Proposal 

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

Split distance - Split amount is not recalculated when entering 0 as input & editing distance

What is the root cause of that problem?

The condition remainingTotal < 0 is not correct in adjustRemainingSplitShares, if the remaining amount is 0 it will update the amount for unmodifiedSharesAccountIDs to 0

https://github.com/Expensify/App/blob/9c479d7f6b353df21d062cfa2a988e427b32865f/src/libs/actions/IOU.ts#L7601-L7630

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

We should update the condition to remainingTotal <= 0.

What alternative solutions did you explore? (Optional)

We can return from the useEffect callback function when the amount is 0 or a falsy value. https://github.com/Expensify/App/blob/9c479d7f6b353df21d062cfa2a988e427b32865f/src/components/MoneyRequestConfirmationList.tsx#L489-L494

    useEffect(() => {
        if (!isTypeSplit || !transaction?.splitShares || !iouAmount) {
            return;
        }
        IOU.adjustRemainingSplitShares(transaction);
    }, [isTypeSplit, transaction, iouAmount]);

Or we can return if shouldCalculateDistanceAmount is true because when shouldCalculateDistanceAmount is true, we will calculate the amount and set the split, so we don't need to call adjustRemainingSplitShares. https://github.com/Expensify/App/blob/9c479d7f6b353df21d062cfa2a988e427b32865f/src/components/MoneyRequestConfirmationList.tsx#L360-L373
Result

Krishna2323 commented 1 month ago

Proposal Updated

Updated alternative solution to include a second option.

gijoe0295 commented 1 month ago

Proposal

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

After updating waypoints in split distance expense:

Some split amounts are 0
Reset button disappears

What is the root cause of that problem?

I don't think 2 is a problem because when new waypoints are updated, all the split shares are re-calculated. Reset button should only appear when any one of them is modified.

For 1, because when we update waypoints, we reset the transaction amount:

https://github.com/Expensify/App/blob/5f417c07b15f227f7fcc6c08586b09d3175708da/src/libs/actions/Transaction.ts#L272

Later when we go to confirmation step, adjustRemainingSplitShares is called with 0 amount causing the split shares for previously-unmodified participants to be 0:

https://github.com/Expensify/App/blob/f882a8bc83a4fc6072d1c5d6eb884df03a3bb492/src/components/MoneyRequestConfirmationList.tsx#L489-L494

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

We should reset the splitShares as well since after updating the waypoints, this transaction becomes a completely new transaction and all the split shares should be recalculated.

Basically, adjustRemainingSplitShares should only be called when one of the split shares are modified.

...(isDraft && {amount: CONST.IOU.DEFAULT_AMOUNT, splitShares: CONST.EMPTY_OBJECT}),

melvin-bot[bot] commented 4 weeks ago

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

johncschuster commented 3 weeks ago

I'm not really sure this behavior is something we will see, let alone something we should optimize. Getting a sense check.

johncschuster commented 3 weeks ago

Ok! We discussed this, and it sounds like we do expect this behavior. I'll triage it.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

nkdengineer commented 3 weeks ago

Proposal

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

The input fields for the other participants become 0, while random amount is calculated for the user that has 0 in input field in Step 5. Reset button also disappears.

What is the root cause of that problem?

When we go back to the distance page and change the way point, we don't reset the splitShares data before we go to the confirmation page. Then after going to this page again, the splitShares is calculated wrongly here.

https://github.com/Expensify/App/blob/8a35256bff4d66d0f0958fd9100e191a8fda404d/src/components/MoneyRequestConfirmationList.tsx#L495-L500

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

We can do the same way as we do in IOURequestStepAmount which will reset the split share data before we go to the confirmation page.

https://github.com/Expensify/App/blob/8a35256bff4d66d0f0958fd9100e191a8fda404d/src/pages/iou/request/step/IOURequestStepAmount.tsx#L272-L274

Because the new amount will be calculated on the confirmation page and the currency hasn't changed, we only need to pass the transaction to resetSplitShares function.

if (transaction?.splitShares) { 
   IOU.resetSplitShares(transaction); 
}
if (backTo) {
    Navigation.goBack(backTo);
    return;
}

https://github.com/Expensify/App/blob/8a35256bff4d66d0f0958fd9100e191a8fda404d/src/pages/iou/request/step/IOURequestStepDistance.tsx#L238-L240

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 3 weeks ago

@johncschuster, @rushatgabhane Huh... This is 4 days overdue. Who can take care of this?

rushatgabhane commented 3 weeks ago

@nkdengineer's proposal LGTM 🎀 👀 🎀

because they explained the root cause well

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 3 weeks ago

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

nkdengineer commented 1 week ago

@rushatgabhane @Beamanator I had a problem when I tried to revert main on this PR and it automatically closed and couldn't be reopened anymore. I created a new PR here. Sorry for the inconvenience.

Expensify / App