WIP Fix: Hide ineligible reports from move expenses list (issue #70423)#89079
WIP Fix: Hide ineligible reports from move expenses list (issue #70423)#89079abzokhattab wants to merge 2 commits intoExpensify:mainfrom
Conversation
…on guards Restores Expensify#70423's intent (don't show non-reimbursable-only reports in the move expenses list when the workspace has instant submit + no approvers) while guarding against the three deploy blockers that defeated the previous attempts: - Expensify#81608: filter lives in the hook so ReportField on the confirmation page sees the same filtered list and correctly disables itself when no destination is eligible, instead of opening a blank picker. - Expensify#88424: the currently-selected source report always survives the filter so outstandingReports.length stays >= 1, shouldShowNotFoundPage stays false, and the "Remove from report" footer renders for retracted reports. - Expensify#88425: optimistically created reports (pendingFields.createReport === ADD) bypass isReportIneligibleForMoveExpenses so the new report doesn't flicker in/out during the create -> deferred-microtask move window.
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c917ddbb3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (moneyRequestReport?.pendingFields?.createReport === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { | ||
| return false; |
There was a problem hiding this comment.
The new early return for pendingFields.createReport === ADD treats every optimistic report as eligible, but failed creates keep that pending field set: createNewReport failure data only sets errorFields.createReport and never clears pendingFields.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 👍 / 👎.
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
abzokhattab
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for (const txn of transactions) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${txn.transactionID}`, txn); | ||
| } |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-await-in-loop directive lacks an accompanying comment explaining why the rule is being suppressed.
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.
| } | ||
|
|
||
| describe('useOutstandingReports', () => { | ||
| beforeAll(() => { |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-await-in-loop directive lacks an accompanying comment explaining why the rule is being suppressed.
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.
|
all regressions are mentioned in the description @huult |
|
@MelvinBot tackled the ai generated comments |
|
@MelvinBot review |
|
@codex review |
|
👍 Noted — thanks for addressing those, |
Code ReviewThe approach is sound — filtering in 1. Performance: duplicate full-collection policy subscription
Suggestion: Instead of a second full subscription, look up the policy for each candidate report from the data already available via 2. Stale
|
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Explanation of Change
Restores the intent of #70423 — when a destination report is ineligible to receive moved expenses, it should not appear in the picker — while specifically guarding against the three deploy blockers that defeated the previous two attempts (PR #78766 → #81608, PR #84832 → #88424 / #88425).
The rule we are enforcing: an expense report is ineligible as a move-expenses destination when its workspace has Instant Submit enabled AND Submit & Close (no approvers in the workflow) AND the report contains only non-reimbursable transactions. Moving expenses to such a report fails server-side with a 403 and the user is left with a silent failure.
Where the filter lives:
useOutstandingReports. This is the same shape PR #84832 took, because it is the only place that lets both consumers — the picker (IOURequestEditReportCommon) and the confirmation page (ReportField) — see a consistent filtered list. The previous attempt at the picker layer (PR #78766) is what produced the "Report field opens a blank page" deploy blocker (#81608).How each previous regression is prevented:
ReportField.tsxstill saw the unfiltered list, soshouldReportBeEditablestayed true and the user navigated into an empty picker.useOutstandingReports, soReportField.shouldReportBeEditablebecomesfalsewhen the list is empty and the field becomes non-interactive.outstandingReports.length === 0,shouldShowNotFoundPageflipped true and the SelectionList footer (which hosts "Remove from report") was unmounted.filterEligibleReports:if (selectedReportID && report.reportID === selectedReportID) return true;. The current source report always stays in the list, so the picker renders and "Remove from report" remains accessible.createReportForPolicywrites an optimistic empty report and then runssetTransactionReporton a deferred microtask. During that gap,hasOnlyNonReimbursableTransactionsflipped fromfalse(zero transactions) totrue(one non-reimbursable transaction), making the new report disappear from the picker after a frame.isReportIneligibleForMoveExpensesitself: ifpendingFields.createReport === ADDwe returnfalse. Optimistically-created reports survive the filter until the server confirms creation and the pending field clears.The admin/manager bypass downstream in
IOURequestEditReportCommon.reportOptionsbecomes a no-op for this case because the ineligible reports are already gone before the bypass runs — admins/managers no longer see destinations the backend would reject either.Fixed Issues
$ #70423
PROPOSAL: #70423 (comment)
Tests
Original bug (#70423):
Regression test for #81608 (deploy-blocker hardening):
+> Create expense > Manual, enter amount, NextRegression test for #88424 (deploy-blocker hardening):
Regression test for #88425 (deploy-blocker hardening):
Offline tests
Same as Tests. The filter operates on Onyx-resident data, so behavior is identical offline. The
pendingFields.createReport === ADDguard ensures optimistic-create flows continue to work correctly while offline (the report only clears the pending field when the server confirms, and until then it stays visible).QA Steps
Same as Tests above, on staging.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Local verification
npx prettier --writenpm run lint(./scripts/lint.sh--quiet)npm run typecheck-tsgonpm run react-compiler-compliance-check check-changednpx jest tests/unit/hooks/useOutstandingReports.test.tsnpx jest tests/unit/ReportUtilsTest.ts -t "isReportIneligibleForMoveExpenses"npx jest tests/ui/IOURequestEditReportCommonTest.tsxScreenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari