search

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.52k stars 2.87k forks source link

[HOLD for payment 2024-10-25] [$250] [P2P Distance] Split - Participants amount displayed 0.00 briefly on confirmation screen #47100

Closed lanitochka17 closed 1 week ago

lanitochka17 commented 2 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: 9.0.18.3 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+0808pm@applause.expensifail.com Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/42302

Action Performed:

  1. Open the app and log in
  2. Tap FAB > Split expense > Distance
  3. Tap + > Split expense > Distance
  4. Enter the start and finish viewpoints
  5. Select a few participants and proceed to the confirmation screen

Expected Result:

The correct participants amounts are displayed

Actual Result:

0.00 is briefly displayed instead of the correct amount

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/9171a2e7-027c-41eb-a00a-19ccb2a4ddd1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0170f1a1791408f62b
  • Upwork Job ID: 1821642103848571473
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • Krishna2323 | Contributor | 103497363
Issue OwnerCurrent Issue Owner: @garrettmknight
github-actions[bot] commented 2 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @jasperhuangg (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

lanitochka17 commented 2 months ago

We think that this bug might be related to #vip-split

neil-marcellini commented 2 months ago

Not a blocker, it's behind the distance split beta.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

Krishna2323 commented 2 months ago

Proposal

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

Split distance - Participants amount displayed 0.00 briefly on confirmation screen

What is the root cause of that problem?

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


        if (isDistanceRequest) {
            let reportUpdated = report;
            if (isPolicyExpenseChat && transaction?.participants?.[0]) {
                reportUpdated = ReportUtils.getReport(transaction?.participants?.[0].reportID ?? '') ?? report;
            }
            const policyID = IOU.getIOURequestPolicyID(transaction, reportUpdated);
            const policy = PolicyUtils.getPolicy(report?.policyID ?? policyID);
            const policyCurrency = policy?.outputCurrency ?? PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;
            const customUnitRateID = TransactionUtils.getRateID(transaction) ?? '-1';
            const mileageRates = DistanceRequestUtils.getMileageRates(policy);
            const defaultMileageRate = DistanceRequestUtils.getDefaultMileageRate(policy);
            const mileageRate = TransactionUtils.isCustomUnitRateIDForP2P(transaction)
                ? DistanceRequestUtils.getRateForP2P(policyCurrency)
                : mileageRates?.[customUnitRateID] ?? defaultMileageRate;
            const {unit, rate} = mileageRate ?? {};
            const distance = TransactionUtils.getDistanceInMeters(transaction, unit);
            const currency = (mileageRate as MileageRate)?.currency ?? policyCurrency;
            const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate ?? 0);
            IOU.setMoneyRequestAmount(transactionID, amount, currency ?? '');

            const participantsMap =
                transaction?.participants?.map((participant) => {
                    if (participant.isSender && iouType === CONST.IOU.TYPE.INVOICE) {
                        return participant;
                    }
                    return participant.accountID ? OptionsListUtils.getParticipantsOption(participant, personalDetails) : OptionsListUtils.getReportOption(participant);
                }) ?? [];
            const participantAccountIDs: number[] = participantsMap.map((participant) => participant.accountID ?? -1);
            if (isTypeSplit && !isPolicyExpenseChat && amount && transaction?.currency) {
                IOU.setSplitShares(transaction, amount, currency, participantAccountIDs);
            }
        }

[!NOTE] The code above is not perfect, it is just pseudocode and can be improved. It might also have some flaws, but these can be addressed after thorough testing.

What alternative solutions did you explore? (Optional)

daledah commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-09 01:17:59 UTC.

Proposal

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

What is the root cause of that problem?

https://github.com/Expensify/App/blob/31c301ed14af0dc60ae1531c8332739444034fed/src/components/MoneyRequestConfirmationList.tsx#L347

the correct amount is displayed afterward.

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

https://github.com/Expensify/App/blob/31c301ed14af0dc60ae1531c8332739444034fed/src/components/MoneyRequestConfirmationList.tsx#L491

will be:

rightElement: transaction?.splitShares?.[participantOption.accountID ?? -1]?.amount !== undefined ? (
    <MoneyRequestAmountInput />
) : (
    <ActivityIndicator color={!true ? theme.textLight : theme.text} />
)
garrettmknight commented 2 months ago

@parasharrajat can you give these proposals a look when you get a chance?

melvin-bot[bot] commented 2 months ago

@garrettmknight, @parasharrajat, @jasperhuangg Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

📣 @Krishna2323 🎉 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 📖

jasperhuangg commented 2 months ago

@Krishna2323's approach seems to be the simplest approach to fixing the issue, assigning them!

FYI I will be going OOO for the rest of the week, so we might need to bring in another internal engineer to help with PR reviews once the PR is done. cc @garrettmknight

parasharrajat commented 2 months ago

Sure, let's go with that approach.

Krishna2323 commented 2 months ago

Draft PR is up, just need to test all cases and add recordings, will be completed by EOD.

Krishna2323 commented 2 months ago

@parasharrajat PR ready for review.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @garrettmknight, @parasharrajat, @jasperhuangg, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

jasperhuangg commented 1 month ago

I think our automation was broken for a while. This was deployed to production a while ago without any regressions.

@garrettmknight Do you mind paying out @Krishna2323 for implementing the solution to this issue and @parasharrajat for reviewing proposals/PRs? I think we can safely close out this issue after that.

Krishna2323 commented 1 month ago

@jasperhuangg, The PR is still open, we were about to complete the testing but in this PR we removed the split expense option from global create button.

I'm not completely sure what to do now but we are discussing in the PR.

jasperhuangg commented 1 month ago

@Krishna2323 Got it, thanks for the update

garrettmknight commented 1 month ago

Since we've removed the global create, I've tried to repro using split in the group chat. I can't really tell if it happens in iOS, there's some glitchy behavior, but I can't see if it's starting with 0 because it's too fast.

garrettmknight commented 1 month ago

It's a lot clearer on Android. @Krishna2323 @parasharrajat is the fix you're working on still applicable to the behavior even if it doesn't come from Global Create? https://github.com/user-attachments/assets/1e963d88-4de5-4384-b252-57a66bb541a6

Krishna2323 commented 1 month ago

is the fix you're working on still applicable to the behavior even if it doesn't come from Global Create?

The approach is the same, but I had to reapply it to a different component (distance page) to make it work with the current split distance creation flow. @parasharrajat will review the updated code soon.

melvin-bot[bot] commented 1 month ago

@garrettmknight, @parasharrajat, @jasperhuangg, @Krishna2323 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 3 weeks ago

@garrettmknight, @parasharrajat, @jasperhuangg, @Krishna2323 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 3 weeks ago

@garrettmknight, @parasharrajat, @jasperhuangg, @Krishna2323 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 2 weeks ago

@garrettmknight, @parasharrajat, @jasperhuangg, @Krishna2323 12 days overdue now... This issue's end is nigh!

parasharrajat commented 2 weeks ago

PR is close to merge.

parasharrajat commented 2 weeks ago

I am no longer able to reproduce this issue on the App. But @Krishna2323 showed that the code still gets the proper values on the second render https://github.com/Expensify/App/pull/47370#issuecomment-2372585951. This is not visible on the UI. Should we pursue the solution further to get the values in the first render? @jasperhuangg

Krishna2323 commented 2 weeks ago

It's a lot clearer on Android. @Krishna2323 @parasharrajat is the fix you're working on still applicable to the behavior even if it doesn't come from Global Create?

@parasharrajat, have you tried this on a physical Android device? I don't have beta access to create P2P distance requests.

parasharrajat commented 2 weeks ago

Ok. I didn't notice that. Looks like this is an issue on low-end devices. Let me get back on the PR review.

melvin-bot[bot] commented 2 weeks ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

neil-marcellini commented 2 weeks ago

In the process of fixing that blocker the PR was reverted https://github.com/Expensify/App/pull/47370. I asked on the blocker issue here if we can re-apply this PR because it sounds like maybe it didn't need to be reverted in the first place.

Either way we need to redo this and make sure it's not causing any bugs. @Krishna2323 when will you be able to put up another PR?

melvin-bot[bot] commented 2 weeks ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.50-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-25. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 2 weeks ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

Krishna2323 commented 2 weeks ago

I will raise the PR on Monday once we get confirmation on this.

parasharrajat commented 1 week ago

is there something pending on this issue? @Krishna2323 It looks like the PR was reapplied https://github.com/Expensify/App/pull/51156

Krishna2323 commented 1 week ago

@parasharrajat, there’s nothing pending here. @garrettmknight, this will be ready for payments in 2 days, as the reapplied PR was deployed to production 5 days ago :)

parasharrajat commented 1 week ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

Regression Test Steps

  1. Open a group chat
  2. Tap + > Split expense > Distance
  3. Enter the start and finish viewpoints
  4. Verify correct participants amounts are displayed without any delay

Do you agree 👍 or 👎 ?

garrettmknight commented 1 week ago

Payment Summary:

melvin-bot[bot] commented 1 week ago

@garrettmknight @jasperhuangg Be sure to fill out the Contact List!