Use SUBMITTED status for cancel payment optimistic data when approvals disabled#90719
Conversation
…ng payment with approvals disabled When canceling a payment on a workspace with approvalMode=OPTIONAL, the optimistic statusNum was set to CLOSED, which caused buildOptimisticNextStep to return "No further action required!" instead of "Waiting for you to pay expenses". Changed to SUBMITTED so the SUBMITTED+OPTIONAL path correctly produces the payment-waiting message. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
| const approvalMode = policy?.approvalMode ?? CONST.POLICY.APPROVAL_MODE.BASIC; | ||
|
|
||
| const stateNum: ValueOf<typeof CONST.REPORT.STATE_NUM> = CONST.REPORT.STATE_NUM.APPROVED; | ||
| const statusNum: ValueOf<typeof CONST.REPORT.STATUS_NUM> = approvalMode === CONST.POLICY.APPROVAL_MODE.OPTIONAL ? CONST.REPORT.STATUS_NUM.CLOSED : CONST.REPORT.STATUS_NUM.APPROVED; |
There was a problem hiding this comment.
@MelvinBot find out original PR which first introduced approvalMode = OPTIONAL ? CLOSED : APPROVED logic
|
PR #73936 introduced the It fixed Expensify/App#72750 — canceling a payment offline with approvals disabled showed "Outstanding" instead of "Done". The fix changed |
|
@MelvinBot your solution causes regression. |
…ext step separately Reverts the statusNum change that caused regression on #72750. The optimistic stateNum/statusNum (Approved/Closed) was correctly set by PR #73936. Instead of changing the report status, we now pass SUBMITTED as the predictedNextStatus only to the next step builders, so the report status stays CLOSED while the next step message correctly shows "waiting to pay". Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Thanks for catching the regression, I've updated the approach: |
|
@MelvinBot complete author checklist |
|
|
||
| // For OPTIONAL approval mode, the report status is CLOSED but the next step should show "waiting to pay" | ||
| // so we use SUBMITTED as the predictedNextStatus which routes through the correct next step path. | ||
| const predictedNextStatus = approvalMode === CONST.POLICY.APPROVAL_MODE.OPTIONAL ? CONST.REPORT.STATUS_NUM.SUBMITTED : statusNum; |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ 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". |
Verifies that when canceling a payment on a workspace with approvals disabled (OPTIONAL), the optimistic statusNum stays CLOSED while the nextStep correctly shows "waiting to pay" instead of "no further action". Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Added a unit test for
Test passes locally. |
|
@MelvinBot update Explanation of Change, Tests, Offline tests, QA steps based on latest commit and complete checklist. Check off all items as I verified. |
|
No regressions found. The change is well-contained — The approach cleanly decouples the report status ( Two minor behavioral differences (not regressions)These are differences from what the APPROVED path would show, but since the prior CLOSED path showed "No further action" with no actor, neither is a regression:
|
|
Updated the PR description:
|
|
@MelvinBot mark as ready for review |
|
Marked as ready for review ✅ |
|
@MelvinBot while testing, I noticed that next step is bug.mov |
Only show "Waiting to pay" after canceling a payment on OPTIONAL approval mode when the workspace has a connected bank account. Without a bank account, show "No further action required" to match backend behavior. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
@marcaaron Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Scoped the fix to only show "Waiting to pay" when the workspace has a connected bank account ( Updated the unit tests to cover both cases:
|
The achAccount object in the test was missing bankAccountID, routingNumber, addressName, bankName, and reimburser fields required by the ACHAccount type. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing typecheck: the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movweb-bank.pending.mov |
Code ReviewVerdict: Looks good. The approach is correct and well-scoped. The fix cleanly decouples the report's actual status ( The Unit tests cover both paths (with/without bank account). All CI checks pass. No issues found. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.3.75-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes required. This PR fixes an internal bug in optimistic next step computation when canceling a payment with approvals disabled ( The existing help site references to "Cancel payment" (in |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.75-14 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
When canceling a payment on a workspace with "Add Approvals" disabled (
approvalMode = OPTIONAL),buildOptimisticNextStepwas receivingstatusNum = CLOSEDas thepredictedNextStatus, which unconditionally returns "No further action required!". The report should instead go back to a payable state after payment cancellation.The optimistic
stateNum/statusNum(APPROVED/CLOSED) set by PR #73936 is correct and must not change. Instead, this PR introduces a separatepredictedNextStatusvariable that passesSUBMITTEDonly to the next step builders (buildOptimisticNextStepandbuildNextStepNew). This routes through theSUBMITTED + OPTIONALpath, which correctly produces "Waiting for you to pay expenses" — while the report's actual status staysCLOSED.A unit test was added to verify that after
cancelPaymentwithOPTIONALapproval mode:statusNumstaysCLOSEDstateNumstaysAPPROVEDnextStep.messageKeyisWAITING_TO_PAYFixed Issues
$ #75500
PROPOSAL: #75500 (comment)
Tests
Offline tests
QA Steps
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)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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari