Skip to content

Fix link to oldDot#5359

Merged
tgolen merged 3 commits into
mainfrom
ionatan_linktoolddot
Sep 21, 2021
Merged

Fix link to oldDot#5359
tgolen merged 3 commits into
mainfrom
ionatan_linktoolddot

Conversation

@iwiznia

@iwiznia iwiznia commented Sep 20, 2021

Copy link
Copy Markdown
Contributor

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/177739

Tests

I tested both scenarios below on dev with carlos@expensifail.com and carlos+test@expensifail.com after @marcaaron pointed out that it wasn't working for him with an email that had a plus sign in it. So I added the encodeURIComponent call and tested with carlos+test.

That said, this only works if the account clicking manage cards is a domain admin, so this isn't quite working for all cases

  • Had to edit some code to see the option to create workspace (but I think that's fine)
  • Sign out of e.com
  • Click on big + button
  • Click Create workspace
  • Click manage cards
  • Check it opened the correct page on e.com and that you are signed in
  • I also live edited the code to link to the settings page (since that is used in some other flow)

  • Clear all cookies on e.com
  • Click on big + button
  • Click Create workspace
  • Click manage cards
  • Check it opened the correct page on e.com and that you are signed in

QA Steps

Run both scenarios below with an email that doesn't have a plus sign in it, and one that does

  • Sign out of e.com
  • Click on big + button
  • Click Create workspace
  • Click manage cards
  • Check it opened the correct page on e.com and that you are signed in

  • Clear all the cookies on e.com
  • Click on big + button
  • Click Create workspace
  • Click manage cards
  • Check it opened the correct page on e.com and that you are signed in

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@iwiznia iwiznia self-assigned this Sep 20, 2021
@iwiznia iwiznia requested a review from tgolen September 20, 2021 19:05
@iwiznia iwiznia marked this pull request as ready for review September 20, 2021 19:05
@iwiznia iwiznia requested a review from a team as a code owner September 20, 2021 19:05
@MelvinBot MelvinBot requested review from Dal-Papa and removed request for a team September 20, 2021 19:06
@iwiznia

iwiznia commented Sep 20, 2021

Copy link
Copy Markdown
Contributor Author

Did not have time to test this in other platforms, but I think we don't really care if this is broken because that flow is broken in mobile anyway. There is (or should be) another issue to change the button to say to log in to e.com in desktop.

@iwiznia iwiznia requested review from cead22 and removed request for Dal-Papa September 20, 2021 19:14
@iwiznia

iwiznia commented Sep 20, 2021

Copy link
Copy Markdown
Contributor Author

If we should test in other platforms, can someone please do that? This is a blocker for N6 and ideally all blockers should be resolved by EOD tomorrow. If no one can, I can do that tomorrow morning.

Comment thread src/libs/actions/App.js
function openSignedInLink(url = '') {
API.GetAccountValidateCode().then((response) => {
const exitToURL = url ? `?exitTo=${url}` : '';
const validateCodeUrl = `v/${currentUserAccountID}/${response.validateCode}${exitToURL}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How come there is no more exitTo param?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The exitTo was there for the validate page to redirect to the page we wanted after validating. Now, we don't need it because we send users directly to the page we want, without any intermediary pages

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it, thanks!

@cead22 cead22 requested a review from marcaaron September 21, 2021 00:23

@marcaaron marcaaron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM and consistently passing the test steps...

However, we noticed here that the user must be a domain admin or else the code here prevents log in.

@marcaaron

Copy link
Copy Markdown
Contributor

If we should test in other platforms, can someone please do that?

I tested on Desktop. We could test on mobile, but I'm not sure if the experience on the OldDot side is too mobile friendly. Seems unlikely we'll improve it at this point.

@tgolen tgolen merged commit 1136bf8 into main Sep 21, 2021
@tgolen tgolen deleted the ionatan_linktoolddot branch September 21, 2021 11:30
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @tgolen in version: 1.1.0-3 🚀

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

@isagoico

Copy link
Copy Markdown

PR is failing because of #5382 (comment)

@isagoico

Copy link
Copy Markdown

@iwiznia When testing this again I was redirected to E.com login instead of being signed in. I also noticed that the URL has Undefined in the auth token and email part.
image

Good news is that the button is clickable now and not completely unresponsive

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @Jag96 in version: 1.1.1-8 🚀

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.

6 participants