Skip to content

Rename route so that we expect traditional query paramters#5430

Merged
TomatoToaster merged 3 commits into
mainfrom
amal-query-param-transition
Sep 23, 2021
Merged

Rename route so that we expect traditional query paramters#5430
TomatoToaster merged 3 commits into
mainfrom
amal-query-param-transition

Conversation

@TomatoToaster

@TomatoToaster TomatoToaster commented Sep 22, 2021

Copy link
Copy Markdown
Contributor

@cead22, please review

All we need to do here is change the route and the query params pulled from here are expected to come after ?.

These lines essentially work the same, it just won't look inbetween the slashes for the params:

const accountID = parseInt(lodashGet(this.props.route.params, 'accountID', ''), 10);
const email = lodashGet(this.props.route.params, 'email', '');
const shortLivedToken = lodashGet(this.props.route.params, 'shortLivedToken', '');
const encryptedAuthToken = lodashGet(this.props.route.params, 'encryptedAuthToken', '');

Details

Discussion from here: https://expensify.slack.com/archives/C020EPP9B9A/p1632347607474000
related to this PR: https://github.com/Expensify/Web-Expensify/pull/31996
and this issue: https://github.com/Expensify/Expensify/issues/177739

Tests

QA Steps

Same as here: #5396, but now it should actually work.
@MitchExpensify and @kevinksullivan will be testing this so QA team can skip over this one 👍🏾

Flow 1: Unvalidated new accounts

  1. Create a new account in OldDot that has access to the freePlan beta.

  2. Go to settings -> policy -> group and select the free plan here:
    image

  3. Verify that a new tab opens and you are logged into NewDot and you are seeing the Workspace Settings page for a newly created Workspace:
    image

  4. Wait a minute

  5. Do any action (like add a comment)

  6. Check you are not logged out

Flow 2: Existing accounts

  1. Log into an account on NewDot Expensify, create a Workspace if you don't already have one using the + on the bottom right.
  2. Log out of NewDot and then log into that same account on OldDot and go to Settings -> Policy and find your Workspace there. It should have a card icon like these:
    image
  3. Click on its name and verify it opens a new tab, automatically logs you in, and brings you to the workspace settings page:
    image
  4. Wait a minute
  5. Do any action (like add a comment)
  6. Check you are not logged out

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

N/A

@github-actions

Copy link
Copy Markdown
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.

@TomatoToaster TomatoToaster marked this pull request as ready for review September 22, 2021 23:25
@TomatoToaster TomatoToaster requested a review from a team as a code owner September 22, 2021 23:25
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team September 22, 2021 23:26
@cead22

cead22 commented Sep 22, 2021

Copy link
Copy Markdown
Contributor

These lines essentially work the same, it just won't look inbetween parentheses for the params:

I'm assuming you means slashes, not parenthesis 👍

@TomatoToaster

Copy link
Copy Markdown
Contributor Author

Ah sorry yes I meant in between the slashes 😄

cead22
cead22 previously approved these changes Sep 22, 2021
thienlnam
thienlnam previously approved these changes Sep 22, 2021
@TomatoToaster TomatoToaster changed the title Rename route so that we expect traditional query paramters [HOLD] Rename route so that we expect traditional query paramters Sep 23, 2021
@TomatoToaster TomatoToaster dismissed stale reviews from thienlnam and cead22 via 434584e September 23, 2021 00:26
@TomatoToaster TomatoToaster changed the title [HOLD] Rename route so that we expect traditional query paramters Rename route so that we expect traditional query paramters Sep 23, 2021
@TomatoToaster

Copy link
Copy Markdown
Contributor Author

Could you review again @cead22 @thienlnam, I realized I should change this log line: https://github.com/Expensify/App/pull/5430/files#diff-c3d3bf8bc4056d8f2e8ff15f6d72d7e81656cd36c30a202a084c50dda1dff582L35

since the route changed and we still don't want to be storing the authToken in the log lines.

@TomatoToaster TomatoToaster merged commit 1c6c70f into main Sep 23, 2021
@TomatoToaster TomatoToaster deleted the amal-query-param-transition branch September 23, 2021 01:22
github-actions Bot pushed a commit that referenced this pull request Sep 23, 2021
Rename route so that we expect traditional query paramters

(cherry picked from commit 1c6c70f)
@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by @TomatoToaster in version: 1.1.1-3 🚀

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.

@MitchExpensify

MitchExpensify commented Sep 23, 2021

Copy link
Copy Markdown
Contributor
  • Flow 1 on desktop
  • Flow 2 on desktop

@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 ✅

@OSBotify

Copy link
Copy Markdown
Contributor

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

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

@OSBotify

Copy link
Copy Markdown
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