Skip to content

Fixing displaying the hold and unhold option in expense report #60436

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

Merged
marcaaron merged 16 commits into from
May 1, 2025
Merged

Fixing displaying the hold and unhold option in expense report #60436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9ff743f
fixing hold and unhold options in MoneyReportHeader.tsx
Tony-MK Apr 17, 2025
9817bb9
Holding all request reports..
Tony-MK Apr 17, 2025
8c2c1a5
typos...
Tony-MK Apr 17, 2025
3bae508
typos ...
Tony-MK Apr 17, 2025
2ac740b
updating imports in MoneyReportHeader.tsx
Tony-MK Apr 17, 2025
ac649ca
Merge branch 'main' of https://github.com/Expensify/App into fix/60111
Tony-MK Apr 22, 2025
749e16c
lint..
Tony-MK Apr 22, 2025
4a7de48
typechecks..
Tony-MK Apr 22, 2025
5c55981
prettier...
Tony-MK Apr 22, 2025
c30c710
Merge branch 'main' into fix/60111
Tony-MK Apr 22, 2025
e8b5f7a
Merge branch 'main' of https://github.com/Expensify/App into fix/60111
Tony-MK Apr 24, 2025
a3d384b
prettier...
Tony-MK Apr 24, 2025
675bfba
Merge branch 'main' into fix/60111
Tony-MK Apr 28, 2025
31cb9d0
lint..
Tony-MK Apr 28, 2025
857a4ec
Merge branch 'main' into fix/60111
Tony-MK Apr 30, 2025
4bf36e5
revert Expensify.tsx
Tony-MK Apr 30, 2025
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
2 changes: 1 addition & 1 deletion src/components/MoneyReportHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
const isPayAtEndExpense = isPayAtEndExpenseTransactionUtils(transaction);
const isArchivedReport = isArchivedReportWithID(moneyRequestReport?.reportID);
const [archiveReason] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${moneyRequestReport?.reportID}`, {selector: getArchiveReason, canBeMissing: true});
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${moneyRequestReport?.reportID}`, {canBeMissing: true});

const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${moneyRequestReport?.reportID}`, {canBeMissing: true});
const getCanIOUBePaid = useCallback(
(onlyShowPayElsewhere = false, shouldCheckApprovedState = true) =>
canIOUBePaidAction(moneyRequestReport, chatReport, policy, transaction ? [transaction] : undefined, onlyShowPayElsewhere, undefined, undefined, shouldCheckApprovedState),
Expand Down
30 changes: 22 additions & 8 deletions src/hooks/useSelectedTransactionsActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import {deleteMoneyRequest, unholdRequest} from '@libs/actions/IOU';
import {exportReportToCSV} from '@libs/actions/Report';
import Navigation from '@libs/Navigation/Navigation';
import {getIOUActionForTransactionID, getOriginalMessage, isDeletedAction, isMoneyRequestAction} from '@libs/ReportActionsUtils';
import {canDeleteCardTransactionByLiabilityType, canDeleteTransaction, isMoneyRequestReport as isMoneyRequestReportUtils} from '@libs/ReportUtils';
import {getTransaction, isOnHold} from '@libs/TransactionUtils';
import {canDeleteCardTransactionByLiabilityType, canDeleteTransaction, canHoldUnholdReportAction, isMoneyRequestReport as isMoneyRequestReportUtils} from '@libs/ReportUtils';
import {getTransaction} from '@libs/TransactionUtils';
import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import type {OriginalMessageIOU, Report, ReportAction, Session} from '@src/types/onyx';
Expand All @@ -25,14 +25,28 @@ function useSelectedTransactionsActions({report, reportActions, session, onExpor
return [];
}
const options = [];
const isMoneyRequestReport = isMoneyRequestReportUtils(report);
const selectedTransactions = selectedTransactionsID.map((transactionID) => getTransaction(transactionID)).filter((t) => !!t);

const anyTransactionOnHold = selectedTransactions.some(isOnHold);
const allTransactionOnHold = selectedTransactions.every(isOnHold);
const isReportReimbursed = report?.stateNum === CONST.REPORT.STATE_NUM.APPROVED && report?.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED;
const isMoneyRequestReport = isMoneyRequestReportUtils(report);
let canHoldTransactions = isMoneyRequestReport && !isReportReimbursed;
let canUnholdTransactions = isMoneyRequestReport;

selectedTransactions.forEach((selectedTransaction) => {
if (!selectedTransaction?.transactionID) {
canHoldTransactions = false;
canUnholdTransactions = false;
return true;
}
const iouReportAction = getIOUActionForTransactionID(reportActions, selectedTransaction.transactionID);
const {canHoldRequest, canUnholdRequest} = canHoldUnholdReportAction(iouReportAction);

canHoldTransactions = canHoldTransactions && canHoldRequest;
canUnholdTransactions = canUnholdTransactions && canUnholdRequest;

return !(canHoldTransactions || canUnholdTransactions);
Copy link
Contributor

@aldo-expensify aldo-expensify May 4, 2025

Choose a reason for hiding this comment

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

@Tony-MK What is this return value supposed to do? This callback is used in a forEach so the return is ignored... is this logic correct? Maybe what you wanted was to use some?

Copy link
Contributor Author

@Tony-MK Tony-MK May 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It should be correct because the return value is only used to stop iterating forEach when canHoldTransactions || canUnholdTransactions is false, meaning we can not hold or unhold all selected transactions.

So, forEach can stop early instead of iterating over all transactions twice, like when using some.

If there is a problem with this method, we can revert to using some.

Copy link
Contributor

@aldo-expensify aldo-expensify May 4, 2025

Choose a reason for hiding this comment

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

I don't think that is how forEach works. forEach ignores the value returned by the callback:

There is no way to stop or break a forEach() loop other than by throwing an exception. If you need such behavior, the forEach() method is the wrong tool.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

Copy link
Contributor Author

@Tony-MK Tony-MK May 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Your correct. So, sorry about that.

Should I open a PR to revert forEach to some?

Copy link
Contributor

@aldo-expensify aldo-expensify May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

no problem!

I'm not so sure really, using some just because it will stop when you return true from the callback and ignoring the return of selectedTransactions.some(...) doesn't sound so clean to me either. Maybe you should just keep the forEach but drop the returned values so the code doesn't look weird.

        selectedTransactions.forEach((selectedTransaction) => {
            if (!canHoldTransactions && !canHoldTransactions) {
                return;
            }
            if (!selectedTransaction?.transactionID) {
                canHoldTransactions = false;
                canUnholdTransactions = false;
                return;
            }
            const iouReportAction = getIOUActionForTransactionID(reportActions, selectedTransaction.transactionID);
            const {canHoldRequest, canUnholdRequest} = canHoldUnholdReportAction(iouReportAction);

            canHoldTransactions = canHoldTransactions && canHoldRequest;
            canUnholdTransactions = canUnholdTransactions && canUnholdRequest;
        });

I wouldn't care much for iterating the rest of the elements.. you can add something like

            if (!canHoldTransactions && !canHoldTransactions) {
                return;
            }

to make it fast, and it is also already inside a useMemo so this shouldn't happen all the time.

});

if (isMoneyRequestReport && !anyTransactionOnHold && selectedTransactions.length === 1 && !isReportReimbursed) {
if (canHoldTransactions) {
options.push({
text: translate('iou.hold'),
icon: Expensicons.Stopwatch,
Expand All @@ -46,7 +60,7 @@ function useSelectedTransactionsActions({report, reportActions, session, onExpor
});
}

if (isMoneyRequestReport && allTransactionOnHold && selectedTransactions.length === 1) {
if (canUnholdTransactions) {
options.push({
text: translate('iou.unhold'),
icon: Expensicons.Stopwatch,
Expand Down
7 changes: 1 addition & 6 deletions src/pages/Search/SearchMoneyRequestReportHoldReasonPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ function SearchMoneyRequestReportHoldReasonPage({route}: PlatformStackScreenProp
const {selectedTransactionsID, setSelectedTransactionsID} = useMoneyRequestReportContext();

const onSubmit = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.MONEY_REQUEST_HOLD_FORM>) => {
const firstTransactionID = selectedTransactionsID.at(0);
if (!firstTransactionID) {
return;
}

putOnHold(firstTransactionID, values.comment, reportID);
selectedTransactionsID.forEach((transactionID) => putOnHold(transactionID, values.comment, reportID));

setSelectedTransactionsID([]);
Navigation.goBack();
Expand Down
Loading