[HOLD for payment 2024-12-19] [Search v1.2] - Different approval behavior when approving one held expense vs multiple held expenses

[IuliiaHerets]

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.66-0
Reproducible in staging?: Y
Reproducible in production?: N/A - new feature, doesn't exist in prod
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• Add approvals feature is enabled.
• Admin is the approver.

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Submit an expense and hold the expense.
4. Go to Search.
5. Click Approve button on the expense.
6. Note that it opens expense RHP and the approval can only be done from the expense RHP.
7. Go back to workspace chat.
8. Submit another expense (so there are two expenses in the same report, with one held and the other unheld).
9. Go to Search > Outstanding.
10. Click Approve.

Expected Result:

App should open expense report RHP so that approval can be done by approving the pending amount or all amount.

Actual Result:

Expense report with held expense can be approved without the confirmation modal in Search.

There is inconsistency when approving only one held expense and approving two expenses with different held status.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6674042_1732360026672.20241123_190310.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

💬 A slack conversation has been started in #expensify-open-source

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

[gijoe0295]

Proposal

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

Expense report with held expense can be approved without the confirmation modal in Search.

What is the root cause of that problem?

We're checking if the report has any held transactions to open the report RHP:

App/src/libs/actions/Search.ts

Lines 60 to 64 in a266c4c

	const hasHeldExpense = ReportUtils.hasHeldExpenses('', allReportTransactions);
	if (hasHeldExpense) {
	goToItem();
	return;
	}

However, allReportTransactions are not computed correctly:

App/src/libs/actions/Search.ts

Lines 52 to 56 in a266c4c

	const allReportTransactions = (
	isReportListItemType(item)
	? Object.entries(data)
	.filter(([itemKey, value]) => itemKey.startsWith(ONYXKEYS.COLLECTION.REPORT) && (value as SearchTransaction)?.reportID === item.reportID)
	.map((report) => report[1])

The above logic does not get the transactions from the report item but just get the report data itself.

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

For item of type ReportListItem, we already include all its transactions inside item.transactions.

const allReportTransactions = isReportListItemType(item) ? item.transactions : [data[`${ONYXKEYS.COLLECTION.TRANSACTION}${item.transactionID}`]];

[allgandalf]

@gijoe0295 were you able to find the offending PR ?

[mountiny]

cc @luacmartins can you have a look please

I dont think this is a blocker as the feature works its just a different UX to discuss

[luacmartins]

I agree it doesn't need to be a blocker. I'll take a look at it since my PR introduced this feature

[trjExpensify]

Just so I'm following along:

• If you click Approve on a single-expense report when the expense is held, it opens the RHP.
• If you click Approve on a multi-expense report when one of the expenses is held, it approves the report not opens the RHP.

So the second one is the bug, right?

[luacmartins]

Yea, that second one should also open the RHP

[melvin-bot]

@trjExpensify, @luacmartins, @rushatgabhane 10 days overdue. Is anyone even seeing these? Hello?

[melvin-bot]

@trjExpensify, @luacmartins, @rushatgabhane 12 days overdue now... This issue's end is nigh!

[trjExpensify]

👋 @rushatgabhane checklist time, please. Thanks!

[trjExpensify]

Same melv, awaiting the checklist.

[rushatgabhane]

1. 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: Show search action buttons v2 #52441

2. If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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 critical. just a consistency issue

3. If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. N.A. because it is a small inconsistency issue that was caused by the original implementation. The bug is unlikely to occur. So I don't think it is valuable to add a regression test here.

[luacmartins]

Nice, so just payment missing now?

[melvin-bot]

@trjExpensify, @luacmartins, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[trjExpensify]

If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. N.A.

Bit of a bug bear of mine, but how is "not applicable" a viable response to the question here? Please do expand on the reason why you don't think we need a regression test for this particular issue.

@luacmartins has a regression test been added for this feature elsewhere?

[trjExpensify]

Payment summary as follows:

• $250 to @rushatgabhane for the C+ review

[rushatgabhane]

updated the regression test response - #53012 (comment)

[luacmartins]

@luacmartins has a regression test been added for this feature elsewhere?

I'm not sure if we're covering this in TCs yet. @IuliiaHerets can you confirm if this bug report came from a TC or just exploration?

[trjExpensify]

CC: @kavimuru @isagoico @mvtglobally as well on that Q!

[IuliiaHerets]

@luacmartins Tester confirmed that find this issue by eploratory, not follow some TR step or PR

[luacmartins]

Thanks! @IuliiaHerets can we add the OP test steps to TR please?

[trjExpensify]

@luliiaHerets any progress on that?

[melvin-bot]

@trjExpensify, @luacmartins, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[trjExpensify]

I've added them to a GH for applause, they can confirm if they've been added there and close, or add them.

We're done here!