Allow inviting new users in the Add approval workflow flow#75589
Merged
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Contributor
Author
|
@hoangzinh since this is a new feature, I will upload all platform recordings once i address any comments / code suggestions you may leave after your review. This helps in avoiding regressions since the PR is tested with new changes, let me know if you require videos before you start testing, looking for your review 😄 |
melvin-bot
Bot
requested review from
hoangzinh and
trjExpensify
and removed request for
a team
|
@hoangzinh 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Author
|
Implementation video for reference Screen.Recording.2025-11-20.at.1.59.29.AM.mov |
trjExpensify
requested review from
JmillsExpensify
and removed request for
trjExpensify
Contributor
|
@JmillsExpensify has been on this one, so switching us out. |
Contributor
|
@twilight2294, can you check code review feedbacks above? |
Contributor
|
I'd like to test an adhoc when we get through C+ review. |
Contributor
Author
|
@hoangzinh PR is ready for your review |
hoangzinh
reviewed
Nov 27, 2025
| const policyName = policy?.name; | ||
|
|
||
| const headerTitle = useMemo(() => { | ||
| if (backTo && typeof backTo === 'string' && backTo.includes('expenses-from')) { |
Contributor
There was a problem hiding this comment.
Can we put this one into a variable backTo && typeof backTo === 'string' && backTo.includes('expenses-from') ? And reuse it from below
Contributor
There was a problem hiding this comment.
Can we put expenses-from into CONST as well? (also use it to generate WORKSPACE_WORKFLOWS_APPROVALS_EXPENSES_FROM.route as well)
| } | ||
|
|
||
| // If backTo is provided and it's not the members page, navigate back to it | ||
| if (backTo && !(backTo as string).endsWith('members')) { |
Contributor
There was a problem hiding this comment.
I think we should check 'expenses-from' ROUTE only and goBack to avoid hidden bugs
Comment on lines
+133
to
+134
| // Note: We don't auto-navigate to the approver page after returning from invite | ||
| // The user should stay on this page to see their selected members and click "Next" manually |
Contributor
There was a problem hiding this comment.
Which code does this comment explain for?
Comment on lines
+46
to
+53
| const ineligibleInvites = getIneligibleInvitees(policy?.employeeList); | ||
| return ineligibleInvites.reduce( | ||
| (acc, login) => { | ||
| acc[login] = true; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, boolean>, | ||
| ); |
Contributor
There was a problem hiding this comment.
I think we can put this one into a sharable func
Comment on lines
+72
to
+77
| const handleSearchChange = useCallback( | ||
| (term: string) => { | ||
| setSearchSelectorTerm(term); | ||
| }, | ||
| [setSearchSelectorTerm], | ||
| ); |
Contributor
There was a problem hiding this comment.
why don't we use setSearchSelectorTerm directly instead of defining new handleSearchChange func?
Comment on lines
+266
to
+278
| // Ensure avatars are only strings (URLs) or undefined, never React components | ||
| const allMembers: Member[] = [ | ||
| ...existingMembers.map((member) => ({ | ||
| ...member, | ||
| avatar: typeof member.avatar === 'string' ? member.avatar : undefined, | ||
| })), | ||
| ...usersToInvite.map((user) => ({ | ||
| displayName: user.email, | ||
| email: user.email, | ||
| avatar: undefined, | ||
| })), | ||
| ]; | ||
| setApprovalWorkflowMembers(allMembers); |
Contributor
There was a problem hiding this comment.
Do you think it's same with https://github.com/Expensify/App/pull/75589/files#diff-0a4739399b93f09a6f9466afba489945b2c9dc18700f579b75e71a0d6e9167ffR290-R298
If yes, we can move it to the top to apply for both usersToInvite.length > 0 and else
Contributor
|
🚧 @JmillsExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
Kicked off an adhoc build |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Addressing the review today |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-25.at.04.26.26.android.movAndroid: mWeb ChromeScreen.Recording.2025-12-25.at.04.30.22.android.chrome.moviOS: HybridAppScreen.Recording.2025-12-25.at.04.41.53.ios.moviOS: mWeb SafariScreen.Recording.2025-12-25.at.04.36.47.ios.s.movMacOS: Chrome / SafariScreen.Recording.2025-12-25.at.04.15.08.web.mov |
Contributor
|
@twilight2294 can you also add recordings for this issue? |
Contributor
Author
|
@hoangzinh i updated |
Contributor
Author
|
bump @hoangzinh , can you approve the PR please |
Contributor
|
@twilight2294 I also have a minor feedback here https://github.com/Expensify/App/pull/75589/files#r2646258642 |
twilight2294
commented
Dec 30, 2025
Co-authored-by: Vinh Hoang <hoangzinhvn@gmail.com>
Contributor
Author
|
@hoangzinh back to you |
youssef-lr
approved these changes
Dec 31, 2025
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
21 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.2.91-0 🚀
|
|
@twilight2294 This PR is failing because of a regression issue #78775 . The issue is reproducible in: Web Bug7040498_1767351637415.2.mp4 |
This was referenced Jan 2, 2026
Contributor
|
Going to revert the pr due to bunch of minor issues, please try to address them all in v2 and include the testing steps in the Pr description please |
Contributor
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.91-1 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #61625
PROPOSAL: #61625 (comment)
Tests
Offline tests
QA Steps
expenses fromsection, type a user who is not a member of the workspaceVerify that that user appears
Verify that an invite screen is shown
Verify that you are taken back to the
expenses fromrhpVerify that you are able to complete the flow successfully
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.Screenshots/Videos
Android: Native
Screen.Recording.2025-12-27.at.8.35.11.PM.mov
Android: mWeb Chrome
Screen.Recording.2025-12-27.at.8.35.52.PM.mov
iOS: Native
Simulator.Screen.Recording.-.Expensify.Testing.18.6.Pro.-.2025-12-27.at.18.53.54.mov
iOS: mWeb Safari
Simulator.Screen.Recording.-.Expensify.Testing.18.6.Pro.-.2025-12-27.at.18.54.41.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-27.at.6.50.07.PM.mov