[hold for payment 2024-08-26] [$250] Split - Unable to split expense with workspace via FAB

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.18-7
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): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Split expense > Manual
3. Enter amount > Next
4. Select a workspace > Next
5. Split the expense

Expected Result:

The expense is split in the workspace chat

Actual Result:

User is unable to split expense with workspace via FAB
App opens a report with RBR on LHN and the report shows infinite skeleton after splitting expense with workspace via FAB

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

Bug6566625_1723204349146.20240809_194957.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fde995cf07525892
• Upwork Job ID: 1821964572979581542
• Last Price Increase: 2024-08-09
• Automatic offers:
• hoangzinh | Reviewer | 103497009

Issue Owner

Current Issue Owner: @hoangzinh

[melvin-bot]

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

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

[lanitochka17]

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

[bernhardoj]

Edited by proposal-police: This proposal was edited at {current_timestamp}.

Proposal

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

Can't split bill with workspace from FAB.

What is the root cause of that problem?

When we split bill with a workspace from FAB, we create a new workspace chat even though there is already a workspace chat. The reason we create a new workspace is because the existing chat report ID below is empty, so getOrCreateOptimisticSplitChatReport decide to create a new report.

App/src/libs/actions/IOU.ts

Lines 3812 to 3814 in db669c4

	function getOrCreateOptimisticSplitChatReport(existingSplitChatReportID: string, participants: Participant[], participantAccountIDs: number[], currentUserAccountID: number) {
	// The existing chat report could be passed as reportID or exist on the sole "participant" (in this case a report option)
	const existingChatReportID = existingSplitChatReportID ?? participants[0].reportID ?? '';

Previously, the logic look like this:

const existingChatReportID = existingSplitChatReportID || participants[0].reportID

When we split bill from FAB, existingSplitChatReportID will be an empty string and if the participant is a workspace, then participants[0].reportID will contain the workspace reportID.

But now, we use ?? which won't fallback to participants[0].reportID because existingSplitChatReportID is an empty string. The logic is updated by this commit.

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

Based on the commit message, we don't want an undefined existingChatReportID which is possible if the participant is not a workspace.

const existingChatReportID = '' || undefined => undefined

That's why we fall back again to an empty string. But we shouldn't use ?? specifically for existingSplitChatReportID because it's an empty string and not undefined.

const existingChatReportID = '' ?? {reportID} ?? '' => ''

So, to fix this, we should change ?? to || back.

const existingChatReportID = existingSplitChatReportID || participants[0].reportID || '';

Btw, I think we should fall back to -1 or just let it be undefined again. If it's an empty string, then the report onyx key will be report_ which will fetch the whole report collection.

App/src/libs/actions/IOU.ts

Line 3817 in db669c4

let existingSplitChatReport = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${existingChatReportID}`];

I think the reason we don't want the reportID to be undefined (report_undefined) is that sometimes the invalid report exists in the storage with error data, without a reportID.

report_undefined: {
    errors: ...,
}

And because the report object is not undefined, the logic thinks that it found an existing report, even though it doesn't.

App/src/libs/actions/IOU.ts

Lines 3820 to 3828 in db669c4

	if (!existingSplitChatReport) {
	existingSplitChatReport = ReportUtils.getChatByParticipants(allParticipantsAccountIDs, undefined, participantAccountIDs.length > 1);
	}
	// We found an existing chat report we are done...
	if (existingSplitChatReport) {
	// Yes, these are the same, but give the caller a way to identify if we created a new report or not
	return {existingSplitChatReport, splitChatReport: existingSplitChatReport};
	}

To fix this, we can check for the reportID to make sure the report object is valid.

if (!existingSplitChatReport?.reportID) {
    existingSplitChatReport = ReportUtils.getChatByParticipants(allParticipantsAccountIDs, undefined, participantAccountIDs.length > 1);
}

[melvin-bot]

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

[melvin-bot]

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

[grgia]

@hoangzinh could you assess the proposal, I think we can assign @bernhardoj

[thienlnam]

@grgia Did you mean to remove the App blocker? Looks like this is a FE issue

[melvin-bot]

Current assignee @grgia is eligible for the DeployBlockerCash assigner, not assigning anyone new.

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

[hoangzinh]

Btw, I think we should fall back to -1 or just let it be undefined again. If it's an empty string, then the report onyx key will be report_ which will fetch the whole report collection.

@bernhardoj because we're getting the report from an array, therefore if it's an empty string, it won't return any data, will it?

App/src/libs/actions/IOU.ts

Lines 3816 to 3817 in 93dc1c9

	// Check if the report is available locally if we do have one
	let existingSplitChatReport = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${existingChatReportID}`];

Btw, I think it's safer if we check existingChatReportID exists before retrieving existingSplitChatReport from array. What do you think?

[bernhardoj]

because we're getting the report from an array

ReportConnection.getAllReports() doesn't return an array, it's an object.

Btw, I think it's safer if we check existingChatReportID exists before retrieving existingSplitChatReport from array. What do you think?

That's a good idea. I remember we did that before, but it was changed in this PR

[hoangzinh]

Oh my bad, it's an object.

I remember we did that before, but it was changed in this PR

It appears that there is a commit 1443b26 from that PR to fix disabling Eslint comment. Btw, because it's a DB, I'm happy with either way

@bernhardoj's proposal #47159 (comment) looks good to me

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @grgia is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[Beamanator]

@bernhardoj if you could please make this fix a priority, that would be great!

[Beamanator]

Actually if this was really caused by #42302, this shouldn't be a blocker anymore since #42302 is now in prod 😅

[melvin-bot]

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

[bernhardoj]

PR is ready

cc: @hoangzinh

[joekaufmanexpensify]

PR merged!

[joekaufmanexpensify]

Hmm, automation did not work here. This went out to prod over a week ago, so payment is due now. Will tackle payment in the next day or so!

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: Enable distance splits #42302
• 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#r1732660564
• 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: 🚫 No, because it was a DB, I think we already have a regression test for this bug.

[joekaufmanexpensify]

Sounds good. TY!

[joekaufmanexpensify]

All set to issue payment here. We need to pay:

• @hoangzinh - $250 for C+ via Upwork.
• @bernhardoj - $250 for PR via NewDot.

[joekaufmanexpensify]

@hoangzinh $250 sent and contract ended!

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

Closing as only thing left here is for @bernhardoj to request payment via NewDot!

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj