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

Address #5047 - Open global create when a user logs in for the first time #5214

Merged
merged 7 commits into from
Sep 13, 2021

Conversation

mateomorris
Copy link
Contributor

@mateomorris mateomorris commented Sep 11, 2021

Details

When a user opens New Expensify for the first time, the Global Create Menu should open automatically.

Fixed Issues

$ #5047

Tests

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

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
  • Desktop
  • iOS
  • Android

Screenshots

Web

OpenGlobalCreate.mov

Desktop

Attached is an example of the menu not opening for an existing user, but not it opening for a new user.
I can't see a way to test for a new user on desktop because I can't replace the address in Electron (meaning I can't create a new account & click the signup link to open the desktop app). If necessary, I could create a url bar in Electron, but that would take some extra work.

Desktop.mov

Mobile Web

mobilewebdemo.mov

iOS

iosDemoOpenMenu.mov

Android

Incoming - still getting my environment set up

@mateomorris mateomorris requested a review from a team as a code owner September 11, 2021 21:40
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team September 11, 2021 21:41
@mateomorris
Copy link
Contributor Author

mateomorris commented Sep 12, 2021

Looks like there's a weird issue on iOS causing the menu not to open on first load. Investigating now.

@mateomorris
Copy link
Contributor Author

mateomorris commented Sep 12, 2021

Here's a demonstration of the issue:

Screen.Recording.2021-09-13.at.12.50.21.AM.mov

The menu opens when you refresh the simulator, but not when the app first loads (which will presumably happen in production).

But I think I've zeroed in on what's causing it.

This is the only PopoverMenu in the app that has an issue rendering on first load [on mobile] because it's the only one inside drawerContent:

<BaseDrawerNavigator
    drawerContent={() => <SidebarScreen />}
    screens={[
        {
            name: SCREENS.REPORT,
            component: ReportScreen,
            initialParams,
        },
    ]}
    isMainScreen
/>

When it gets passed directly as a screen, it has no issue rendering on first load

<BaseDrawerNavigator
    screens={[
        {
            name: SCREENS.REPORT,
            component: SidebarScreen,
            initialParams,
        },
    ]}
    isMainScreen
/>
demo2.mov

The only workaround I've found is to set a timeout in SidebarScreen.js's componentDidMount, which makes me think it's a race condition.

    componentDidMount() {
       ...
        
        setTimeout(() => {
            this.toggleCreateMenu()
        }, 200) // Setting the timeout to 0 seconds (i.e. waiting for the tick) doesn't work
    }

But obviously that's... not ideal.

Since it seems like the issue is with react-native-modal (note that using the standard react-native modal didn't help) inside @react-navigation/drawer, I don't think the root issue can be solved within this codebase. My suggestion - besides reopening the issue - would be to go with the non-ideal workaround (which could very well break on different devices) or only automatically open the menu on desktop/web. For what it's worth, it strikes me as nonstandard behavior on mobile anyway.

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Please keep semicolons on each line, and trailing commas :D Expensify style preference

@Beamanator
Copy link
Contributor

@mateomorris thanks for the investigation into the issues on mobile! If your temporary workaround (using setTimeout) works fine on all platforms now (I'll test in a minute), maybe we can move forward with your workaround and create a new issue for what you're reporting. I'll look into this a bit today too to see if I can get anywhere

@Beamanator Beamanator self-requested a review September 13, 2021 12:26
@Beamanator
Copy link
Contributor

3 more little linting errors (we're close!)

Screen Shot 2021-09-13 at 2 34 27 PM

@Beamanator Beamanator changed the title WIP: Address #5047 - Open global create when a user logs in for the first time Address #5047 - Open global create when a user logs in for the first time Sep 13, 2021
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Nice! Tests well, please feel free to report the issue regarding the race condition so we don't have to keep the setTimeout for iOS!

@Beamanator Beamanator merged commit b6074a1 into Expensify:main Sep 13, 2021
@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 @Beamanator in version: 1.0.97-1 🚀

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

@marcaaron marcaaron added the DeployBlockerCash This issue or pull request should block deployment label Sep 15, 2021
setTimeout(() => {
this.toggleCreateMenu();

// Set the NVP back to false (this may need to be moved if this NVP is used for anything else later)
Copy link
Contributor

Choose a reason for hiding this comment

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

(this may need to be moved if this NVP is used for anything else later)

This comment isn't really useful. Why are we thinking about what we might do with this later. Let's just focus on what we are using it for now because we don't know how else it would be used or why that would make a difference to the code here.


if (this.props.isFirstTimeNewExpensifyUser) {
// For some reason, the menu doesn't open without the timeout
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, the menu doesn't open without the timeout

This isn't really an acceptable solution and we should try to avoid adding things to the code that we don't understand.

@marcaaron
Copy link
Contributor

Just one idea that I haven't tested but to get around the setTimeout could we instead check for a change to the prop in componentDidUpdate like this?

SidebarScreen.propTypes = propTypes;
export default compose(
    withLocalize,
    withWindowDimensions,
    withOnyx({
        betas: {
            key: ONYXKEYS.BETAS,
        },
        isFirstTimeNewExpensifyUser: {
            key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER,
            initWithStoredValues: false, // Ensure we always use the value from the server
        },
    }),
)(SidebarScreen);

Then have a defaultProp of false for this value. Which will allow us to check the the NVP in the server and then decide to open the menu like this:

componentDidUpdate(prevProps) {
    if (!prevProps.isFirstTimeNewExpensifyUser && this.props.isFirstTimeNewExpensifyUser) {
        this.toggleCreateMenu();
        NameValuePair.set(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, false, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER);        
    }
}

@botify
Copy link

botify commented Sep 15, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-22. 🎊

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀

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

@Beamanator Beamanator removed the DeployBlockerCash This issue or pull request should block deployment label Sep 16, 2021
@Beamanator
Copy link
Contributor

Discussed with @marcaaron , I'm removing DeployBlockerCash as this fix is needed for n6. I'll make a new issue to get to the bottom of the weirdness experienced here so we can remove the need for setTimeout

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