When openOnAdminRoom is set, do not redirect first time users #24346

StefanNemeth · 2023-08-09T20:52:51Z

Details

Currently first time users will be redirected to the workspace chat report even if they should be redirected to the admin report after creating a group on staging.expensify.com.

With this change, there is a check if the admin report should be opened first even for new users, so that the workspace details and admin report stays open for them. As there will only be a change in behaviour if the openOnAdminRoom parameter is set, there shouldn't be any regressions.

Fixed Issues

$ #23566
PROPOSAL: #23566 (comment)

Tests

Go to staging.expensify.com and login with a new account
Go to Settings > Policies > Group
Open the developer console and set CONST.URL_TO_NEW_DOT = 'http://localhost:8082/'; to the local environment so that it can be tested properly
Select Free Policy
Verify that User is navigated to #admins page report and also to the workspace details on the right.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Go to staging.expensify.com and login with a new account
Go to Settings > Policies > Group
Select Free Policy
Verify that User is navigated to #admins page report and also to the workspace details on the right.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

257898236-861e3d56-847a-494c-8c5c-51ef3d1f54c1.mp4

Mobile Web - Chrome

android-2.mov

Mobile Web - Safari

ios.mov

Desktop

There aren't any web url handlers for the /transition route which is opened when creating the group, so the native app won't be opened.

iOS

There aren't any web url handlers for the /transition route which is opened when creating the group, so the native app won't be opened.

Android

There aren't any web url handlers for the /transition route which is opened when creating the group, so the native app won't be opened.

melvin-bot · 2023-08-09T20:52:58Z

@sobitneupane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

github-actions · 2023-08-09T20:53:08Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

StefanNemeth · 2023-08-09T20:54:56Z

I have read the CLA Document and I hereby sign the CLA

…her chat

sobitneupane

Screenshots/Videos

Web

Screen.Recording.2023-08-11.at.17.00.05.mov

Screen.Recording.2023-08-11.at.17.02.48.mov

Mobile Web - Chrome

Mobile Web - Safari

Screen.Recording.2023-08-11.at.17.20.42.mov

Desktop

Screen.Recording.2023-08-11.at.17.42.05.mov

iOS

Screen.Recording.2023-08-11.at.17.43.59.mov

Android

Screen.Recording.2023-08-11.at.17.45.50.mov

sobitneupane

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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
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.

dangrous

One change, let me know if that makes sense to you or if you have questions! I think this is the best way to otherwise mirror existing behavior (just direct to a different room). Please feel free to push back if you disagree.

dangrous · 2023-08-11T16:17:47Z

src/libs/actions/Welcome.js

+                (report) => ReportUtils.isPolicyExpenseChat(report) && report.ownerAccountID === currentUserAccountID && report.statusNum !== CONST.REPORT.STATUS.CLOSED,
+            );
+
+            if (workspaceChatReport) {


I think we might want to move this outside of the shouldNavigateToWorkspaceChat loop, and update the condition to if (workspaceChatReport || openOnAdminRoom), since we would want to set the NVP and show the popover on the first visit, even if they're going to the admins room.

Does that sound right?

As far as I understand your suggestion, changing it to if (workspaceChatReport || openOnAdminRoom) will cause it to have the old behaviour of navigating away.

If we want to show the popover but not navigate away, what about this approach? Maybe that's what you meant.

const isDisplayingWorkspaceRoute = isWorkspaceRoute || exitingToWorkspaceRoute; const workspaceChatReport = _.find( allReports, (report) => ReportUtils.isPolicyExpenseChat(report) && report.ownerAccountID === currentUserAccountID && report.statusNum !== CONST.REPORT.STATUS.CLOSED, ); if (workspaceChatReport || openOnAdminRoom) { // This key is only updated when we call ReconnectApp, setting it to false now allows the user to navigate normally instead of always redirecting to the workspace chat Onyx.set(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, false); } // If we already opened the workspace settings or want the admin room to stay open, do not // navigate away to the workspace chat report if (workspaceChatReport && !openOnAdminRoom && !isDisplayingWorkspaceRoute) { Navigation.navigate(ROUTES.getReportRoute(workspaceChatReport.reportID)); } if (workspaceChatReport || openOnAdminRoom) { // If showPopoverMenu exists and returns true then it opened the Popover Menu successfully, and we can update isFirstTimeNewExpensifyUser // so the Welcome logic doesn't run again if (showPopoverMenu()) { isFirstTimeNewExpensifyUser = false; } return; } // ....

Ah sorry my selection was bad. I meant only moving the Onyx.set and the if (showPopoverMenu())

I think we could do something like:

// All code up to this point is the same as what you have already if (shouldNavigateToWorkspaceChat) { // This is the same loop as you have it currently, but removing the lines below } if (workspaceChatReport || openOnAdminRoom) { // This key is only updated when we call ReconnectApp, setting it to false now allows the user to navigate normally instead of always redirecting to the workspace chat Onyx.set(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, false); // If showPopoverMenu exists and returns true then it opened the Popover Menu successfully, and we can update isFirstTimeNewExpensifyUser // so the Welcome logic doesn't run again if (showPopoverMenu()) { isFirstTimeNewExpensifyUser = false; } }

So a little simpler than what you suggested but pretty much the same effect.

All right good to know, I suggested something else because I thought changing the order of calling Onyx.set and Navigation.navigate would make a difference.

Though, do we really want to show the popover on the admin chat? There is one comment before calling Welcome.show: Shows Popover Menu on Workspace Chat at first sign-in.

Issue

If we still want to show the popover on the admin chat, one issue occurs. We will have to check whether isWorkspaceRoute is true, so we don't show the popup while the workspace settings are open (that looks a bit off). However, I don't think that the current value of isWorkspaceRoute really makes sense.

App/src/libs/actions/Welcome.js

Lines 113 to 116 in 70c5259

// If we are rendering the SidebarScreen at the same time as a workspace route that means we've already created a workspace via workspace/new and should not open the global

// create menu right now. We should also stay on the workspace page if that is our destination.

const topRoute = _.last(routes);

const isWorkspaceRoute = topRoute.name === 'Settings' && topRoute.params.path.includes('workspace');

My reasoning

There are two components that call Welcome.show. One is FloatingActionButtonAndPopover (responsible for the sidebar create menu) and the other one is ReportActionCompose (responsible for the chat popover menu).

FloatingActionButtonAndPopover only calls Welcome.show if the active route is within the central pane navigator or the home screen. So, in this case it is not relevant. When having the workspace settings open, the right modal navigator is active. So, if we are rendering the SidebarScreen at the same time as a workspace route (as the comment above suggests), Welcome.show will not be called. See here:

App/src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js

Lines 153 to 156 in 61ab4bc

if (currentRoute && ![NAVIGATORS.CENTRAL_PANE_NAVIGATOR, SCREENS.HOME].includes(currentRoute.name)) {

return;

}

Welcome.show({routes, showCreateMenu});

ReportActionCompose does not care about the active navigator and will always execute Welcome.show if a report is displayed. However, it is used within the report screen and only has access to the central pane navigator whose routes it will forward to Welcome.show. Its only route will be the admin report, even though globally the workspace route is active. So I don't see a scenario in which isWorkspaceRoute will be true.

https://github.com/Expensify/App/blob/61ab4bcf100500ce49bf1e2dfaa128f9511ea941/src/pages/home/report/ReportActionCompose.js#L260C6-L266

Possible resolutions

Do not show popover on the admin chat. I don't think it the admin chat will open without the workspace settings anyway.

Show popover on admin chat while workspace settings are open simultaneously.

Use navigator.getParent() to access the root navigator within ReportActionCompose and do not run Welcome.show in case the right modal navigator is active (just like it was done for FloatingActionButtonAndPopover).

@dangrous What are your thoughts here?

Let me bring this up in Slack - I'm not entirely sure what we'd expect as the expected behavior in this case. I think you're right that it's probably fine not to show the pop up, but I want to get some other opinions.

And re: navigate vs. onyx.set, I don't think that should matter, since onyx will persist wherever, and it looks like code was already set to run after navigate, so it should continue on.

EDIT: https://expensify.slack.com/archives/C01GTK53T8Q/p1692112789241719

Okay so we haven't heard from QA, I think it's fine not to show the pop up.

But, we SHOULD still set these two lines even when going to the admin room, I think:
Onyx.set(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, false);
and
isFirstTimeNewExpensifyUser = false;

Do you think that makes sense? If so, can you make that update and then we can merge?

That makes sense to me. I updated the code so Onyx.set(ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, false); and isFirstTimeNewExpensifyUser = false; will be called when going to the admin room and the logic is the same as before otherwise.

dangrous · 2023-08-16T15:37:44Z

I'm asking QA on the issue whether the pop up is supposed to be open.

The more I think about it, the more I agree with you @StefanNemeth that we should just not show the pop up, but I'd like a bit of confirmation.

If we don't hear anything soon I think we can go ahead and merge.

…ened

dangrous

This looks good to me! @sobitneupane can you give this one more look through and test? Changed a bit since your review. Thank you!

sobitneupane

Thanks for the update @StefanNemeth.

Changes look good and tests well.

Screen.Recording.2023-08-21.at.14.45.22.mov

OSBotify · 2023-08-21T17:55:44Z

✋ 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-08-21T22:23:10Z

🚀 Deployed to staging by https://github.com/dangrous in version: 1.3.56-0 🚀

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

OSBotify · 2023-08-24T03:06:11Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀

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

StefanNemeth requested a review from a team as a code owner August 9, 2023 20:52

melvin-bot bot requested review from sobitneupane and removed request for a team August 9, 2023 20:52

StefanNemeth force-pushed the fix/23566-admin-chat-redirect branch from 0509b0d to d1b9134 Compare August 9, 2023 21:12

When openOnAdminRoom is set, do not redirect first time users to anot…

2e95708

…her chat

StefanNemeth force-pushed the fix/23566-admin-chat-redirect branch from d1b9134 to 2e95708 Compare August 9, 2023 21:13

sobitneupane reviewed Aug 11, 2023

View reviewed changes

sobitneupane approved these changes Aug 11, 2023

View reviewed changes

melvin-bot bot requested a review from dangrous August 11, 2023 12:09

dangrous requested changes Aug 11, 2023

View reviewed changes

Set NVP is first time user to false even when admin chat should be op…

691beed

…ened

dangrous approved these changes Aug 18, 2023

View reviewed changes

sobitneupane approved these changes Aug 21, 2023

View reviewed changes

melvin-bot bot requested a review from dangrous August 21, 2023 09:04

dangrous approved these changes Aug 21, 2023

View reviewed changes

dangrous merged commit 8cfeafb into Expensify:main Aug 21, 2023

OSBotify mentioned this pull request Aug 21, 2023

Deploy Checklist: New Expensify 2023-08-21 #25625

Closed

ikevin127 mentioned this pull request Mar 15, 2024

[$500] Opens report instead of LHN on Native apps for new users when invited to a workspace #29207

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When openOnAdminRoom is set, do not redirect first time users #24346

When openOnAdminRoom is set, do not redirect first time users #24346

StefanNemeth commented Aug 9, 2023 •

edited

Loading

melvin-bot bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023 •

edited

Loading

StefanNemeth commented Aug 9, 2023

sobitneupane left a comment

sobitneupane left a comment

dangrous left a comment

dangrous Aug 11, 2023

StefanNemeth Aug 11, 2023 •

edited

Loading

dangrous Aug 14, 2023

StefanNemeth Aug 15, 2023 •

edited

Loading

dangrous Aug 15, 2023 •

edited

Loading

dangrous Aug 17, 2023

StefanNemeth Aug 17, 2023

dangrous commented Aug 16, 2023 •

edited

Loading

dangrous left a comment

sobitneupane left a comment

OSBotify commented Aug 21, 2023

OSBotify commented Aug 21, 2023

OSBotify commented Aug 24, 2023

	// If we are rendering the SidebarScreen at the same time as a workspace route that means we've already created a workspace via workspace/new and should not open the global
	// create menu right now. We should also stay on the workspace page if that is our destination.
	const topRoute = _.last(routes);
	const isWorkspaceRoute = topRoute.name === 'Settings' && topRoute.params.path.includes('workspace');

	if (currentRoute && ![NAVIGATORS.CENTRAL_PANE_NAVIGATOR, SCREENS.HOME].includes(currentRoute.name)) {
	return;
	}
	Welcome.show({routes, showCreateMenu});

When openOnAdminRoom is set, do not redirect first time users #24346

When openOnAdminRoom is set, do not redirect first time users #24346

Conversation

StefanNemeth commented Aug 9, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 9, 2023

github-actions bot commented Aug 9, 2023 • edited Loading

StefanNemeth commented Aug 9, 2023

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

dangrous left a comment

Choose a reason for hiding this comment

dangrous Aug 11, 2023

Choose a reason for hiding this comment

StefanNemeth Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

dangrous Aug 14, 2023

Choose a reason for hiding this comment

StefanNemeth Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Issue

My reasoning

Possible resolutions

dangrous Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

dangrous Aug 17, 2023

Choose a reason for hiding this comment

StefanNemeth Aug 17, 2023

Choose a reason for hiding this comment

dangrous commented Aug 16, 2023 • edited Loading

dangrous left a comment

Choose a reason for hiding this comment

sobitneupane left a comment

Choose a reason for hiding this comment

OSBotify commented Aug 21, 2023

OSBotify commented Aug 21, 2023

OSBotify commented Aug 24, 2023

StefanNemeth commented Aug 9, 2023 •

edited

Loading

github-actions bot commented Aug 9, 2023 •

edited

Loading

StefanNemeth Aug 11, 2023 •

edited

Loading

StefanNemeth Aug 15, 2023 •

edited

Loading

dangrous Aug 15, 2023 •

edited

Loading

dangrous commented Aug 16, 2023 •

edited

Loading