Remove `setTimeout()`. Fix behavior of create menu. #8827

marcaaron · 2022-04-28T22:28:20Z

Details

@Beamanator @luacmartins I don't think we need this setTimeout() anymore. Since this was moved into WelcomeActions it seems to open the create menu on iOS just fine.

I removed the setTimeout() and tested on a few versions of iOS and it works fine.

Fixed Issues

$ #5296
Regression caused by: #8692

Tests (iOS)

Sign in as a new user
Verify the create menu opens

Verify that no errors appear in the JS console

PR Review Checklist

PR Reviewer Checklist

QA Steps

Sign in as a new user
Verify the create menu opens

Verify that no errors appear in the JS console

Screenshots

Web

2022-05-02_11-31-27.mp4

Mobile Web

2022-05-02_12-44-14.mp4

Desktop

2022-05-02_12-42-19.mp4

iOS

2022-04-28_12-22-57.mp4

Android

XRecorder_02052022_115912.mp4

techievivek

Works smoothly.

Beamanator

A few minor comments - tests well!

src/libs/actions/WelcomeActions.js

Beamanator · 2022-04-29T10:40:26Z

src/libs/actions/WelcomeActions.js

+function show({
+    routes, showCreateMenu, isFirstTimeNewExpensifyUser, allReports, allPolicies,
+}) {


nab; is a lint rule enforcing doing this on multiple lines? Or personal preference? My personal preference is to keep this 1 line 😅 but def not super worried about it

yeah, it was the linter. I don't have any strong feelings on this though - we can change the linter if it bothers enough people 😄

github-actions · 2022-04-29T16:18:10Z

⚠️ ⚠️ 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.

marcaaron · 2022-04-29T16:22:06Z

Thanks for the comments @Beamanator. Updated!

kidroca

I don't know how others tests this but here's what I did to recreate a related bug

Add a delay for retrieving the NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER key
Longer delays recreate the bug more consistently

Update the code with the example below and play with the timeout (I managed to reduce the timeout to 1000 and still recreate the bug)
Be logged out
Log in
Observe that even though we're not a first time user "create menu" is opened automatically

src/libs/actions/NameValuePair.js

/**
 * Gets the value of an NVP
 *
 * @param {String} name
 * @param {String} onyxKey
 * @param {*} [defaultValue]
 */
function get(name, onyxKey, defaultValue) {
    API.Get({
        returnValueList: 'nameValuePairs',
        name,
    })
        .then((response) => {
            if (name === 'isFirstTimeNewExpensifyUser') {
                setTimeout(() => {
                    console.warn('Overriding isFirstTimeNewExpensifyUser');
                    Onyx.set(onyxKey, false);
                }, 1250);
                return;
            }

            const value = lodashGet(response.nameValuePairs, [name], defaultValue || '');
            Onyx.set(onyxKey, value);
        });
}

If the NVP request is slow enough the "Create Menu" will star opened (even if NVP resolves to false)(only after login / clean onyx)
Seems like the old 1500 delay was covering this issue as well

We need to either check that we already have a false value stored in Onyx, or wait for the NVP request to be over before evaluating whether to show the "create menu" or not

The setTimeout seem no longer needed otherwise

kidroca · 2022-04-29T18:49:51Z

src/pages/home/sidebar/SidebarScreen.js

+            isFirstTimeNewExpensifyUser: _.isBoolean(this.props.isFirstTimeNewExpensifyUser)
+                ? this.props.isFirstTimeNewExpensifyUser
+                : true,


Usually we'll be using defaultProps for this
Maybe because value comes from Onyx, it'll might be null despite a default prop value?

BTW isFirstTimeNewExpensifyUser isn't added to propTypes either

kidroca · 2022-04-29T19:21:09Z

Maybe we can wait for a boolean value to be added for NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER in Onyx and then open the create menu if necessary?

componentDidMount passes show/hide methods to WellcomeActions
WellcomeActions subscribe to Onyx
When boolean value is returned from the subscription it disconnects
If the value is true the "create menu is displayed"

marcaaron · 2022-04-29T19:38:22Z

Longer delays recreate the bug more consistently

Thanks @kidroca, just to clarify here the bug is:

The create menu is showing when we are not a new user because we must wait to know the true value of isFirstTimeNewExpensifyUser from the server

?

marcaaron · 2022-04-29T19:48:23Z

Does seem like we would probably want to wait for the reports and policies in that case as well 🤔

marcaaron · 2022-04-29T20:29:00Z

Gonna try something else here. I think it will be too confusing to have this logic in the view and we can't get around the fact that we need to wait for data to arrive from the server in this case 😄

…hey cannot run reliably

marcaaron · 2022-04-29T21:15:23Z

Ok tried a different approach. I'm not sure if it's super great, but I believe it to be more correct than what we had before and here's my full reasoning...

Fact 1:

The "welcome action" must only happen until after we have read these values from the server:
- isFirstTimeNewExpensifyUser
- allPolicies
- allReports

I can't think of a way around this right now, but that seems like the primary reason why we have the setTimeout()

Fact 2:

We can't trigger the "welcome task" until the Sidebar mounts so calling WelcomeActions.show() once in componentDidMount() seems correct. This guarantees the modal is ready to open, etc.

Fact 3:

We don't have a great way to detect whether the data we need has loaded from inside the SidebarScreen.

This is a new one for me, but I observed that the value of something like isReportDataLoading can be quickly set before the SidebarScreen even mounts. If we wanted to move that logic into the view layer it would be kind of ugly and require we do some checks inside componentDidUpdate() to see if the data has "loaded" yet. But if it loads before the component even mounts then we'd never be able to know that happened from inside the component because it's possible that a first time user sign in will:

quickly fetch the report data
mount the SidebarScreen
give us no way to tell from within SidebarScreen that we have loaded all the reports, policies, or anything else

Sometimes a component needs to know whether we have at least attempted to load data from the server once and that the data it gets from withOnyx is the most up to date based on that data load.

It's not clear how often we might run into this situation, but I feel a more correct approach would be one where the component that needs to know whether this data has loaded would be the one to trigger it. Or we could create some kind of "monitor" that allows any component or lib to know if data has been loaded from the server for the current session.

…those promises

luacmartins

The new solution works well and LGTM. Should we add videos for all platforms?

marcaaron · 2022-05-02T21:05:11Z

Should we add videos for all platforms?

~~I can. It will take me some time to do :D~~ Done

Changes no longer relevant

kidroca · 2022-05-03T17:34:18Z

src/libs/actions/Welcome.js

 Onyx.connect({
    key: ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER,
-    callback: val => isFirstTimeNewExpensifyUser = val,
+    initWithStoredValues: false,
+    callback: (val) => {
+        isFirstTimeNewExpensifyUser = val;
+        checkOnReady();
+    },
+});
+
+Onyx.connect({
+    key: ONYXKEYS.IS_LOADING_REPORT_DATA,
+    initWithStoredValues: false,
+    callback: (val) => {
+        isLoadingReportData = val;
+        checkOnReady();
+    },
+});
+
+Onyx.connect({
+    key: ONYXKEYS.IS_LOADING_POLICY_DATA,
+    initWithStoredValues: false,
+    callback: (val) => {
+        isLoadingPolicyData = val;
+        checkOnReady();
+    },


Not a big deal, but after setReady we no longer need to be connected to these Onyx keys

Actually we might need - for when you logout and login again

kidroca · 2022-05-03T17:37:10Z

src/libs/actions/Welcome.js

@@ -8,17 +8,60 @@ import * as Policy from './Policy';
 import ONYXKEYS from '../../ONYXKEYS';
 import NameValuePair from './NameValuePair';
 import CONST from '../../CONST';
+import createOnReadyTask from '../createOnReadyTask';
+
+const [isReady, setReady] = createOnReadyTask();


Would isReady remain resolved if we logout and login with another account?

I guess chances are slim, but if you login, logout, then login with a new user, they might not have the "Create Menu" opened

This is a nice example for a static (libs) vs instance (component) based code

Good point and nope, but I made this task resettable in another PR so let me see about that...

marcaaron · 2022-05-03T18:23:40Z

Updated, thanks for the spot @kidroca

marcaaron · 2022-05-03T22:55:55Z

Gonna be bold and merge this since I think it is blocking the deploy...?

marcaaron · 2022-05-03T23:01:11Z

@sketchydroide my CP staging label didn't seem to work here 🤔 despite this comment and this one I am pretty sure this needs to staging. But let me know if I missed something.

Remove `setTimeout()`. Fix behavior of create menu. (cherry picked from commit 334b989)

kbecciv · 2022-05-06T15:08:02Z

@marcaaron

Login- Create menu is not open

Action Performed:

Go to staging.new.expensifail.com
Sign in as a new user
Go back to your email and click on Link
Change the URL to staging and set a password for a new user
Verify the create menu opens

Expected Result:

The create menu opens

Actual Result:

Create menu is not open

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App
Mobile Web

Version Number: 1.1.57.7

Notes/Photos/Videos: Any additional supporting documentation

Recording.350.mp4

marcaaron · 2022-05-06T16:34:09Z

@kbecciv doesn't appear to be on staging yet...

…8827 🍒 Cherry pick PR #8827 to staging 🍒

kbecciv · 2022-05-09T15:16:58Z

@marcaaron Issue is not fixed. #8827 (comment)

Recording.365.mp4

sketchydroide · 2022-05-09T15:22:26Z

the code seems to be on staging, let me check if the build for it was deployed

kbecciv · 2022-05-09T15:26:24Z

@sketchydroide We tested on 1.1.57.7 build and it failed

Remove `setTimeout()`. Fix behavior of create menu. (cherry picked from commit 334b989)

…ging-8827 🍒 Cherry pick PR #8827 to staging 🍒

chiragsalian · 2022-05-09T20:38:53Z

CP code to staging, there is no post about it because the automated steps failed. Either way, this fix should be testable on version 1.1.57-8. I tested it on web and native mobile and it looks like the issue is resolved but can you re-test as well @kbecciv?

marcaaron added 2 commits April 28, 2022 12:16

Remove setTimeout from code

a2e6182

Fix comment

3bd6c56

marcaaron requested a review from a team as a code owner April 28, 2022 22:28

marcaaron self-assigned this Apr 28, 2022

melvin-bot bot requested review from techievivek and removed request for a team April 28, 2022 22:28

Move subscriptions to SidebarScreen

96a2a0f

techievivek previously approved these changes Apr 29, 2022

View reviewed changes

Beamanator previously requested changes Apr 29, 2022

View reviewed changes

Add extra params

af7232e

marcaaron dismissed techievivek’s stale review via af7232e April 29, 2022 16:13

marcaaron added the CP Staging label Apr 29, 2022

marcaaron mentioned this pull request Apr 29, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

marcaaron requested a review from Beamanator April 29, 2022 16:21

marcaaron mentioned this pull request Apr 29, 2022

[iOS] Global create is open on account creation/sign-in, but isn't visible #5303

Closed

kidroca reviewed Apr 29, 2022

View reviewed changes

marcaaron added 3 commits April 29, 2022 10:46

Welcome actions are dependent on having certain data loaded or else t…

095f8bd

…hey cannot run reliably

remove allReports from sidebarScreen

49510a4

add comment

d62833f

marcaaron added 4 commits April 29, 2022 11:19

remove policy stuff I think the cache means we dont have to wait for …

9e7c70c

…those promises

use single line

d3eebfa

add missing check

00a207a

rename WelcomeActions to Welcome

e11e362

luacmartins previously approved these changes May 2, 2022

View reviewed changes

kidroca reviewed May 3, 2022

View reviewed changes

marcaaron added 2 commits May 3, 2022 07:38

Merge remote-tracking branch 'origin' into marcaaron-fixSetTimeout

6425748

Update to use new signature of task and reset when user signs out

5230617

marcaaron dismissed luacmartins’s stale review via 5230617 May 3, 2022 17:45

luacmartins approved these changes May 3, 2022

View reviewed changes

marcaaron merged commit 334b989 into main May 3, 2022

marcaaron deleted the marcaaron-fixSetTimeout branch May 3, 2022 22:56

OSBotify pushed a commit that referenced this pull request May 3, 2022

Merge pull request #8827 from Expensify/marcaaron-fixSetTimeout

0a6497e

Remove `setTimeout()`. Fix behavior of create menu. (cherry picked from commit 334b989)

OSBotify mentioned this pull request May 3, 2022

🍒 Cherry pick PR #8827 to staging 🍒 #8871

Merged

sketchydroide added a commit that referenced this pull request May 6, 2022

Merge pull request #8871 from Expensify/OSBotify-cherry-pick-staging-…

9248a50

…8827 🍒 Cherry pick PR #8827 to staging 🍒

OSBotify pushed a commit that referenced this pull request May 9, 2022

Merge pull request #8827 from Expensify/marcaaron-fixSetTimeout

ac42e55

Remove `setTimeout()`. Fix behavior of create menu. (cherry picked from commit 334b989)

OSBotify mentioned this pull request May 9, 2022

🍒 Cherry pick PR #8827 to staging 🍒 #8904

Merged

Beamanator added a commit that referenced this pull request May 9, 2022

Merge pull request #8904 from Expensify/sketchydroide-cherry-pick-sta…

2fd6d5a

…ging-8827 🍒 Cherry pick PR #8827 to staging 🍒

chiragsalian mentioned this pull request May 9, 2022

[$250] IOS - Fab Menu - The pop up opens again after doing double tapping on new chat #8329

Closed

Stutikuls mentioned this pull request May 16, 2022

[med] : All Delete popup: Chrome + Jaws: Focus order is not sequential on 'Delete dialogue'. #8525

Closed

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `setTimeout()`. Fix behavior of create menu. #8827

Remove `setTimeout()`. Fix behavior of create menu. #8827

marcaaron commented Apr 28, 2022 •

edited

Loading

techievivek left a comment

Beamanator left a comment

Beamanator Apr 29, 2022

marcaaron Apr 29, 2022

github-actions bot commented Apr 29, 2022

marcaaron commented Apr 29, 2022

kidroca left a comment •

edited

Loading

kidroca Apr 29, 2022

kidroca commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022 •

edited

Loading

luacmartins left a comment

marcaaron commented May 2, 2022 •

edited

Loading

kidroca May 3, 2022

kidroca May 3, 2022

kidroca May 3, 2022 •

edited

Loading

kidroca May 3, 2022

marcaaron May 3, 2022

marcaaron commented May 3, 2022

marcaaron commented May 3, 2022

marcaaron commented May 3, 2022

kbecciv commented May 6, 2022 •

edited

Loading

marcaaron commented May 6, 2022

kbecciv commented May 9, 2022 •

edited

Loading

sketchydroide commented May 9, 2022

kbecciv commented May 9, 2022

chiragsalian commented May 9, 2022

Remove setTimeout(). Fix behavior of create menu. #8827

Remove setTimeout(). Fix behavior of create menu. #8827

Conversation

marcaaron commented Apr 28, 2022 • edited Loading

Details

Fixed Issues

Tests (iOS)

PR Review Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

techievivek left a comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

Beamanator Apr 29, 2022

Choose a reason for hiding this comment

marcaaron Apr 29, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2022

marcaaron commented Apr 29, 2022

kidroca left a comment • edited Loading

Choose a reason for hiding this comment

src/libs/actions/NameValuePair.js

kidroca Apr 29, 2022

Choose a reason for hiding this comment

kidroca commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022

marcaaron commented Apr 29, 2022 • edited Loading

luacmartins left a comment

Choose a reason for hiding this comment

marcaaron commented May 2, 2022 • edited Loading

kidroca May 3, 2022

Choose a reason for hiding this comment

kidroca May 3, 2022

Choose a reason for hiding this comment

kidroca May 3, 2022 • edited Loading

Choose a reason for hiding this comment

kidroca May 3, 2022

Choose a reason for hiding this comment

marcaaron May 3, 2022

Choose a reason for hiding this comment

marcaaron commented May 3, 2022

marcaaron commented May 3, 2022

marcaaron commented May 3, 2022

kbecciv commented May 6, 2022 • edited Loading

Login- Create menu is not open

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

marcaaron commented May 6, 2022

kbecciv commented May 9, 2022 • edited Loading

sketchydroide commented May 9, 2022

kbecciv commented May 9, 2022

chiragsalian commented May 9, 2022

Remove `setTimeout()`. Fix behavior of create menu. #8827

Remove `setTimeout()`. Fix behavior of create menu. #8827

marcaaron commented Apr 28, 2022 •

edited

Loading

kidroca left a comment •

edited

Loading

marcaaron commented Apr 29, 2022 •

edited

Loading

marcaaron commented May 2, 2022 •

edited

Loading

kidroca May 3, 2022 •

edited

Loading

kbecciv commented May 6, 2022 •

edited

Loading

kbecciv commented May 9, 2022 •

edited

Loading