Fix report preview approval policy flow - #93876
Conversation
|
@FitseTLT Kind ping here. Thanks! |
|
conflicts |
…ing-flow # Conflicts: # tests/actions/IOUTest/ReportWorkflowTest.ts
| expect(updatedReport?.statusNum).toBe(CONST.REPORT.STATUS_NUM.APPROVED); | ||
| }); | ||
|
|
||
| it('uses the expense report policy for approval optimism when active policy is DEW', () => { |
There was a problem hiding this comment.
| it('uses the expense report policy for approval optimism when active policy is DEW', () => { | |
| it('uses the expense report policy (not the activePolicy) to determine if it is a DEW approval', () => { |
There was a problem hiding this comment.
Updated the test name as suggested.
| // since some call sites pass `policy` as the active/personal policy rather than the report's workspace policy. | ||
| const policyToUpgrade = expenseReportPolicy ?? policy; | ||
| // IMPORTANT: gate by the workspace that owns the expense report. | ||
| const policyToUpgrade = expenseReportPolicy; |
There was a problem hiding this comment.
I don't think we need this var and comment anymore
There was a problem hiding this comment.
Updated. Since approveMoneyRequest now only uses expenseReportPolicy, the extra variable/comment is no longer needed.
|
|
||
| if (!isDEWPolicy && expenseReport.parentReportID && expenseReport.parentReportActionID) { | ||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReport.parentReportID}`, | ||
| value: { | ||
| [expenseReport.parentReportActionID]: { | ||
| childStateNum: predictedNextState, | ||
| childStatusNum: predictedNextStatus, | ||
| ...(nextApproverAccountID ? {childManagerAccountID: managerID} : {}), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReport.parentReportID}`, | ||
| value: { | ||
| [expenseReport.parentReportActionID]: { | ||
| childStateNum: expenseReport.stateNum, | ||
| childStatusNum: expenseReport.statusNum, | ||
| ...(nextApproverAccountID ? {childManagerAccountID: expenseReport.managerID} : {}), | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
It mirrors the predicted approval state into the parent REPORT_PREVIEW action's childStateNum, childStatusNum, and childManagerAccountID, matching the existing reopen/retract/unapprove patterns. Without this, the parent preview can keep stale child state during optimistic approval.
There was a problem hiding this comment.
totally out of scope. It should be removed.
There was a problem hiding this comment.
Hmm, i got it, the confussion occured for me as i mentioned this "update parent preview childStateNum, childStatusNum, childManagerAccountID" in proposal and thought i have to add it inorder to keep pr scoprd to approved proposal. but i agree that i should have reconsidered it while applying in this pr. as the parent preview mirror was included as secondary consistency work, but it is not required for the approve-button flicker fix. Applogies
|
|
||
| const reportAction = Object.values(reportActionsUpdate?.value as Record<string, ReportAction>).at(0); | ||
| expect(reportAction?.delegateAccountID).toBe(DELEGATE_ACCOUNT_ID); | ||
| expect(getOptimisticReportAction(expenseReport.reportID)?.delegateAccountID).toBe(DELEGATE_ACCOUNT_ID); |
There was a problem hiding this comment.
What are these unrelated changes you made in this test file? Pls avoid out of scope changes. You always need to confirm yourself changes made by AI platforms according to Expensify rules. Stop committing AI changes directly and expect the C+ reviewer to do the checking for you. This a formal warning @KJ21-ENG
There was a problem hiding this comment.
Actually the idea behind introducing the helper was to avoid repeated mock-call inspection while updating the approveMoneyRequest tests. But I reconsider it as applying that cleanup across unrelated delegate cases was broader than this pr needs.
…ing-flow # Conflicts: # src/hooks/useHoldMenuSubmit.ts
|
@FitseTLT Can you please shade some light on for what 'formal warning' stands for ? i went through matt's this message on slack but i didnt get much clarity. and one more humble request from my side that can you please wave off the 'formal warning' , my intensiion was not to breach any guidline and i will be more sharp for this kind of thing in future. Its humble request from my side and sincere applogy as well. |
|
@KJ21-ENG Read https://github.com/Expensify/App/blob/main/contributingGuides/AI_ETIQUETTE.md You should always make sure to take responsibility and check before making or suggesting changes generated by AI. Random unrelated changes here and there made me suspect you are directly pushing AI changes. If it is you no problem everyone makes mistakes, but if it is not, you are violating the AI_ETIQUETTE rules that's what I am pointing to. |
No I meant my own |
|
Ah, got it. Now I understand Matt's message as well as yours. I will definitely keep note of this for future. Thanks! |
| it('gates upgrade by expenseReportPolicy even when the report policyID is Submit', () => { | ||
| const expenseReport = createSubmittedExpenseReport(submitPolicyID); | ||
|
|
||
| approveMoneyRequest(createApproveMoneyRequestParams(expenseReport, submitPolicy, teamPolicy)); | ||
| approveMoneyRequest(createApproveMoneyRequestParams(expenseReport, teamPolicy)); | ||
|
|
||
| expect(Navigation.navigate).toHaveBeenCalledTimes(1); | ||
| expect(Navigation.navigate).toHaveBeenCalledWith(expectedRoute); | ||
| expect(Navigation.navigate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('falls back to policy when expenseReportPolicy is undefined', () => { | ||
| it('does not fall back to another policy when expenseReportPolicy is undefined', () => { | ||
| const expenseReport = createSubmittedExpenseReport(submitPolicyID); | ||
| const expectedRoute = ROUTES.WORKSPACE_UPGRADE.getRoute(submitPolicyID, upgradeFeatureAlias, ROUTES.REPORT_WITH_ID.getRoute(expenseReport.reportID), expenseReport.reportID); | ||
|
|
||
| approveMoneyRequest({ | ||
| ...createApproveMoneyRequestParams(expenseReport, submitPolicy), | ||
| expenseReportPolicy: undefined, | ||
| policy: submitPolicy, | ||
| }); | ||
| approveMoneyRequest(createApproveMoneyRequestParams(expenseReport, undefined)); | ||
|
|
||
| expect(Navigation.navigate).toHaveBeenCalledTimes(1); | ||
| expect(Navigation.navigate).toHaveBeenCalledWith(expectedRoute); | ||
| expect(Navigation.navigate).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
I think these two tests are obsolete now that we don't have policy param
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-06-25.15-32-20.mp4Android: mWeb ChromeiOS: HybridApp2026-06-25.15-49-46.mp4iOS: mWeb Safari2026-06-25.15-56-37.mp4MacOS: Chrome / Safari2026-06-25.15-54-53.mp4 |
|
🚧 carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.4.21-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.21-9 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This fixes the report preview approval path so approval workflow decisions use the expense report's policy instead of the currently active workspace policy.
The preview approval callers now pass the expense report policy to
approveMoneyRequest().approveMoneyRequest()also defensively derives a workflow policy fromexpenseReportPolicyand uses it for DEW detection, optimistic next-step generation, duplicate checks, held-expense approval data, and parent report preview status mirroring.This keeps DEW behavior backend-driven while allowing normal reports to optimistically update
stateNum,statusNum, andmanagerIDeven ifNVP_ACTIVE_POLICY_IDpoints at a DEW workspace.Fixed Issues
$ #93067
PROPOSAL: #93067 (comment)
Tests
Approvebutton on the report preview.Approveonce and does not briefly showApproveagain after the approved animation clears.Validation run:
python3 /Users/kushjariwla/.codex/skills/expensify-app-targeted-checks/scripts/touched_files.py --base upstream/mainpython3 /Users/kushjariwla/.codex/skills/expensify-app-targeted-checks/scripts/touched_files.py --kind format | xargs npx prettier --writepython3 /Users/kushjariwla/.codex/skills/expensify-app-targeted-checks/scripts/touched_files.py --kind lint | xargs npx eslint --cache --cache-location=node_modules/.cache/eslint --cache-strategy content --quiet --concurrency=auto --no-warn-ignoredpython3 /Users/kushjariwla/.codex/skills/expensify-app-targeted-checks/scripts/touched_files.py --kind react | xargs npm run react-compiler-compliance-check checkgit diff --checkgit diff --check upstream/main...HEADnpm test -- tests/actions/IOUTest/ReportWorkflowTest.ts --runInBand -t "uses the expense report policy for approval optimism when active policy is DEW" --moduleNameMapper='{"^@expensify/react-native-hybrid-app$":"<rootDir>/modules/hybrid-app/src/index.ts"}'npm run typecheck-tsgowas not run because this PR does not change TypeScript signatures, exported helper types, or React component props.Offline tests
N/A. This change fixes the online approval transition for an action that requires the backend approval request. It does not add or change offline behavior, offline copy, offline queueing, or offline error handling.
QA Steps
Approvebutton on the report preview.Approveonce and does not briefly showApproveagain after the approved animation clears.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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)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. (N/A: no new page was added)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
VIDEO-2026-06-18-19-59-04.mp4
Android: mWeb Chrome
VIDEO-2026-06-18-18-47-45.mp4
iOS: Native
Screen.Recording.2026-06-18.at.8.39.01.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-06-18.at.6.52.37.PM.mov
MacOS: Chrome / Safari
ezyZip.52.mp4