Fix unsafe assertions across receipt and settings flows #98847
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,11 @@ type FilterValueProps = { | |
| value: SearchFilter['value']; | ||
| }; | ||
|
|
||
| type ArrayFilterValueProps = { | ||
| /** The array-valued search filter displayed by this component. */ | ||
| value: Extract<SearchFilter['value'], string[]>; | ||
|
Contributor
There was a problem hiding this comment. ❌ CONSISTENCY-13 (docs)The newly added Add a block comment above the prop: type ArrayFilterValueProps = {
/** The array of filter values to render */
value: Extract<SearchFilter['value'], string[]>;
};Reviewed at: 360f4f0 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
There was a problem hiding this comment. 🟢
|
||
| }; | ||
|
|
||
| type FilterValueWithKeyProps = FilterValueProps & { | ||
| filterKey: SearchFilter['key']; | ||
| }; | ||
|
|
@@ -53,16 +58,16 @@ function FilterWorkspaceValue({value}: FilterValueProps) { | |
| return useFilterWorkspaceValue(value); | ||
| } | ||
|
|
||
| function FilterFeedValue({value}: ArrayFilterValueProps) { | ||
| return useFilterFeedValue(value); | ||
| } | ||
|
|
||
| function FilterCardValue({value}: FilterValueProps) { | ||
| return useFilterCardValue(Array.isArray(value) ? value : value.split(', ')); | ||
| } | ||
|
|
||
| function FilterTaxRateValue({value}: ArrayFilterValueProps) { | ||
| return useFilterTaxRateValue(value); | ||
| } | ||
|
|
||
| function FilterReportValue({value}: FilterValueProps) { | ||
|
|
@@ -87,15 +92,15 @@ function FilterValue({filterKey, value}: FilterValueWithKeyProps) { | |
| return <FilterWorkspaceValue value={value} />; | ||
| } | ||
|
|
||
| if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.FEED && Array.isArray(value)) { | ||
| return <FilterFeedValue value={value} />; | ||
| } | ||
|
|
||
| if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID) { | ||
| return <FilterCardValue value={value} />; | ||
| } | ||
|
|
||
| if (filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAX_RATE && Array.isArray(value)) { | ||
| return <FilterTaxRateValue value={value} />; | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,30 +1,74 @@ | ||
| import {act, renderHook} from '@testing-library/react-native'; | ||
|
|
||
| import useFilesValidation from '@hooks/useFilesValidation'; | ||
| import useReceiptScanDrop from '@hooks/useReceiptScanDrop'; | ||
|
|
||
| import {navigateToParticipantPage} from '@libs/IOUUtils'; | ||
| import Navigation from '@libs/Navigation/Navigation'; | ||
|
|
||
| import {initMoneyRequest, setMoneyRequestParticipantsFromReport} from '@userActions/IOU/MoneyRequest'; | ||
| import {setMoneyRequestReceipt} from '@userActions/IOU/Receipt'; | ||
| import {buildOptimisticTransactionAndCreateDraft} from '@userActions/TransactionEdit'; | ||
|
|
||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| import Onyx from 'react-native-onyx'; | ||
|
|
||
| import createMock from '../../utils/createMock'; | ||
| import waitForBatchedUpdatesWithAct from '../../utils/waitForBatchedUpdatesWithAct'; | ||
|
|
||
| jest.mock('@hooks/useFilesValidation'); | ||
| jest.mock('@expensify/react-native-hybrid-app', () => ({__esModule: true, default: {isHybridApp: jest.fn(() => false)}})); | ||
| jest.mock('@libs/IOUUtils', () => ({navigateToParticipantPage: jest.fn()})); | ||
| jest.mock('@libs/Navigation/Navigation'); | ||
| jest.mock('@userActions/IOU/MoneyRequest', () => ({initMoneyRequest: jest.fn(), setMoneyRequestParticipantsFromReport: jest.fn()})); | ||
| jest.mock('@userActions/IOU/Receipt', () => ({setMoneyRequestReceipt: jest.fn()})); | ||
| jest.mock('@userActions/TransactionEdit', () => ({buildOptimisticTransactionAndCreateDraft: jest.fn()})); | ||
| let mockOnFilesValidated: Parameters<typeof useFilesValidation>[0] = jest.fn(); | ||
| describe('useReceiptScanDrop', () => { | ||
| beforeEach(async () => { | ||
| await Onyx.clear(); | ||
| jest.clearAllMocks(); | ||
| jest.mocked(useFilesValidation).mockImplementation((onFilesValidated) => { | ||
| mockOnFilesValidated = onFilesValidated; | ||
| return {validateFiles: jest.fn(), PDFValidationComponent: undefined}; | ||
| }); | ||
| }); | ||
| it('should disable drag for anonymous users', async () => { | ||
| await Onyx.merge(ONYXKEYS.SESSION, {authTokenType: CONST.AUTH_TOKEN_TYPES.ANONYMOUS}); | ||
| const {result} = renderHook(() => useReceiptScanDrop()); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| expect(result.current.isDragDisabled).toBe(true); | ||
| }); | ||
| it('should enable drag for logged-in users', async () => { | ||
| await Onyx.merge(ONYXKEYS.SESSION, {authToken: 'test-token'}); | ||
| const {result} = renderHook(() => useReceiptScanDrop()); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| expect(result.current.isDragDisabled).toBe(false); | ||
| }); | ||
| it('keeps one and multiple valid receipts ordered on their intended transactions', async () => { | ||
| const createObjectURLSpy = jest.spyOn(URL, 'createObjectURL').mockReturnValueOnce('blob:first').mockReturnValueOnce('blob:first').mockReturnValueOnce('blob:second'); | ||
| renderHook(() => useReceiptScanDrop()); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| const [firstFile, secondFile] = [new File(['first'], 'first.png', {type: 'image/png'}), new File(['second'], 'second.png', {type: 'image/png'})]; | ||
| jest.mocked(initMoneyRequest).mockReturnValue(createMock<NonNullable<ReturnType<typeof initMoneyRequest>>>({})); | ||
| mockOnFilesValidated([firstFile], []); | ||
| expect(jest.mocked(setMoneyRequestReceipt)).toHaveBeenLastCalledWith(CONST.IOU.OPTIMISTIC_TRANSACTION_ID, 'blob:first', 'first.png', true, 'image/png'); | ||
| expect(jest.mocked(navigateToParticipantPage)).toHaveBeenCalledWith(CONST.IOU.TYPE.CREATE, CONST.IOU.OPTIMISTIC_TRANSACTION_ID, expect.any(String)); | ||
| jest.clearAllMocks(); | ||
| await act(() => Onyx.set(ONYXKEYS.NVP_ACTIVE_POLICY_ID, 'policy')); | ||
| await act(() => Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}policy`, {id: 'policy', type: CONST.POLICY.TYPE.TEAM})); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| jest.mocked(initMoneyRequest).mockReturnValue(createMock<NonNullable<ReturnType<typeof initMoneyRequest>>>({transactionID: CONST.IOU.OPTIMISTIC_TRANSACTION_ID})); | ||
| jest.mocked(buildOptimisticTransactionAndCreateDraft).mockReturnValue(createMock<ReturnType<typeof buildOptimisticTransactionAndCreateDraft>>({transactionID: 'later'})); | ||
| mockOnFilesValidated([firstFile, secondFile], []); | ||
| await waitForBatchedUpdatesWithAct(); | ||
| expect(createObjectURLSpy.mock.calls.slice(-2).map(([file]) => file)).toEqual([firstFile, secondFile]); | ||
| expect(jest.mocked(setMoneyRequestReceipt).mock.calls.map(([transactionID]) => transactionID)).toEqual([CONST.IOU.OPTIMISTIC_TRANSACTION_ID, 'later']); | ||
| expect(jest.mocked(buildOptimisticTransactionAndCreateDraft)).toHaveBeenCalledTimes(1); | ||
| expect(jest.mocked(setMoneyRequestParticipantsFromReport).mock.calls.map(([transactionID]) => transactionID)).toEqual([CONST.IOU.OPTIMISTIC_TRANSACTION_ID, 'later']); | ||
| expect(jest.mocked(Navigation.navigate)).toHaveBeenCalledTimes(1); | ||
| expect(jest.mocked(navigateToParticipantPage)).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴
USDVerifiedBankAccountFlowPage.tsx:BankInfoPage'sif (!policyID) return nullbreaks the policy-less bank-account flow (codex P1 confirmed)The old code cast
BankInfo as React.ComponentType<USDPageProps>(hiding thatBankInforequirespolicyID: stringwhileUSDPageProps.policyIDis optional). The honest fix would keepBankInforendering; instead this short-circuits tonullwheneverpolicyIDis falsy, andpolicyIDis legitimately absent/empty here:route.params?.policyIDis optional, andnavigateToBankAccountRoute(ReimbursementAccount/navigation.ts) documents "There can be bank accounts that are not linked to any workspace" and defaultspolicyID = ''.WalletPage/index.tsx:243callsnavigateToBankAccountRoute({bankAccountID, backTo: ROUTES.SETTINGS_WALLET})(bankAccountID only), andCountrySelection.tsx:75passes neither.BankInfoderives the account frombankAccountID = getBankAccountIDAsNumber(reimbursementAccount?.achData)(Onyx), not policyID, so it functioned fine when the old code rendered<BankInfo policyID={''} />, and it passes that''straight through toconnectBankAccountManually/WithPlaidon submit.Because the guard is a falsy check,
''(thenavigateToBankAccountRoutedefault) also returnsnull, so a user continuing setup for a personal/wallet bank account now lands on a blank Bank info step and can't proceed, a flow that worked before this diff.Why the test doesn't catch it:
USDVerifiedBankAccountFlowPageTestassertsmockBankInfo.mock.calls.length === 0forrenderPage({page: BANK_ACCOUNT})(no policyID), i.e. it encodes "policy-less → BankInfo not rendered" as expected. So the suite is green while the regression is baked in.Suggested fix (removes the cast without the regression): make
BankInfo'spolicyIDoptional to match how it's actually used, e.g.policyID?: stringinBankInfoProps(it already readsbankAccountIDfrom Onyx and forwardspolicyIDto the connect actions, which the policy-less path invokes with''today).Then
BankInfoPagecan render unconditionally:<BankInfo policyID={policyID ?? ''} .../>. If BankInfo genuinely cannot work without a policyID, that's a product decision that needs an explicit non-blank handling (redirect/error), not a silentreturn null.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 694e0e0.
The Bank info page no longer returns null when policyID is absent or empty. BankInfo and both BankAccount action boundaries now truthfully accept string | undefined, while the action bodies remain unchanged. Focused coverage verifies that undefined, '', and a valid policy ID all reach BankInfo unchanged, and that Country-to-Plaid routing remains intact.