WIP Fix: Hide ineligible reports from move expenses list (issue #70423) #89079
+314
−4
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,250 @@ | ||
| import {act, renderHook, waitFor} from '@testing-library/react-native'; | ||
| import Onyx from 'react-native-onyx'; | ||
| import useOutstandingReports from '@hooks/useOutstandingReports'; | ||
| import initOnyxDerivedValues from '@userActions/OnyxDerived'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {Policy, Report} from '@src/types/onyx'; | ||
| import createRandomPolicy from '../../utils/collections/policies'; | ||
| import waitForBatchedUpdates from '../../utils/waitForBatchedUpdates'; | ||
|
|
||
| const POLICY_ID = 'policy1'; | ||
| const ACCOUNT_ID = 100; | ||
|
|
||
| function buildPolicy(overrides: Partial<Policy> = {}): Policy { | ||
| return { | ||
| ...createRandomPolicy(1, CONST.POLICY.TYPE.TEAM), | ||
| id: POLICY_ID, | ||
| pendingAction: undefined, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| function buildExpenseReport(reportID: string, overrides: Partial<Report> = {}): Report { | ||
| return { | ||
| reportID, | ||
| policyID: POLICY_ID, | ||
| ownerAccountID: ACCOUNT_ID, | ||
| type: CONST.REPORT.TYPE.EXPENSE, | ||
| stateNum: CONST.REPORT.STATE_NUM.OPEN, | ||
| statusNum: CONST.REPORT.STATUS_NUM.OPEN, | ||
| reportName: `Report ${reportID}`, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| function buildInstantSubmitNoApproversPolicy(): Policy { | ||
| return buildPolicy({ | ||
| autoReporting: true, | ||
| autoReportingFrequency: CONST.POLICY.AUTO_REPORTING_FREQUENCIES.INSTANT, | ||
| approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL, | ||
| }); | ||
| } | ||
|
|
||
| async function setupOnyxData(policy: Policy, reports: Report[], transactions: Array<{transactionID: string; reportID: string; reimbursable: boolean}>) { | ||
| await Onyx.merge(ONYXKEYS.SESSION, {accountID: ACCOUNT_ID}); | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${POLICY_ID}`, policy); | ||
|
|
||
| for (const report of reports) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); | ||
| } | ||
|
|
||
| for (const txn of transactions) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${txn.transactionID}`, txn); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. ❌ CONSISTENCY-5 (docs)The Add a justification comment, for example: // eslint-disable-next-line no-await-in-loop -- Onyx.merge calls must be sequential to avoid race conditions during test data setup
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report);Reviewed at: 6de392f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
|
|
||
| await waitForBatchedUpdates(); | ||
| } | ||
|
|
||
| describe('useOutstandingReports', () => { | ||
| beforeAll(() => { | ||
|
Contributor
There was a problem hiding this comment. ❌ CONSISTENCY-5 (docs)The Add a justification comment, for example: // eslint-disable-next-line no-await-in-loop -- Onyx.merge calls must be sequential to avoid race conditions during test data setup
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${txn.transactionID}`, txn);Reviewed at: 6de392f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| Onyx.init({keys: ONYXKEYS}); | ||
| initOnyxDerivedValues(); | ||
| return waitForBatchedUpdates(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| await act(async () => { | ||
| await Onyx.clear(); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await act(async () => { | ||
| await Onyx.clear(); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
| }); | ||
|
|
||
| it('returns reports when policy does not have instant submit with no approvers', async () => { | ||
| // Given a workspace without instant submit and a report containing a non-reimbursable expense | ||
| await act(async () => { | ||
| await setupOnyxData(buildPolicy({autoReporting: false}), [buildExpenseReport('report1')], [{transactionID: 'txn1', reportID: 'report1', reimbursable: false}]); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports for that workspace | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then the report should be included because the policy doesn't trigger the ineligibility filter | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('report1'); | ||
| }); | ||
| }); | ||
|
|
||
| it('filters out reports with only non-reimbursable transactions when policy has instant submit and submit & close', async () => { | ||
| // Given a workspace with instant submit and no approvers, and a report containing only non-reimbursable expenses | ||
| await act(async () => { | ||
| await setupOnyxData(buildInstantSubmitNoApproversPolicy(), [buildExpenseReport('report1')], [{transactionID: 'txn1', reportID: 'report1', reimbursable: false}]); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports for that workspace | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then the report should be excluded because moving expenses to it would fail server-side with a 403 (issue #70423) | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| it('keeps reports with reimbursable transactions even with instant submit and submit & close', async () => { | ||
| // Given a workspace with instant submit and no approvers, but a report that has reimbursable expenses | ||
| await act(async () => { | ||
| await setupOnyxData(buildInstantSubmitNoApproversPolicy(), [buildExpenseReport('report1')], [{transactionID: 'txn1', reportID: 'report1', reimbursable: true}]); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports for that workspace | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then the report should be included because it contains reimbursable transactions that keep the report open | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('report1'); | ||
| }); | ||
| }); | ||
|
|
||
| it('returns empty array when all reports are ineligible so the confirmation Report field can be disabled', async () => { | ||
| // Given a workspace with instant submit and no approvers, where every report has only non-reimbursable expenses | ||
| await act(async () => { | ||
| await setupOnyxData( | ||
| buildInstantSubmitNoApproversPolicy(), | ||
| [buildExpenseReport('report1'), buildExpenseReport('report2')], | ||
| [ | ||
| {transactionID: 'txn1', reportID: 'report1', reimbursable: false}, | ||
| {transactionID: 'txn2', reportID: 'report2', reimbursable: false}, | ||
| ], | ||
| ); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports for that workspace, with no selected report (the confirmation page case) | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then no reports should be returned. ReportField uses outstandingReports.length to decide whether the field is interactive, | ||
| // so an empty list correctly disables the field instead of opening a blank picker (deploy blocker #81608) | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| it('filters only ineligible reports and keeps eligible ones', async () => { | ||
| // Given a workspace with instant submit and no approvers, one report with only non-reimbursable expenses and another with reimbursable expenses | ||
| await act(async () => { | ||
| await setupOnyxData( | ||
| buildInstantSubmitNoApproversPolicy(), | ||
| [buildExpenseReport('reportIneligible'), buildExpenseReport('reportEligible')], | ||
| [ | ||
| {transactionID: 'txnIneligible', reportID: 'reportIneligible', reimbursable: false}, | ||
| {transactionID: 'txnEligible', reportID: 'reportEligible', reimbursable: true}, | ||
| ], | ||
| ); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports for that workspace | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then only the eligible report should remain since the ineligible one would cause a 403 if expenses were moved to it | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('reportEligible'); | ||
| }); | ||
| }); | ||
|
|
||
| // Regression test for deploy blocker https://github.com/Expensify/App/issues/88424. | ||
| // The currently selected (source) report must always remain in the outstanding list. Otherwise | ||
| // outstandingReports.length === 0 in IOURequestEditReportCommon, which makes | ||
| // shouldShowNotFoundPage true and unmounts the SelectionList, hiding the "Remove from report" | ||
| // footer that lives inside it. | ||
| it('keeps the currently selected source report in the list even when it is otherwise ineligible', async () => { | ||
| // Given a retracted report containing only a non-reimbursable expense on a workspace | ||
| // with instant submit and no approvers — the same conditions that flag a destination as ineligible | ||
| await act(async () => { | ||
| await setupOnyxData(buildInstantSubmitNoApproversPolicy(), [buildExpenseReport('sourceReport')], [{transactionID: 'txnSource', reportID: 'sourceReport', reimbursable: false}]); | ||
| }); | ||
|
|
||
| // When the picker is rendered with that report as the selected source | ||
| const {result} = renderHook(() => useOutstandingReports('sourceReport', POLICY_ID, ACCOUNT_ID, true)); | ||
|
|
||
| // Then the source report must still be returned so the picker renders and the "Remove from report" footer remains accessible | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('sourceReport'); | ||
| }); | ||
| }); | ||
|
|
||
| it('still filters out an ineligible non-source report even when a different source report is selected', async () => { | ||
| // Given a selected source report and another report that is ineligible as a destination | ||
| await act(async () => { | ||
| await setupOnyxData( | ||
| buildInstantSubmitNoApproversPolicy(), | ||
| [buildExpenseReport('sourceReport'), buildExpenseReport('ineligibleDestination')], | ||
| [ | ||
| {transactionID: 'txnSource', reportID: 'sourceReport', reimbursable: true}, | ||
| {transactionID: 'txnIneligible', reportID: 'ineligibleDestination', reimbursable: false}, | ||
| ], | ||
| ); | ||
| }); | ||
|
|
||
| // When the hook is invoked with sourceReport as the selected report | ||
| const {result} = renderHook(() => useOutstandingReports('sourceReport', POLICY_ID, ACCOUNT_ID, true)); | ||
|
|
||
| // Then only the source remains; the ineligible destination is filtered out as before | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('sourceReport'); | ||
| }); | ||
| }); | ||
|
|
||
| // Regression test for deploy blocker https://github.com/Expensify/App/issues/88425. | ||
| // When "Create report" runs, an optimistic empty report is written to Onyx with | ||
| // pendingFields.createReport === ADD, then the transaction is moved to it on a deferred | ||
| // microtask. During that gap, hasOnlyNonReimbursableTransactions flips from false (zero | ||
| // transactions) to true (one non-reimbursable transaction) and would otherwise cause the | ||
| // new report to be filtered out, making it appear briefly and then disappear from the picker. | ||
| it('keeps optimistically created reports (pendingFields.createReport === ADD) in the list', async () => { | ||
| // Given an optimistically created empty report on a workspace with instant submit and no approvers, | ||
| // plus a non-reimbursable transaction that has just been associated with it | ||
| await act(async () => { | ||
| await setupOnyxData( | ||
| buildInstantSubmitNoApproversPolicy(), | ||
| [ | ||
| buildExpenseReport('optimisticReport', { | ||
| pendingFields: {createReport: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}, | ||
| }), | ||
| ], | ||
| [{transactionID: 'txnOptimistic', reportID: 'optimisticReport', reimbursable: false}], | ||
| ); | ||
| }); | ||
|
|
||
| // When the hook computes outstanding reports | ||
| const {result} = renderHook(() => useOutstandingReports(undefined, POLICY_ID, ACCOUNT_ID, false)); | ||
|
|
||
| // Then the optimistic report remains visible until the server confirms creation and clears pendingFields | ||
| await waitFor(() => { | ||
| expect(result.current.length).toBe(1); | ||
| expect(result.current.at(0)?.reportID).toBe('optimisticReport'); | ||
| }); | ||
| }); | ||
| }); | ||
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.
The new early return for
pendingFields.createReport === ADDtreats every optimistic report as eligible, but failed creates keep that pending field set:createNewReportfailure data only setserrorFields.createReportand never clearspendingFields.createReport(src/libs/actions/Report/index.ts:3762-3767). In an instant-submit/no-approver workspace, a failed optimistic report with only non-reimbursable transactions will now stay in the destination list indefinitely and can be selected again even though backend move operations are still rejected. The bypass should be limited to in-flight creates (for example, no create error present) or the pending field must be cleared on failure.Useful? React with 👍 / 👎.