diff --git a/src/components/Search/SearchSelectionFooter.tsx b/src/components/Search/SearchSelectionFooter.tsx index 0daa952154c3..deda0dcb2623 100644 --- a/src/components/Search/SearchSelectionFooter.tsx +++ b/src/components/Search/SearchSelectionFooter.tsx @@ -60,15 +60,16 @@ function getTransactionCount(transactionKeys: string[], transactions: SelectedTr }, 0); } -function getTransactionTotal(transactions: SelectedTransactionInfo[]): number { - return transactions.reduce((total, transaction) => total - (transaction.groupAmount ?? -Math.abs(transaction.amount)), 0); -} - // The live default-currency figure a row contributes to the footer total (also what the footer falls back to before a // conversion arrives). The footer stamps each conversion against this value and compares it on every render, so an // inline edit that moves it is detected and the cached conversion is fetched again. +// Sources are expense-signed (the negation of the displayed amount), so callers sum them with `total - source`. function getEntrySource(entry: SelectedTransactionInfo): number { - return entry.groupAmount ?? -Math.abs(entry.amount); + return entry.groupAmount ?? -entry.displayAmount; +} + +function getTransactionTotal(transactions: SelectedTransactionInfo[]): number { + return transactions.reduce((total, transaction) => total - getEntrySource(transaction), 0); } // Every selected row needs a fresh cached conversion for the target currency before the selected total can be shown @@ -215,7 +216,7 @@ function SearchSelectionFooter({searchResults}: SearchSelectionFooterProps) { } const group: unknown = data[key]; if (group && typeof group === 'object' && 'total' in group && typeof group.total === 'number') { - sources[key] = -Math.abs(group.total); + sources[key] = -group.total; } } return sources; @@ -482,7 +483,7 @@ function SearchSelectionFooter({searchResults}: SearchSelectionFooterProps) { convertedAmount = convertedTransactions?.[transaction.transaction.transactionID]?.[selectedCurrency]; } } - return acc - (convertedAmount ?? transaction.groupAmount ?? -Math.abs(transaction.amount)); + return acc - (convertedAmount ?? getEntrySource(transaction)); }, 0); } @@ -500,7 +501,7 @@ function SearchSelectionFooter({searchResults}: SearchSelectionFooterProps) { } else if (transactionID) { convertedAmount = convertedTransactions?.[transactionID]?.[selectedCurrency]; } - return total - (convertedAmount ?? transaction.groupAmount ?? -Math.abs(transaction.amount)); + return total - (convertedAmount ?? getEntrySource(transaction)); }, 0) : 0; return { diff --git a/src/components/Search/selectionBuilders.ts b/src/components/Search/selectionBuilders.ts index 84def38f4db9..0ab0ff4c57ae 100644 --- a/src/components/Search/selectionBuilders.ts +++ b/src/components/Search/selectionBuilders.ts @@ -92,6 +92,7 @@ function mapTransactionItemToSelectedEntry({ reportID: item.reportID, policyID: item.policyID, amount: allowNegativeAmount ? amount : Math.abs(amount), + displayAmount: item.formattedTotal, groupAmount: item.groupAmount, currency: item.currency, isFromOneTransactionReport: isOneTransactionReport(item.report), @@ -121,6 +122,7 @@ function mapEmptyReportToSelectedEntry(item: TransactionReportGroupListItemType reportID: item.reportID, policyID: item.policyID ?? CONST.POLICY.ID_FAKE, amount: item.totalDisplaySpend ?? item.total ?? 0, + displayAmount: item.totalDisplaySpend ?? 0, currency, ...(currency ? {groupCurrency: currency} : {}), }, @@ -145,6 +147,7 @@ function mapEmptyReportToSelectedEntry(item: TransactionReportGroupListItemType reportID: item.reportID, policyID: item.policyID ?? CONST.POLICY.ID_FAKE, amount: item.total ?? 0, + displayAmount: item.total ?? 0, currency, ...(currency ? {groupCurrency: currency} : {}), }, diff --git a/src/components/Search/types.ts b/src/components/Search/types.ts index 0ad23ce0f847..90c76fe1ba32 100644 --- a/src/components/Search/types.ts +++ b/src/components/Search/types.ts @@ -69,9 +69,12 @@ type SelectedTransactionInfo = { /** The policyID tied to the report the transaction is reported on */ policyID: string | undefined; - /** The transaction amount */ + /** The transaction amount as a magnitude, used for bulk pay. Signed only on the reconcile path. */ amount: number; + /** The signed amount the row displays */ + displayAmount: number; + /** The transaction currency */ currency: string; diff --git a/src/components/TransactionItemRow/DataCells/TotalCell.tsx b/src/components/TransactionItemRow/DataCells/TotalCell.tsx index f5be661b5e64..e40a5310e8ec 100644 --- a/src/components/TransactionItemRow/DataCells/TotalCell.tsx +++ b/src/components/TransactionItemRow/DataCells/TotalCell.tsx @@ -12,13 +12,11 @@ import useThemeStyles from '@hooks/useThemeStyles'; import {convertToBackendAmount, convertToFrontendAmountAsString, sanitizeCurrencyCode} from '@libs/CurrencyUtils'; import {formatToParts} from '@libs/NumberFormatUtils'; import {parseFloatAnyLocale, roundToTwoDecimalPlaces} from '@libs/NumberUtils'; -import {isGroupPolicy} from '@libs/PolicyUtils'; -import {isExpenseReport, isInvoiceReport, shouldEnableNegative} from '@libs/ReportUtils'; -import {getAmount as getTransactionAmount, getCurrency as getTransactionCurrency, isDeletedTransaction, isExpenseUnreported, isScanning} from '@libs/TransactionUtils'; +import {getTransactionDisplayAmount, isInvoiceReport, shouldEnableNegative} from '@libs/ReportUtils'; +import {getCurrency as getTransactionCurrency, isExpenseUnreported, isScanning} from '@libs/TransactionUtils'; import CONST from '@src/CONST'; import type {Policy, Report} from '@src/types/onyx'; -import {isEmptyObject} from '@src/types/utils/EmptyObject'; import React, {useRef, useState} from 'react'; @@ -51,9 +49,7 @@ function TotalCell({shouldShowTooltip, transactionItem, canEdit, onSave, report, const effectiveReport = report ?? transactionItem.report; const effectivePolicy = policy ?? transactionItem.policy; - const isDeleted = isDeletedTransaction(transactionItem); - const isFromExpenseReport = (!isEmptyObject(effectiveReport) && isExpenseReport(effectiveReport)) || (isEmptyObject(effectiveReport) && isGroupPolicy(effectivePolicy)); - const amount = getTransactionAmount(transactionItem, isFromExpenseReport, transactionItem.reportID === CONST.REPORT.UNREPORTED_REPORT_ID, isDeleted); + const amount = getTransactionDisplayAmount(transactionItem, effectiveReport, effectivePolicy); let amountToDisplay = convertToDisplayString(amount, currency); if (isScanning(transactionItem)) { amountToDisplay = translate('iou.receiptStatusTitle'); diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 57f5d4641afb..c10ba6145e15 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -286,6 +286,7 @@ import { hasViolation, hasWarningTypeViolation, isManagedCardTransaction as isCardTransactionTransactionUtils, + isDeletedTransaction, isDemoTransaction, isDistanceRequest, isFetchingWaypointsFromServer, @@ -5075,6 +5076,20 @@ function getAvailableReportFields(report: OnyxEntry, policyReportFields: return fields.filter(Boolean) as PolicyReportField[]; } +function isTransactionFromExpenseReport(report: OnyxInputOrEntry, policy: OnyxInputOrEntry): boolean { + return isEmptyObject(report) ? isGroupPolicyPolicyUtils(policy) : isExpenseReport(report); +} + +/** + * Returns a transaction's amount with the sign it is displayed with. A transaction on an expense report, on a group + * policy with no report of its own, unreported, or deleted is stored with the opposite sign, so its stored amount is + * negated. Any other transaction returns its magnitude. + */ +function getTransactionDisplayAmount(transaction: OnyxInputOrEntry, report: OnyxInputOrEntry, policy: OnyxInputOrEntry): number { + const isFromTrackedExpense = transaction?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID; + return getTransactionAmount(transaction, isTransactionFromExpenseReport(report, policy), isFromTrackedExpense, !!transaction && isDeletedTransaction(transaction)); +} + /** * Gets transaction created, amount, currency, comment, and waypoints (for distance expense) * into a flat object. Used for displaying transactions and sending them in API commands @@ -5094,7 +5109,7 @@ function getTransactionDetails( } const report = getReportOrDraftReport(transaction?.reportID, undefined, 'report' in transaction ? transaction.report : undefined); - const isFromExpenseReport = (!isEmptyObject(report) && isExpenseReport(report)) || (isEmptyObject(report) && isGroupPolicyPolicyUtils(policy)); + const isFromExpenseReport = isTransactionFromExpenseReport(report, policy); return { created: getFormattedCreated(transaction, createdDateFormat, dateFnsLocale), @@ -14634,6 +14649,7 @@ export { isTeachersUniteReport, getTaskAssigneeChatOnyxData, getTransactionDetails, + getTransactionDisplayAmount, getTransactionReportName, getDisplayedReportID, getTransactionsWithReceipts, diff --git a/src/libs/SearchUIUtils.ts b/src/libs/SearchUIUtils.ts index 56f5b595274e..d25ea976aa65 100644 --- a/src/libs/SearchUIUtils.ts +++ b/src/libs/SearchUIUtils.ts @@ -168,6 +168,7 @@ import { getReportOrDraftReport, getReportStatusTooltipTranslation, getReportStatusTranslation, + getTransactionDisplayAmount, hasHeldExpenses, hasInvoiceReports, hasOnlyNonReimbursableTransactions, @@ -221,7 +222,6 @@ import { getTag, getTaxAmount, getTaxName, - getAmount as getTransactionAmount, getCreated as getTransactionCreatedDate, getMerchant as getTransactionMerchant, getTransactionViolations, @@ -1278,8 +1278,6 @@ function getTransactionItemCommonFormattedProperties( report: OnyxTypes.Report | undefined, translate: LocalizedTranslate, ): Pick { - const isExpenseReport = report?.type === CONST.REPORT.TYPE.EXPENSE; - const formattedFrom = temporaryGetDisplayNameOrDefault({passedPersonalDetails: from, translate, formatPhoneNumber}); // Sometimes the search data personal detail for the 'to' account might not hold neither the display name nor the login @@ -1289,8 +1287,8 @@ function getTransactionItemCommonFormattedProperties( formattedTo = temporaryGetDisplayNameOrDefault({passedPersonalDetails: getPersonalDetailsForAccountID(to?.accountID), translate, formatPhoneNumber}); } - const isDeleted = isDeletedTransaction(transactionItem); - const formattedTotal = getTransactionAmount(transactionItem, isExpenseReport, false, isDeleted); + // formattedTotal is the Amount column's sort key and holds the same signed value the row displays. + const formattedTotal = getTransactionDisplayAmount(transactionItem, report, policy); const date = transactionItem?.modifiedCreated ? transactionItem.modifiedCreated : transactionItem?.created; const merchant = getTransactionMerchant(transactionItem); const formattedMerchant = isInvalidMerchantValue(merchant) ? '' : merchant; diff --git a/tests/unit/Search/SearchBulkActionsButtonTest.tsx b/tests/unit/Search/SearchBulkActionsButtonTest.tsx index e935b8843561..211ee55dac2d 100644 --- a/tests/unit/Search/SearchBulkActionsButtonTest.tsx +++ b/tests/unit/Search/SearchBulkActionsButtonTest.tsx @@ -113,6 +113,7 @@ function makeTransaction(): SelectedTransactions[string] { reportID: 'report1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', }; } diff --git a/tests/unit/Search/SearchSelectionFooterTest.tsx b/tests/unit/Search/SearchSelectionFooterTest.tsx index c356272eb005..fd178d0d8499 100644 --- a/tests/unit/Search/SearchSelectionFooterTest.tsx +++ b/tests/unit/Search/SearchSelectionFooterTest.tsx @@ -104,6 +104,7 @@ function buildSelectedTransaction(currency: string, groupCurrency?: string, grou action: CONST.SEARCH.ACTION_TYPES.VIEW, policyID: undefined, amount: 100, + displayAmount: 100, currency, groupCurrency, groupAmount, @@ -166,6 +167,42 @@ describe('SearchSelectionFooter', () => { expect(mockCapturedFooterProps.current).toEqual(expect.objectContaining({count: 10, total: 36000, currency: CONST.CURRENCY.USD})); }); + it('nets a selected credit against a selected expense instead of summing their magnitudes', async () => { + mockSelectedTransactions.current = { + transaction1: {...buildSelectedTransaction(CONST.CURRENCY.USD), displayAmount: 10000}, + transaction2: {...buildSelectedTransaction(CONST.CURRENCY.USD), displayAmount: -10000}, + }; + + render(); + await waitForBatchedUpdates(); + + expect(mockCapturedFooterProps.current).toEqual(expect.objectContaining({count: 2, total: 0})); + }); + + it('nets a selected credit against a selected expense when the amounts differ', async () => { + mockSelectedTransactions.current = { + transaction1: {...buildSelectedTransaction(CONST.CURRENCY.USD), displayAmount: 10000}, + transaction2: {...buildSelectedTransaction(CONST.CURRENCY.USD), displayAmount: -4000}, + }; + + render(); + await waitForBatchedUpdates(); + + expect(mockCapturedFooterProps.current).toEqual(expect.objectContaining({count: 2, total: 6000})); + }); + + it('adds back an excluded credit rather than subtracting it from the server total', async () => { + // The server total already counts the credit as -$100, so dropping it from the selection raises the total. + mockSelectedTransactions.current = {}; + mockExcludedTransactions.current = {transaction1: {...buildSelectedTransaction(CONST.CURRENCY.USD), displayAmount: -10000}}; + mockAreAllMatchingItemsSelected.current = true; + + render(); + await waitForBatchedUpdates(); + + expect(mockCapturedFooterProps.current).toEqual(expect.objectContaining({count: 171, total: 46000, currency: CONST.CURRENCY.USD})); + }); + it("offers the user's live payment currency as the Reset target when there is no active workspace", async () => { // A fresh no-workspace account: the active policy is the personal policy, and the only selected expense // happens to be in a different currency (JPY) from the live payment currency (GBP). diff --git a/tests/unit/Search/SearchSelectionProviderTest.tsx b/tests/unit/Search/SearchSelectionProviderTest.tsx index dfad6d4729cf..b6594539c325 100644 --- a/tests/unit/Search/SearchSelectionProviderTest.tsx +++ b/tests/unit/Search/SearchSelectionProviderTest.tsx @@ -47,6 +47,7 @@ function buildSelected(...keys: string[]): SelectedTransactions { reportID: 'report_1', policyID: 'policy_1', amount: 100, + displayAmount: 100, currency: 'USD', }, ]), diff --git a/tests/unit/Search/SearchUIUtilsTest.ts b/tests/unit/Search/SearchUIUtilsTest.ts index ff9b66f284f5..9105180f81dc 100644 --- a/tests/unit/Search/SearchUIUtilsTest.ts +++ b/tests/unit/Search/SearchUIUtilsTest.ts @@ -6456,6 +6456,42 @@ describe('SearchUIUtils', () => { const item = sections.find((s) => s.transactionID === filterTestTxID); expect(item?.submitted).toBe(''); }); + + it('should keep the negative sign on formattedTotal for an unreported (tracked) credit', () => { + const data = makeFilterTestData({}, {reportID: CONST.REPORT.UNREPORTED_REPORT_ID, amount: 5000}); + const [sections] = callGetTransactionsSections(data); + const item = sections.find((s) => s.transactionID === filterTestTxID); + expect(item?.formattedTotal).toBe(-5000); + }); + + // Amounts are stored with the opposite sign, so these three rows render +$80.00, -$40.00 and +$10.00. + function makeAmountSortData() { + const baseTransaction = searchResults.data[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]; + return makeFilterTestData( + {}, + {reportID: CONST.REPORT.UNREPORTED_REPORT_ID, amount: -8000}, + { + [`${ONYXKEYS.COLLECTION.TRANSACTION}sort-credit`]: {...baseTransaction, transactionID: 'sort-credit', reportID: CONST.REPORT.UNREPORTED_REPORT_ID, amount: 4000}, + [`${ONYXKEYS.COLLECTION.TRANSACTION}sort-small`]: {...baseTransaction, transactionID: 'sort-small', reportID: CONST.REPORT.UNREPORTED_REPORT_ID, amount: -1000}, + }, + ); + } + + function getAmountSortedIDs(sortOrder: SortOrder) { + const [sections] = callGetTransactionsSections(makeAmountSortData()); + const rows = sections.filter((section) => [filterTestTxID, 'sort-credit', 'sort-small'].includes(section.transactionID)); + expect(rows).toHaveLength(3); + const sorted = SearchUIUtils.getSortedSections(CONST.SEARCH.DATA_TYPES.EXPENSE, rows, localeCompare, translateLocal, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT, sortOrder); + return sorted.map((item) => ('transactionID' in item ? item.transactionID : undefined)); + } + + it('should rank a credit below every positive expense when sorting the Amount column descending', () => { + expect(getAmountSortedIDs(CONST.SEARCH.SORT_ORDER.DESC)).toEqual([filterTestTxID, 'sort-small', 'sort-credit']); + }); + + it('should rank a credit above every positive expense when sorting the Amount column ascending', () => { + expect(getAmountSortedIDs(CONST.SEARCH.SORT_ORDER.ASC)).toEqual(['sort-credit', 'sort-small', filterTestTxID]); + }); }); describe('getReportSections filtering and edge cases', () => { diff --git a/tests/unit/Search/selectionBuildersTest.ts b/tests/unit/Search/selectionBuildersTest.ts new file mode 100644 index 000000000000..5cd1d66deada --- /dev/null +++ b/tests/unit/Search/selectionBuildersTest.ts @@ -0,0 +1,43 @@ +import type {TransactionGroupListItemType, TransactionReportGroupListItemType} from '@components/Search/SearchList/ListItem/types'; +import {mapEmptyReportToSelectedEntry} from '@components/Search/selectionBuilders'; + +import CONST from '@src/CONST'; + +import createMock from '../../utils/createMock'; + +describe('selectionBuilders', () => { + describe('mapEmptyReportToSelectedEntry', () => { + it('takes displayAmount from the report-signed total for a report row', () => { + // totalDisplaySpend is already negated for expense reports, so a credit report keeps its negative sign. + const item = createMock({ + keyForList: 'report1', + reportID: 'report1', + policyID: 'policy1', + currency: CONST.CURRENCY.USD, + groupedBy: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT, + transactions: [], + total: 10000, + totalDisplaySpend: -10000, + }); + + const [, entry] = mapEmptyReportToSelectedEntry(item); + + expect(entry.displayAmount).toBe(-10000); + }); + + it('takes displayAmount from the group total for a group row', () => { + const item = createMock({ + keyForList: `${CONST.SEARCH.GROUP_PREFIX}category1`, + reportID: undefined, + policyID: 'policy1', + currency: CONST.CURRENCY.USD, + transactions: [], + total: -4000, + }); + + const [, entry] = mapEmptyReportToSelectedEntry(item); + + expect(entry.displayAmount).toBe(-4000); + }); + }); +}); diff --git a/tests/unit/Search/useRowSelectionTest.tsx b/tests/unit/Search/useRowSelectionTest.tsx index a6acb762045b..9cf8ec54f2e0 100644 --- a/tests/unit/Search/useRowSelectionTest.tsx +++ b/tests/unit/Search/useRowSelectionTest.tsx @@ -44,6 +44,7 @@ function buildSelected(...keys: string[]): SelectedTransactions { reportID: 'report_1', policyID: 'policy_1', amount: 100, + displayAmount: 100, currency: 'USD', }; return acc; diff --git a/tests/unit/Search/useSyncSelectedReportsTest.tsx b/tests/unit/Search/useSyncSelectedReportsTest.tsx index 9c4b8f2a0650..275c23aacd67 100644 --- a/tests/unit/Search/useSyncSelectedReportsTest.tsx +++ b/tests/unit/Search/useSyncSelectedReportsTest.tsx @@ -54,6 +54,7 @@ function buildSelected(...keys: string[]): SelectedTransactions { reportID: 'report_1', policyID: 'policy_1', amount: 100, + displayAmount: 100, currency: 'USD', }; return acc; diff --git a/tests/unit/SearchActionsTest.ts b/tests/unit/SearchActionsTest.ts index 3d3cc5897a42..e9a86febd5e0 100644 --- a/tests/unit/SearchActionsTest.ts +++ b/tests/unit/SearchActionsTest.ts @@ -494,6 +494,7 @@ describe('getPayOption', () => { reportID, policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }; diff --git a/tests/unit/hooks/useSearchBulkActionsDeleteTest.ts b/tests/unit/hooks/useSearchBulkActionsDeleteTest.ts index 682fdc14d414..4e9d87f5410c 100644 --- a/tests/unit/hooks/useSearchBulkActionsDeleteTest.ts +++ b/tests/unit/hooks/useSearchBulkActionsDeleteTest.ts @@ -296,6 +296,7 @@ function makeSelectedTransaction(overrides: Partial { reportID: '1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }, @@ -299,6 +300,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }, @@ -336,6 +338,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }, @@ -378,6 +381,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }, @@ -394,6 +398,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '2', policyID: 'policy1', amount: 200, + displayAmount: 200, currency: 'USD', isFromOneTransactionReport: false, }, @@ -429,6 +434,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '1', policyID: 'policy1', amount: 100, + displayAmount: 100, currency: 'USD', isFromOneTransactionReport: false, }, @@ -445,6 +451,7 @@ describe('useSearchBulkActions - Download as PDF', () => { reportID: '2', policyID: 'policy1', amount: 200, + displayAmount: 200, currency: 'USD', isFromOneTransactionReport: false, }, diff --git a/tests/unit/hooks/useSearchBulkActionsDownloadReceiptsTest.ts b/tests/unit/hooks/useSearchBulkActionsDownloadReceiptsTest.ts index aa269f37c8cf..9d8a9fc1896b 100644 --- a/tests/unit/hooks/useSearchBulkActionsDownloadReceiptsTest.ts +++ b/tests/unit/hooks/useSearchBulkActionsDownloadReceiptsTest.ts @@ -189,6 +189,7 @@ function makeSelectedTransaction(overrides: Partial