Show Pop-over on Workspace Welcome by PauloGasparSv · Pull Request #16962 · Expensify/App

PauloGasparSv · 2023-04-05T14:11:42Z

Details

This changes our logic for new accounts that access newDot for the first time!

If the new account is part of a workspace because it was invited then in their first signin the app should:

Open the workspace chat by default
Open the "Create Menu"/"Popover Menu" inside the workspace chat by itself showing the "Request Money" and "Add Attachment" options.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/270487
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

This requires creating multiple accounts to test/QA!

Create a workspace in an existing NewDot account.
Invite a new user (an email that isn't registered in Expensify/NewDot) to that Workspace
- [QA ONLY] Sadly you'll have to invite 1 new user for each device to QA this
- [DEV ONLY] For engineering testing in dev, you can create 1 account and run the following query to reset the sate of isFirstTimeNewExpensifyUser:
```
UPDATE nameValuePairs SET value = 'true' WHERE name = 'isFirstTimeNewExpensifyUser' AND accountID = <accountID>;
```
Create an account for that new invited user in NewDot
Make sure that in your first sign-in you are redirected to the Workspace Chat and the "Create Menu" opens by itself.
Make sure you can navigate off the workspace chat and into other screens.
Sign off that account and sign in again
Make sure you are not redirected to the workspace chat with the "Create Menu" opened.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-04-14.at.16.02.10.mov

Screen.Recording.2023-04-14.at.16.06.12.mov

Mobile Web - Chrome

Screen.Recording.2023-04-14.at.16.14.27.mov

Mobile Web - Safari

Screen.Recording.2023-04-14.at.16.16.20.mov

Desktop

Screen.Recording.2023-04-14.at.16.13.14.mov

iOS

Screen.Recording.2023-04-14.at.16.38.31.mov

Android

Screen.Recording.2023-04-14.at.16.20.29.mov

…portActionCompose

…rkspace-popover-welcome

luacmartins · 2023-04-11T19:48:22Z

Looking good. Just left a comment.

…rkspace-popover-welcome

PauloGasparSv · 2023-04-12T19:43:11Z

Found a bug in Android Native that I'm testing here! Can't exit the chat after it redirects you there.

Screen.Recording.2023-04-12.at.16.41.59.mov

Also checking if these console errors are related, will test signing in on main

PauloGasparSv · 2023-04-12T19:53:45Z

I think the issue is happening because we only update the isFirstTimeNewExpensifyUser NVP and send it back in Onyx in ReconnectApp and that call isn't made during this flow!

That's why I can replicate the error in Web (if the screen is small like mWeb) but refreshing the page fixes the problem.

PauloGasparSv · 2023-04-12T20:05:07Z

Both problems are coming from this branch, trying to fix here!

Edit:

Nevermind, I replicated the valid prop reportActionsof typeobjectsupplied toReportScreen, expected an array. in main so that's not related!
Will get back to it in a bit to investigate where it comes from.

PauloGasparSv · 2023-04-12T21:24:20Z

The console logs were already reported and discussed and has a closed issue. Not sure what happened or how/if it was solved but I'll focus on the actual bug where you can't leave the chat on first sign-in now.

… on first sign-in

PauloGasparSv · 2023-04-12T22:19:22Z

The problem was happening because Welcome.show is being called a bunch of times now (instead of only once like before).
Those extra calls came from ReportActionCompose/index.js inside componentDidMount where we added the Welcome.show call. That makes sense as that component is mounted/re-created every time we access another chat.

So as I first imagined the problem is related to the fact that we only update the isFirstTimeNewExpensifyUser NVP when ReconnectApp is called and we don't call that when a user first signs in and gets redirected to the workspace chat.

I fixed this by setting the local Onyx value for the isFirstTimeNewExpensifyUser NVP to false after using it cc @luacmartins

luacmartins · 2023-04-12T23:05:30Z

@PauloGasparSv is this ready for review?

PauloGasparSv · 2023-04-12T23:06:08Z

@PauloGasparSv is this ready for review?

Not yet @luacmartins still adding evidences!

PauloGasparSv · 2023-04-12T23:28:13Z

The behavior doesn't work in mWeb! I investigated a bit but still have no idea why that's happening.
This is also reproducible on desktop, replicating the tests steps with a small Chrome windows break the feature too.

Screen.Recording.2023-04-12.at.20.33.08.mov

…rkspace-popover-welcome

PauloGasparSv · 2023-04-13T20:08:49Z

All right, this has to do with OpenApp calling ReconnectApp (wasn't aware of that).

Problem is that the screen and chat lifecycle is a bit different in that mode because of how the LNH works there in comparison to a fullscreen Web/Desktop.
By the time the chat window is open and ready to open the Popover menu is the third time Welcome.show is called and the NVP isFirstTimeNewExpensifyUser was already updated by our API to false.

I'm still trying to figure a way to bypass this.

…rkspace-popover-welcome

thesahindia · 2023-04-17T20:38:34Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-04-18.at.1.52.54.AM.mov

Mobile Web - Chrome

Screen.Recording.2023-04-18.at.1.47.16.AM.mov

Mobile Web - Safari

Screen.Recording.2023-04-18.at.1.44.04.AM.mov

Desktop

Screen.Recording.2023-04-18.at.1.59.53.AM.mov

iOS

Screen.Recording.2023-04-18.at.1.37.10.AM.mov

Android

Screen.Recording.2023-04-18.at.2.04.05.AM.mov

thesahindia

Works well!

cc: @luacmartins

MelvinBot · 2023-04-17T20:39:54Z

🎯 @thesahindia, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #17533.

luacmartins · 2023-04-17T20:44:00Z

@chiragsalian all yours

chiragsalian

I think code LGTM, just left some minor clarifying comments and a suggestion to remove "state" from your comments since I initially thought it was a react state variable. So removing it makes it easier to read imo.

chiragsalian · 2023-04-18T23:08:31Z

    initWithStoredValues: false,
    callback: (val) => {
-        isFirstTimeNewExpensifyUser = val;
+        // If this is a first time new expensify user, let's only update the state of isFirstTimeNewExpensifyUser from true to false


Suggested change

// If this is a first time new expensify user, let's only update the state of isFirstTimeNewExpensifyUser from true to false

// If this is a first time new Expensify user, let's only update isFirstTimeNewExpensifyUser from true to false

having a mention of "state" is tripping me up a little bit since it's not a state variable (in a react sense). Just my opinion to make it simpler.

chiragsalian · 2023-04-18T23:20:25Z

-        isFirstTimeNewExpensifyUser = val;
+        // If this is a first time new expensify user, let's only update the state of isFirstTimeNewExpensifyUser from true to false
+        // after running all Welcome related logic in Welcome.show
+        if (!isFirstTimeNewExpensifyUser) {


I'm a bit confused, you mention in the comment above this block updates isFirstTimeNewExpensifyUser from true to false.

But for a new user, isFirstTimeNewExpensifyUser is first null from the top and then when backend returns true for ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER won't this be set from null to true?

Additionally from logic here if isFirstTimeNewExpensifyUser is true it will never set it to false. The condition here will give true only for !false so you can only change isFirstTimeNewExpensifyUser from false or null to true/false here.

So maybe your comment needs to be rephrased. Let me know if I'm mistaken.

I think my comment is very poorly written @chiragsalian so I'll explain each question and rework the comment too.

But for a new user, isFirstTimeNewExpensifyUser is first null from the top and then when backend returns true for ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER won't this be set from null to true?

Yes, the first time this logic is reached is when we first set isFirstTimeNewExpensifyUser to true.

Additionally from logic here if isFirstTimeNewExpensifyUser is true it will never set it to false. The condition here will give true only for !false so you can only change isFirstTimeNewExpensifyUser from false or null to true/false here.

Yes! It only updates the value of isFirstTimeNewExpensifyUser if it already was falsy.
Since we never update isFirstTimeNewExpensifyUser to true, we only set isFirstTimeNewExpensifyUser when it first loads through Onyx.

chiragsalian · 2023-04-18T23:24:01Z

+            Onyx.set(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, false);
            Navigation.navigate(ROUTES.getReportRoute(workspaceChatReport.reportID));
+
+            // If showPopoverMenu exists and returns true then it opened the Popover Menu succesfully and we can update the state of isFirstTimeNewExpensifyUser


Suggested change

// If showPopoverMenu exists and returns true then it opened the Popover Menu succesfully and we can update the state of isFirstTimeNewExpensifyUser

// If showPopoverMenu exists and returns true then it opened the Popover Menu successfully, and we can update isFirstTimeNewExpensifyUser

chiragsalian · 2023-04-18T23:29:03Z

            showCreateMenu();
        }
+
+        // Update state of isFirstTimeNewExpensifyUser so the Welcome logic doesn't run again


Suggested change

// Update state of isFirstTimeNewExpensifyUser so the Welcome logic doesn't run again

// Update isFirstTimeNewExpensifyUser so the Welcome logic doesn't run again

luacmartins · 2023-04-19T14:41:21Z

@PauloGasparSv let's address Chirag's comment above and get this one merged today!

luacmartins

We also have conflicts now

…rkspace-popover-welcome

PauloGasparSv · 2023-04-19T17:49:59Z

Fixed the conflicts with main and reworked the comments!

Re-testing everything here.

PauloGasparSv · 2023-04-19T18:44:26Z

Re-tested and so far everything is working.
This is ready for another round of reviews @chiragsalian @thesahindia @luacmartins

Web

Web.mov

Web.small.screen.mov

Mobile Web - Chrome

mWeb.Chrome.mov

Mobile Web - Safari

mWeb.Safari.mov

Desktop

Desktop.mov

iOS

Screen.Recording.2023-04-19.at.16.37.15.mov

Android

https://user-images.githubusercontent.com/6564265/233170196-44c52cfc-2376-465c-98aa-7bc83037ecb9.mov

chiragsalian

LGTM

chiragsalian · 2023-04-19T23:10:45Z

Going ahead and merging since it was mostly minor changes since the latest approvals.

OSBotify · 2023-04-19T23:18:35Z

✋ 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 · 2023-04-20T22:33:18Z

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.3-0 🚀

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

OSBotify · 2023-04-21T20:48:52Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.3-2 🚀

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

Paulo Vale added 2 commits April 5, 2023 10:58

Add optional parameter showPopoverMenu to Welcome.show

c8951b4

Call welcome show method to check for welcome popover when opening re…

612317f

…portActionCompose

PauloGasparSv self-assigned this Apr 5, 2023

PauloGasparSv changed the title ~~Paulogasparsv workspace popover welcome~~ Show Pop-over on Workspace Welcome Apr 5, 2023

Paulo Vale added 5 commits April 5, 2023 16:43

Bind showPopoverMenu to this so it updates the correct state

78d1062

Not take closed/archived workspace chats in consideration for new users

910cf23

Linting Welcome

904cdbe

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…

a3896c5

…rkspace-popover-welcome

Add missing trailing comma to ReportActionCompose

304abe3

luacmartins self-requested a review April 11, 2023 18:15

luacmartins reviewed Apr 11, 2023

View reviewed changes

Comment thread src/libs/actions/Welcome.js

Paulo Vale added 5 commits April 12, 2023 15:52

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…

20bc260

…rkspace-popover-welcome

Add missing reference to CONST

29f9ad7

Remove extra CONST import

ae34acf

Remove remaining reference to withwithNavigation

576b196

Add imports to Welcome and withNavigation removed during merge with main

5d70fed

Clear NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER after opening Popover menu…

b4bcbf0

… on first sign-in

Update showPopoverMenu comments

7065e94

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…

9a1379c

…rkspace-popover-welcome

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…

b39d606

…rkspace-popover-welcome

thesahindia previously approved these changes Apr 17, 2023

View reviewed changes

melvin-bot Bot mentioned this pull request Apr 17, 2023

[Hold for Payment - Friday April 28] Payment issue for E/App PR#16962 #17533

Closed

luacmartins approved these changes Apr 17, 2023

View reviewed changes

chiragsalian requested changes Apr 18, 2023

View reviewed changes

luacmartins previously approved these changes Apr 19, 2023

View reviewed changes

luacmartins reviewed Apr 19, 2023

View reviewed changes

Paulo Vale added 3 commits April 19, 2023 14:24

Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…

ffb37e2

…rkspace-popover-welcome

Remove mentions of state from stateless Welcome.js component comments

2bc4c59

Rephrase NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER callback logic comment

af0539e

PauloGasparSv dismissed stale reviews from luacmartins and thesahindia via af0539e April 19, 2023 17:46

Remove last mention of state from Welcome.js comments

64c4b7c

PauloGasparSv requested review from chiragsalian, luacmartins and thesahindia April 19, 2023 17:50

chiragsalian approved these changes Apr 19, 2023

View reviewed changes

chiragsalian merged commit 541a5ef into main Apr 19, 2023

chiragsalian deleted the paulogasparsv-workspace-popover-welcome branch April 19, 2023 23:11

OSBotify mentioned this pull request Apr 20, 2023

Deploy Checklist: New Expensify 2023-04-20 #17747

Closed

24 tasks

kbecciv mentioned this pull request Apr 21, 2023

[$1000] Workspace - Unable to add a new unvalidated account as a workspace member #17787

Closed

6 tasks

	// If this is a first time new expensify user, let's only update the state of isFirstTimeNewExpensifyUser from true to false
	// If this is a first time new Expensify user, let's only update isFirstTimeNewExpensifyUser from true to false

	// If showPopoverMenu exists and returns true then it opened the Popover Menu succesfully and we can update the state of isFirstTimeNewExpensifyUser
	// If showPopoverMenu exists and returns true then it opened the Popover Menu successfully, and we can update isFirstTimeNewExpensifyUser

	// Update state of isFirstTimeNewExpensifyUser so the Welcome logic doesn't run again
	// Update isFirstTimeNewExpensifyUser so the Welcome logic doesn't run again

Uh oh!

Conversation

PauloGasparSv commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Uh oh!

luacmartins commented Apr 11, 2023

Uh oh!

PauloGasparSv commented Apr 12, 2023

Uh oh!

PauloGasparSv commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PauloGasparSv commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PauloGasparSv commented Apr 12, 2023

Uh oh!

PauloGasparSv commented Apr 12, 2023

Uh oh!

luacmartins commented Apr 12, 2023

Uh oh!

PauloGasparSv commented Apr 12, 2023

Uh oh!

PauloGasparSv commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PauloGasparSv commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesahindia commented Apr 17, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

thesahindia left a comment

Choose a reason for hiding this comment

Uh oh!

MelvinBot commented Apr 17, 2023

Uh oh!

luacmartins commented Apr 17, 2023

Uh oh!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

chiragsalian Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

chiragsalian Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

PauloGasparSv Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

chiragsalian Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

chiragsalian Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Apr 19, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

PauloGasparSv commented Apr 19, 2023

Uh oh!

PauloGasparSv commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

chiragsalian commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

PauloGasparSv commented Apr 5, 2023 •

edited

Loading

PauloGasparSv commented Apr 12, 2023 •

edited

Loading

PauloGasparSv commented Apr 12, 2023 •

edited

Loading

PauloGasparSv commented Apr 12, 2023 •

edited

Loading

PauloGasparSv commented Apr 13, 2023 •

edited

Loading

PauloGasparSv commented Apr 19, 2023 •

edited

Loading

chiragsalian commented Apr 19, 2023 •

edited

Loading