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

[lanitochka17]

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 #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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6565894_1723139401565.video_2024-08-08_13-48-49.mp4

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 Owner

Current Issue Owner: @garrettmknight

[github-actions]

👋 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]

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

[lanitochka17]

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

[neil-marcellini]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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?

• The amount and split amount for each user are calculated as a side effect after the component is rendered on the IOURequestStepConfirmation page. Initially, the amount is 0, which causes 0.00 to be displayed briefly. The calculation occurs on the second render due to the isFirstUpdatedDistanceAmount check.
  

  App/src/components/MoneyRequestConfirmationList.tsx

  Lines 322 to 349 in e14fc68

  	const isFirstUpdatedDistanceAmount = useRef(false);
  	useEffect(() => {
  	if (isFirstUpdatedDistanceAmount.current) {
  	return;
  	}
  	if (!isDistanceRequest) {
  	return;
  	}
  	const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit ?? CONST.CUSTOM_UNITS.DISTANCE_UNIT_MILES, rate ?? 0);
  	IOU.setMoneyRequestAmount(transactionID, amount, currency ?? '');
  	isFirstUpdatedDistanceAmount.current = true;
  	}, [distance, rate, unit, transactionID, currency, isDistanceRequest]);
  	useEffect(() => {
  	if (!shouldCalculateDistanceAmount) {
  	return;
  	}
  	const amount = distanceRequestAmount;
  	IOU.setMoneyRequestAmount(transactionID, amount, currency ?? '');
  	// If it's a split request among individuals, set the split shares
  	const participantAccountIDs: number[] = selectedParticipantsProp.map((participant) => participant.accountID ?? -1);
  	if (isTypeSplit && !isPolicyExpenseChat && amount && transaction?.currency) {
  	IOU.setSplitShares(transaction, amount, currency, participantAccountIDs);
  	}
  	}, [shouldCalculateDistanceAmount, distanceRequestAmount, transactionID, currency, isTypeSplit, isPolicyExpenseChat, selectedParticipantsProp, transaction]);

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

• We should update the amount and splitShare before moving to IOURequestStepConfirmation from IOURequestStepParticipants.
• We can calculate the amount and split shares inside goToNextStep function in IOURequestStepParticipants.

        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]

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.

• The issue is that 0.00 is briefly displayed instead of the correct amount.

What is the root cause of that problem?

• The participant's amount is calculated and displayed based on this logic:
  

  App/src/components/MoneyRequestConfirmationList.tsx

  Line 494 in 31c301e

  amount={transaction?.splitShares?.[participantOption.accountID ?? -1]?.amount}

  
  If the value transaction?.splitShares?.[participantOption.accountID ?? -1]?.amount is undefined, the default amount shown is 0.00.

• In step 5, when the user navigates to the confirmation page for the first time, transaction?.splitShares is undefined, which results in an initial display of 0.00 for a brief moment.

• Once the transaction?.splitShares data is fetched and updated via:

App/src/components/MoneyRequestConfirmationList.tsx

Line 347 in 31c301e

IOU.setSplitShares(transaction, amount, currency, participantAccountIDs);

the correct amount is displayed afterward.

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

• To address this, we can add a loading indicator to the input where the participant's amount is displayed. This will indicate that the amount is being calculated. Thus, the following code:

App/src/components/MoneyRequestConfirmationList.tsx

Line 491 in 31c301e

rightElement: (

will be:

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

[garrettmknight]

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

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

PR is close to merge.

[parasharrajat]

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 #47370 (comment). This is not visible on the UI. Should we pursue the solution further to get the values in the first render? @jasperhuangg

[Krishna2323]

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]

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]

⚠️ 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]

In the process of fixing that blocker the PR was reverted #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]

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

• fix: [P2P Distance] Split - Participants amount displayed 0.00 briefly on confirmation screen. #47370

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

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

• @parasharrajat requires payment through NewDot Manual Requests
• @Krishna2323 requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Krishna2323]

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

[parasharrajat]

is there something pending on this issue? @Krishna2323 It looks like the PR was reapplied #51156

[Krishna2323]

@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]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Enable distance splits #42302
• [@parasharrajat] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/42302/files#r1818387886
• [@parasharrajat] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not needed
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

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]

Payment Summary:

• Contributor: @Krishna2323 $250 paid via upwork
• Contributor+: @parasharrajat $250 via NewDot

[melvin-bot]

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

[parasharrajat]

Payment requested as per #47100 (comment)