Skip to content

Exclude all-held reports from the Inbox to-do, keep them for whoever placed the hold #96178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
allgandalf wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Exclude all-held reports from the Inbox to-do, keep them for whoever placed the hold #96178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
435e130
Exclude all-held reports from the Inbox to-do unless the current user…
allgandalf Jul 15, 2026
8fdf6ab
Test the Inbox all-held exclusion and the hold-placer exception
allgandalf Jul 15, 2026
8c399fa
Keep all-held reports in the Approve and Pay to-dos for the hold placer
allgandalf Jul 15, 2026
5bbd22f
Test the to-do bucket exception for the hold placer
allgandalf Jul 15, 2026
a1ef2f1
Test the to-do count exception for the hold placer
allgandalf Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4481,11 +4481,21 @@ function getReasonAndReportActionThatRequiresAttention(
const transactions = getReportTransactions(iouReportID);
const hasOnlyPendingTransactions = transactions.length > 0 && transactions.every((t) => isPending(t));

const iouReportActions = getAllReportActions(iouReportID);
const currentUserPlacedHold = didCurrentUserPlaceHoldOnReportExpense(iouReportActions, transactions, currentUserAccountID);

@github-actions github-actions Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-2 (docs)

didCurrentUserPlaceHoldOnReportExpense is the expensive part of this block — it iterates every report action and, for each money-request action, scans reportTransactions.find(...) plus a getReportAction lookup (roughly O(actions × transactions)). It runs unconditionally here, but its result (currentUserPlacedHold) is only ever consumed when hasOnlyHeldExpenses(transactions) is true (the short-circuit && on the next line). Since getReasonAndReportActionThatRequiresAttention is called across the LHN/report-attributes derivation for many reports, this overhead is paid on every report even when no expense is held.

Gate the expensive call behind the cheap hasOnlyHeldExpenses(transactions) check so it only runs when it can actually change the outcome:

const hasOnlyHeld = hasOnlyHeldExpenses(transactions);
const currentUserPlacedHold = hasOnlyHeld && didCurrentUserPlaceHoldOnReportExpense(getAllReportActions(iouReportID), transactions, currentUserAccountID);
const isExcludedForHeldExpenses = hasOnlyHeld && (actionBadge === CONST.REPORT.ACTION_BADGE.SUBMIT ? true : !currentUserPlacedHold);

Reviewed at: a1ef2f1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

// An all-held report can't move to its next state, so it isn't a to-do. Keep it only for an Approve or Pay report
// where the current user placed a hold, since only they can unhold it. Submit stays excluded even then, because on
// an open report only the owner can hold, so the exception would apply to every all-held report and defeat it.
const isExcludedForHeldExpenses = hasOnlyHeldExpenses(transactions) && (actionBadge === CONST.REPORT.ACTION_BADGE.SUBMIT ? true : !currentUserPlacedHold);

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude fallback submit holds without a badge

When the chat reaches the fallback path above (hasOutstandingChildRequest/iouReportID but no REPORT_PREVIEW action loaded), actionBadge is undefined. For an open submit report whose only expense is held by the current owner, currentUserPlacedHold is true, so this expression does not exclude it and hasValidIOUAction still raises a GBR for the policy expense chat, even though submit all-held reports are supposed to stay excluded (and TodosUtils excludes them unconditionally). This leaves the Inbox/LHN to-do inconsistent for the no-preview fallback case already handled by this function.

Useful? React with 👍 / 👎.


// Has a child report that is awaiting action (e.g. approve, pay, add bank account) from current user.
// A report whose only expenses are pending Expensify Card transactions can't be actioned until they post, so it
// shouldn't demand attention even when the chat still carries an outstanding-child flag.
const hasStaleChildRequest = isTripRoom(optionOrReport) && (optionOrReport.transactionCount ?? 0) === 0;
const hasValidIOUAction = ((optionOrReport.hasOutstandingChildRequest === true && !hasStaleChildRequest) || iouReportActionToApproveOrPay?.reportActionID) && !hasOnlyPendingTransactions;
const hasValidIOUAction =
((optionOrReport.hasOutstandingChildRequest === true && !hasStaleChildRequest) || iouReportActionToApproveOrPay?.reportActionID) &&
!hasOnlyPendingTransactions &&
!isExcludedForHeldExpenses;

if (actionTypeForAssigneeToComplete) {
const isAssigneeExpenseAction = actionTypeForAssigneeToComplete === CONST.REPORT.ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE.EXPENSE;
Expand Down Expand Up @@ -4710,9 +4720,24 @@ function isReportFieldOfTypeTitle(reportField: OnyxEntry<PolicyReportField>): bo
/**
* Check if Report has any held expenses
*/
function isHoldCreator(transaction: OnyxEntry<Transaction>, reportID: string | undefined): boolean {
function isHoldCreator(transaction: OnyxEntry<Transaction>, reportID: string | undefined, currentUserAccountID?: number): boolean {
const holdReportAction = getReportAction(reportID, `${transaction?.comment?.hold ?? ''}`);
return isActionCreator(holdReportAction);
return isActionCreator(holdReportAction, currentUserAccountID);
}

/**
* Whether the current user placed the hold on at least one of the report's held expenses. Used to keep an
* all-held report in that user's to-do queue, since only the person who placed a hold can remove it.
*/
function didCurrentUserPlaceHoldOnReportExpense(reportActions: OnyxEntry<ReportActions>, reportTransactions: Transaction[], currentUserAccountID?: number): boolean {
return Object.values(reportActions ?? {}).some((action) => {
if (!isMoneyRequestAction(action)) {
return false;
}
const transactionID = getOriginalMessage(action)?.IOUTransactionID;
const transaction = reportTransactions.find((reportTransaction) => reportTransaction.transactionID === transactionID);
return !!transaction && isOnHoldTransactionUtils(transaction) && isHoldCreator(transaction, action.childReportID, currentUserAccountID);
});
}

/**
Expand Down Expand Up @@ -13717,6 +13742,7 @@ export {
isGroupChatAdmin,
isHarvestCreatedExpenseReport,
isHoldCreator,
didCurrentUserPlaceHoldOnReportExpense,
isIOUOwnedByCurrentUser,
isIOUReport,
isIOUReportUsingReport,
Expand Down
33 changes: 29 additions & 4 deletions src/libs/TodosUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type {SearchKey} from './SearchUIUtils';

import {getLoginByAccountID} from './PersonalDetailsUtils';
import {isApproveAction, isExportAction, isPrimaryPayAction, isSubmitAction} from './ReportPrimaryActionUtils';
import {hasOnlyHeldExpenses, hasOnlyNonReimbursableTransactions} from './ReportUtils';
import {didCurrentUserPlaceHoldOnReportExpense, hasOnlyHeldExpenses, hasOnlyNonReimbursableTransactions} from './ReportUtils';

type CreateTodosReportsAndTransactionsParams = {
/** Every report, keyed by report Onyx key - iterated to find the expense reports that belong in a to-do bucket */
Expand Down Expand Up @@ -78,6 +78,9 @@ type TodoBucketContext = {
/** Whether every transaction on the report is on hold - precomputed once so held reports are excluded from submit/approve/pay */
allExpensesHeld: boolean;

/** Whether the current user placed the hold on one of the report's expenses - keeps an all-held report in their approve/pay to-do */
currentUserPlacedHold: boolean;

/** The report owner's login, resolved from `ownerAccountID` - the submit predicate matches it against the submitter */
ownerLogin: string | undefined;

Expand All @@ -99,13 +102,25 @@ type TodoBucketContext = {
function reportMatchesTodoBucket(
searchKey: SearchKey,
report: Report,
{policy, reportNameValuePair, reportTransactions, reportMetadata, allReportActions, allExpensesHeld, ownerLogin, bankAccountList, currentUserAccountID, login}: TodoBucketContext,
{
policy,
reportNameValuePair,
reportTransactions,
reportMetadata,
allReportActions,
allExpensesHeld,
currentUserPlacedHold,
ownerLogin,
bankAccountList,
currentUserAccountID,
login,
}: TodoBucketContext,
): boolean {
switch (searchKey) {
case CONST.SEARCH.SEARCH_KEYS.SUBMIT:
return isSubmitAction(report, reportTransactions, reportMetadata, ownerLogin, policy, reportNameValuePair, undefined, login, currentUserAccountID) && !allExpensesHeld;
case CONST.SEARCH.SEARCH_KEYS.APPROVE:
return isApproveAction(report, reportTransactions, currentUserAccountID, reportMetadata, policy) && !allExpensesHeld;
return isApproveAction(report, reportTransactions, currentUserAccountID, reportMetadata, policy) && (!allExpensesHeld || currentUserPlacedHold);
case CONST.SEARCH.SEARCH_KEYS.PAY:
return (
isPrimaryPayAction({
Expand All @@ -118,7 +133,7 @@ function reportMatchesTodoBucket(
reportNameValuePairs: reportNameValuePair,
}) &&
!hasOnlyNonReimbursableTransactions(report.reportID, reportTransactions) &&
!allExpensesHeld
(!allExpensesHeld || currentUserPlacedHold)
);
case CONST.SEARCH.SEARCH_KEYS.EXPORT: {
const reportActions = Object.values(allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`] ?? []);
Expand Down Expand Up @@ -170,6 +185,11 @@ function createTodosReportsAndTransactions({
reportMetadata: allReportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`],
allReportActions,
allExpensesHeld: hasOnlyHeldExpenses(reportTransactions),
currentUserPlacedHold: didCurrentUserPlaceHoldOnReportExpense(
allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`],
reportTransactions,
currentUserAccountID,
),
ownerLogin: getLoginByAccountID(report.ownerAccountID, personalDetailsList),
bankAccountList,
currentUserAccountID,
Expand Down Expand Up @@ -226,6 +246,11 @@ function getTodoReportsForSearchKey(
reportMetadata: allReportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`],
allReportActions,
allExpensesHeld: hasOnlyHeldExpenses(reportTransactions),
currentUserPlacedHold: didCurrentUserPlaceHoldOnReportExpense(
allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`],
reportTransactions,
currentUserAccountID,
),
ownerLogin: getLoginByAccountID(report.ownerAccountID, personalDetailsList),
bankAccountList,
currentUserAccountID,
Expand Down
119 changes: 118 additions & 1 deletion tests/unit/ReportUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3661,6 +3661,90 @@ describe('ReportUtils', () => {
expect(requiresAttentionFromCurrentUser(policyExpenseChat, currentUserEmail, currentUserAccountID)).toBe(false);
});

describe('when the outstanding child expense is all on hold', () => {
const expenseReportID = '7201';
const transactionThreadReportID = '7202';
const transactionID = '7201';
const moneyRequestActionID = 'mr_7201';
const HOLD_ACTION_ID = 'hold_7201';
const otherUserAccountID = 99;

// Seeds an all-held expense report awaiting the current user, plus the money-request action and the thread's
// HOLD action (whose actor is the holder), so the derivation can resolve who placed the hold.
const seedHeldChildExpense = async (holderAccountID: number) => {
const expenseReport = {
...LHNTestUtils.getFakeReport(),
reportID: expenseReportID,
policyID: '1',
ownerAccountID: otherUserAccountID,
managerID: currentUserAccountID,
type: CONST.REPORT.TYPE.EXPENSE,
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS_NUM.SUBMITTED,
};

const policyExpenseChat = {
...createPolicyExpenseChat(201, true),
policyID: '1',
ownerAccountID: currentUserAccountID,
hasOutstandingChildRequest: true,
iouReportID: expenseReportID,
};

const heldTransaction = {
...createRandomTransaction(7201),
transactionID,
reportID: expenseReportID,
status: CONST.TRANSACTION.STATUS.POSTED,
comment: {hold: HOLD_ACTION_ID},
};

const moneyRequestAction: ReportAction = {
reportActionID: moneyRequestActionID,
actionName: CONST.REPORT.ACTIONS.TYPE.IOU,
created: '2024-01-01 00:00:00.000',
actorAccountID: otherUserAccountID,
childReportID: transactionThreadReportID,
originalMessage: {
IOUTransactionID: transactionID,
type: CONST.IOU.REPORT_ACTION_TYPE.CREATE,
amount: 100,
currency: 'USD',
},
};

const holdAction: ReportAction = {
reportActionID: HOLD_ACTION_ID,
actionName: CONST.REPORT.ACTIONS.TYPE.HOLD,
created: '2024-01-01 00:00:00.000',
actorAccountID: holderAccountID,
};

await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}1`, {reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES});
await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${expenseReportID}`, expenseReport);
await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, heldTransaction);
await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`, {[moneyRequestActionID]: moneyRequestAction});
await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`, {[HOLD_ACTION_ID]: holdAction});
await waitForBatchedUpdates();

return policyExpenseChat;
};

it('does not require attention when another user placed the hold', async () => {
const policyExpenseChat = await seedHeldChildExpense(otherUserAccountID);

// An all-held report can't move to its next state, so it isn't a to-do when someone else placed the hold.
expect(requiresAttentionFromCurrentUser(policyExpenseChat, currentUserEmail, currentUserAccountID)).toBe(false);
});

it('still requires attention when the current user placed the hold', async () => {
const policyExpenseChat = await seedHeldChildExpense(currentUserAccountID);

// Only the person who placed the hold can remove it, so the report stays in their to-do queue.
expect(requiresAttentionFromCurrentUser(policyExpenseChat, currentUserEmail, currentUserAccountID)).toBe(true);
});
});

it('returns true for expense report awaiting user payment/reimbursement', async () => {
const report = {
...LHNTestUtils.getFakeReport(),
Expand Down Expand Up @@ -14731,7 +14815,7 @@ describe('ReportUtils', () => {
});
});

it('should surface a GBR for admin with held expenses requiring approval or payment and avoid showing an RBR', async () => {
it('should surface a GBR for the admin who placed the hold on an all-held report requiring approval, and avoid showing an RBR', async () => {
await Onyx.clear();

const adminAccountID = currentUserAccountID;
Expand All @@ -14740,6 +14824,8 @@ describe('ReportUtils', () => {
const expenseReportID = 'expense-hold';
const transactionID = 'transaction-hold';
const holdReportActionID = 'hold-action';
const transactionThreadReportID = 'transaction-thread-hold';
const moneyRequestActionID = 'money-request-action-hold';

const policy1: Policy = {
id: policyID,
Expand Down Expand Up @@ -14804,6 +14890,30 @@ describe('ReportUtils', () => {
childReportID: expenseReportID,
};

// The current user (admin) placed the hold, so the all-held report stays in their to-do queue (GBR) - only the
// person who placed a hold can unhold it. The money-request action links the held transaction to the thread
// that carries the HOLD action, so the derivation can resolve who placed the hold.
const moneyRequestAction: ReportAction = {
reportActionID: moneyRequestActionID,
actionName: CONST.REPORT.ACTIONS.TYPE.IOU,
created: '2024-01-01 00:00:00.000',
actorAccountID: employeeAccountID,
childReportID: transactionThreadReportID,
originalMessage: {
IOUTransactionID: transactionID,
type: CONST.IOU.REPORT_ACTION_TYPE.CREATE,
amount: 12345,
currency: CONST.CURRENCY.USD,
},
};

const holdAction: ReportAction = {
reportActionID: holdReportActionID,
actionName: CONST.REPORT.ACTIONS.TYPE.HOLD,
created: '2024-01-01 00:00:00.000',
actorAccountID: adminAccountID,
};

const transactionViolationsKey = `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}` as OnyxKey;
const transactionViolationsCollection: OnyxCollection<TransactionViolation[]> = {
[transactionViolationsKey]: [
Expand All @@ -14822,6 +14932,12 @@ describe('ReportUtils', () => {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`, {
[reportPreviewAction.reportActionID]: reportPreviewAction,
}),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`, {
[moneyRequestAction.reportActionID]: moneyRequestAction,
}),
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`, {
[holdAction.reportActionID]: holdAction,
}),
Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction),
Onyx.merge(transactionViolationsKey, transactionViolationsCollection[transactionViolationsKey]),
]);
Expand All @@ -14841,6 +14957,7 @@ describe('ReportUtils', () => {
draftComment: '',
isReportArchived: undefined,
conciergeReportID: undefined,
currentUserAccountID: adminAccountID,
});

expect(reason).toBe(CONST.REPORT_IN_LHN_REASONS.HAS_GBR);
Expand Down
Loading
Loading