Travel-user navigate to homepage instead travel site after accept the term of Book travel #69769
Conversation
… term of Book travel
|
I'm facing this issue on iOS native. I'll try again soon 
|
melvin-bot
bot
requested review from
QichenZhu
and removed request for
a team
|
@QichenZhu 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] |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
It doesn't look like you got a tangible answer on this question in the issue.
@stitesExpensify @rlinoz @Gonals ya'll would be good to chime in here before this PR potentially goes through to redirect to TravelDot in the same tab on mWeb Safari. It sounds like from the code note we don't want that.
|
Looking at the original doc/code I see no reason we decided to open a different tab, also cc: @cristipaval in case you know. |
|
So when we originally implemented travel it was always opened in a new tab. Now the idea is that in the mobile apps it opens inside of the same tab for a more cohesive experience. Given that this is mobile-web, I think that we could really go either way. If you're using the app in mobile web though I think that it could be frustrating to lose your expensify tab to do travel since there isn't any easy way to get back to expensify once you are on expensify travel |
I agree with this, it seems cumbersome without a clean way back, so I favour a different tab on mWeb. |
|
@stitesExpensify @trjExpensify Open in the same tab is the behavior we fix for the Safari mWeb/web, not all platforms. On safari, opening a new tab doesn't work in this case due to pop-up blocking policies |
|
We can't be the only app needing to navigate the need to open a new tab. |
| cleanupTravelProvisioningSession(); | ||
|
|
||
| // TravelDot is a standalone white-labeled implementation of Spotnana so it has to be opened in a new tab | ||
| Linking.openURL(buildTravelDotURL(travelProvisioning.spotnanaToken, travelProvisioning.isTestAccount ?? false)); |
There was a problem hiding this comment.
@trjExpensify, We opened this in a new tab with logic. But it doesn't work on Safari. The reason is that the navigation logic is called inside a useEffect, not in response to a user interaction, which violates pop-up blocking policies on Safari. So, on Safari, we can only set it to open in the same tab.
There was a problem hiding this comment.
We can take the navigation logic outside of the useEffect though can't we? So if we want to maintain the consistency of all mWeb/Web browsers always opening a new tab to access TravelDot, that is something we should explore. Would you agree, @stitesExpensify?
There was a problem hiding this comment.
We can take the navigation logic outside of the useEffect though can't we?
Yes, that's possible. Since we only get the URL after an API call, we can open a blank page first and then redirect once we have the URL. The downside is that users will briefly see a blank page if the API call fails.
There was a problem hiding this comment.
For my understanding, why can't we keep the user on the terms page with a loading spinner on the confirmation button until we get an API response, and then navigate (or not if it fails)? Isn't that still tied to the user's action?
The downside is that users will briefly see a blank page if the API call fails.
but maybe we're okay to live with this anyway to maintain consistency, especially if it's not super prevalent. I'd be curious for Brandon's take too.
There was a problem hiding this comment.
For my understanding, why can't we keep the user on the terms page with a loading spinner on the confirmation button until we get an API response, and then navigate (or not if it fails)? Isn't that still tied to the user's action?
This way works for other browsers but not Safari. It will block the new tab if it opens even 1s after the user's action.
There was a problem hiding this comment.
Isn't that the same behavior we have in other places that use asyncOpenURL?
Yes, it behaves like other places.
There was a problem hiding this comment.
Cool, in that case I think we should be consistent and use the same solution.
There was a problem hiding this comment.
@luacmartins To do with asyncOpenURL solution, we need to change acceptSpotnanaTerms to side effect API to get the response via promise, and then do something like this. Do you agree with this?
onSubmit={() => {
if (!hasAcceptedTravelTerms) {
setErrorMessage(translate('travel.termsAndConditions.error'));
return;
}
if (errorMessage) {
setErrorMessage('');
}
asyncOpenURL(
acceptSpotnanaTerms(domain).then((response) => {
if (response?.jsonCode !== 200) {
return;
}
if (response?.spotnanaToken) {
return buildTravelDotURL(response.spotnanaToken, response.isTestAccount ?? false);
}
}),
(travelDotURL) => travelDotURL ?? '',
)
}}
Screen.Recording.2025-09-22.at.22.19.07.mov
There was a problem hiding this comment.
Yea, that's fine. I know it's an anti-pattern in App, but we have exceptions when contacting external APIs like Spotnana's. You can see other usages of asyncOpenUrl also call the API with makeRequestWithSideEffects. Let's just add a comment explaining why we do this.
|
@QichenZhu It's ready for review again |
| return buildTravelDotURL(response.spotnanaToken, response.isTestAccount ?? false); | ||
| } | ||
| }), | ||
| (travelDotURL) => travelDotURL ?? '', |
There was a problem hiding this comment.
On error it opens a tab of NewDot. It should close the new tab.
There was a problem hiding this comment.
We already had the logic to close the tab on error here:
App/src/libs/asyncOpenURL/index.website.ts
Lines 37 to 39 in fa5e06d
|
|
||
| API.write(WRITE_COMMANDS.ACCEPT_SPOTNANA_TERMS, params, {optimisticData, successData, failureData}); | ||
| // eslint-disable-next-line rulesdir/no-api-side-effects-method | ||
| return API.makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.ACCEPT_SPOTNANA_TERMS, params, {optimisticData, successData, failureData}); |
There was a problem hiding this comment.
Please add a comment as @luacmartins asked.
Yea, that's fine. I know it's an anti-pattern in App, but we have exceptions when contacting external APIs like Spotnana's. You can see other usages of
asyncOpenUrlalso call the API withmakeRequestWithSideEffects. Let's just add a comment explaining why we do this.
|
Bump @dukenv0307 |
|
on it now |
|
@QichenZhu Can you please review again? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.webmandroid-native-error.webmAndroid: mWeb Chromeandroid-web.webmandroid-web-error.webmiOS: HybridAppios-native.movios-native-error.moviOS: mWeb Safariios-web.movios-web-error.movMacOS: Chrome / Safarimac-web.movmac-web-error.movMacOS: Desktopmac-desktop.movmac-desktop-error.mov |
|
@stitesExpensify @tylerkaraszewski all yours |
|
✋ 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/tylerkaraszewski in version: 9.2.23-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.23-3 🚀
|
8 participants
Explanation of Change
Fixed Issues
$ #69008
PROPOSAL: #69008 (comment)
Tests
Navigate to OldDot staging
Create a new expensifail account
Create a workspace
Open the console and paste this command:
var settings = NVP.get('travelSettings') || {};
settings.testAccount = true;
NVP.set('travelSettings', settings);
Refresh the browser
In workspace settings - set the following Company Address "548 Market St San Francisco, CA 94104, United States"
Offline tests
Same as above
QA Steps
Same as above
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))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-09-03.at.18.32.58.mov
Android: mWeb Chrome
Screen.Recording.2025-09-03.at.18.27.08.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2025-09-03.at.18.24.13.mov
MacOS: Chrome / Safari
Screen.Recording.2025-09-03.at.18.20.22.mov
MacOS: Desktop
Screen.Recording.2025-09-03.at.18.32.07.mov