Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open OldDot links on Safari #5802

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Open OldDot links on Safari #5802

merged 2 commits into from
Oct 13, 2021

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Oct 13, 2021

Details

Safari blocks any pop up windows that are open within async calls. This PR adds a workaround to correctly open async links in Safari.

cc @tgolen

Fixed Issues

$ #5798

Tests

  1. Log in to NewDot in Safari
  2. Create a Workspace
  3. Click on Pay Bills > View All Bills
  4. Verify that the OldDot page opens
  5. Repeat steps 3-4 for Reimburse Receipts > View All Receipts, Send Invoices > Send Invoice, and Send Invoices > View All Invoices.
  6. Repeat steps 3-5 on Chrome

QA Steps

Steps above.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web-safari.mov
web-chrome.mov

Mobile Web

mWeb.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

Note: my android emulator is not connecting to my VM, so the webpage does not load. However, the link shown in the browser is correct.

@luacmartins luacmartins self-assigned this Oct 13, 2021
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@luacmartins luacmartins marked this pull request as ready for review October 13, 2021 06:51
@luacmartins luacmartins requested a review from a team as a code owner October 13, 2021 06:51
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team October 13, 2021 06:51
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this call blocked as well?

PushNotification.onSelected(PushNotification.TYPE.REPORT_COMMENT, ({reportID}) => {
Navigation.setDidTapNotification();
Linking.openURL(`${CONST.DEEPLINK_BASE_URL}${ROUTES.getReportRoute(reportID)}`);
});

Is there no way to adapt this existing code? Looks very similar.

export default function asyncOpenURL(promise, url) {
if (!url) {
return;
}
const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent);
if (!isSafari) {
promise.then(() => {
Linking.openURL(url);
});
} else {
const windowRef = window.open();
promise
.then(() => {
windowRef.location = url;
})
.catch(() => windowRef.close());
}
}

One idea I have is to maybe make the url parameter either a string or function. The function can take the .then() value of the promise and return the url. Then you can do something like this...

const buildOldDotURL = ({shortLivedAuthToken}) => `${CONFIG.EXPENSIFY.URL_EXPENSIFY_COM}${url}${url.indexOf('?') === -1 ? '?' : '&'}authToken=${shortLivedAuthToken}&email=${encodeURIComponent(currentUserEmail)}`;
asyncOpenURL(API.GetShortLivedAuthToken(), buildOldDotURL);

marcaaron
marcaaron previously approved these changes Oct 13, 2021
@luacmartins
Copy link
Contributor Author

Thanks for the review @marcaaron!

Is this call blocked as well?

This call seems to work for me.

Is there no way to adapt this existing code? Looks very similar.

I'll take a look! I tried to adapt asyncOpenLink yesterday but I didn't get it to work. Maybe second time is the charm 😄

@luacmartins
Copy link
Contributor Author

Updated with asyncOpenUrl refactor.

@luacmartins luacmartins requested a review from marcaaron October 13, 2021 18:01
@luacmartins luacmartins merged commit 6e74f2a into main Oct 13, 2021
@luacmartins luacmartins deleted the cmartins-fixOpenOldDotLink branch October 13, 2021 19:20
github-actions bot pushed a commit that referenced this pull request Oct 13, 2021
Open OldDot links on Safari

(cherry picked from commit 6e74f2a)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @luacmartins in version: 1.1.7-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.7-25 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants