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

Use correct api params for NewDot>OldDot links #5466

Merged
merged 3 commits into from
Sep 23, 2021

Conversation

marcaaron
Copy link
Contributor

Details

Looks like we changed the GetShortLivedAuthToken command to just return shortLivedAuthToken and encryptedAuthToken but didn't consider the change for NewDot > OldDot links.

Fixed Issues

#5359 (comment)

Tests

To test this I just quickly applied this diff and then tapped on manage cards (I don't have this set up on dev but I think it will work fine). Without the other changes we get the undefined stuff.

diff --git a/src/pages/workspace/WorkspaceCardPage.js b/src/pages/workspace/WorkspaceCardPage.js
index 4eee5d7f9..386c5a66e 100644
--- a/src/pages/workspace/WorkspaceCardPage.js
+++ b/src/pages/workspace/WorkspaceCardPage.js
@@ -100,13 +100,13 @@ const WorkspaceCardPage = ({
     }

     const onPress = () => {
-        if (user.isFromPublicDomain) {
-            openSignedInLink(CONST.ADD_SECONDARY_LOGIN_URL);
-        } else if (user.isUsingExpensifyCard) {
+        // if (user.isFromPublicDomain) {
+        //     openSignedInLink(CONST.ADD_SECONDARY_LOGIN_URL);
+        // } else if (user.isUsingExpensifyCard) {
             openSignedInLink(CONST.MANAGE_CARDS_URL);
-        } else {
-            openBankSetupModal();
-        }
+        // } else {
+        //     openBankSetupModal();
+        // }
     };

QA Steps

Repeat steps in #5359 (comment)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron requested a review from a team as a code owner September 23, 2021 19:23
@marcaaron marcaaron self-assigned this Sep 23, 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.

@MelvinBot MelvinBot requested review from chiragsalian and removed request for a team September 23, 2021 19:23
cead22
cead22 previously approved these changes Sep 23, 2021
Copy link
Contributor

@cead22 cead22 left a comment

Choose a reason for hiding this comment

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

I reproduced without this fix, and couldn't reproduce with this fix

Jag96
Jag96 previously approved these changes Sep 23, 2021
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM, good find!

@marcaaron marcaaron dismissed stale reviews from Jag96 and cead22 via da910c3 September 23, 2021 21:04
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jag96
Copy link
Contributor

Jag96 commented Sep 23, 2021

Skipping unnecessary e2e to get this to staging

@Jag96 Jag96 merged commit 420504f into main Sep 23, 2021
@Jag96 Jag96 deleted the marcaaron-fixNewToOldDotLinks branch September 23, 2021 21:11
github-actions bot pushed a commit that referenced this pull request Sep 23, 2021
Use correct api params for NewDot>OldDot links

(cherry picked from commit 420504f)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @Jag96 in version: 1.1.1-7 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 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 @Jag96 in version: 1.1.1-8 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Jag96 in version: 1.1.1-9 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 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.

5 participants