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

Skip showing create for new users if user already has a workspace #5244

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Sep 14, 2021

cc @kevinksullivan @MitchExpensify @mateomorris

Details

In this comment we discovered that the global create menu still shows up if a user has a workspace, which is not what we want. This PR fixes that.

Fixed Issues

$ #5047 (part 2)

Tests

New test:

  1. Run the app locally (on whatever device you're testing on)
  2. Create a brand new account in expensify.com.dev
  3. Go to https://www.expensify.com.dev/admin_policies, create a free plan
  4. Verify the global create menu doesn't pop up

To make sure there's no regressions (from the original PR):

Ensure that menu opens for new users & existing users logging in for the first time since update

  1. Sign in
  2. Validate that menu opens
  3. Refresh
  4. Validate menu doesn't open

Ensure that menu doesn't open for existing users who the menu has already opened for

  1. Sign in
  2. Validate that menu doesn't open

QA Steps

New test:

  1. Create a brand new account in expensify.com
  2. Go to https://www.staging.expensify.com/admin_policies, create a free plan
  3. Verify the global create menu doesn't pop up behind

To make sure there's no regressions (from the original PR):

Using a brand new account:

  1. Sign in after signing up
  2. Verify that the global create menu opens automatically

Using an existing account which hasn't seen the menu open automatically yet

  1. Sign in
  2. Verify that the global create menu opens automatically

Using the same existing account

  1. Refresh the page
  2. Verify that the global create menu doesn't open

Tested On

  • Web
  • Mobile Web (having trouble with this)
  • Desktop (Can't test here since we don't have a desktop old expensify app that could open the desktop new expensify app)
  • iOS (Can't create a workspace from old mobile expensify)
  • Android (Can't create a workspace from old mobile expensify)

Screenshots

Web

Screen.Recording.2021-09-14.at.3.37.23.PM.mov

Mobile Web

Really difficult to get this... May only be possible in staging

Desktop

N/A

iOS

N/A

Android

N/A

@Beamanator Beamanator requested a review from a team as a code owner September 14, 2021 13:31
@Beamanator Beamanator self-assigned this Sep 14, 2021
@MelvinBot MelvinBot requested review from robertjchen and removed request for a team September 14, 2021 13:31
@Beamanator Beamanator changed the title Skip showing create for new users if has workspace Skip showing create for new users if user already has a workspace Sep 14, 2021
Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@robertjchen robertjchen merged commit 09e2d94 into main Sep 15, 2021
@robertjchen robertjchen deleted the beaman-skipShowingCreateForNewUsersIfHasWorkspace branch September 15, 2021 17:07
@OSBotify
Copy link
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
Contributor

🚀 Deployed to staging by @robertjchen in version: 1.0.98-2 🚀

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

@isagoico
Copy link

@Beamanator I was unable to test the New Test Section because of this issue https://github.com/Expensify/Expensify/issues/177739. The rest of the steps were a pass.

@isagoico
Copy link

@Beamanator Friendly bump here. https://github.com/Expensify/Expensify/issues/177739 is blocking a section of QA

@francoisl
Copy link
Contributor

Testing this @kevinksullivan right now

@francoisl
Copy link
Contributor

Hm yeah so it sounds like we can't fully test this until https://github.com/Expensify/Expensify/issues/177739 is resolved unfortunately, you can't create a workspace right now.

@Beamanator feel free to post an update if you can think of an alternative way to test

@Beamanator
Copy link
Contributor Author

Sorry for being super slow responding! :O Not sure how I missed this

@Beamanator
Copy link
Contributor Author

Since the new workspace creation happens once we are redirected to New Expensify, I was hoping we should be able to do:

  1. Create brand new account in staging.expensify.com/inbox
  2. Figure out your account ID via User.getAccountID()
  3. Watch network requests for GetAccountValidateCode when creating a free plan from https://www.staging.expensify.com/admin_policies
    • keep Validate Code for next step
  4. Manually navigate to http://staging.new.expensify.com/v/<accountID>/<validateCode>/workspace/new-workspace
  5. Verify the global create menu doesn't show up, since a workspace should have been created for the user

However, GetAccountValidateCode failed 🤷 @francoisl do you know of another way to get a validate code?

@flodnv
Copy link
Contributor

flodnv commented Sep 20, 2021

GetAccountValidateCode failed

This is expected. See https://expensify.slack.com/archives/CC7NECV4L/p1631817495283000

@jboniface
Copy link

jboniface commented Sep 20, 2021

This PR is blocking deploy and can't be tested until https://github.com/Expensify/Expensify/issues/177739 is fixed. can we please revert this PR so that we can deploy without it?

cc @kevinksullivan

(dm'd robert)

// For some reason, the menu doesn't open without the timeout
setTimeout(() => {
this.toggleCreateMenu();
}, 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤮 we should figure out how to do this without a setTimeout these hacks end up biting us later down the road because anything else that depends on this code then will have to use a timeout/delay to push it's rendering to the end of the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This setTimeout is currently our only solution for auto-opening the global create on load, I made this issue for people to look into solutions - read the discussions, it's not a straightforward solutions sadly :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We even increased from 200 ms to 1500 ms here: #5369

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't say one comment from a contributor is an "exhaustive" discussion on how best to fix this 😅 but I do agree with Rory's comment so did we ever follow up with react-navigation on this?

Copy link
Contributor Author

@Beamanator Beamanator Sep 21, 2021

Choose a reason for hiding this comment

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

LOL I meant this issue: #5303 :D (many more comments haha)

@bondydaa
Copy link
Contributor

reverting this here #5360 to unblock the deploy

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Jag96 in version: 1.0.99-4 🚀

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

@MitchExpensify
Copy link
Contributor

a note on this - We are continuing to experience this issue and investigating why here cc @Beamanator

Alex is going to continue looking while online today and we'll need to find someone to keep the work going if not resolved by the time Alex clocks off for the night. Looking for volunteers for the baton pass now

@MitchExpensify
Copy link
Contributor

Let's focus our investigation here #5392

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.

9 participants