Fix incorrect first-submit next-step approver#90877
Conversation
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Steps make sense to me
|
Sorry for the delay. I have some priority PRs to review. I will review this one tomorrow. Could you merge the latest main? @emkhalid |
|
@emkhalid please merge the latest main |
|
@dmkt9 sorry for the delay, I merged the main |
|
@dmkt9 gentle bump, thanks |
@emkhalid Sorry for the delay. I think we have corrected the submit and approve flow. However, our changes broke the unapprove flow. Example: 2026-05-27.00-09-12.mp4 |
|
checking this, thanks |
|
@dmkt9 Thanks for catching the regression. I updated the fix to keep I manually tested the relevant flows while offline and after reconnecting:
All flows now work as expected. In particular, the first optimistic submit shows the rule approver correctly, while unapproving keeps the existing workflow approver. Video demos: User A: Create and submit expense Submitter-side.movUser B: Approve and unapprove expense approver-side.mp4Could you please take another look? Thanks! |
|
@codex review |
|
@dmkt9 thanks for the review, Removed the unnecessary |
|
The failing test is also not related to our PR, thanks. |
|
@emkhalid The tests are failing. Could you merge the latest main branch into this PR and re-trigger them? |
Done, thanks |
chuckdries
left a comment
There was a problem hiding this comment.
Can you talk me through how we're computing optimisticNextStepApproverID? Basically, I'm wondering why we need to call isValidAccountRoute with DEFAULT_NUMBER_ID.
| const adminAccountID = policy?.role === CONST.POLICY.ROLE.ADMIN ? currentUserAccountIDParam : undefined; | ||
| const parentReport = getReportOrDraftReport(expenseReport.parentReportID); | ||
| const managerID = getSubmitReportManagerAccountID(policy, expenseReport); | ||
| const optimisticNextStepApproverID = !isSubmitAndClosePolicy && isValidAccountRoute(managerID ?? CONST.DEFAULT_NUMBER_ID) ? managerID : undefined; |
There was a problem hiding this comment.
This is kind of awkward if I'm interpreting it correctly. Will isValidAccountRoute ever be true for CONST.DEFAULT_NUMBER_ID? If not, I think we can simplify to ...managerID !== undefined && isValidAccountRoute(managerID)... (and maybe ditch the ternary? I'm less sure whether false is equivalent to undefined here)
There was a problem hiding this comment.
(and maybe ditch the ternary? I'm less sure whether false is equivalent to undefined here)
We still need to keep undefined here because optimisticNextStepApproverID only accepts number | undefined.
@emkhalid Please update based on this
|
@emkhalid ESLint and typecheck failed |
|
@dmkt9 and @chuckdries thanks for the review, I addressed the comment. |
|
@emkhalid I see that specific test job is passing in main, try merging main one more time 🙏 |
|
@emkhalid bump |
|
Sorry for the delay. I merged the latest |
|
@dmkt9 @chuckdries Thanks for the reviews. Everything looks good from my side, so I think we’re good to move forward with this. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @chuckdries 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/chuckdries in version: 9.4.0-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no changes required. I reviewed the changes in this PR and no updates to the help site files under Why This is a backend bug fix, not a feature or behavior change. It only affects the first optimistic next-step message shown immediately after submitting an expense report that hits an approval (category approver) rule. Before this fix, that transient message could briefly show the previous/default approver; after the fix it shows the correct rule-based approver. The diff is limited to:
There are no new or changed user-facing features, tab/setting labels, buttons, or workflows. The intended end-state behavior — approval rules (e.g. a category approver) route a report to the correct approver — is already accurately documented in articles like Add approval workflows and Set workspace rules. This PR simply makes the optimistic message match that already-documented behavior, so nothing in the docs is now inaccurate or incomplete. @emkhalid — since no docs change is required, there is no linked help site PR to review. If you believe a specific article should still be updated, let me know which behavior you'd like documented and I'll create the draft PR. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.4.0-7 🚀
|
Explanation of Change
This PR fixes the incorrect approver shown in the first optimistic next-step message after submitting an expense report with an approval rule.
The submit flow already calculates the correct
managerID, but the optimistic next-step builders receive the existing report before itsmanagerIDis updated. As a result, the first optimistic message can show the previous/default approver.This change keeps
getNextApproverAccountID()unchanged and passes the freshly calculated submit manager to the optimistic next-step builders through the existingbypassNextApproverIDparameter.This fixes the first-submit message without affecting approve, unapprove, or reapprove flows.
Fixed Issues
$ #88788
PROPOSAL: #88788 (comment)
Tests
Travel.Travel -> User C.Travel.Offline tests
Travel.QA Steps
Same as tests and offline tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-native-approver-fix.mp4
Android: mWeb Chrome
android-web-approver-fix.mp4
iOS: Native
ios-native-approver-fix.mp4
iOS: mWeb Safari
ios-web-approver-fix.mp4
MacOS: Chrome / Safari
macos-approver-fix.mp4