From 90880c4e2f7dc01a4ec016a6b35646d769347878 Mon Sep 17 00:00:00 2001 From: "Yauheni Horbach (via MelvinBot)" Date: Wed, 12 Aug 2026 10:44:33 +0000 Subject: [PATCH 1/8] Support 'Select all matching' when moving expenses to a report Send the search jsonQuery + hash to CHANGE_TRANSACTIONS_REPORT so the backend moves all matching unreported expenses, not just the loaded page. Co-authored-by: Yauheni Horbach --- config/eslint/eslint.seatbelt.tsv | 1 - src/CONST/index.ts | 2 + src/components/Search/SearchQueryProvider.tsx | 33 +++- src/hooks/useSearchBulkActions.ts | 93 +++++---- .../ChangeTransactionsReportParams.ts | 2 + src/libs/actions/Transaction.ts | 48 ++++- .../Search/SearchTransactionsChangeReport.tsx | 26 ++- .../request/step/IOURequestStepUpgrade.tsx | 18 +- tests/unit/TransactionTest.ts | 177 ++++++++++++++++++ 9 files changed, 351 insertions(+), 49 deletions(-) diff --git a/config/eslint/eslint.seatbelt.tsv b/config/eslint/eslint.seatbelt.tsv index 71c23ede5e25..78093a89919c 100644 --- a/config/eslint/eslint.seatbelt.tsv +++ b/config/eslint/eslint.seatbelt.tsv @@ -378,7 +378,6 @@ "../../src/components/Search/SearchPageHeader/SearchSaveButton.tsx" "no-restricted-imports" 1 "../../src/components/Search/SearchPageHeader/useSearchPageInput.tsx" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/components/Search/SearchPageHeader/useSearchPageInput.tsx" "react-hooks/set-state-in-effect" 2 -"../../src/components/Search/SearchQueryProvider.tsx" "@typescript-eslint/no-unsafe-type-assertion" 2 "../../src/components/Search/SearchRouter/SearchRouter.tsx" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/components/Search/SearchRouter/SearchRouterUtils.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 "../../src/components/Search/SearchRouter/buildSubstitutionsMap.ts" "@typescript-eslint/no-unsafe-type-assertion" 1 diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 423a5b21e1d0..c26f13a59293 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -6786,6 +6786,8 @@ const CONST = { RESULTS_PAGE_SIZE: 50, EXITING_ANIMATION_DURATION: 200, ME: 'me', + // Null byte can't appear in a query, so it's safe to join query parts with it + QUERY_PARAMS_SEPARATOR: '\x00', DATA_TYPES: { EXPENSE: 'expense', EXPENSE_REPORT: 'expense-report', diff --git a/src/components/Search/SearchQueryProvider.tsx b/src/components/Search/SearchQueryProvider.tsx index e045589f5336..91a4559190ff 100644 --- a/src/components/Search/SearchQueryProvider.tsx +++ b/src/components/Search/SearchQueryProvider.tsx @@ -26,23 +26,40 @@ type SearchQueryProviderProps = { children: React.ReactNode; }; -function selectSearchQueryParam(state: NavigationState | undefined) { +/** Joins `q` and `rawQuery` so they can't drift apart: restoring only `q` would drop `rawFilterList` from the + * query JSON while keeping the same hash. */ +function selectSearchQueryParams(state: NavigationState | undefined) { const focused = getDeepestFocusedScreen(state); - return focused?.name === SCREENS.SEARCH.ROOT ? (focused.params?.q as string | undefined) : undefined; + if (focused?.name !== SCREENS.SEARCH.ROOT) { + return undefined; + } + const query = focused.params?.q; + if (typeof query !== 'string') { + return undefined; + } + const rawQuery = focused.params?.rawQuery; + return `${query}${CONST.SEARCH.QUERY_PARAMS_SEPARATOR}${typeof rawQuery === 'string' ? rawQuery : ''}`; } -function selectSearchRawQueryParam(state: NavigationState | undefined) { - const focused = getDeepestFocusedScreen(state); - return focused?.name === SCREENS.SEARCH.ROOT ? (focused.params?.rawQuery as string | undefined) : undefined; +function splitSearchQueryParams(queryParams: string | undefined) { + if (queryParams === undefined) { + return {query: undefined, rawQuery: undefined}; + } + const separatorIndex = queryParams.indexOf(CONST.SEARCH.QUERY_PARAMS_SEPARATOR); + if (separatorIndex === -1) { + return {query: queryParams, rawQuery: undefined}; + } + const rawQuery = queryParams.slice(separatorIndex + CONST.SEARCH.QUERY_PARAMS_SEPARATOR.length); + return {query: queryParams.slice(0, separatorIndex), rawQuery: rawQuery || undefined}; } function SearchQueryProvider({children}: SearchQueryProviderProps) { const navigation = useNavigation(); // Extract only the primitive values we need from the focused screen to avoid // re-renders from new object references returned by getDeepestFocusedScreen. - const queryParam = useRootNavigationState((state) => selectSearchQueryParam(state ?? navigation.getState())); - const rawQueryParam = useRootNavigationState((state) => selectSearchRawQueryParam(state ?? navigation.getState())); - const definedQueryParam = usePreviousDefined(queryParam) ?? buildSearchQueryString(); + const queryParams = useRootNavigationState((state) => selectSearchQueryParams(state ?? navigation.getState())); + const {query: queryParam, rawQuery: rawQueryParam} = splitSearchQueryParams(usePreviousDefined(queryParams)); + const definedQueryParam = queryParam ?? buildSearchQueryString(); const currentSearchQueryJSON = buildSearchQueryJSON(definedQueryParam, rawQueryParam); const shouldLoadCategoryData = currentSearchQueryJSON?.flatFilters.some((filter) => filter.key === CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY) ?? false; useLoadSearchCategoryData({shouldLoad: shouldLoadCategoryData}); diff --git a/src/hooks/useSearchBulkActions.ts b/src/hooks/useSearchBulkActions.ts index 8ce122c96e2a..d4229ef8a63e 100644 --- a/src/hooks/useSearchBulkActions.ts +++ b/src/hooks/useSearchBulkActions.ts @@ -64,7 +64,7 @@ import { isSelfDM, shouldShowMarkAsDone, } from '@libs/ReportUtils'; -import {buildSearchQueryJSON, buildSearchQueryString, isDefaultExpensesQuery, serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; +import {buildSearchQueryJSON, buildSearchQueryString, getFilterFromQuery, isDefaultExpensesQuery, serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; import {getColumnsToShow, getSearchColumnTranslationKey, getSelectedGroupFilterEntry, getValidGroupBy, isGroupEntry, navigateToSearchRHP, shouldShowDeleteOption} from '@libs/SearchUIUtils'; import showConfirmModalAfterMoreMenuDismiss from '@libs/showConfirmModalAfterMoreMenuDismiss'; import playSound, {SOUNDS} from '@libs/Sound'; @@ -99,7 +99,7 @@ import {doesPersonalDetailExistSelector} from '@src/selectors/PersonalDetails'; import type {BillingGraceEndPeriod, ExportTemplate, Policy, Report, ReportAction, ReportNameValuePairs, SearchResults, Transaction, TransactionViolations} from '@src/types/onyx'; import type {SearchResultDataType} from '@src/types/onyx/SearchResults'; import type DeepValueOf from '@src/types/utils/DeepValueOf'; -import {getEmptyObject} from '@src/types/utils/EmptyObject'; +import {getEmptyObject, isEmptyObject} from '@src/types/utils/EmptyObject'; import type {OnyxCollection, OnyxEntry} from 'react-native-onyx'; import type {ValueOf} from 'type-fest'; @@ -1657,6 +1657,13 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { }); }, [selectedReports, currentSearchResults?.data, isTrackIntentUser, policies, selectedTransactions]); + // Matches exist that the client hasn't loaded, and so can't validate + const hasUnloadedMatchingExpenses = !!currentSearchResults?.search?.hasMoreResults; + const isUnreportedOnlyQuery = useMemo(() => { + const statusFilter = queryJSON ? getFilterFromQuery(queryJSON, CONST.SEARCH.SYNTAX_FILTER_KEYS.STATUS) : undefined; + return !statusFilter?.isNegated && !!statusFilter?.value?.length && statusFilter.value.every((status) => status === CONST.SEARCH.STATUS.EXPENSE.UNREPORTED); + }, [queryJSON]); + const headerButtonsOptions = useMemo(() => { if ((selectedTransactionsKeys.length === 0 && !(isExpenseType && areAllMatchingItemsSelected)) || !hash) { return CONST.EMPTY_ARRAY as unknown as Array>; @@ -1895,8 +1902,53 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { subMenuItems, }; + // Move-eligibility only sees the loaded page; the backend enforces it on the full set + const moveOwnerAccountIDs = new Set(); + let moveHasUnknownOwner = false; + for (const id of selectedTransactionsKeys) { + const transactionEntry = selectedTransactions[id]; + if (!transactionEntry) { + continue; + } + const ownerAccountID = + transactionEntry.ownerAccountID ?? + getReportOrDraftReport(transactionEntry.reportID, undefined, undefined, undefined, allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionEntry.reportID}`])?.ownerAccountID; + if (typeof ownerAccountID === 'number') { + moveOwnerAccountIDs.add(ownerAccountID); + if (moveOwnerAccountIDs.size > 1) { + break; + } + } else { + moveHasUnknownOwner = true; + } + } + const moveHasMultipleOwners = moveOwnerAccountIDs.size > 1 || (moveHasUnknownOwner && (moveOwnerAccountIDs.size > 0 || selectedTransactionsKeys.length > 1)); + // `every` is vacuously true on an empty selection, and the destination page bails out when nothing is selected + const canAllTransactionsBeMoved = selectedTransactionsKeys.length > 0 && selectedTransactionsKeys.every((id) => selectedTransactions[id].canChangeReport); + const canMoveExpenses = canAllTransactionsBeMoved && !moveHasMultipleOwners && !isExpenseReportType; + + const moveExpensesOption: DropdownOption = { + text: translate('iou.moveExpenses'), + icon: expensifyIcons.DocumentMerge, + value: CONST.SEARCH.BULK_ACTION_TYPES.CHANGE_REPORT, + shouldCloseModalOnSelect: true, + onSelected: () => { + // A queued all-matching move would send a stale query, so ask the user to reconnect (same as export) + if (areAllMatchingItemsSelected && isOffline) { + setIsOfflineModalVisible(true); + return; + } + Navigation.navigate(ROUTES.MOVE_TRANSACTIONS_SEARCH_RHP.getRoute()); + }, + }; + if (areAllMatchingItemsSelected) { - return [exportButtonOption]; + // The backend moves the whole query, so the query must guarantee every match is movable: a non-movable + // expense hiding in unloaded results makes it reject the entire move. Unreported-only queries are safe + // because those expenses live in their owner's self DM. Exclusions can't be expressed in a query. + const isAllMatchingSelectionMovable = isEmptyObject(excludedTransactions) && (!hasUnloadedMatchingExpenses || isUnreportedOnlyQuery); + + return canMoveExpenses && isAllMatchingSelectionMovable ? [exportButtonOption, moveExpensesOption] : [exportButtonOption]; } if (allSelectedAreDeleted) { @@ -2292,37 +2344,8 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { } } - const ownerAccountIDs = new Set(); - let hasUnknownOwner = false; - for (const id of selectedTransactionsKeys) { - const transactionEntry = selectedTransactions[id]; - if (!transactionEntry) { - continue; - } - const ownerAccountID = - transactionEntry.ownerAccountID ?? - getReportOrDraftReport(transactionEntry.reportID, undefined, undefined, undefined, allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transactionEntry.reportID}`])?.ownerAccountID; - if (typeof ownerAccountID === 'number') { - ownerAccountIDs.add(ownerAccountID); - if (ownerAccountIDs.size > 1) { - break; - } - } else { - hasUnknownOwner = true; - } - } - const hasMultipleOwners = ownerAccountIDs.size > 1 || (hasUnknownOwner && (ownerAccountIDs.size > 0 || selectedTransactionsKeys.length > 1)); - - const canAllTransactionsBeMoved = selectedTransactionsKeys.every((id) => selectedTransactions[id].canChangeReport); - - if (canAllTransactionsBeMoved && !hasMultipleOwners && !isExpenseReportType) { - options.push({ - text: translate('iou.moveExpenses'), - icon: expensifyIcons.DocumentMerge, - value: CONST.SEARCH.BULK_ACTION_TYPES.CHANGE_REPORT, - shouldCloseModalOnSelect: true, - onSelected: () => Navigation.navigate(ROUTES.MOVE_TRANSACTIONS_SEARCH_RHP.getRoute()), - }); + if (canMoveExpenses) { + options.push(moveExpensesOption); } const firstTransactionKey = selectedTransactionsKeys.at(0); @@ -2520,6 +2543,8 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { noReportsShouldMarkAsDone, queryJSON?.groupBy, delegateAccountID, + hasUnloadedMatchingExpenses, + isUnreportedOnlyQuery, ]); const handleOfflineModalClose = useCallback(() => { diff --git a/src/libs/API/parameters/ChangeTransactionsReportParams.ts b/src/libs/API/parameters/ChangeTransactionsReportParams.ts index deae34bec13a..9654c3a4db4e 100644 --- a/src/libs/API/parameters/ChangeTransactionsReportParams.ts +++ b/src/libs/API/parameters/ChangeTransactionsReportParams.ts @@ -20,6 +20,8 @@ type ChangeTransactionsReportParams = { reportID: string; transactionIDToReportActionAndThreadData: string; // A map of transactionID to TransactionThreadInfo transactionIDToUpdatedCustomUnitRateID?: string; // A JSON map of transactionID to the new customUnitRateID (for distance expenses moving to a workspace with an invalid rate) + jsonQuery?: string; + hash?: number; }; export type {ChangeTransactionsReportParams, TransactionThreadInfo}; diff --git a/src/libs/actions/Transaction.ts b/src/libs/actions/Transaction.ts index 94a022251cec..a7759ff71c67 100644 --- a/src/libs/actions/Transaction.ts +++ b/src/libs/actions/Transaction.ts @@ -15,6 +15,7 @@ import DateUtils from '@libs/DateUtils'; import DistanceRequestUtils from '@libs/DistanceRequestUtils'; import {toLocaleDigit} from '@libs/LocaleDigitUtils'; import {translateLocal} from '@libs/Localize'; +import Log from '@libs/Log'; import {buildOptimisticNextStep} from '@libs/NextStepUtils'; import * as NumberUtils from '@libs/NumberUtils'; import {rand64, roundToTwoDecimalPlaces} from '@libs/NumberUtils'; @@ -824,6 +825,8 @@ type ChangeTransactionsReportProps = { selfDMReportActions: OnyxEntry; delegateAccountID: number | undefined; getCurrencyDecimals: CurrencyListActionsContextType['getCurrencyDecimals']; + jsonQuery?: string; + hash?: number; }; function getChangeTransactionsReportOnyxData({ @@ -1955,14 +1958,54 @@ function getChangeTransactionsReportOnyxData({ } function changeTransactionsReport(props: ChangeTransactionsReportProps) { + const reportID = props.newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID; + + // Without the hash this falls back to the explicit list and moves only the loaded page, so surface it + if (props.jsonQuery && props.hash === undefined) { + Log.warn('changeTransactionsReport: received an all-matching jsonQuery without a hash; falling back to the explicit transaction list, which only moves the loaded transactions.'); + } + + if (props.jsonQuery && props.hash !== undefined) { + const optimisticData: Array> = []; + const successData: Array> = []; + const failureData: Array> = []; + + if (props.newReport) { + optimisticData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, + value: {pendingFields: {reportID: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}}, + }); + successData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, + value: {pendingFields: {reportID: null}}, + }); + failureData.push({ + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, + value: {pendingFields: {reportID: null}}, + }); + } + + const queryParameters: ChangeTransactionsReportParams = { + transactionList: '', + reportID, + transactionIDToReportActionAndThreadData: '{}', + jsonQuery: props.jsonQuery, + hash: props.hash, + }; + + API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, queryParameters, {optimisticData, successData, failureData}); + return; + } + const changeTransactionsReportOnyxData = getChangeTransactionsReportOnyxData(props); if (!changeTransactionsReportOnyxData) { return; } const {optimisticData, successData, failureData, transactionIDToReportActionAndThreadData, transactionIDToUpdatedCustomUnitRateID} = changeTransactionsReportOnyxData; - const reportID = props.newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID; - const parameters: ChangeTransactionsReportParams = { transactionList: props.transactionIDs.join(','), reportID, @@ -1972,6 +2015,7 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { }), }; + // eslint-disable-next-line rulesdir/no-multiple-api-calls API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, parameters, { optimisticData, successData, diff --git a/src/pages/Search/SearchTransactionsChangeReport.tsx b/src/pages/Search/SearchTransactionsChangeReport.tsx index d043ae3e92ac..9a56594a57b1 100644 --- a/src/pages/Search/SearchTransactionsChangeReport.tsx +++ b/src/pages/Search/SearchTransactionsChangeReport.tsx @@ -1,5 +1,5 @@ import {usePersonalDetails, useSession} from '@components/OnyxListItemProvider'; -import {useSearchResultsContext, useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; +import {useSearchQueryContext, useSearchResultsContext, useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; import type {ListItem} from '@components/SelectionList/types'; import useChangeTransactionsReportReports from '@hooks/useChangeTransactionsReportReports'; @@ -15,10 +15,12 @@ import usePolicyForMovingExpenses from '@hooks/usePolicyForMovingExpenses'; import {createNewReport} from '@libs/actions/Report'; import {changeTransactionsReport} from '@libs/actions/Transaction'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; +import Log from '@libs/Log'; import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute'; import setNavigationActionToMicrotaskQueue from '@libs/Navigation/helpers/setNavigationActionToMicrotaskQueue'; import Navigation from '@libs/Navigation/Navigation'; import {generateReportID, getPersonalDetailsForAccountID, getReportOrDraftReport, hasViolations as hasViolationsReportUtils} from '@libs/ReportUtils'; +import {serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; import {shouldRestrictUserBillableActions} from '@libs/SubscriptionUtils'; import {isUnreportedManagedCardTransaction} from '@libs/TransactionUtils'; @@ -28,6 +30,7 @@ import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES, {DYNAMIC_ROUTES} from '@src/ROUTES'; import type {PersonalDetails, Report, Transaction} from '@src/types/onyx'; +import {isEmptyObject} from '@src/types/utils/EmptyObject'; import {isTrackIntentUserSelector} from '@selectors/Onboarding'; import React, {useEffect, useMemo} from 'react'; @@ -39,11 +42,27 @@ type TransactionGroupListItem = ListItem & { }; function SearchTransactionsChangeReport() { - const {selectedTransactions} = useSearchSelectionContext(); + const {selectedTransactions, areAllMatchingItemsSelected, excludedTransactions} = useSearchSelectionContext(); const delegateAccountID = useDelegateAccountID(); const {clearSelectedTransactions} = useSearchSelectionActions(); const {currentSearchResults} = useSearchResultsContext(); + const {currentSearchQueryJSON} = useSearchQueryContext(); const selectedTransactionsKeys = useMemo(() => Object.keys(selectedTransactions), [selectedTransactions]); + + /** The backend resolves an "all matching" move from the query, so it needs the query and its hash together. + * Without them only the loaded page moves while the UI claims otherwise. */ + const getAllMatchingQueryParams = (): {jsonQuery?: string; hash?: number} => { + // The query can't express unchecked rows, so sending it would move them back in + if (!areAllMatchingItemsSelected || !isEmptyObject(excludedTransactions ?? {})) { + return {}; + } + if (!currentSearchQueryJSON) { + Log.warn('[SearchTransactionsChangeReport] All matching expenses are selected but the search query is unavailable; only the loaded expenses will be moved.'); + return {}; + } + return {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash}; + }; + // Search-selected transactions are not in COLLECTION.TRANSACTION — extract from `selectedTransactions` directly. const transactions = Object.values(selectedTransactions) .map((transactionItem) => transactionItem.transaction) @@ -189,6 +208,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, + ...getAllMatchingQueryParams(), }); clearSelectedTransactions(); }); @@ -276,6 +296,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, + ...getAllMatchingQueryParams(), }); Navigation.goBack(undefined, {afterTransition: clearSelectedTransactions}); }; @@ -300,6 +321,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, + ...getAllMatchingQueryParams(), }); clearSelectedTransactions(); Navigation.goBack(); diff --git a/src/pages/iou/request/step/IOURequestStepUpgrade.tsx b/src/pages/iou/request/step/IOURequestStepUpgrade.tsx index 1ff814cdbd7a..0be74572c2b5 100644 --- a/src/pages/iou/request/step/IOURequestStepUpgrade.tsx +++ b/src/pages/iou/request/step/IOURequestStepUpgrade.tsx @@ -3,7 +3,7 @@ import HeaderWithBackButton from '@components/HeaderWithBackButton'; import {usePersonalDetails} from '@components/OnyxListItemProvider'; import ScreenWrapper from '@components/ScreenWrapper'; import ScrollView from '@components/ScrollView'; -import {useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; +import {useSearchQueryContext, useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; import WorkspaceConfirmationForm from '@components/WorkspaceConfirmationForm'; import type {WorkspaceConfirmationSubmitFunctionParams} from '@components/WorkspaceConfirmationForm'; @@ -36,6 +36,7 @@ import type {MoneyRequestNavigatorParamList} from '@libs/Navigation/types'; import {isTrackOnboardingChoice} from '@libs/OnboardingUtils'; import {getParticipantsOption} from '@libs/OptionsListUtils'; import {getPersonalDetailsForAccountID, hasViolations as hasViolationsReportUtils} from '@libs/ReportUtils'; +import {serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; import UpgradeConfirmation from '@pages/workspace/upgrade/UpgradeConfirmation'; import UpgradeIntro from '@pages/workspace/upgrade/UpgradeIntro'; @@ -49,6 +50,7 @@ import type {Route} from '@src/ROUTES'; import ROUTES, {DYNAMIC_ROUTES} from '@src/ROUTES'; import type SCREENS from '@src/SCREENS'; import type {PersonalDetails, Transaction} from '@src/types/onyx'; +import {isEmptyObject} from '@src/types/utils/EmptyObject'; import {hasSeenTourSelector} from '@selectors/Onboarding'; import React, {useCallback, useMemo, useRef, useState} from 'react'; @@ -96,8 +98,9 @@ function IOURequestStepUpgrade({ const createReportForCurrentUser = useCreateNewReport(); // Hooks for bulk move functionality - const {selectedTransactions} = useSearchSelectionContext(); + const {selectedTransactions, areAllMatchingItemsSelected, excludedTransactions} = useSearchSelectionContext(); const {clearSelectedTransactions} = useSearchSelectionActions(); + const {currentSearchQueryJSON} = useSearchQueryContext(); const selectedTransactionsKeys = useMemo(() => Object.keys(selectedTransactions), [selectedTransactions]); const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS); const [allPolicyCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES); @@ -150,6 +153,13 @@ function IOURequestStepUpgrade({ [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; + // The all-matching query has to travel through the upgrade flow too, or only the loaded page moves + // Unchecked rows can't be expressed in the query, so exclusions keep the explicit list + const allMatchingQueryParams = + areAllMatchingItemsSelected && isEmptyObject(excludedTransactions ?? {}) && currentSearchQueryJSON + ? {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash} + : {}; + // Move ALL selected transactions to the new report changeTransactionsReport({ transactionIDs: selectedTransactionsKeys, @@ -169,6 +179,7 @@ function IOURequestStepUpgrade({ personalPolicyOutputCurrency: undefined, delegateAccountID, getCurrencyDecimals, + ...allMatchingQueryParams, }); clearSelectedTransactions(); @@ -277,6 +288,9 @@ function IOURequestStepUpgrade({ isTrackIntentUser, delegateAccountID, getCurrencyDecimals, + areAllMatchingItemsSelected, + currentSearchQueryJSON, + excludedTransactions, ]); const participant = transaction?.participants?.[0]; diff --git a/tests/unit/TransactionTest.ts b/tests/unit/TransactionTest.ts index 43bb8c0c2d92..4b090d7af5f3 100644 --- a/tests/unit/TransactionTest.ts +++ b/tests/unit/TransactionTest.ts @@ -2042,6 +2042,183 @@ describe('Transaction', () => { // Rate should remain unchanged since it was already valid expect(updatedTransaction?.comment?.customUnit?.customUnitRateID).toBe(validRateID); }); + + describe('when all matching items are selected (jsonQuery + hash)', () => { + const FAKE_JSON_QUERY = 'type:expense status:all'; + const FAKE_HASH = 123456; + + it('sends the search jsonQuery and hash with an empty transaction list instead of the explicit transactions', async () => { + const mockAPIWrite = jest.spyOn(require('@libs/API'), 'write').mockImplementation(() => Promise.resolve()); + + const transaction = generateTransaction({reportID: FAKE_OLD_REPORT_ID}); + await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction); + const report = await getReportFromUseOnyx(FAKE_NEW_REPORT_ID); + const allTransactions = { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`]: transaction, + }; + + changeTransactionsReport({ + transactionIDs: [transaction.transactionID], + isASAPSubmitBetaEnabled: false, + accountID: CURRENT_USER_ID, + email: 'test@example.com', + newReport: report, + policy: undefined, + allTransactions, + policyTagList: undefined, + transactionViolations: {}, + reports: undefined, + isTrackIntentUser: false, + jsonQuery: FAKE_JSON_QUERY, + hash: FAKE_HASH, + }); + await waitForBatchedUpdates(); + + expect(mockAPIWrite).toHaveBeenCalled(); + + const parameters = mockAPIWrite.mock.calls.at(0)?.[1]; + + expect(parameters).toEqual( + expect.objectContaining({ + reportID: FAKE_NEW_REPORT_ID, + // The explicit transaction list must be dropped so the backend moves ALL matching expenses via the query. + transactionList: '', + transactionIDToReportActionAndThreadData: '{}', + jsonQuery: FAKE_JSON_QUERY, + hash: FAKE_HASH, + }), + ); + + mockAPIWrite.mockRestore(); + }); + + it('falls back to the unreported report ID when removing all matching expenses from a report', async () => { + const mockAPIWrite = jest.spyOn(require('@libs/API'), 'write').mockImplementation(() => Promise.resolve()); + + const transaction = generateTransaction({reportID: FAKE_OLD_REPORT_ID}); + await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction); + const allTransactions = { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`]: transaction, + }; + + changeTransactionsReport({ + transactionIDs: [transaction.transactionID], + isASAPSubmitBetaEnabled: false, + accountID: CURRENT_USER_ID, + email: 'test@example.com', + newReport: undefined, + policy: undefined, + allTransactions, + policyTagList: undefined, + transactionViolations: {}, + reports: undefined, + isTrackIntentUser: false, + jsonQuery: FAKE_JSON_QUERY, + hash: FAKE_HASH, + }); + await waitForBatchedUpdates(); + + expect(mockAPIWrite).toHaveBeenCalled(); + + const parameters = mockAPIWrite.mock.calls.at(0)?.[1]; + + expect(parameters).toEqual( + expect.objectContaining({ + reportID: CONST.REPORT.UNREPORTED_REPORT_ID, + transactionList: '', + jsonQuery: FAKE_JSON_QUERY, + hash: FAKE_HASH, + }), + ); + + mockAPIWrite.mockRestore(); + }); + + it('optimistically flags the destination report as pending and clears it after the request succeeds', async () => { + const destinationReport = { + ...createRandomReport(7, undefined), + ownerAccountID: CURRENT_USER_ID, + type: CONST.REPORT.TYPE.EXPENSE, + }; + const destinationReportKey = `${ONYXKEYS.COLLECTION.REPORT}${destinationReport.reportID}` as const; + + mockFetch.pause(); + try { + await Onyx.merge(destinationReportKey, destinationReport); + + changeTransactionsReport({ + transactionIDs: [], + isASAPSubmitBetaEnabled: false, + accountID: CURRENT_USER_ID, + email: 'test@example.com', + newReport: destinationReport, + policy: undefined, + allTransactions: {}, + policyTagList: undefined, + transactionViolations: {}, + reports: undefined, + isTrackIntentUser: false, + jsonQuery: FAKE_JSON_QUERY, + hash: FAKE_HASH, + }); + await waitForBatchedUpdates(); + + // While the request is in flight the destination report should be flagged as pending. + const pendingReport = await getOnyxValue(destinationReportKey); + expect(pendingReport?.pendingFields?.reportID).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE); + } finally { + await mockFetch.resume(); + } + await waitForBatchedUpdates(); + + // Once the request resolves the pending flag should be cleared. + const resolvedReport = await getOnyxValue(destinationReportKey); + expect(resolvedReport?.pendingFields?.reportID).toBeFalsy(); + }); + + it('uses the normal explicit-transaction path when a hash is passed without a jsonQuery', async () => { + const mockAPIWrite = jest.spyOn(require('@libs/API'), 'write').mockImplementation(() => Promise.resolve()); + + const transaction = generateTransaction({reportID: FAKE_OLD_REPORT_ID}); + await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction); + const report = await getReportFromUseOnyx(FAKE_NEW_REPORT_ID); + const allTransactions = { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`]: transaction, + }; + + changeTransactionsReport({ + transactionIDs: [transaction.transactionID], + isASAPSubmitBetaEnabled: false, + accountID: CURRENT_USER_ID, + email: 'test@example.com', + newReport: report, + policy: undefined, + allTransactions, + policyTagList: undefined, + transactionViolations: {}, + reports: undefined, + isTrackIntentUser: false, + jsonQuery: undefined, + hash: FAKE_HASH, + }); + await waitForBatchedUpdates(); + + expect(mockAPIWrite).toHaveBeenCalled(); + + const parameters = mockAPIWrite.mock.calls.at(0)?.[1]; + + // Without a jsonQuery the explicit transaction list must be sent and no all-matching params leak through. + expect(parameters).toEqual( + expect.objectContaining({ + transactionList: transaction.transactionID, + }), + ); + expect(parameters).not.toHaveProperty('jsonQuery'); + expect(parameters).not.toHaveProperty('hash'); + + mockAPIWrite.mockRestore(); + }); + }); }); describe('getAllNonDeletedTransactions', () => { From c499c08bc32a97fe8515f6fa065238190a82a41f Mon Sep 17 00:00:00 2001 From: "Yauheni Horbach (via MelvinBot)" Date: Wed, 19 Aug 2026 10:51:15 +0000 Subject: [PATCH 2/8] Recheck connectivity before submitting an all-matching query move If the connection drops after the move RHP is open, block the query-based move and prompt the user to reconnect instead of queuing a retryable write. A queued all-matching move replays a stale query on reconnect (the hash resolves the match set at backend execution time), so expenses that started matching while offline could be swept in. Co-authored-by: Yauheni Horbach --- .../Search/SearchTransactionsChangeReport.tsx | 75 ++++++++++++++----- 1 file changed, 58 insertions(+), 17 deletions(-) diff --git a/src/pages/Search/SearchTransactionsChangeReport.tsx b/src/pages/Search/SearchTransactionsChangeReport.tsx index 023879c2437f..d50f243242c1 100644 --- a/src/pages/Search/SearchTransactionsChangeReport.tsx +++ b/src/pages/Search/SearchTransactionsChangeReport.tsx @@ -1,3 +1,4 @@ +import DecisionModal from '@components/DecisionModal'; import {usePersonalDetails, useSession} from '@components/OnyxListItemProvider'; import {useSearchQueryContext, useSearchResultsContext, useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; import type {ListItem} from '@components/SelectionList/types'; @@ -7,10 +8,13 @@ import useConditionalCreateEmptyReportConfirmation from '@hooks/useConditionalCr import {useCurrencyListActions} from '@hooks/useCurrencyList'; import useDelegateAccountID from '@hooks/useDelegateAccountID'; import useHasPerDiemTransactions from '@hooks/useHasPerDiemTransactions'; +import useLocalize from '@hooks/useLocalize'; +import useNetwork from '@hooks/useNetwork'; import useOnyx from '@hooks/useOnyx'; import usePermissions from '@hooks/usePermissions'; import usePersonalPolicy from '@hooks/usePersonalPolicy'; import usePolicyForMovingExpenses from '@hooks/usePolicyForMovingExpenses'; +import useResponsiveLayout from '@hooks/useResponsiveLayout'; import {createNewReport} from '@libs/actions/Report'; import {changeTransactionsReport} from '@libs/actions/Transaction'; @@ -33,7 +37,7 @@ import type {PersonalDetails, Report, Transaction} from '@src/types/onyx'; import {isEmptyObject} from '@src/types/utils/EmptyObject'; import {isTrackIntentUserSelector} from '@selectors/Onboarding'; -import React, {useEffect, useMemo} from 'react'; +import React, {useEffect, useMemo, useState} from 'react'; import Onyx from 'react-native-onyx'; type TransactionGroupListItem = ListItem & { @@ -48,6 +52,12 @@ function SearchTransactionsChangeReport() { const {currentSearchResults} = useSearchResultsContext(); const {currentSearchQueryJSON} = useSearchQueryContext(); const selectedTransactionsKeys = useMemo(() => Object.keys(selectedTransactions), [selectedTransactions]); + const {translate} = useLocalize(); + const {isOffline} = useNetwork(); + // We need isSmallScreenWidth (not just shouldUseNarrowLayout) because DecisionModal requires it for correct modal type + // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth + const {isSmallScreenWidth} = useResponsiveLayout(); + const [isOfflineModalVisible, setIsOfflineModalVisible] = useState(false); /** The backend resolves an "all matching" move from the query, so it needs the query and its hash together. * Without them only the loaded page moves while the UI claims otherwise. */ @@ -63,6 +73,14 @@ function SearchTransactionsChangeReport() { return {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash}; }; + /** + * A queued all-matching move would replay a stale query on reconnect: the hash resolves the match set at + * backend execution time, not when the user submitted, so expenses that started matching while offline would be + * swept in. The upstream check in `useSearchBulkActions` only runs while opening this RHP, so re-check here in + * case the connection dropped after. Block the query-based move and ask the user to reconnect (same as export). + */ + const shouldBlockOfflineAllMatchingMove = () => isOffline && !!getAllMatchingQueryParams().jsonQuery; + // Search-selected transactions are not in COLLECTION.TRANSACTION — extract from `selectedTransactions` directly. const transactions = Object.values(selectedTransactions) .map((transactionItem) => transactionItem.transaction) @@ -223,6 +241,10 @@ function SearchTransactionsChangeReport() { }); const createReport = () => { + if (shouldBlockOfflineAllMatchingMove()) { + setIsOfflineModalVisible(true); + return; + } if (shouldNavigateToUpgradePath && selectedTransactionsKeys.length > 0) { const firstTransactionID = selectedTransactionsKeys.at(0); if (firstTransactionID) { @@ -274,6 +296,10 @@ function SearchTransactionsChangeReport() { if (selectedTransactionsKeys.length === 0) { return; } + if (shouldBlockOfflineAllMatchingMove()) { + setIsOfflineModalVisible(true); + return; + } const destinationReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${item.value}`]; const policyTagList = item?.policyID ? allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${item.policyID}`] : {}; @@ -309,6 +335,10 @@ function SearchTransactionsChangeReport() { if (selectedTransactionsKeys.length === 0) { return; } + if (shouldBlockOfflineAllMatchingMove()) { + setIsOfflineModalVisible(true); + return; + } const policyTagList = personalPolicyID ? allPolicyTags?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${personalPolicyID}`] : {}; changeTransactionsReport({ transactionIDs: selectedTransactionsKeys, @@ -332,22 +362,33 @@ function SearchTransactionsChangeReport() { }; return ( - + <> + + setIsOfflineModalVisible(false)} + secondOptionText={translate('common.buttonConfirm')} + isVisible={isOfflineModalVisible} + onClose={() => setIsOfflineModalVisible(false)} + /> + ); } From f38ed1794f7e574a775f28a7598078a963e70d34 Mon Sep 17 00:00:00 2001 From: "Yauheni Horbach (via MelvinBot)" Date: Mon, 24 Aug 2026 09:30:09 +0000 Subject: [PATCH 3/8] Address review comments: propagate all-matching query through workspace/upgrade paths, dedupe helper, recheck offline, lint nits Co-authored-by: Yauheni Horbach --- src/hooks/useSearchBulkActions.ts | 2 +- src/libs/actions/Transaction.ts | 2 ++ src/libs/getAllMatchingQueryParams.ts | 34 +++++++++++++++++++ ...DynamicNewReportWorkspaceSelectionPage.tsx | 11 ++++-- .../Search/SearchTransactionsChangeReport.tsx | 26 ++++---------- .../step/DynamicIOURequestStepUpgrade.tsx | 16 ++++----- 6 files changed, 60 insertions(+), 31 deletions(-) create mode 100644 src/libs/getAllMatchingQueryParams.ts diff --git a/src/hooks/useSearchBulkActions.ts b/src/hooks/useSearchBulkActions.ts index 243d34e7e093..121f960192e3 100644 --- a/src/hooks/useSearchBulkActions.ts +++ b/src/hooks/useSearchBulkActions.ts @@ -2114,7 +2114,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { subMenuItems, }; - // Move-eligibility only sees the loaded page; the backend enforces it on the full set + // Move-eligibility only sees the loaded page. The backend enforces it on the full set. const moveOwnerAccountIDs = new Set(); let moveHasUnknownOwner = false; for (const id of selectedTransactionsKeys) { diff --git a/src/libs/actions/Transaction.ts b/src/libs/actions/Transaction.ts index 06ab05c3a76f..39644a738826 100644 --- a/src/libs/actions/Transaction.ts +++ b/src/libs/actions/Transaction.ts @@ -2066,6 +2066,8 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { }), }; + // The query-based all-matching path above already called API.write and returned, so this branch only runs for the + // explicit-transaction move. The two writes are mutually exclusive and never fire within the same invocation. // eslint-disable-next-line rulesdir/no-multiple-api-calls API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, parameters, { optimisticData, diff --git a/src/libs/getAllMatchingQueryParams.ts b/src/libs/getAllMatchingQueryParams.ts new file mode 100644 index 000000000000..c58231352966 --- /dev/null +++ b/src/libs/getAllMatchingQueryParams.ts @@ -0,0 +1,34 @@ +import type {SearchQueryJSON, SelectedTransactions} from '@components/Search/types'; + +import {isEmptyObject} from '@src/types/utils/EmptyObject'; + +import Log from './Log'; +import {serializeQueryJSONForBackend} from './SearchQueryUtils'; + +type AllMatchingQueryParams = {jsonQuery?: string; hash?: number}; + +/** + * The backend resolves an "all matching" move from the search query, so it needs the query and its hash together. + * Without them only the loaded page moves while the UI claims every match was selected. + * + * Returns an empty object (so callers fall back to the explicit transaction list) when the query can't safely + * represent the selection: not an all-matching selection, some rows were unchecked (a query can't express + * exclusions), or the query is unavailable. + */ +function getAllMatchingQueryParams( + areAllMatchingItemsSelected: boolean, + excludedTransactions: SelectedTransactions | undefined, + currentSearchQueryJSON: Readonly | undefined, +): AllMatchingQueryParams { + // The query can't express unchecked rows, so sending it would move them back in + if (!areAllMatchingItemsSelected || !isEmptyObject(excludedTransactions ?? {})) { + return {}; + } + if (!currentSearchQueryJSON) { + Log.warn('[getAllMatchingQueryParams] All matching expenses are selected but the search query is unavailable; only the loaded expenses will be moved.'); + return {}; + } + return {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash}; +} + +export default getAllMatchingQueryParams; diff --git a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx index e6a735273037..9bc435fee4dd 100644 --- a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx +++ b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx @@ -1,7 +1,7 @@ import ActivityIndicator from '@components/ActivityIndicator'; import HeaderWithBackButton from '@components/HeaderWithBackButton'; import ScreenWrapper from '@components/ScreenWrapper'; -import {useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; +import {useSearchQueryContext, useSearchSelectionActions, useSearchSelectionContext} from '@components/Search/SearchContext'; import SelectionList from '@components/SelectionList'; import UserListItem from '@components/SelectionList/ListItem/UserListItem'; import type {ListItem} from '@components/SelectionList/types'; @@ -25,6 +25,7 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout'; import useThemeStyles from '@hooks/useThemeStyles'; import useTransactionsByID from '@hooks/useTransactionsByID'; +import getAllMatchingQueryParams from '@libs/getAllMatchingQueryParams'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import isSearchTopmostFullScreenRoute from '@libs/Navigation/helpers/isSearchTopmostFullScreenRoute'; import setNavigationActionToMicrotaskQueue from '@libs/Navigation/helpers/setNavigationActionToMicrotaskQueue'; @@ -66,8 +67,9 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec const {isMovingExpenses} = route.params ?? {}; const backPath = useDynamicBackPath(DYNAMIC_ROUTES.NEW_REPORT_WORKSPACE_SELECTION.path); const {isOffline} = useNetwork(); - const {selectedTransactions, selectedTransactionIDs} = useSearchSelectionContext(); + const {selectedTransactions, selectedTransactionIDs, areAllMatchingItemsSelected, excludedTransactions} = useSearchSelectionContext(); const {clearSelectedTransactions} = useSearchSelectionActions(); + const {currentSearchQueryJSON} = useSearchQueryContext(); const styles = useThemeStyles(); const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState(''); const {translate, localeCompare} = useLocalize(); @@ -133,6 +135,10 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec ...reports, [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; + // The all-matching query has to travel through the workspace-picker path too, or only the loaded page + // moves while the UI claims every match was selected. Drop the query params when offline so a queued + // move can't replay a stale query on reconnect; the frozen explicit list is moved instead. + const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); setNavigationActionToMicrotaskQueue(() => { changeTransactionsReport({ transactionIDs, @@ -151,6 +157,7 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec selfDMReportActions, delegateAccountID, getCurrencyDecimals, + ...allMatchingQueryParams, }); // eslint-disable-next-line rulesdir/no-default-id-values diff --git a/src/pages/Search/SearchTransactionsChangeReport.tsx b/src/pages/Search/SearchTransactionsChangeReport.tsx index d50f243242c1..4f76a1eb77ec 100644 --- a/src/pages/Search/SearchTransactionsChangeReport.tsx +++ b/src/pages/Search/SearchTransactionsChangeReport.tsx @@ -18,13 +18,12 @@ import useResponsiveLayout from '@hooks/useResponsiveLayout'; import {createNewReport} from '@libs/actions/Report'; import {changeTransactionsReport} from '@libs/actions/Transaction'; +import getAllMatchingQueryParams from '@libs/getAllMatchingQueryParams'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; -import Log from '@libs/Log'; import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute'; import setNavigationActionToMicrotaskQueue from '@libs/Navigation/helpers/setNavigationActionToMicrotaskQueue'; import Navigation from '@libs/Navigation/Navigation'; import {generateReportID, getPersonalDetailsForAccountID, getReportOrDraftReport, hasViolations as hasViolationsReportUtils} from '@libs/ReportUtils'; -import {serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; import {shouldRestrictUserBillableActions} from '@libs/SubscriptionUtils'; import {isManualDistanceRequest as isManualDistanceRequestUtil, isOdometerDistanceRequest as isOdometerDistanceRequestUtil, isUnreportedManagedCardTransaction} from '@libs/TransactionUtils'; @@ -34,7 +33,6 @@ import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES, {DYNAMIC_ROUTES} from '@src/ROUTES'; import type {PersonalDetails, Report, Transaction} from '@src/types/onyx'; -import {isEmptyObject} from '@src/types/utils/EmptyObject'; import {isTrackIntentUserSelector} from '@selectors/Onboarding'; import React, {useEffect, useMemo, useState} from 'react'; @@ -59,19 +57,7 @@ function SearchTransactionsChangeReport() { const {isSmallScreenWidth} = useResponsiveLayout(); const [isOfflineModalVisible, setIsOfflineModalVisible] = useState(false); - /** The backend resolves an "all matching" move from the query, so it needs the query and its hash together. - * Without them only the loaded page moves while the UI claims otherwise. */ - const getAllMatchingQueryParams = (): {jsonQuery?: string; hash?: number} => { - // The query can't express unchecked rows, so sending it would move them back in - if (!areAllMatchingItemsSelected || !isEmptyObject(excludedTransactions ?? {})) { - return {}; - } - if (!currentSearchQueryJSON) { - Log.warn('[SearchTransactionsChangeReport] All matching expenses are selected but the search query is unavailable; only the loaded expenses will be moved.'); - return {}; - } - return {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash}; - }; + const allMatchingQueryParams = getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); /** * A queued all-matching move would replay a stale query on reconnect: the hash resolves the match set at @@ -79,7 +65,7 @@ function SearchTransactionsChangeReport() { * swept in. The upstream check in `useSearchBulkActions` only runs while opening this RHP, so re-check here in * case the connection dropped after. Block the query-based move and ask the user to reconnect (same as export). */ - const shouldBlockOfflineAllMatchingMove = () => isOffline && !!getAllMatchingQueryParams().jsonQuery; + const shouldBlockOfflineAllMatchingMove = () => isOffline && !!allMatchingQueryParams.jsonQuery; // Search-selected transactions are not in COLLECTION.TRANSACTION — extract from `selectedTransactions` directly. const transactions = Object.values(selectedTransactions) @@ -226,7 +212,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, - ...getAllMatchingQueryParams(), + ...allMatchingQueryParams, }); clearSelectedTransactions(); }); @@ -326,7 +312,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, - ...getAllMatchingQueryParams(), + ...allMatchingQueryParams, }); Navigation.goBack(undefined, {afterTransition: clearSelectedTransactions}); }; @@ -355,7 +341,7 @@ function SearchTransactionsChangeReport() { selfDMReportActions, delegateAccountID, getCurrencyDecimals, - ...getAllMatchingQueryParams(), + ...allMatchingQueryParams, }); clearSelectedTransactions(); Navigation.goBack(); diff --git a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx index 0da7c5cdfa76..e1bb2284610d 100644 --- a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx +++ b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx @@ -26,6 +26,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; import {createNewReport} from '@libs/actions/Report'; import {changeTransactionsReport, setTransactionReport} from '@libs/actions/Transaction'; import type CreateWorkspaceParams from '@libs/API/parameters/CreateWorkspaceParams'; +import getAllMatchingQueryParams from '@libs/getAllMatchingQueryParams'; import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; import getPlatform from '@libs/getPlatform'; import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute'; @@ -36,7 +37,6 @@ import type {MoneyRequestNavigatorParamList} from '@libs/Navigation/types'; import {isTrackOnboardingChoice} from '@libs/OnboardingUtils'; import {getParticipantsOption} from '@libs/OptionsListUtils'; import {getPersonalDetailsForAccountID, hasViolations as hasViolationsReportUtils} from '@libs/ReportUtils'; -import {serializeQueryJSONForBackend} from '@libs/SearchQueryUtils'; import UpgradeConfirmation from '@pages/workspace/upgrade/UpgradeConfirmation'; import UpgradeIntro from '@pages/workspace/upgrade/UpgradeIntro'; @@ -50,7 +50,6 @@ import type {Route} from '@src/ROUTES'; import ROUTES, {DYNAMIC_ROUTES} from '@src/ROUTES'; import type SCREENS from '@src/SCREENS'; import type {PersonalDetails, Transaction} from '@src/types/onyx'; -import {isEmptyObject} from '@src/types/utils/EmptyObject'; import {hasSeenTourSelector} from '@selectors/Onboarding'; import React, {useCallback, useMemo, useRef, useState} from 'react'; @@ -153,12 +152,12 @@ function DynamicIOURequestStepUpgrade({ [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; - // The all-matching query has to travel through the upgrade flow too, or only the loaded page moves - // Unchecked rows can't be expressed in the query, so exclusions keep the explicit list - const allMatchingQueryParams = - areAllMatchingItemsSelected && isEmptyObject(excludedTransactions ?? {}) && currentSearchQueryJSON - ? {jsonQuery: serializeQueryJSONForBackend(currentSearchQueryJSON), hash: currentSearchQueryJSON.hash} - : {}; + // The all-matching query has to travel through the upgrade flow too, or only the loaded page moves. + // The confirmation button stays enabled offline, so recheck connectivity here. A queued query-based move + // would replay a stale query on reconnect (the hash resolves the match set at backend execution time), + // sweeping in expenses that started matching while offline, so drop the query params when offline and + // fall back to moving the frozen explicit list. + const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); // Move ALL selected transactions to the new report changeTransactionsReport({ @@ -293,6 +292,7 @@ function DynamicIOURequestStepUpgrade({ areAllMatchingItemsSelected, currentSearchQueryJSON, excludedTransactions, + isOffline, ]); const participant = transaction?.participants?.[0]; From dd9acb72d273b5e4bf784c104ca582d29221538a Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Mon, 31 Aug 2026 17:47:42 +0000 Subject: [PATCH 4/8] Surface backend rejections generically per API command Co-authored-by: Cong Pham --- src/ONYXKEYS.ts | 4 + src/components/CommandErrorModal.tsx | 50 ++++++++++ src/libs/API/makeRequest.ts | 5 + src/libs/Middleware/SurfaceCommandError.ts | 62 ++++++++++++ src/libs/Middleware/index.ts | 15 ++- .../Navigation/AppNavigator/AuthScreens.tsx | 2 + src/libs/actions/CommandError.ts | 19 ++++ src/setup/index.ts | 1 + src/types/onyx/CommandError.ts | 17 ++++ src/types/onyx/index.ts | 2 + .../unit/SurfaceCommandErrorMiddlewareTest.ts | 97 +++++++++++++++++++ 11 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 src/components/CommandErrorModal.tsx create mode 100644 src/libs/Middleware/SurfaceCommandError.ts create mode 100644 src/libs/actions/CommandError.ts create mode 100644 src/types/onyx/CommandError.ts create mode 100644 tests/unit/SurfaceCommandErrorMiddlewareTest.ts diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index dadc439cfc7c..83a0a3ce2dc1 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -613,6 +613,9 @@ const ONYXKEYS = { /** Indicates a global supportal permission denial that should surface a modal */ SUPPORTAL_PERMISSION_DENIED: 'supportalPermissionDenied', + /** Holds the backend rejection of the last opted-in API command so it can surface a modal from anywhere in the app */ + RAM_ONLY_COMMAND_ERROR: 'commandError', + /** Indicates whether an forced reset is required. Used in emergency situations where we must completely erase the Onyx data in the client because it is in a bad state. This will clear Onyx data without signing the user out. */ RESET_REQUIRED: 'resetRequired', @@ -1756,6 +1759,7 @@ type OnyxValuesMapping = { [ONYXKEYS.RECENTLY_USED_REPORT_FIELDS]: OnyxTypes.RecentlyUsedReportFields; [ONYXKEYS.RAM_ONLY_UPDATE_REQUIRED]: boolean; [ONYXKEYS.SUPPORTAL_PERMISSION_DENIED]: OnyxTypes.SupportalPermissionDenied | null; + [ONYXKEYS.RAM_ONLY_COMMAND_ERROR]: OnyxTypes.CommandError | null; [ONYXKEYS.RESET_REQUIRED]: boolean; [ONYXKEYS.PLAID_CURRENT_EVENT]: string; [ONYXKEYS.NVP_PRIVATE_TAX_EXEMPT]: boolean; diff --git a/src/components/CommandErrorModal.tsx b/src/components/CommandErrorModal.tsx new file mode 100644 index 000000000000..cfb59d0cfa9d --- /dev/null +++ b/src/components/CommandErrorModal.tsx @@ -0,0 +1,50 @@ +import useConfirmModal from '@hooks/useConfirmModal'; +import useLocalize from '@hooks/useLocalize'; +import useOnyx from '@hooks/useOnyx'; + +import {clearCommandError} from '@userActions/CommandError'; + +import ONYXKEYS from '@src/ONYXKEYS'; + +import {useEffect, useEffectEvent, useRef} from 'react'; + +/** + * Shows the backend's rejection message for any API command that opted into user-facing error surfacing through + * the `SurfaceCommandError` middleware. It is mounted globally because the screen that started the request is + * usually already dismissed by the time the response arrives. + */ +function CommandErrorModal() { + const {translate} = useLocalize(); + const [commandError] = useOnyx(ONYXKEYS.RAM_ONLY_COMMAND_ERROR); + const {showConfirmModal} = useConfirmModal(); + const isModalOpenRef = useRef(false); + + const showCommandErrorModal = useEffectEvent((message: string | undefined) => { + if (isModalOpenRef.current) { + return; + } + isModalOpenRef.current = true; + showConfirmModal({ + // The backend message is a complete, self-contained sentence that carries its own lead-in, so a title + // would only repeat it. Only the generic fallback copy needs one. + title: message ? undefined : translate('common.headsUp'), + prompt: message ?? translate('common.genericErrorMessage'), + confirmText: translate('common.buttonConfirm'), + shouldShowCancelButton: false, + }).then(() => { + isModalOpenRef.current = false; + clearCommandError(); + }); + }); + + useEffect(() => { + if (!commandError) { + return; + } + showCommandErrorModal(commandError.message); + }, [commandError]); + + return null; +} + +export default CommandErrorModal; diff --git a/src/libs/API/makeRequest.ts b/src/libs/API/makeRequest.ts index 05139c6066e5..61b2231b9679 100644 --- a/src/libs/API/makeRequest.ts +++ b/src/libs/API/makeRequest.ts @@ -10,6 +10,7 @@ import { RecordFullReconnectTime, SaveResponseInOnyx, SupportalPermission, + SurfaceCommandError, } from '@libs/Middleware'; import FraudMonitoring from '@libs/Middleware/FraudMonitoring'; import LoadPostDataForOpenOrReconnect from '@libs/Middleware/LoadPostDataForOpenOrReconnect'; @@ -57,6 +58,10 @@ addMiddleware(handleDeletedAccount); // Handle supportal permission denial centrally addMiddleware(SupportalPermission); +// SurfaceCommandError - Raises a global modal with the backend's rejection message for the commands that opted in. +// It runs after the middlewares above so the jsonCodes they own (407, 408, 411) never reach it. +addMiddleware(SurfaceCommandError); + // If an optimistic ID is not used by the server, this will update the remaining serialized requests using that optimistic ID to use the correct ID instead. addMiddleware(HandleUnusedOptimisticID); diff --git a/src/libs/Middleware/SurfaceCommandError.ts b/src/libs/Middleware/SurfaceCommandError.ts new file mode 100644 index 000000000000..c4b51959d16d --- /dev/null +++ b/src/libs/Middleware/SurfaceCommandError.ts @@ -0,0 +1,62 @@ +import {WRITE_COMMANDS} from '@libs/API/types'; +import Log from '@libs/Log'; + +import {showCommandError} from '@userActions/CommandError'; + +import CONST from '@src/CONST'; +import type Request from '@src/types/onyx/Request'; +import type {PaginatedRequest} from '@src/types/onyx/Request'; +import type Response from '@src/types/onyx/Response'; + +import type {OnyxKey} from 'react-native-onyx'; + +import type Middleware from './types'; + +/** + * Commands that opted into having their backend rejection shown to the user. + * + * A command belongs here when the client cannot predict the rejection itself (so it has no pre-check to run) and + * the request is fire-and-forget, meaning the screen that started it is gone by the time the response lands and + * nothing else would tell the user the action silently rolled back. + */ +const COMMANDS_WITH_USER_FACING_ERRORS = new Set([WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT]); + +/** + * jsonCodes another middleware or a global handler already reacts to: reauthentication retries 407, deleted + * copilot accounts sign the user out on 408, `SupportalPermission` owns 411, and 426 raises the update-required + * modal. Surfacing those here would double up on UI or nag about a request that is about to be retried. + * `unableToRetry` is not a backend rejection at all — it means the request never got a verdict from the server. + */ +const GLOBALLY_HANDLED_JSON_CODES = new Set([CONST.JSON_CODE.NOT_AUTHENTICATED, CONST.JSON_CODE.UPDATE_REQUIRED, CONST.JSON_CODE.UNABLE_TO_RETRY, 408, 411]); + +/** + * Middleware that surfaces a backend rejection to the user for the commands listed above. + * + * The `failureData` of a request can only revert optimistic Onyx data — it is built before the request is sent, so + * it cannot carry the reason the backend gave. That leaves a rejection invisible whenever the initiating screen has + * already closed. This stores the response message under a single Onyx key that a globally mounted modal reads, + * keyed by command so each command opts in explicitly instead of every failure in the app raising a modal. + */ +const SurfaceCommandError: Middleware = (responsePromise: Promise | void>, request: Request | PaginatedRequest) => + responsePromise.then((response) => { + const command = request?.command; + if (!command || !COMMANDS_WITH_USER_FACING_ERRORS.has(command)) { + return response; + } + + const jsonCode = response?.jsonCode; + + // A network failure rejects the promise instead of resolving it, and the queue retries the request, so an + // absent jsonCode here is not a rejection we should report. + if (jsonCode === undefined || jsonCode === CONST.JSON_CODE.SUCCESS || GLOBALLY_HANDLED_JSON_CODES.has(jsonCode)) { + return response; + } + + const message = typeof response?.message === 'string' && response.message.length > 0 ? response.message : undefined; + Log.info('[SurfaceCommandError] Surfacing a backend rejection to the user', false, {command, jsonCode}); + showCommandError({command, message, jsonCode}); + + return response; + }); + +export default SurfaceCommandError; diff --git a/src/libs/Middleware/index.ts b/src/libs/Middleware/index.ts index ce28f2a8b4b2..03baac8bce1a 100644 --- a/src/libs/Middleware/index.ts +++ b/src/libs/Middleware/index.ts @@ -8,5 +8,18 @@ import Reauthentication from './Reauthentication'; import RecordFullReconnectTime from './RecordFullReconnectTime'; import SaveResponseInOnyx from './SaveResponseInOnyx'; import SupportalPermission from './SupportalPermission'; +import SurfaceCommandError from './SurfaceCommandError'; -export {HandleUnusedOptimisticID, LoadTest, Logging, Reauthentication, RecordFullReconnectTime, FailureTracking, SaveResponseInOnyx, Pagination, handleDeletedAccount, SupportalPermission}; +export { + HandleUnusedOptimisticID, + LoadTest, + Logging, + Reauthentication, + RecordFullReconnectTime, + FailureTracking, + SaveResponseInOnyx, + Pagination, + handleDeletedAccount, + SupportalPermission, + SurfaceCommandError, +}; diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.tsx b/src/libs/Navigation/AppNavigator/AuthScreens.tsx index 7b55de5ae473..0f36277225f5 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.tsx +++ b/src/libs/Navigation/AppNavigator/AuthScreens.tsx @@ -1,3 +1,4 @@ +import CommandErrorModal from '@components/CommandErrorModal'; import ComposeProviders from '@components/ComposeProviders'; import DelegateNoAccessModalProvider from '@components/DelegateNoAccessModalProvider'; import ExportDownloadStatusManager from '@components/ExportDownloadStatusManager'; @@ -168,6 +169,7 @@ function AuthScreens() { + ): Promise { + const request: OnyxRequest = {command, data: {}}; + await SurfaceCommandError(Promise.resolve(response), request, true); + await waitForBatchedUpdates(); + + let commandError: CommandError | null | undefined; + await TestHelper.getOnyxData({ + key: ONYXKEYS.RAM_ONLY_COMMAND_ERROR, + callback: (value) => { + commandError = value; + }, + }); + return commandError; +} + +describe('SurfaceCommandError middleware', () => { + beforeEach(async () => { + await Onyx.clear(); + await waitForBatchedUpdates(); + }); + + it('surfaces the backend message when an opted-in command is rejected', async () => { + const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, { + jsonCode: CONST.JSON_CODE.EXP_ERROR, + message: PER_DIEM_REJECTION, + }); + + expect(commandError).toEqual({ + command: WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, + message: PER_DIEM_REJECTION, + jsonCode: CONST.JSON_CODE.EXP_ERROR, + }); + }); + + it('surfaces a rejection with no message so the modal can fall back to generic copy', async () => { + const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {jsonCode: CONST.JSON_CODE.EXP_ERROR}); + + expect(commandError?.command).toBe(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT); + expect(commandError?.message).toBeUndefined(); + }); + + it('does not surface anything for a successful response', async () => { + const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, { + jsonCode: CONST.JSON_CODE.SUCCESS, + message: 'all good', + }); + + expect(commandError).toBeFalsy(); + }); + + it('does not surface anything for a command that did not opt in', async () => { + const commandError = await runMiddleware(WRITE_COMMANDS.OPEN_REPORT, { + jsonCode: CONST.JSON_CODE.EXP_ERROR, + message: PER_DIEM_REJECTION, + }); + + expect(commandError).toBeFalsy(); + }); + + it.each([ + ['an expired authToken that gets retried', CONST.JSON_CODE.NOT_AUTHENTICATED], + ['a deleted account handled globally', 408], + ['a supportal denial handled globally', 411], + ['a required app update handled globally', CONST.JSON_CODE.UPDATE_REQUIRED], + ['a request that never got a verdict from the server', CONST.JSON_CODE.UNABLE_TO_RETRY], + ])('does not surface %s', async (_label, jsonCode) => { + const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {jsonCode, message: 'handled elsewhere'}); + + expect(commandError).toBeFalsy(); + }); + + it('does not surface anything when the request never reached the server', async () => { + // A network failure rejects the promise chain instead of resolving with a jsonCode, and the queue retries it. + const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {}); + + expect(commandError).toBeFalsy(); + }); +}); From c1618dc40fad8cf0c510bc5ce72153e8b59f0575 Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Wed, 2 Sep 2026 18:33:01 +0000 Subject: [PATCH 5/8] Revert "Surface backend rejections generically per API command" This reverts commit dd9acb72d273b5e4bf784c104ca582d29221538a. --- src/ONYXKEYS.ts | 4 - src/components/CommandErrorModal.tsx | 50 ---------- src/libs/API/makeRequest.ts | 5 - src/libs/Middleware/SurfaceCommandError.ts | 62 ------------ src/libs/Middleware/index.ts | 15 +-- .../Navigation/AppNavigator/AuthScreens.tsx | 2 - src/libs/actions/CommandError.ts | 19 ---- src/setup/index.ts | 1 - src/types/onyx/CommandError.ts | 17 ---- src/types/onyx/index.ts | 2 - .../unit/SurfaceCommandErrorMiddlewareTest.ts | 97 ------------------- 11 files changed, 1 insertion(+), 273 deletions(-) delete mode 100644 src/components/CommandErrorModal.tsx delete mode 100644 src/libs/Middleware/SurfaceCommandError.ts delete mode 100644 src/libs/actions/CommandError.ts delete mode 100644 src/types/onyx/CommandError.ts delete mode 100644 tests/unit/SurfaceCommandErrorMiddlewareTest.ts diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 83a0a3ce2dc1..dadc439cfc7c 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -613,9 +613,6 @@ const ONYXKEYS = { /** Indicates a global supportal permission denial that should surface a modal */ SUPPORTAL_PERMISSION_DENIED: 'supportalPermissionDenied', - /** Holds the backend rejection of the last opted-in API command so it can surface a modal from anywhere in the app */ - RAM_ONLY_COMMAND_ERROR: 'commandError', - /** Indicates whether an forced reset is required. Used in emergency situations where we must completely erase the Onyx data in the client because it is in a bad state. This will clear Onyx data without signing the user out. */ RESET_REQUIRED: 'resetRequired', @@ -1759,7 +1756,6 @@ type OnyxValuesMapping = { [ONYXKEYS.RECENTLY_USED_REPORT_FIELDS]: OnyxTypes.RecentlyUsedReportFields; [ONYXKEYS.RAM_ONLY_UPDATE_REQUIRED]: boolean; [ONYXKEYS.SUPPORTAL_PERMISSION_DENIED]: OnyxTypes.SupportalPermissionDenied | null; - [ONYXKEYS.RAM_ONLY_COMMAND_ERROR]: OnyxTypes.CommandError | null; [ONYXKEYS.RESET_REQUIRED]: boolean; [ONYXKEYS.PLAID_CURRENT_EVENT]: string; [ONYXKEYS.NVP_PRIVATE_TAX_EXEMPT]: boolean; diff --git a/src/components/CommandErrorModal.tsx b/src/components/CommandErrorModal.tsx deleted file mode 100644 index cfb59d0cfa9d..000000000000 --- a/src/components/CommandErrorModal.tsx +++ /dev/null @@ -1,50 +0,0 @@ -import useConfirmModal from '@hooks/useConfirmModal'; -import useLocalize from '@hooks/useLocalize'; -import useOnyx from '@hooks/useOnyx'; - -import {clearCommandError} from '@userActions/CommandError'; - -import ONYXKEYS from '@src/ONYXKEYS'; - -import {useEffect, useEffectEvent, useRef} from 'react'; - -/** - * Shows the backend's rejection message for any API command that opted into user-facing error surfacing through - * the `SurfaceCommandError` middleware. It is mounted globally because the screen that started the request is - * usually already dismissed by the time the response arrives. - */ -function CommandErrorModal() { - const {translate} = useLocalize(); - const [commandError] = useOnyx(ONYXKEYS.RAM_ONLY_COMMAND_ERROR); - const {showConfirmModal} = useConfirmModal(); - const isModalOpenRef = useRef(false); - - const showCommandErrorModal = useEffectEvent((message: string | undefined) => { - if (isModalOpenRef.current) { - return; - } - isModalOpenRef.current = true; - showConfirmModal({ - // The backend message is a complete, self-contained sentence that carries its own lead-in, so a title - // would only repeat it. Only the generic fallback copy needs one. - title: message ? undefined : translate('common.headsUp'), - prompt: message ?? translate('common.genericErrorMessage'), - confirmText: translate('common.buttonConfirm'), - shouldShowCancelButton: false, - }).then(() => { - isModalOpenRef.current = false; - clearCommandError(); - }); - }); - - useEffect(() => { - if (!commandError) { - return; - } - showCommandErrorModal(commandError.message); - }, [commandError]); - - return null; -} - -export default CommandErrorModal; diff --git a/src/libs/API/makeRequest.ts b/src/libs/API/makeRequest.ts index 61b2231b9679..05139c6066e5 100644 --- a/src/libs/API/makeRequest.ts +++ b/src/libs/API/makeRequest.ts @@ -10,7 +10,6 @@ import { RecordFullReconnectTime, SaveResponseInOnyx, SupportalPermission, - SurfaceCommandError, } from '@libs/Middleware'; import FraudMonitoring from '@libs/Middleware/FraudMonitoring'; import LoadPostDataForOpenOrReconnect from '@libs/Middleware/LoadPostDataForOpenOrReconnect'; @@ -58,10 +57,6 @@ addMiddleware(handleDeletedAccount); // Handle supportal permission denial centrally addMiddleware(SupportalPermission); -// SurfaceCommandError - Raises a global modal with the backend's rejection message for the commands that opted in. -// It runs after the middlewares above so the jsonCodes they own (407, 408, 411) never reach it. -addMiddleware(SurfaceCommandError); - // If an optimistic ID is not used by the server, this will update the remaining serialized requests using that optimistic ID to use the correct ID instead. addMiddleware(HandleUnusedOptimisticID); diff --git a/src/libs/Middleware/SurfaceCommandError.ts b/src/libs/Middleware/SurfaceCommandError.ts deleted file mode 100644 index c4b51959d16d..000000000000 --- a/src/libs/Middleware/SurfaceCommandError.ts +++ /dev/null @@ -1,62 +0,0 @@ -import {WRITE_COMMANDS} from '@libs/API/types'; -import Log from '@libs/Log'; - -import {showCommandError} from '@userActions/CommandError'; - -import CONST from '@src/CONST'; -import type Request from '@src/types/onyx/Request'; -import type {PaginatedRequest} from '@src/types/onyx/Request'; -import type Response from '@src/types/onyx/Response'; - -import type {OnyxKey} from 'react-native-onyx'; - -import type Middleware from './types'; - -/** - * Commands that opted into having their backend rejection shown to the user. - * - * A command belongs here when the client cannot predict the rejection itself (so it has no pre-check to run) and - * the request is fire-and-forget, meaning the screen that started it is gone by the time the response lands and - * nothing else would tell the user the action silently rolled back. - */ -const COMMANDS_WITH_USER_FACING_ERRORS = new Set([WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT]); - -/** - * jsonCodes another middleware or a global handler already reacts to: reauthentication retries 407, deleted - * copilot accounts sign the user out on 408, `SupportalPermission` owns 411, and 426 raises the update-required - * modal. Surfacing those here would double up on UI or nag about a request that is about to be retried. - * `unableToRetry` is not a backend rejection at all — it means the request never got a verdict from the server. - */ -const GLOBALLY_HANDLED_JSON_CODES = new Set([CONST.JSON_CODE.NOT_AUTHENTICATED, CONST.JSON_CODE.UPDATE_REQUIRED, CONST.JSON_CODE.UNABLE_TO_RETRY, 408, 411]); - -/** - * Middleware that surfaces a backend rejection to the user for the commands listed above. - * - * The `failureData` of a request can only revert optimistic Onyx data — it is built before the request is sent, so - * it cannot carry the reason the backend gave. That leaves a rejection invisible whenever the initiating screen has - * already closed. This stores the response message under a single Onyx key that a globally mounted modal reads, - * keyed by command so each command opts in explicitly instead of every failure in the app raising a modal. - */ -const SurfaceCommandError: Middleware = (responsePromise: Promise | void>, request: Request | PaginatedRequest) => - responsePromise.then((response) => { - const command = request?.command; - if (!command || !COMMANDS_WITH_USER_FACING_ERRORS.has(command)) { - return response; - } - - const jsonCode = response?.jsonCode; - - // A network failure rejects the promise instead of resolving it, and the queue retries the request, so an - // absent jsonCode here is not a rejection we should report. - if (jsonCode === undefined || jsonCode === CONST.JSON_CODE.SUCCESS || GLOBALLY_HANDLED_JSON_CODES.has(jsonCode)) { - return response; - } - - const message = typeof response?.message === 'string' && response.message.length > 0 ? response.message : undefined; - Log.info('[SurfaceCommandError] Surfacing a backend rejection to the user', false, {command, jsonCode}); - showCommandError({command, message, jsonCode}); - - return response; - }); - -export default SurfaceCommandError; diff --git a/src/libs/Middleware/index.ts b/src/libs/Middleware/index.ts index 03baac8bce1a..ce28f2a8b4b2 100644 --- a/src/libs/Middleware/index.ts +++ b/src/libs/Middleware/index.ts @@ -8,18 +8,5 @@ import Reauthentication from './Reauthentication'; import RecordFullReconnectTime from './RecordFullReconnectTime'; import SaveResponseInOnyx from './SaveResponseInOnyx'; import SupportalPermission from './SupportalPermission'; -import SurfaceCommandError from './SurfaceCommandError'; -export { - HandleUnusedOptimisticID, - LoadTest, - Logging, - Reauthentication, - RecordFullReconnectTime, - FailureTracking, - SaveResponseInOnyx, - Pagination, - handleDeletedAccount, - SupportalPermission, - SurfaceCommandError, -}; +export {HandleUnusedOptimisticID, LoadTest, Logging, Reauthentication, RecordFullReconnectTime, FailureTracking, SaveResponseInOnyx, Pagination, handleDeletedAccount, SupportalPermission}; diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.tsx b/src/libs/Navigation/AppNavigator/AuthScreens.tsx index 0f36277225f5..7b55de5ae473 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.tsx +++ b/src/libs/Navigation/AppNavigator/AuthScreens.tsx @@ -1,4 +1,3 @@ -import CommandErrorModal from '@components/CommandErrorModal'; import ComposeProviders from '@components/ComposeProviders'; import DelegateNoAccessModalProvider from '@components/DelegateNoAccessModalProvider'; import ExportDownloadStatusManager from '@components/ExportDownloadStatusManager'; @@ -169,7 +168,6 @@ function AuthScreens() { - ): Promise { - const request: OnyxRequest = {command, data: {}}; - await SurfaceCommandError(Promise.resolve(response), request, true); - await waitForBatchedUpdates(); - - let commandError: CommandError | null | undefined; - await TestHelper.getOnyxData({ - key: ONYXKEYS.RAM_ONLY_COMMAND_ERROR, - callback: (value) => { - commandError = value; - }, - }); - return commandError; -} - -describe('SurfaceCommandError middleware', () => { - beforeEach(async () => { - await Onyx.clear(); - await waitForBatchedUpdates(); - }); - - it('surfaces the backend message when an opted-in command is rejected', async () => { - const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, { - jsonCode: CONST.JSON_CODE.EXP_ERROR, - message: PER_DIEM_REJECTION, - }); - - expect(commandError).toEqual({ - command: WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, - message: PER_DIEM_REJECTION, - jsonCode: CONST.JSON_CODE.EXP_ERROR, - }); - }); - - it('surfaces a rejection with no message so the modal can fall back to generic copy', async () => { - const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {jsonCode: CONST.JSON_CODE.EXP_ERROR}); - - expect(commandError?.command).toBe(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT); - expect(commandError?.message).toBeUndefined(); - }); - - it('does not surface anything for a successful response', async () => { - const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, { - jsonCode: CONST.JSON_CODE.SUCCESS, - message: 'all good', - }); - - expect(commandError).toBeFalsy(); - }); - - it('does not surface anything for a command that did not opt in', async () => { - const commandError = await runMiddleware(WRITE_COMMANDS.OPEN_REPORT, { - jsonCode: CONST.JSON_CODE.EXP_ERROR, - message: PER_DIEM_REJECTION, - }); - - expect(commandError).toBeFalsy(); - }); - - it.each([ - ['an expired authToken that gets retried', CONST.JSON_CODE.NOT_AUTHENTICATED], - ['a deleted account handled globally', 408], - ['a supportal denial handled globally', 411], - ['a required app update handled globally', CONST.JSON_CODE.UPDATE_REQUIRED], - ['a request that never got a verdict from the server', CONST.JSON_CODE.UNABLE_TO_RETRY], - ])('does not surface %s', async (_label, jsonCode) => { - const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {jsonCode, message: 'handled elsewhere'}); - - expect(commandError).toBeFalsy(); - }); - - it('does not surface anything when the request never reached the server', async () => { - // A network failure rejects the promise chain instead of resolving with a jsonCode, and the queue retries it. - const commandError = await runMiddleware(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, {}); - - expect(commandError).toBeFalsy(); - }); -}); From c4dbba05e026c4f120ce1c16654c79b13d84a8cb Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Tue, 8 Sep 2026 11:17:40 +0000 Subject: [PATCH 6/8] Simplify the code comments added by this PR --- src/components/Search/SearchQueryProvider.tsx | 4 +-- src/hooks/useSearchBulkActions.ts | 14 +++++------ src/libs/actions/Transaction.ts | 25 +++++++++---------- src/libs/getAllMatchingQueryParams.ts | 10 +++----- ...DynamicNewReportWorkspaceSelectionPage.tsx | 6 ++--- .../Search/SearchTransactionsChangeReport.tsx | 10 ++++---- .../step/DynamicIOURequestStepUpgrade.tsx | 9 +++---- tests/unit/TransactionTest.ts | 20 +++++++-------- 8 files changed, 47 insertions(+), 51 deletions(-) diff --git a/src/components/Search/SearchQueryProvider.tsx b/src/components/Search/SearchQueryProvider.tsx index 91a4559190ff..5afaed951c30 100644 --- a/src/components/Search/SearchQueryProvider.tsx +++ b/src/components/Search/SearchQueryProvider.tsx @@ -26,8 +26,8 @@ type SearchQueryProviderProps = { children: React.ReactNode; }; -/** Joins `q` and `rawQuery` so they can't drift apart: restoring only `q` would drop `rawFilterList` from the - * query JSON while keeping the same hash. */ +/** Joins `q` and `rawQuery` so they can't drift apart. Restoring only `q` would drop `rawFilterList` from the query + * JSON while keeping the same hash. */ function selectSearchQueryParams(state: NavigationState | undefined) { const focused = getDeepestFocusedScreen(state); if (focused?.name !== SCREENS.SEARCH.ROOT) { diff --git a/src/hooks/useSearchBulkActions.ts b/src/hooks/useSearchBulkActions.ts index 0c4ef7741e14..e539b86fc437 100644 --- a/src/hooks/useSearchBulkActions.ts +++ b/src/hooks/useSearchBulkActions.ts @@ -1779,7 +1779,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { }); }, [selectedReports, currentSearchResults?.data, isTrackIntentUser, policies, selectedTransactions]); - // Matches exist that the client hasn't loaded, and so can't validate + // Some matches haven't been loaded yet, so the client can't check whether they are movable const hasUnloadedMatchingExpenses = !!currentSearchResults?.search?.hasMoreResults; const isUnreportedOnlyQuery = useMemo(() => { const statusFilter = queryJSON ? getFilterFromQuery(queryJSON, CONST.SEARCH.SYNTAX_FILTER_KEYS.STATUS) : undefined; @@ -2163,7 +2163,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { return isExportTheOnlyAction && subMenuItems.length > 0 ? subMenuItems : builtOptions; }; - // Move-eligibility only sees the loaded page. The backend enforces it on the full set. + // These checks only see the loaded page. The backend enforces the same rules on the full set. const moveOwnerAccountIDs = new Set(); let moveHasUnknownOwner = false; for (const id of selectedTransactionsKeys) { @@ -2184,7 +2184,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { } } const moveHasMultipleOwners = moveOwnerAccountIDs.size > 1 || (moveHasUnknownOwner && (moveOwnerAccountIDs.size > 0 || selectedTransactionsKeys.length > 1)); - // `every` is vacuously true on an empty selection, and the destination page bails out when nothing is selected + // `every` returns true for an empty selection, so check the length too. The destination page has nothing to move. const canAllTransactionsBeMoved = selectedTransactionsKeys.length > 0 && selectedTransactionsKeys.every((id) => selectedTransactions[id].canChangeReport); const canMoveExpenses = canAllTransactionsBeMoved && !moveHasMultipleOwners && !isExpenseReportType; @@ -2194,7 +2194,7 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { value: CONST.SEARCH.BULK_ACTION_TYPES.CHANGE_REPORT, shouldCloseModalOnSelect: true, onSelected: () => { - // A queued all-matching move would send a stale query, so ask the user to reconnect (same as export) + // A queued all-matching move would send a stale query, so ask the user to reconnect like export does if (areAllMatchingItemsSelected && isOffline) { setIsOfflineModalVisible(true); return; @@ -2204,9 +2204,9 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { }; if (areAllMatchingItemsSelected) { - // The backend moves the whole query, so the query must guarantee every match is movable: a non-movable - // expense hiding in unloaded results makes it reject the entire move. Unreported-only queries are safe - // because those expenses live in their owner's self DM. Exclusions can't be expressed in a query. + // The backend moves everything the query matches, so one expense it can't move rejects the whole move. + // Only offer this when the query guarantees every match is movable. An unreported query is safe because + // those expenses live in their owner's self DM. A query also can't express excluded rows. const isAllMatchingSelectionMovable = isEmptyObject(excludedTransactions) && (!hasUnloadedMatchingExpenses || isUnreportedOnlyQuery); return openExportOptionsDirectlyIfSoleAction(canMoveExpenses && isAllMatchingSelectionMovable ? [exportButtonOption, moveExpensesOption] : [exportButtonOption]); diff --git a/src/libs/actions/Transaction.ts b/src/libs/actions/Transaction.ts index c872b8be5c90..ec21ef1a0a44 100644 --- a/src/libs/actions/Transaction.ts +++ b/src/libs/actions/Transaction.ts @@ -2006,17 +2006,17 @@ function getChangeTransactionsReportOnyxData({ function changeTransactionsReport(props: ChangeTransactionsReportProps) { const reportID = props.newReport?.reportID ?? CONST.REPORT.UNREPORTED_REPORT_ID; - // Without the hash this falls back to the explicit list and moves only the loaded page, so surface it + // Without a hash the move falls back to the explicit list and only the loaded page moves, so surface that if (props.jsonQuery && props.hash === undefined) { Log.warn('changeTransactionsReport: received an all-matching jsonQuery without a hash; falling back to the explicit transaction list, which only moves the loaded transactions.'); } if (props.jsonQuery && props.hash !== undefined) { // The backend resolves the whole matching set from the query, but the client has only loaded part of it. - // Build the normal optimistic updates for the transactions that ARE loaded so their rows leave the list - // immediately, exactly as a per-page selection does; the unloaded remainder arrives with the response. - // This returns undefined when none of the loaded transactions actually move (they may already sit in the - // destination), and the request still has to go out for the expenses the client never loaded. + // Build the normal optimistic updates for the loaded transactions so their rows leave the list right away, + // just like a per-page selection does. The rest of the set arrives with the response. + // This is undefined when none of the loaded transactions move, but the request still has to go out for the + // expenses the client never loaded. const loadedTransactionsOnyxData = getChangeTransactionsReportOnyxData(props); const optimisticData = [...(loadedTransactionsOnyxData?.optimisticData ?? [])]; @@ -2024,8 +2024,8 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { const failureData = [...(loadedTransactionsOnyxData?.failureData ?? [])]; if (props.newReport) { - // The loaded rows are already updated above, but expenses the client never loaded are still being - // moved server-side, so the destination stays flagged as pending for the whole request. + // The expenses the client never loaded are still being moved on the server, so the destination stays + // pending for the whole request optimisticData.push({ onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, @@ -2046,12 +2046,12 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { const transactionIDToUpdatedCustomUnitRateID = loadedTransactionsOnyxData?.transactionIDToUpdatedCustomUnitRateID ?? {}; const queryParameters: ChangeTransactionsReportParams = { - // The explicit list must stay empty so the backend moves every matching expense via the query - // rather than just the page the client happens to have loaded. + // The list stays empty so the backend moves every matching expense from the query instead of only the + // page the client loaded transactionList: '', reportID, - // Hand over the report action and thread IDs we just created optimistically for the loaded - // transactions so the backend reuses them instead of adding a second moved message to each. + // Send the report action and thread IDs we just created optimistically so the backend reuses them + // instead of adding a second moved message to each loaded transaction transactionIDToReportActionAndThreadData: JSON.stringify(loadedTransactionsOnyxData?.transactionIDToReportActionAndThreadData ?? {}), ...(Object.keys(transactionIDToUpdatedCustomUnitRateID).length > 0 && { transactionIDToUpdatedCustomUnitRateID: JSON.stringify(transactionIDToUpdatedCustomUnitRateID), @@ -2085,8 +2085,7 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { }), }; - // The query-based all-matching path above already called API.write and returned, so this branch only runs for the - // explicit-transaction move. The two writes are mutually exclusive and never fire within the same invocation. + // The all-matching path above already returned after its own API.write, so only one of the two writes ever runs // eslint-disable-next-line rulesdir/no-multiple-api-calls API.write(WRITE_COMMANDS.CHANGE_TRANSACTIONS_REPORT, parameters, { optimisticData, diff --git a/src/libs/getAllMatchingQueryParams.ts b/src/libs/getAllMatchingQueryParams.ts index c58231352966..3428682711c8 100644 --- a/src/libs/getAllMatchingQueryParams.ts +++ b/src/libs/getAllMatchingQueryParams.ts @@ -8,19 +8,17 @@ import {serializeQueryJSONForBackend} from './SearchQueryUtils'; type AllMatchingQueryParams = {jsonQuery?: string; hash?: number}; /** - * The backend resolves an "all matching" move from the search query, so it needs the query and its hash together. - * Without them only the loaded page moves while the UI claims every match was selected. + * The backend needs the query and its hash to move every matching expense. Without them only the loaded page moves. * - * Returns an empty object (so callers fall back to the explicit transaction list) when the query can't safely - * represent the selection: not an all-matching selection, some rows were unchecked (a query can't express - * exclusions), or the query is unavailable. + * @returns The query params, or an empty object when the query can't represent the selection. Callers then fall back + * to the explicit transaction list. */ function getAllMatchingQueryParams( areAllMatchingItemsSelected: boolean, excludedTransactions: SelectedTransactions | undefined, currentSearchQueryJSON: Readonly | undefined, ): AllMatchingQueryParams { - // The query can't express unchecked rows, so sending it would move them back in + // A query can't express unchecked rows, so sending it would move them back in if (!areAllMatchingItemsSelected || !isEmptyObject(excludedTransactions ?? {})) { return {}; } diff --git a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx index 512532edb0b6..6c2402b6cd6b 100644 --- a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx +++ b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx @@ -135,9 +135,9 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec ...reports, [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; - // The all-matching query has to travel through the workspace-picker path too, or only the loaded page - // moves while the UI claims every match was selected. Drop the query params when offline so a queued - // move can't replay a stale query on reconnect; the frozen explicit list is moved instead. + // The query has to travel through the workspace picker too, or only the loaded page moves while the UI + // says every match was selected. Drop it when offline so a queued move can't replay a stale query on + // reconnect, and move the explicit list instead. const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); setNavigationActionToMicrotaskQueue(() => { changeTransactionsReport({ diff --git a/src/pages/Search/SearchTransactionsChangeReport.tsx b/src/pages/Search/SearchTransactionsChangeReport.tsx index 1c6d7a11c027..82cd6ee74036 100644 --- a/src/pages/Search/SearchTransactionsChangeReport.tsx +++ b/src/pages/Search/SearchTransactionsChangeReport.tsx @@ -52,7 +52,7 @@ function SearchTransactionsChangeReport() { const selectedTransactionsKeys = useMemo(() => Object.keys(selectedTransactions), [selectedTransactions]); const {translate} = useLocalize(); const {isOffline} = useNetwork(); - // We need isSmallScreenWidth (not just shouldUseNarrowLayout) because DecisionModal requires it for correct modal type + // We need to use isSmallScreenWidth instead of shouldUseNarrowLayout to apply the correct modal type for the decision modal // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth const {isSmallScreenWidth} = useResponsiveLayout(); const [isOfflineModalVisible, setIsOfflineModalVisible] = useState(false); @@ -60,10 +60,10 @@ function SearchTransactionsChangeReport() { const allMatchingQueryParams = getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); /** - * A queued all-matching move would replay a stale query on reconnect: the hash resolves the match set at - * backend execution time, not when the user submitted, so expenses that started matching while offline would be - * swept in. The upstream check in `useSearchBulkActions` only runs while opening this RHP, so re-check here in - * case the connection dropped after. Block the query-based move and ask the user to reconnect (same as export). + * A queued all-matching move would replay a stale query on reconnect. The backend resolves the match set when it + * runs the query, not when the user submitted it, so expenses that started matching while offline get swept in. + * `useSearchBulkActions` only checks this when the user opens this RHP, so check again in case the connection + * dropped since then, and ask the user to reconnect like export does. */ const shouldBlockOfflineAllMatchingMove = () => isOffline && !!allMatchingQueryParams.jsonQuery; diff --git a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx index b08bfc511fc9..e31eb80084b6 100644 --- a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx +++ b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx @@ -152,11 +152,10 @@ function DynamicIOURequestStepUpgrade({ [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; - // The all-matching query has to travel through the upgrade flow too, or only the loaded page moves. - // The confirmation button stays enabled offline, so recheck connectivity here. A queued query-based move - // would replay a stale query on reconnect (the hash resolves the match set at backend execution time), - // sweeping in expenses that started matching while offline, so drop the query params when offline and - // fall back to moving the frozen explicit list. + // The query has to travel through the upgrade flow too, or only the loaded page moves. The confirmation + // button stays enabled offline, so check the connection again here. A queued move would replay a stale + // query on reconnect and sweep in expenses that started matching while offline, so drop the query when + // offline and move the explicit list instead. const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); // Move ALL selected transactions to the new report diff --git a/tests/unit/TransactionTest.ts b/tests/unit/TransactionTest.ts index b078be940305..9fdd06b4b6d6 100644 --- a/tests/unit/TransactionTest.ts +++ b/tests/unit/TransactionTest.ts @@ -2112,15 +2112,15 @@ describe('Transaction', () => { expect(parameters).toEqual( expect.objectContaining({ reportID: FAKE_NEW_REPORT_ID, - // The explicit transaction list must be dropped so the backend moves ALL matching expenses via the query. + // The list stays empty so the backend moves every matching expense from the query transactionList: '', jsonQuery: FAKE_JSON_QUERY, hash: FAKE_HASH, }), ); - // The loaded transaction is optimistically moved, so its optimistic action/thread IDs must be sent - // for the backend to reuse instead of creating a second moved message for it. + // The loaded transaction moves optimistically, so its action and thread IDs go out for the backend to + // reuse instead of creating a second moved message for it const transactionData = parseJSONRecord(readProperty(parameters, 'transactionIDToReportActionAndThreadData')); expect(hasDefinedProperty(transactionData, transaction.transactionID)).toBe(true); @@ -2198,7 +2198,7 @@ describe('Transaction', () => { }); await waitForBatchedUpdates(); - // While the request is in flight the destination report should be flagged as pending. + // The destination stays pending while the request is in flight const pendingReport = await getOnyxValue(destinationReportKey); expect(pendingReport?.pendingFields?.reportID).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE); } finally { @@ -2206,7 +2206,7 @@ describe('Transaction', () => { } await waitForBatchedUpdates(); - // Once the request resolves the pending flag should be cleared. + // The pending flag clears once the request resolves const resolvedReport = await getOnyxValue(destinationReportKey); expect(resolvedReport?.pendingFields?.reportID).toBeFalsy(); }); @@ -2218,8 +2218,8 @@ describe('Transaction', () => { const report = await getReportFromUseOnyx(FAKE_NEW_REPORT_ID); const allTransactions = {[transactionKey]: transaction}; - // Pause the request rather than mocking API.write, so the optimistic data is really applied to Onyx - // and can be observed while the move is still in flight. + // Pause the request instead of mocking API.write so the optimistic data really reaches Onyx and can be + // read while the move is still in flight mockFetch.pause(); try { changeTransactionsReport({ @@ -2239,8 +2239,8 @@ describe('Transaction', () => { }); await waitForBatchedUpdates(); - // The all-matching move must apply the same optimistic update as a per-page move, so the loaded - // transaction already points at the destination report instead of waiting for the response. + // The all-matching move applies the same optimistic update as a per-page move, so the loaded + // transaction already points at the destination instead of waiting for the response const movedTransaction = await getOnyxValue(transactionKey); expect(movedTransaction?.reportID).toBe(FAKE_NEW_REPORT_ID); } finally { @@ -2279,7 +2279,7 @@ describe('Transaction', () => { const parameters = mockAPIWrite.mock.calls.at(0)?.[1]; - // Without a jsonQuery the explicit transaction list must be sent and no all-matching params leak through. + // Without a jsonQuery the explicit list goes out and no all-matching params leak through expect(parameters).toEqual( expect.objectContaining({ transactionList: transaction.transactionID, From 8f2b1cc0b00c87943c71dc4ca3ca27ece671b1ef Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Fri, 18 Sep 2026 13:31:17 +0000 Subject: [PATCH 7/8] Pass isVendorMatchingBetaEnabled in the all-matching move tests after merging main --- tests/unit/TransactionTest.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/unit/TransactionTest.ts b/tests/unit/TransactionTest.ts index 23fc5703a485..c82c9f317690 100644 --- a/tests/unit/TransactionTest.ts +++ b/tests/unit/TransactionTest.ts @@ -2122,6 +2122,7 @@ describe('Transaction', () => { }; changeTransactionsReport({ + isVendorMatchingBetaEnabled: false, transactionIDs: [transaction.transactionID], isASAPSubmitBetaEnabled: false, accountID: CURRENT_USER_ID, @@ -2170,6 +2171,7 @@ describe('Transaction', () => { }; changeTransactionsReport({ + isVendorMatchingBetaEnabled: false, transactionIDs: [transaction.transactionID], isASAPSubmitBetaEnabled: false, accountID: CURRENT_USER_ID, @@ -2215,6 +2217,7 @@ describe('Transaction', () => { await Onyx.merge(destinationReportKey, destinationReport); changeTransactionsReport({ + isVendorMatchingBetaEnabled: false, transactionIDs: [], isASAPSubmitBetaEnabled: false, accountID: CURRENT_USER_ID, @@ -2256,6 +2259,7 @@ describe('Transaction', () => { mockFetch.pause(); try { changeTransactionsReport({ + isVendorMatchingBetaEnabled: false, transactionIDs: [transaction.transactionID], isASAPSubmitBetaEnabled: false, accountID: CURRENT_USER_ID, @@ -2292,6 +2296,7 @@ describe('Transaction', () => { }; changeTransactionsReport({ + isVendorMatchingBetaEnabled: false, transactionIDs: [transaction.transactionID], isASAPSubmitBetaEnabled: false, accountID: CURRENT_USER_ID, From 67bee6d19020eb8cd443c9a3480d2dff2f8d95e7 Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Tue, 22 Sep 2026 16:45:25 +0000 Subject: [PATCH 8/8] Simplify the code comments added by the all-matching move change Co-authored-by: Cong Pham --- src/components/Search/SearchQueryProvider.tsx | 3 +-- src/hooks/useSearchBulkActions.ts | 16 ++++------------ src/libs/actions/Transaction.ts | 16 +++++----------- .../DynamicNewReportWorkspaceSelectionPage.tsx | 5 ++--- .../Search/SearchTransactionsChangeReport.tsx | 6 ++---- .../step/DynamicIOURequestStepUpgrade.tsx | 6 ++---- tests/unit/TransactionTest.ts | 9 +++------ 7 files changed, 19 insertions(+), 42 deletions(-) diff --git a/src/components/Search/SearchQueryProvider.tsx b/src/components/Search/SearchQueryProvider.tsx index 2a8e74312a44..1ed6b66a3099 100644 --- a/src/components/Search/SearchQueryProvider.tsx +++ b/src/components/Search/SearchQueryProvider.tsx @@ -28,8 +28,7 @@ type SearchQueryProviderProps = { children: React.ReactNode; }; -/** Joins `q` and `rawQuery` so they can't drift apart. Restoring only `q` would drop `rawFilterList` from the query - * JSON while keeping the same hash. */ +/** Joins `q` and `rawQuery` so they can't drift apart. Restoring only `q` would drop `rawFilterList` while keeping the same hash. */ function selectSearchQueryParams(state: NavigationState | undefined) { const focused = getDeepestFocusedScreen(state); if (focused?.name !== SCREENS.SEARCH.ROOT) { diff --git a/src/hooks/useSearchBulkActions.ts b/src/hooks/useSearchBulkActions.ts index f710e4df126d..d825301b90d8 100644 --- a/src/hooks/useSearchBulkActions.ts +++ b/src/hooks/useSearchBulkActions.ts @@ -2325,15 +2325,8 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { } const moveHasMultipleOwners = moveOwnerAccountIDs.size > 1 || (moveHasUnknownOwner && (moveOwnerAccountIDs.size > 0 || selectedTransactionsKeys.length > 1)); - // Across submitters the only destination the App can offer is "Auto report". Every other mixed-owner selection - // stays hidden as before, so there is no entry into a screen that could only offer one submitter's reports to - // everybody else's expenses. Requirements: - // - every owner resolved, or the count below cannot tell one cardholder's bulk selection from a mixed one - // - every expense on a managed card, because the backend resolves each destination through the card; one - // expense without a card fails the whole request with "404 Card not found" - // - nothing whose validity depends on the destination workspace, which the backend picks: per diem rates and - // the map/GPS rules on manual and odometer distance can only be checked against a known workspace - // An expense we cannot read fails all three, so it withholds the flow rather than risking a rejected move. + // For selections across submitters, offer only Auto report when every expense has a resolved owner, + // is on a managed card, and does not depend on the destination workspace. Otherwise hide the flow. const canAutoReportAcrossSubmitters = moveOwnerAccountIDs.size > 1 && !moveHasUnknownOwner && @@ -2381,9 +2374,8 @@ function useSearchBulkActions({queryJSON}: UseSearchBulkActionsParams) { }; if (areAllMatchingItemsSelected) { - // The backend moves everything the query matches, so one expense it can't move rejects the whole move. - // Only offer this when the query guarantees every match is movable. An unreported query is safe because - // those expenses live in their owner's self DM. A query also can't express excluded rows. + // Offer an all-matching move only when no rows are excluded and unloaded matches are unreported. + // The backend rejects the entire move if any matching expense is invalid const isAllMatchingSelectionMovable = isEmptyObject(excludedTransactions) && (!hasUnloadedMatchingExpenses || isUnreportedOnlyQuery); const allMatchingOptions: Array> = []; diff --git a/src/libs/actions/Transaction.ts b/src/libs/actions/Transaction.ts index 5417299841d1..852bd1f53a44 100644 --- a/src/libs/actions/Transaction.ts +++ b/src/libs/actions/Transaction.ts @@ -2038,11 +2038,8 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { } if (props.jsonQuery && props.hash !== undefined) { - // The backend resolves the whole matching set from the query, but the client has only loaded part of it. - // Build the normal optimistic updates for the loaded transactions so their rows leave the list right away, - // just like a per-page selection does. The rest of the set arrives with the response. - // This is undefined when none of the loaded transactions move, but the request still has to go out for the - // expenses the client never loaded. + // The backend moves the full query result while the client only has loaded transactions. + // Apply normal optimistic updates to loaded transactions, then send the request even if none are loaded. const loadedTransactionsOnyxData = getChangeTransactionsReportOnyxData(props); const optimisticData = [...(loadedTransactionsOnyxData?.optimisticData ?? [])]; @@ -2050,8 +2047,7 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { const failureData = [...(loadedTransactionsOnyxData?.failureData ?? [])]; if (props.newReport) { - // The expenses the client never loaded are still being moved on the server, so the destination stays - // pending for the whole request + // Unloaded expenses are still moving server-side, so the destination stays pending for the whole request optimisticData.push({ onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.REPORT}${props.newReport.reportID}`, @@ -2072,12 +2068,10 @@ function changeTransactionsReport(props: ChangeTransactionsReportProps) { const transactionIDToUpdatedCustomUnitRateID = loadedTransactionsOnyxData?.transactionIDToUpdatedCustomUnitRateID ?? {}; const queryParameters: ChangeTransactionsReportParams = { - // The list stays empty so the backend moves every matching expense from the query instead of only the - // page the client loaded + // Stays empty so the backend moves every matching expense from the query, not just the loaded page transactionList: '', reportID, - // Send the report action and thread IDs we just created optimistically so the backend reuses them - // instead of adding a second moved message to each loaded transaction + // Send the optimistic action and thread IDs so the backend reuses them instead of adding a second moved message transactionIDToReportActionAndThreadData: JSON.stringify(loadedTransactionsOnyxData?.transactionIDToReportActionAndThreadData ?? {}), ...(Object.keys(transactionIDToUpdatedCustomUnitRateID).length > 0 && { transactionIDToUpdatedCustomUnitRateID: JSON.stringify(transactionIDToUpdatedCustomUnitRateID), diff --git a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx index 3174cb4efa66..80732fcf8417 100644 --- a/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx +++ b/src/pages/DynamicNewReportWorkspaceSelectionPage.tsx @@ -137,9 +137,8 @@ function DynamicNewReportWorkspaceSelectionPage({route}: NewReportWorkspaceSelec ...reports, [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; - // The query has to travel through the workspace picker too, or only the loaded page moves while the UI - // says every match was selected. Drop it when offline so a queued move can't replay a stale query on - // reconnect, and move the explicit list instead. + // Do not send all-matching query params offline because reconnecting reevaluates the query and can include newly matching expenses. + // Move the explicit transaction list instead const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); setNavigationActionToMicrotaskQueue(() => { changeTransactionsReport({ diff --git a/src/pages/Search/SearchTransactionsChangeReport.tsx b/src/pages/Search/SearchTransactionsChangeReport.tsx index a258cb899d5d..145ea72dda54 100644 --- a/src/pages/Search/SearchTransactionsChangeReport.tsx +++ b/src/pages/Search/SearchTransactionsChangeReport.tsx @@ -66,10 +66,8 @@ function SearchTransactionsChangeReport() { const allMatchingQueryParams = getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); /** - * A queued all-matching move would replay a stale query on reconnect. The backend resolves the match set when it - * runs the query, not when the user submitted it, so expenses that started matching while offline get swept in. - * `useSearchBulkActions` only checks this when the user opens this RHP, so check again in case the connection - * dropped since then, and ask the user to reconnect like export does. + * Block offline all-matching moves because the backend reevaluates the query after reconnecting, + * which can include newly matching expenses. Recheck here because the connection may have dropped after the RHP opened */ const shouldBlockOfflineAllMatchingMove = () => isOffline && !!allMatchingQueryParams.jsonQuery; diff --git a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx index 7a2ccdcbb9da..e4ba39cdfe1f 100644 --- a/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx +++ b/src/pages/iou/request/step/DynamicIOURequestStepUpgrade.tsx @@ -156,10 +156,8 @@ function DynamicIOURequestStepUpgrade({ [`${ONYXKEYS.COLLECTION.REPORT}${optimisticReport.reportID}`]: {...optimisticReport, transactionCount: 0, unheldNonReimbursableTotal: 0}, }; - // The query has to travel through the upgrade flow too, or only the loaded page moves. The confirmation - // button stays enabled offline, so check the connection again here. A queued move would replay a stale - // query on reconnect and sweep in expenses that started matching while offline, so drop the query when - // offline and move the explicit list instead. + // Do not send all-matching query params offline because reconnecting reevaluates the query and can include newly matching expenses. + // Move the explicit transaction list instead const allMatchingQueryParams = isOffline ? {} : getAllMatchingQueryParams(areAllMatchingItemsSelected, excludedTransactions, currentSearchQueryJSON); // Move ALL selected transactions to the new report diff --git a/tests/unit/TransactionTest.ts b/tests/unit/TransactionTest.ts index c82c9f317690..7c1c8d88aeb5 100644 --- a/tests/unit/TransactionTest.ts +++ b/tests/unit/TransactionTest.ts @@ -2153,8 +2153,7 @@ describe('Transaction', () => { }), ); - // The loaded transaction moves optimistically, so its action and thread IDs go out for the backend to - // reuse instead of creating a second moved message for it + // The loaded transaction's optimistic action and thread IDs go out so the backend reuses them const transactionData = parseJSONRecord(readProperty(parameters, 'transactionIDToReportActionAndThreadData')); expect(hasDefinedProperty(transactionData, transaction.transactionID)).toBe(true); @@ -2254,8 +2253,7 @@ describe('Transaction', () => { const report = await getReportFromUseOnyx(FAKE_NEW_REPORT_ID); const allTransactions = {[transactionKey]: transaction}; - // Pause the request instead of mocking API.write so the optimistic data really reaches Onyx and can be - // read while the move is still in flight + // Pause the request instead of mocking API.write so the optimistic data really reaches Onyx mockFetch.pause(); try { changeTransactionsReport({ @@ -2276,8 +2274,7 @@ describe('Transaction', () => { }); await waitForBatchedUpdates(); - // The all-matching move applies the same optimistic update as a per-page move, so the loaded - // transaction already points at the destination instead of waiting for the response + // The all-matching move applies the same optimistic update as a per-page move const movedTransaction = await getOnyxValue(transactionKey); expect(movedTransaction?.reportID).toBe(FAKE_NEW_REPORT_ID); } finally {