Use new api command AddWorkspaceRoom #10863

neil-marcellini · 2022-09-07T11:57:34Z

cc @jasperhuangg since you reviewed the Web-PR and @tgolen I chatted with you briefly about adding an extra OfflineIndicator.

Details

AddWorkspaceRoom creates an optimistic room and navigates to it. If the user is offline, the same thing happens but the whole page is grayed out until they come back online. You can still send chats in the room while offline.

Front end validation should prevent any errors creating rooms, but it is possible that a room with a duplicate name could be created. In that case, we gray out the page, remove the composer, and show an error message. When the error is dismissed the user will be navigated to the concierge chat and the room and its actions (chats) will be deleted.

I also fixed two additional issues with this PR. The first issue was fixed by removing the actorEmail for created actions on rooms. The second issue was fixed by adding a separate OfflineIndicator to the ReportScreen for large screens when the composer is hidden. See the issues for more details.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/215186
$ #10948
$ #10895

Tests

Online

Log in to any account A
Create a workspace by clicking the green plus, New workspace (if needed)
Using another platform or incognito window, log in to a different account B
As user A, invite user B to your workspace by clicking on the use avatar in the header, workspace name, Manage members, Invite, enter user B's email, select them, and click Invite.
Click the green plus then click New room
Fill out the new room form as a restricted workspace and click save
Verify that you are navigated to the newly created room and you see the "Collaboration starts here" message.
Click the room header bar and make sure you can view the details.
As user B, click the search icon in the header
Enter the room name, make sure it shows up, and click on it
Verify that the room opens
Send a message
As user A, verify that you receive the message

Offline

Log in to any account
Create a workspace by clicking the green plus, New workspace (if needed)
Go offline
Click the green plus then click New room
Fill out the new room form as a restricted workspace and click save
Verify that you are navigated to the newly created room and you see the "Collaboration starts here" message and everything is grayed out
Go back online
Verify that the room page is no longer grayed out.

Errors

Comment out the front end report name validation here

App/src/pages/workspace/WorkspaceNewRoomPage.js

Lines 83 to 85 in 70bc06e

if (!this.validate()) {

return;

}
Log in to any account
Create a workspace by clicking the green plus, New workspace (if needed)
Click the green plus then click New room
Fill out the new room form with a name like test-duplicate-name as a restricted workspace and click save
Verify that you are navigated to the newly created room and you see the "Collaboration starts here" message.
Repeat steps 4-6 to create a duplicate room
Verify that the RBR and error shows
Hit the X to dismiss the error
Verify that you are navigated to another chat and the room no longer exists

For #10948

Sign into an account
Go offline
Create a workspace. If you don't have any workspaces, click the green plus, New workspace. Otherwise, open settings by clicking the avatar in the header, click on a workspace, click the 3 dots in the header, New workspace.
Close the settings
Click the search bar in the header
Type announce in the search bar
Click on the #announce room for the workspace you just created
Send a message
Verify that a message appears with your avatar and name
Note that creating a workspace while offline is currently broken. That is a separate issue reported here Web-Workspace-The workspace that was created offline disappears after returning online #10897.

For #10895

Sign into any account on a large screen (Web or desktop).
Click on an archived room in the side bar and skip to step 6. If you don't have an archived room do the steps below to create one.
Create a workspace by clicking the green plus, New workspace (if needed)
Open the settings by clicking the avatar in the header, workspace name, 3 dots in the header, delete workspace
Click the search bar in the header, enter announce and click on the archived announce room
Go offline
Verify that the offline indicator should appear

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

All tests above except for the Errors test.

Verify that no errors appear in the JS console

Screenshots

Web

Online success

Creating a room offline, commenting, and editing the comment.

Error

Mobile Web

iOS / Safari
Online success

Error

Android / Chrome
Online success

Error

Desktop

Online success

Error

iOS

Online success

Error

Android

Success

Error

neil-marcellini · 2022-09-14T23:12:06Z

src/libs/actions/Report.js

+    // Onyx.set is used on the optimistic data so that it is present before navigating to the workspace room. With Onyx.merge the workspace room reportID is not present when
+    // storeCurrentlyViewedReport is called on the ReportScreen, so fetchChatReportsByIDs is called which is unnecessary since the optimistic data will be stored in Onyx.
+    // If there was an error creating the room, then fetchChatReportsByIDs throws an error and the user is navigated away from the report instead of showing the RBR error message.
+    const optimisticData = [
+        {
+            onyxMethod: CONST.ONYX.METHOD.SET,
+            key: `${ONYXKEYS.COLLECTION.REPORT}${workspaceRoom.reportID}`,
+            value: {
+                pendingFields: {
+                    addWorkspaceRoom: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
+                },
+                ...workspaceRoom,
+            },
+        },
+        {
+            onyxMethod: CONST.ONYX.METHOD.SET,
+            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${workspaceRoom.reportID}`,
+            value: createdActionData,
+        },
+    ];


I want to point this comment out because it's interesting and a little funky.

neil-marcellini · 2022-09-20T19:40:00Z

Ready for more reviews!

jasperhuangg · 2022-09-20T23:05:49Z

src/libs/OptionsListUtils.js

        const shouldFilterReportIfEmpty = !showReportsWithNoComments && report.lastMessageTimestamp === 0

                // We make exceptions for defaultRooms and policyExpenseChats so we can immediately
                // highlight them in the LHN when they are created and have no messsages yet. We do
                // not give archived rooms this exception since they do not need to be higlihted.
-                && !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat));
+                && !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat))
+
+                // Also make an exception for workspace rooms that failed to be added
+                && !hasAddWorkspaceRoomError;


NAB: this is definitely a complex and difficult to understand condition, I wonder if there are any ways to simplify it

Yeah that's pretty ugly. It would help to break it out into several descriptive boolean variables. I think @tgolen might be cleaning that up with his LHN cleanup issues? I'm going to leave it as is for now.

Also, this logic has actually been moved here in main

App/src/libs/SidebarUtils.js

Lines 125 to 134 in 7fbe704

const hasAddWorkspaceRoomError = report.errorFields && !_.isEmpty(report.errorFields.addWorkspaceRoom);

const shouldFilterReportIfEmpty = report.lastMessageTimestamp === 0

// We make exceptions for defaultRooms and policyExpenseChats so we can immediately

// highlight them in the LHN when they are created and have no messsages yet. We do

// not give archived rooms this exception since they do not need to be higlihted.

&& !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat))

// Also make an exception for workspace rooms that failed to be added

&& !hasAddWorkspaceRoomError;

Yeah, let me take a try at cleaning up this logic 👍 I created a GH for it here: https://github.com/Expensify/Expensify/issues/229633

jasperhuangg · 2022-09-20T23:08:45Z

src/pages/home/report/ReportActionItemCreated.js

+            pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom')}
+            errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom')}
+            errorRowStyles={styles.addWorkspaceRoomErrorRow}
+            onClose={() => Report.navigateToConciergeChatAndDeletePolicyReport(props.report.reportID)}


any chance report could be null here? I'm personally not sure but to be safe let's use lodashGet?

report will always be non-null because of withOnyx. It will not render the component until the report is read https://github.com/Expensify/react-native-onyx/blob/bac8d308cb2eaf971174837e30a1f27f6b54c71e/lib/withOnyx.js#L159-L161

There might still be something to be careful of here though.

It will not render the component until the report is read

While this is true, it does not guarantee that a report is returned. It could read the report from Onyx, find nothing there, then pass null as a prop. In fact, you see that report is not a required prop.

Oh yeah, good point. report is a required prop of ReportActionList, which renders the action items and in turn this component, so I think it's safe. ReportActionList also accesses props.report.reportID without any checks.

OK, but just calling it "safe" really isn't a good place to leave this. You should mark the report prop as required. Because, it is required, and if a report wasn't provided to props, then this code would crash.

Oh right, yes. My bad. I will create a follow up PR for that.

jasperhuangg

Overall lookin good, few super tiny concerns

neil-marcellini · 2022-09-21T15:59:53Z

I think I have addressed all your comments @jasperhuangg. I'll wait for your approval and then I think we're good to merge!

jasperhuangg · 2022-09-21T19:22:44Z

Loooookin good :)

PR 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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 */
- Any functional components have the displayName property
- 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 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

jasperhuangg · 2022-09-21T19:24:10Z

https://github.com/Expensify/Web-Expensify/pull/34745 in on prod, so taking this off HOLD and merging!

melvin-bot · 2022-09-21T19:24:28Z

@jasperhuangg looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-09-21T19:28:33Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

neil-marcellini · 2022-09-21T21:31:18Z

Last set of checks passed.

OSBotify · 2022-09-22T19:17:31Z

🚀 Deployed to staging by @jasperhuangg in version: 1.2.5-0 🚀

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

OSBotify · 2022-09-23T22:47:55Z

🚀 Deployed to production by @luacmartins in version: 1.2.5-2 🚀

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

francoisl · 2023-06-23T01:30:14Z

src/libs/actions/Report.js

+            visibility,
+            reportID: policyReport.reportID,
+        },
+        {optimisticData, successData},


Coming from #20502 - it would have been best to add a failureData here, otherwise if the backend fails to send a proper onyxUpdate in case of an error, the NewDot client won't know that the room wasn't created.

neil-marcellini added 3 commits September 7, 2022 11:22

Add fake owner email constants

8d51541

Create addWorkspaceRoom action

f8bf291

Use addWorkspaceRoom

29e5449

neil-marcellini self-assigned this Sep 7, 2022

neil-marcellini added 21 commits September 7, 2022 13:00

Pass the selected policy to addWorkspaceRoom

126095f

Access the policy id properly

7795cac

Convert the workspaceRoom to json before sending

ae97e15

Add the room's created action optimistically

e9fe6cf

Clear the created room pendingAction

e0080da

Fix clearing the pending action under key 0

fc3f25a

Hide the composer when pending add room or error

0285d03

Remove duplicate conditional

6aee812

OfflineWithFeedback for add workspace room

893fb26

Only hide composer for an archived room or error

1ac43d0

Wrap created message in OfflineWithFeedback

59d991a

Remove OfflineWithFeedback around report contents

a565376

Remove actorEmail from created report actions

2fe7dce

Fix room created pendingAction

3e48e25

Add room offline feedback for composer

5384829

Merge branch 'main' into neil-AddWorkspaceRoom

70bc06e

Use updated function names

9bf1106

Fix pendingAction default type

a1157f9

Room participants from current user and employees

edf0158

Use Onyx.set so the room exists after navigating

ab70b5c

Update comment to explain error case

f540685

neil-marcellini commented Sep 14, 2022

View reviewed changes

neil-marcellini added 4 commits September 15, 2022 13:54

Set optimistic report notification pref to daily

d16b08d

Pass individual params to AddWorkspaceRoom

6237de8

Show add workspace room error messages

d8b1efd

Simplify hiding the composer

14ab87c

neil-marcellini mentioned this pull request Sep 20, 2022

Optimize the left-hand-nav to only render options when they need to be visible #10800

Merged

93 tasks

Use backend policyReport naming vs workspaceRoom

b6f4319

tgolen approved these changes Sep 20, 2022

View reviewed changes

jasperhuangg self-requested a review September 20, 2022 23:03

jasperhuangg reviewed Sep 20, 2022

View reviewed changes

neil-marcellini requested a review from jasperhuangg September 21, 2022 15:58

jasperhuangg approved these changes Sep 21, 2022

View reviewed changes

jasperhuangg changed the title ~~[HOLD Web 34745] Use new api command AddWorkspaceRoom~~ Use new api command AddWorkspaceRoom Sep 21, 2022

jasperhuangg merged commit d1830a2 into main Sep 21, 2022

jasperhuangg deleted the neil-AddWorkspaceRoom branch September 21, 2022 19:24

melvin-bot bot added the Emergency label Sep 21, 2022

neil-marcellini removed the Emergency label Sep 21, 2022

OSBotify mentioned this pull request Sep 22, 2022

Deploy Checklist: New Expensify 2022-09-22 #11215

Closed

20 tasks

kavimuru mentioned this pull request Sep 23, 2022

Workspace - App crashed when creating New room in offline #11246

Closed

thienlnam mentioned this pull request Sep 23, 2022

Room - Created room does not appear in LHN after sending the message #11201

Closed

neil-marcellini mentioned this pull request Sep 27, 2022

[No QA] Require report prop on ReportActionItemCreate and ReportWelcomeText #11344

Merged

93 tasks

tgolen mentioned this pull request Sep 30, 2022

[$250] [Bug] Clean up confusing logic for when a report is exempt from being filtered out of the LHN #11481

Closed

francoisl reviewed Jun 23, 2023

View reviewed changes

francoisl mentioned this pull request Jun 23, 2023

[HOLD for payment 2023-06-28] Workspace rooms no longer exist #20502

Closed

5 tasks

	const hasAddWorkspaceRoomError = report.errorFields && !_.isEmpty(report.errorFields.addWorkspaceRoom);
	const shouldFilterReportIfEmpty = report.lastMessageTimestamp === 0

	// We make exceptions for defaultRooms and policyExpenseChats so we can immediately
	// highlight them in the LHN when they are created and have no messsages yet. We do
	// not give archived rooms this exception since they do not need to be higlihted.
	&& !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom \|\| isPolicyExpenseChat))

	// Also make an exception for workspace rooms that failed to be added
	&& !hasAddWorkspaceRoomError;

	if (!this.validate()) {
	return;
	}

Use new api command AddWorkspaceRoom #10863

Use new api command AddWorkspaceRoom #10863

Uh oh!

Conversation

neil-marcellini commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasperhuangg left a comment

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Sep 21, 2022

Uh oh!

jasperhuangg commented Sep 21, 2022

PR Reviewer Checklist

Uh oh!

jasperhuangg commented Sep 21, 2022

Uh oh!

melvin-bot bot commented Sep 21, 2022

Uh oh!

OSBotify commented Sep 21, 2022

Uh oh!

neil-marcellini commented Sep 21, 2022

Uh oh!

OSBotify commented Sep 22, 2022

Uh oh!

OSBotify commented Sep 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

neil-marcellini commented Sep 7, 2022 •

edited

Loading