-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Use new api command AddWorkspaceRoom #10863
Conversation
src/libs/actions/Report.js
Outdated
// 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, | ||
}, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to point this comment out because it's interesting and a little funky.
Ready for more reviews! |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: this is definitely a complex and difficult to understand condition, I wonder if there are any ways to simplify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this logic has actually been moved here in main
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pendingAction={lodashGet(props.report, 'pendingFields.addWorkspaceRoom')} | ||
errors={lodashGet(props.report, 'errorFields.addWorkspaceRoom')} | ||
errorRowStyles={styles.addWorkspaceRoomErrorRow} | ||
onClose={() => Report.navigateToConciergeChatAndDeletePolicyReport(props.report.reportID)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any chance report could be null
here? I'm personally not sure but to be safe let's use lodashGet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, yes. My bad. I will create a follow up PR for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lookin good, few super tiny concerns
I think I have addressed all your comments @jasperhuangg. I'll wait for your approval and then I think we're good to merge! |
Loooookin good :)
|
https://github.com/Expensify/Web-Expensify/pull/34745 in on prod, so taking this off HOLD and merging! |
@jasperhuangg looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Last set of checks passed. |
🚀 Deployed to staging by @jasperhuangg in version: 1.2.5-0 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.5-2 🚀
|
visibility, | ||
reportID: policyReport.reportID, | ||
}, | ||
{optimisticData, successData}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 separateOfflineIndicator
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
Offline
Errors
App/src/pages/workspace/WorkspaceNewRoomPage.js
Lines 83 to 85 in 70bc06e
test-duplicate-name
as a restricted workspace and click saveFor #10948
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
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
All tests above except for the Errors test.
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