[HOLD for payment 2023-11-22] [Payment due: 17 Nov] [$500] Dev: amount does not appears after split bill in offline mode

[kbecciv]

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

---

Action Performed:

1. Go offline
2. Click on FAB icon and select request money
3. Enter amount and select users for split bill
4. Split the money which will redirects to report screen
5. Check amount

Expected Result:

Only owes: appears

Actual Result:

Should show amount

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: Dev 1.3.74-2
Reproducible in staging?: n
Reproducible in production?: n
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-09-26.at.3.01.04.PM.mov

Screen.Recording.2023-09-26.at.2.56.57.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695720688727649

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

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

[studentofcoding]

Proposal

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

Dev: amount does not appears after split bill in offline mode

What is the root cause of that problem?

On MoneyRequestPreview component, specifically in the getDisplayAmountText function. We didn't ensure the function correctly retrieves and displays the split bill amounts even when the device is offline.

Here is the relevant code block:

    const getDisplayAmountText = () => {
        if (isDistanceRequest) {
            return CurrencyUtils.convertToDisplayString(TransactionUtils.getAmount(props.transaction), props.transaction.currency);
        }

        if (isScanning) {
            return props.translate('iou.receiptScanning');
        }

        return CurrencyUtils.convertToDisplayString(requestAmount, requestCurrency);
    };

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

We should ensure that this function can correctly retrieve and display the split bill amounts even when the device is offline, with checking if requestAmount and requestCurrency are not null or undefined.

const getDisplayAmountText = () => {
    if (isDistanceRequest) {
        return CurrencyUtils.convertToDisplayString(TransactionUtils.getAmount(props.transaction), props.transaction.currency);
    }

    if (isScanning) {
        return props.translate('iou.receiptScanning');
    }

    // Check if requestAmount and requestCurrency are not null or undefined
    if (requestAmount && requestCurrency) {
        return CurrencyUtils.convertToDisplayString(requestAmount, requestCurrency);
    }
};

What alternative solutions did you explore? (Optional)

None

Result

amount.mp4

[tienifr]

Proposal

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

What is the root cause of that problem?

In here, when the oneOnOneChatReport.iouReportID is undefined, it's retrieving the report_undefined key, which exists in allReports with the default value

{
    isLoadingReportActions: true,
    isLoadingMoreReportActions: false,
    reportName: 'Chat Report',
}

So it's unable to generate (and later find) an optimistic IOU report here.

So it will show only owes: in the UI.

The reason why allReports contains report_undefined is that sometimes the openReport is called with an undefined reportID value, so it saves a report_undefined key in Onyx.

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

1. Fix openReport so that it will early return if the reportID is falsy

if (!reportID) {
    return;
}

This will fix it for all occurrences of openReport, an alternative is to fix it at all the places where we call openReport to make sure we don't call it with falsy reportID

2. We can make this condition more robust by returning undefined if the reportID is undefined, so it will never query for such invalid report_undefined key in Onyx. We can extract it to another util function to getReport and reuse it. Or we can fix and reuse this getReport

What alternative solutions did you explore? (Optional)

We might also want to guard against potential invalid values of reportID like 'null' as well.

[melvin-bot]

Triggered auto assignment to @sophiepintoraetz (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[sophiepintoraetz]

Not overdue - just waiting on proposal review!

[sophiepintoraetz]

Bump @situchan

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[sophiepintoraetz]

Hey @adelekennedy, can you please baby sit this issue for me while I'm OOO till the 17 Oct? I've asked for another C+ volunteer here to review the issue, as it seems like @situchan can't get to this.

[situchan]

I will provide update by Monday

[adelekennedy]

@situchan will provide update today!

[melvin-bot]

@sophiepintoraetz @adelekennedy @situchan 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!

[tienifr]

is that constant repro step?

Yes

And can you also share video after fix?

Here you are

Screen.Recording.2023-10-30.at.16.22.08.mp4

[situchan]

If same root cause, makes sense to fix it here.
@johnmlee101 will decide whether to continue PR or not

[johnmlee101]

To clarify, the fix is the exact same as previously?

[tienifr]

@johnmlee101 Yes, can you pls open the GH again and I can raise the PR soon

[tienifr]

@johnmlee101 @sophiepintoraetz The fixed PR is merged and deployed some time ago, can we proceed with next step here? cc @situchan

[sophiepintoraetz]

Yes, payment will be subject to the usual 7 day regression period, so we're still a couple of days out.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix: Amount not show when split bill in offline mode #29953

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @situchan requires payment
• @tienifr requires payment
• @gadhiyamanan requires payment

[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:

• [@situchan] The PR that introduced the bug has been identified. Link to the PR:
• [@situchan] 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:
• [@situchan] 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:
• [@situchan] Determine if we should create a regression test for this bug.
• [@situchan] 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.
• [@sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sophiepintoraetz]

Offers sent - @situchan - I'll complete payment for yours once you've done the checklist, thank you!

[melvin-bot]

@johnmlee101, @sophiepintoraetz, @situchan, @tienifr Huh... This is 4 days overdue. Who can take care of this?

[situchan]

• The PR that introduced the bug has been identified. Link to the PR: Implement report approvals #24639
• 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/24639/files#r1406299214
• 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: N/A
• Determine if we should create a regression test for this bug.
• 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 Proposal

1. Invite new member to WS
2. Go offline
3. Press FAB > Request money > Manual
4. Enter amount > Select invited member > Split
5. Verify that split request is created and amount shows in split preview

[sophiepintoraetz]

All paid here.