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

Reintroduce workspace new without a modal, just the url #5396

Merged
merged 17 commits into from
Sep 22, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Sep 21, 2021

CC: @iwiznia, @cead22

Details

Reintroduces the Workspace/New command which will be used in inboxTasks and the pricing page from OldDot.

Note: this PR includes changes from this one and is held on it:

Fixed Issues

related to Expensify/Expensify#177739

Tests

Same as QA done locally.

Make sure you have these other PRs running locally
Expensify/Web-Expensify#31977
Expensify/Auth#5963

QA Steps

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

Included in QA

@TomatoToaster TomatoToaster changed the base branch from ionatan_link_fromolddot to main September 22, 2021 04:27
@TomatoToaster TomatoToaster changed the title Reintroduce workspace new without a modal, just the url [HOLD] Reintroduce workspace new without a modal, just the url Sep 22, 2021
@TomatoToaster TomatoToaster changed the title [HOLD] Reintroduce workspace new without a modal, just the url Reintroduce workspace new without a modal, just the url Sep 22, 2021
@TomatoToaster TomatoToaster marked this pull request as ready for review September 22, 2021 14:52
@TomatoToaster TomatoToaster requested a review from a team as a code owner September 22, 2021 14:52
@TomatoToaster TomatoToaster removed the request for review from a team September 22, 2021 14:52
@TomatoToaster TomatoToaster requested review from marcaaron, cead22 and Julesssss and removed request for nickmurray47 September 22, 2021 14:52
github-actions bot pushed a commit that referenced this pull request Sep 22, 2021
Reintroduce workspace new without a modal, just the url

(cherry picked from commit cb6b55a)
@marcaaron
Copy link
Contributor

Heads up was testing this some more and running into an issue when tapping on the existing workspace in OldDot

2021-09-22_08-14-21

Error here is thrown ->

// After all the policies have loaded, we can know if the given policyID points to a nonexistant workspace
// When free plan is out of beta and Permissions.canUseFreePlan() gets removed,
// all code involving 'allPolicies' can be removed since policy loading will no longer be delayed on login.
if (allPolicies !== null && _.isEmpty(policy)) {
Growl.error(translate('workspace.error.growlMessageInvalidPolicy'), CONST.GROWL.DURATION_LONG);
Navigation.dismissModal();
return null;
}

@marcaaron
Copy link
Contributor

also looks like we are logging this authToken here.

2021-09-22_08-19-24

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

also looks like we are logging this authToken here.

Is it being logged in the logs? If is just console then does not matter much

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

Heads up was testing this some more and running into an issue when tapping on the existing workspace in OldDot

Can you give steps to reproduce please?

@marcaaron
Copy link
Contributor

Log.info('Navigating to route', false, {path});

@marcaaron
Copy link
Contributor

  1. Pull main + npm run web
  2. Log out of New Dot
  3. Tap on the existing workspace link in OldDot /admin_policies?param={"section":"group"}
  4. 💥

@TomatoToaster
Copy link
Contributor Author

TomatoToaster commented Sep 22, 2021

Oh we could skip that log line if the path contains /transition/ and log `"transition from OldDot" or something. It's a token that expires in a minute but we probably still don't want to be logging that.

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @AndrewGable in version: 1.1.1-1 🚀

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.

@TomatoToaster
Copy link
Contributor Author

Fixing the authToken logging here: #5425. Set put the CP label on it so it'll be ready as soon as it's merged.

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

Oh, so it's probably being logged. We should remove it, although they are short lived tokens.

@mvtglobally
Copy link

@TomatoToaster @iwiznia are we ok to QA this PR now or need to wait ?

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

You can skip QA on this one. @kevinksullivan, @MitchExpensify and some other volunteers are testing this whole flow.

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

Tried doing this in dev, I reproduced it once and never again. Must be a race condition.

@iwiznia
Copy link
Contributor

iwiznia commented Sep 22, 2021

Scratch that, I forgot to sign out again. Seems it is reproducible.

@TomatoToaster
Copy link
Contributor Author

Here's a summary of how to recreate this issue:

Following the testing flow for an 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

^ This last step fails if you are logged out of NewDot beforehand. It briefly flickers the modal but then says workspace is invalid and closes it.

This error is being thrown here:

// After all the policies have loaded, we can know if the given policyID points to a nonexistant workspace
// When free plan is out of beta and Permissions.canUseFreePlan() gets removed,
// all code involving 'allPolicies' can be removed since policy loading will no longer be delayed on login.
if (allPolicies !== null && _.isEmpty(policy)) {
Growl.error(translate('workspace.error.growlMessageInvalidPolicy'), CONST.GROWL.DURATION_LONG);
Navigation.dismissModal();
return null;
}

I assumed this had to do with the wonky logic we implemented ^ in order to wait for the policies to be loaded before we check that it doesn't exist. A solution might just be to get rid of that error and not care if users enter invalid links to reach the workspaces. I think this will work because eventually Onyx will load with the policy details and show the correct thing (haven't tested this yet).

I noticed that if you get rid of isDevelopment() here:

return isDevelopment() || _.contains(betas, CONST.BETAS.ALL);

It just never appears to load the Workspace component. This is likely because isDevelopment() makes all betas active (allows us to load the policies instantly) and that's why we see the modal flicker and work at first. It seems like if we wait for Onyx to load the betas, then run it could work.

If just getting rid of the error works, then I'm fine with that solution but I think the ideal solution keeps the Invalid Workspace

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Sep 23, 2021

  • Flow 1 tested on desktop - No issues
  • Flow 2 tested on desktop - No issues

Nether flow is available on Mobile so I think we're good here

@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 @marcaaron in version: 1.1.1-9 🚀

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

@isagoico
Copy link

isagoico commented Sep 24, 2021

I see this PR has been added to the new checklist. Should this PR be QAd again? CC @iwiznia @TomatoToaster

@Jag96
Copy link
Contributor

Jag96 commented Sep 24, 2021

Looks like its already on production, @isagoico we can check this one off

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

10 participants