fix: show not found page on change owner page for invalid account#68935
Conversation
|
@hoangzinh Can you please continue review this PR |
|
sure @s77rt. I will start to review it today |
|
@hoangzinh Sorry for delays, I'll update soon! |
|
@hoangzinh I updated. Additional changes is to fix some bugs. |
|
Hi @daledah can you check my comment here https://github.com/Expensify/App/pull/68935/files#r2357347709? TY |
|
friendly bump @daledah |
|
@hoangzinh Sorry for the delay. We'll call So the call here is redundant now |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@hoangzinh Sorry for the delay. The bug I mentioned here still exists so I keep current change here with some updates to prevent duplicated API calls. There's one issue I've discovered with current implementation: When we deeplink using a nonexistent account, the flow works well, but after going back we get not found page: Screen.Recording.2025-10-20.at.10.23.18.movTo fix this we should reapply this commit. What do you think? |
|
@hoangzinh sorry for the delay, here's the deeplink behaviour in prod: Screen.Recording.2025-11-26.at.14.52.22.mov |
trjExpensify
left a comment
There was a problem hiding this comment.
So niche for someone to do this 😅
| Navigation.goBack(); | ||
| Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID)); | ||
| } | ||
| InteractionManager.runAfterInteractions(() => { |
There was a problem hiding this comment.
We have ESLint failure here. We just marked using InteractionManager as deprecated for new RN version ref unless it's need to fix a critical bug.
Can we simply solve original issue and don't worry too much other edge cases until it's a real issue for customers? As long as it doesn't break the happy case then it LGTM.
There was a problem hiding this comment.
da.mov
If don't use it, the page will be not found when closing RHP.
There was a problem hiding this comment.
Should we move clearWorkspaceOwnerChangeFlow on page unmount to fix this issue? Or can we use a boolean state to fix this issue? We should avoid using InteractionManager
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-13.at.21.15.05.android.movAndroid: mWeb ChromeScreen.Recording.2025-12-13.at.20.53.27.android.c.moviOS: HybridAppScreen.Recording.2025-12-13.at.21.15.47.ios.moviOS: mWeb SafariScreen.Recording.2025-12-13.at.21.17.13.movMacOS: Chrome / SafariScreen.Recording.2025-12-13.at.20.49.54.web.mov |
|
@daledah can you resolve conflicts and add recording for IOS? |
|
@hoangzinh i resolved and added the ios record |
|
Looks like some checks are failing @daledah |
|
@srikarparsi all looks good now |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.2.82-0 🚀
|
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.2.82-0 🚀
|
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.2.82-0 🚀
|
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.2.84-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.84-8 🚀
|
| return; | ||
| } | ||
| requestWorkspaceOwnerChange(policyID, currentUserPersonalDetails.accountID, currentUserPersonalDetails.login ?? ''); | ||
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps |
There was a problem hiding this comment.
@daledah Can you please give a reason why we use only policyID as dependency here?
There was a problem hiding this comment.
This effect is meant to call requestWorkspaceOwnerChange once on page mount, so I only use policyID as a dependency.
Explanation of Change
Fixed Issues
$ #68574
PROPOSAL: #68574 (comment)
Tests
workspaces/:workspaceID/change-owner/12345/failureOffline tests
QA Steps
workspaces/:workspaceID/change-owner/12345/failurePR 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-08-22.at.00.20.56.mov
Android: mWeb Chrome
Screen.Recording.2025-08-22.at.00.30.19.mov
iOS: Native
Screen_Recording_2025-12-15_at_13.27.15.mov
iOS: mWeb Safari
Screen.Recording.2025-08-22.at.00.30.59.mov
MacOS: Chrome / Safari
Screen.Recording.2025-08-22.at.00.31.39.mov
MacOS: Desktop
Screen.Recording.2025-08-22.at.00.33.04.mov