Repository navigation
Fix Travel booking flow for non-admin workspace members - #98227
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@Krishna2323 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4989d6fc5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // check would wrongly send them to the public-domain error page. | ||
| if (!isPolicyAdmin(policy, primaryContactMethod)) { | ||
| if (!isUserValidated) { | ||
| Navigation.navigate(ROUTES.TRAVEL_VERIFY_ACCOUNT.getRoute(undefined, activePolicyID, Navigation.getActiveRoute())); |
There was a problem hiding this comment.
Resume non-admin booking after account verification
When an unvalidated non-admin taps Book travel on a provisioned/accepted workspace while the IS_TRAVEL_VERIFIED beta is enabled, this navigation includes a policyID but does not set a non-admin resume target. VerifyAccountPage interprets any policyID in that beta as navigateForwardTo = TRAVEL_ENABLE, so after OTP validation the member is sent into the enablement/terms flow instead of going straight to TravelDot as this branch intends; they never hit the openTravelDotLink(..., onError) path unless they back out and press the button again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. The verification flow could close without resuming booking or redirect beta users to Travel enablement. Added an explicit booking-resume intent and reused the existing verification-resume helper so non-admin booking continues to TravelDot, while admin provisioning remains unchanged. Added regression tests for both paths.
trjExpensify
left a comment
There was a problem hiding this comment.
Good from product on avoiding a NotFoundPage.
CC'ing @stitesExpensify for eyes on the solution though.
|
Will start reviewing today. |
There was a problem hiding this comment.
@emkhalid A member of a travel-enabled but unprovisioned workspace now gets "Something went wrong. Please try again later." It's better than the old dead end, but still misleading since Travel just isn't set up yet and retrying won't help.
Travel already has similar messaging elsewhere — travel.blockedFeatureModal and travel.domainPermissionInfo.restriction — so a generic error feels a bit out of place here.
@stitesExpensify two things:
- Should we have a specific message for this state, e.g. "Travel isn't set up for this workspace yet — ask a workspace admin to enable it"?
- Does
GenerateSpotnanaTokendistinguish "workspace not provisioned" from a real transient failure? We'd need that to show the right message.
web_chrome.mp4
|
I'm a bit confused by this solution tbh cc @marcochavezf The behavior should be:
|
|
Oh I see, I thought this was a regression of an expected behavior, also I wasn't aware of @emkhalid could you change the solution to what @stitesExpensify suggested? (cc @Krishna2323) Apologies for the confusion! |
|
Thanks for the clarification, I am working on it. |
|
@Krishna2323, I’ve updated the solution based on the above clarification:
Could you please re-review the updated approach when available? |
|
@Krishna2323 conflicts resolved can you please review, Thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_hybrid.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_hybrid.mp4iOS: mWeb Safariios_mWeb.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
|
@MelvinBot review |
|
🤖 Code review — looks solid, aligned with the requested behavior. The changes correctly implement both points from Root cause fix is correct: the NotFound was caused by Verified:
Minor observations (non-blocking)
Verdict: the implementation matches the agreed approach and the linked issue's expected behavior. Once CI is green and the manual test steps (especially step 5 — Travel hidden on a Travel-enabled but unprovisioned workspace) are confirmed on the recorded videos, this looks good to me. |
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM!
@emkhalid please merged main.
Done. |
|
All you @marcochavezf |
|
🚧 marcochavezf 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/marcochavezf in version: 9.4.66-0 🚀
|
|
Yes — help site changes were required. Draft PR: #100104 Why: this PR changed when the Travel booking entry points appear. Before, Book travel in the + menu (and the Travel quick action on Home) showed as soon as a workspace had
What the draft PR changes (docs only, 10 files):
UI verification and authoring notesLabels were checked against dev NewDot web, not written from memory:
Applied Two things I deliberately left out:
One thing I could not do: GitHub rejects @emkhalid, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
@emkhalid Applause does not have private domain, could you help us how to execute this one? Invite a private-domain user with missing legal-name details as an admin. |
Thanks for checking. You can use a fresh Please leave its legal name unset, invite it to the Travel workspace as an admin, then demote it to member and continue with the test steps. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.68-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.68-1 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Only shows the Home and FAB Travel actions for provisioned workspaces. Normal booking no longer collects legal names through the admin-only workspace Travel route; legal names remain part of the admin provisioning flow.
This prevents members from reaching the workspace Travel route that renders NotFound after their role changes.
Fixed Issues
$ #85803
PROPOSAL: #85803 (comment)
Tests
Preconditions:
+menu.Offline tests
Repeat step 5 offline after loading the workspace data while online.
QA Steps
Same as tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
macos-fix.mov