[Submit] Submit button with manager email prompt take 2 - #94392
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@MelvinBot Fix the eslint |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 The ESLint check is now passing ✅ (run, 1m23s). When you posted, the check was still in progress on the latest commit ( If you're seeing an ESLint error locally that CI isn't catching, let me know the exact message and file and I'll take a look. |
|
@MelvinBot Resolve the conflicts |
…7998 Co-authored-by: Hans Vu <hungvu193@users.noreply.github.com> # Conflicts: # src/hooks/useSelectionModeReportActions.ts
|
🤖 Resolved the merge conflicts with Conflict: How I resolved it:
Verification
The PR now shows as mergeable. Please re-test per your |
|
Fixed 3/4 blockers, there's one remaining with the landscape mode. I'll take another look today. |
|
Fixed the last blocker. Screen.Recording.2026-06-27.at.18.29.29.mov |
|
@abzokhattab 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] |
|
@abzokhattab This is ready for your review 🙇 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b12fee0c21
ℹ️ 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".
|
@MelvinBot resolve the failing test and review comments |
This comment has been minimized.
This comment has been minimized.
|
🚧 iwiznia 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/iwiznia in version: 9.4.26-0 🚀
|
|
Deploy Blocker ##95093 was identified to be related to this PR. |
|
Deploy Blocker ##95095 was identified to be related to this PR. |
|
Deploy Blocker ##95096 was identified to be related to this PR. |
|
Deploy Blocker ##95111 was identified to be related to this PR. |
|
Deploy Blocker #95139 was identified to be related to this PR. |
|
Deploy Blocker #95155 was identified to be related to this PR. |
| * Returns the email the expense report should submit to per workspace approval config | ||
| * (approval rules, employee submitsTo, or default approver for basic/optional workflows). | ||
| */ | ||
| function getSubmitToEmail(policy: OnyxEntry<Policy>, expenseReport: OnyxEntry<Report>): string { |
There was a problem hiding this comment.
@hungvu193 hi, I was trying to refactor this to remove the getLoginsByAccountIDs usage. I was wondering why getSubmitToEmail has slightly different logic from getSubmitToAccountID?
Since the name is similar, I was expecting getSubmitToEmail would just call getSubmitToAccountID and get the email from it.
There was a problem hiding this comment.
I remember AI complained about a case where submitToAccountID could be invalid, so we have to add an extra check:
if (!isValidAccountRoute(submitToAccountID)) {
return defaultApprover;
}
return getLoginsByAccountIDs([submitToAccountID]).at(0)?.trim() ?? defaultApprover;There was a problem hiding this comment.
@hungvu193 I don't think we need to check whether it's valid or not, if it's not valid, getLoginsByAccountIDs([submitToAccountID]).at(0)?.trim() will be undefined, and the fallback, which is the defaultApprover, will be returned.
Line 1804 in fec6b33
I'm actually more curious about the first 3 lines
Lines 1792 to 1796 in fec6b33
If we look at the getSubmitToAccountID, there is no early return when the expenseReport is undefined.
Lines 1751 to 1761 in fec6b33
When expenseReport is undefined, getFirstRuleApprover will return an empty string because allReportTransactions is empty.
Lines 1692 to 1696 in fec6b33
So, it'll go through the getManagerAccountID/getManagerAccountEmail logic.
Lines 1727 to 1746 in fec6b33
So, getSubmitToEmail and getSubmitToAccountID could behave differently.
There was a problem hiding this comment.
So, getSubmitToEmail and getSubmitToAccountID could behave differently.
Yeah, they serves different purposes.
getSubmitToAccountIDreturns the accountID to whom the given expenseReport submits reports to in the given Policy. While getSubmitToEmail returns the email of the default approver of the workspace, I think I actually need to rename getSubmitToEmail more cleaner since it is inside scope of Submit Workspace project.
With submit workspace, when you tried to submit the expense, a popup will be shown to select the user you want to submit your expense report to, default to the default approver of that workspace
There was a problem hiding this comment.
I think that makes sense 😄
There was a problem hiding this comment.
@hungvu193 Cool. Btw, how can I test this PR? What is submit workspace. Just want to make sure I don't break anything.
(I don't see the popover on a normal workspace)
There was a problem hiding this comment.
- Turn on the betas (Edit Permission.ts).
- Create a brand new account.
- On onboarding steps > Choose "Submit Expense to my employeer" > Complete onboarding.
- Submit an expense to your workspace you've created in onboarding.
- Notice when submitting expense a popup will be opened for you to select mananger email.
There was a problem hiding this comment.
That works, thanks!
Btw, just want to report, I think the vertical padding is too big
There was a problem hiding this comment.
I think you're right 😄 Will fix it in another PR
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Add a new ReportSubmitTo Popover that will be shown when a member of submit workspace try to submit an expense.
Design doc section
Fixed Issues
$ #87264
$ #94254
$ #94323
$ #94304
$ #94281
PROPOSAL: N/A
Tests
Same as QA Steps
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Prerequisite: You're a member of a submit workspace.
Test 1: Expense view
Test 2: Search view
** Test 3: More Button**
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
Screen.Recording.2026-05-27.at.17.19.12.mov
Android: mWeb Chrome
Screen.Recording.2026-05-27.at.17.22.55.mov
iOS: Native
Screen.Recording.2026-05-27.at.17.09.33.mov
iOS: mWeb Safari
Screen.Recording.2026-05-27.at.17.11.41.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-26.at.22.30.54.mov