Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ function MoneyRequestView({

// When this component is used when merging from the search page, we might not have the parent report stored in the main collection
const [parentReportFromOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`);
const parentReport = parentReportFromOnyx ?? currentSearchResults?.data[`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`];
const parentReport = parentReportFromOnyx ?? currentSearchResults?.data?.[`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`];
const [iouReportOwnerLogin] = usePersonalDetail(parentReport?.ownerAccountID, loginSelector);
const [reportPolicyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${getNonEmptyStringOnyxID(parentReport?.policyID)}`);

Expand Down
2 changes: 1 addition & 1 deletion src/hooks/useMergeTransactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function getTransaction(
return transaction;
}

return currentSearchResults?.data[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`] ?? onyxTransaction;
return currentSearchResults?.data?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`] ?? onyxTransaction;
}

function useMergeTransactions({mergeTransaction}: UseMergeTransactionsProps): UseMergeTransactionsReturn {
Expand Down
2 changes: 1 addition & 1 deletion src/pages/Search/SearchReportsMergeReports.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ function SearchMergeReports() {
isLoadingOnyxData ||
selectedReportIDs?.some((reportID) => {
const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${reportID}` as const;
return !allReports?.[reportKey] && !!currentSearchResults?.data[reportKey];
return !allReports?.[reportKey] && !!currentSearchResults?.data?.[reportKey];
});

if (!isReportMergeBetaEnabled) {
Expand Down
59 changes: 59 additions & 0 deletions tests/unit/hooks/useMergeTransactionsTest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {renderHook, waitFor} from '@testing-library/react-native';

import useMergeTransactions from '@hooks/useMergeTransactions';

import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {MergeTransaction, SearchResults} from '@src/types/onyx';

import Onyx from 'react-native-onyx';

import createRandomTransaction from '../../utils/collections/transaction';
import createMock from '../../utils/createMock';

let mockCurrentSearchResults: SearchResults | undefined;

jest.mock('@components/Search/SearchContext', () => ({
useSearchQueryContext: () => ({
currentSearchHash: 12345,
}),
useSearchResultsContext: () => ({
currentSearchResults: mockCurrentSearchResults,
}),
}));

jest.mock('@hooks/usePolicyForMovingExpenses', () => ({
__esModule: true,
default: () => ({policyForMovingExpensesID: undefined}),
}));

describe('useMergeTransactions', () => {
beforeAll(() => {
Onyx.init({keys: ONYXKEYS});
});

beforeEach(async () => {
await Onyx.clear();
mockCurrentSearchResults = undefined;
});

it('falls back to the Onyx transactions while the search snapshot has no data yet', async () => {
// Given a merge started from search while the new search's snapshot only has `search` (optimistic loading state),
// with transactions that exist in Onyx but not in the merge draft, so the hook looks them up in the snapshot first
mockCurrentSearchResults = createMock<SearchResults>({search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE}});
const onyxTransaction1 = createRandomTransaction(1);
const onyxTransaction2 = createRandomTransaction(2);
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}1`, onyxTransaction1);
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}2`, onyxTransaction2);
const mergeTransaction = createMock<MergeTransaction>({targetTransactionID: '1', sourceTransactionID: '2'});

// When the hook resolves the target and source transactions
const {result} = renderHook(() => useMergeTransactions({mergeTransaction}));

// Then it falls back to the Onyx transactions instead of throwing on the missing snapshot data
await waitFor(() => {
expect(result.current.targetTransaction).toEqual(onyxTransaction1);
expect(result.current.sourceTransaction).toEqual(onyxTransaction2);
});
});
});
31 changes: 30 additions & 1 deletion tests/unit/hooks/useSearchBulkActionsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ let mockSelectedTransactions: SelectedTransactions = {};
let mockExcludedTransactions: SelectedTransactions = {};
let mockSelectedReports: SelectedReports[] = [];
let mockAreAllMatchingItemsSelected = false;
let mockCurrentSearchResults: {search: {type: string}; data: Record<string, unknown>} | undefined;
let mockCurrentSearchResults: {search: {type: string}; data?: Record<string, unknown>} | undefined;

jest.mock('@components/Search/SearchContext', () => ({
useSearchSelectionContext: () => ({
Expand Down Expand Up @@ -515,6 +515,35 @@ describe('useSearchBulkActions - CSV export flow', () => {
});
});

it('does not crash when selected reports remain while the snapshot has no data', async () => {
// Given a report is still selected while a new search's snapshot only has `search` (its optimistic loading state),
// because the selection is cleared from an effect that runs after this render
mockSelectedTransactions = {tx1: makeSelectedTransaction({reportID: 'report1'})};
mockSelectedReports = [
{
reportID: 'report1',
policyID: 'policy1',
action: CONST.SEARCH.ACTION_TYPES.VIEW,
canPay: false,
canApprove: false,
canSubmit: false,
canChangeApprover: false,
total: 100,
chatReportID: undefined,
},
];
mockCurrentSearchResults = {search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT}};

// When the bulk actions are created for an expense-report search
const {result} = renderHook(() => useSearchBulkActions({queryJSON: expenseReportQueryJSON}), {wrapper: OnyxListItemProvider});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exercise a snapshot reader changed by this commit

This renders only useSearchBulkActions, whose selectedMergeReports access already used currentSearchResults?.data?.[...] in the parent commit; it never executes MoneyRequestView, useMergeTransactions, or SearchMergeReports, which are the three production readers changed here. Consequently, this test passes with all three fixes reverted and cannot prevent this crash from regressing; add coverage that supplies a data-less snapshot to the changed call paths.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot 🟡 Agree with Codex here. This test protects the selectedMergeReports line that #100674 already fixed, and that's still useful since #100674 shipped without a test.

But none of the 3 lines this PR changes run here, so reverting the fixes in MoneyRequestView, useMergeTransactions and SearchReportsMergeReports still passes. Can we add one case that hits a changed line? useMergeTransactions is the cheapest one: renderHook it with useSearchResultsContext mocked to return a snapshot that has only search, plus a mergeTransaction whose target ID isn't in the merge draft, so getTransaction reaches the snapshot read:

// Given a merge started from search while the new search's snapshot only has `search` (optimistic loading state)
mockCurrentSearchResults = {search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE}};

// When the hook resolves the target transaction
const {result} = renderHook(() => useMergeTransactions({mergeTransaction: {targetTransactionID: '1', sourceTransactionID: '2'}}));

// Then it falls back to the Onyx transaction instead of throwing
expect(result.current.targetTransaction).toEqual(onyxTransaction1);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added the case in c30a062, as a new file: tests/unit/hooks/useMergeTransactionsTest.ts.

It renders useMergeTransactions with a snapshot that has only search. The merge draft has no eligibleTransactions, so getTransaction reaches the snapshot read changed at useMergeTransactions.ts:59. It asserts that both target and source fall back to the Onyx transactions. If you revert the data?. fix, the read throws during render and the test fails.

It passed in CI (test (job 7)), along with typecheck, ESLint and Oxfmt.


view run


// Then the hook renders its options instead of throwing, and Merge stays hidden until the results load
await waitFor(() => {
expect(result.current.headerButtonsOptions.length).toBeGreaterThan(0);
});
expect(result.current.headerButtonsOptions.some((option) => option.value === CONST.SEARCH.BULK_ACTION_TYPES.MERGE_REPORTS)).toBe(false);
});

it('handleBasicExport with manual selection does not track any export', async () => {
mockAreAllMatchingItemsSelected = false;
mockSelectedTransactions = {tx1: makeSelectedTransaction()};
Expand Down
Loading