-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor Creating a Chat to be optimistic #11439
Conversation
…ic data when a report is passed
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.
Lets add this to the testing steps #11503
Ready for re-review after merge. |
No longer on hold. I'm gonna merge. |
I don't know why github is blocking on weeks-old reviews occasionally.
@tylerkaraszewski looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
@tylerkaraszewski was the Web-E PR deployed to prod? |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @tylerkaraszewski in version: 1.2.19-0 🚀
|
🚀 Deployed to production by @chiragsalian in version: 1.2.19-2 🚀
|
FYI I think this PR partly introduced this edge-case participant order bug. Explanation here |
Not an emergency, if there was one we would have found it by now. |
errorRowStyles={styles.addWorkspaceRoomErrorRow} | ||
onClose={() => Report.navigateToConciergeChatAndDeletePolicyReport(props.report.reportID)} | ||
onClose={() => Report.navigateToConciergeChatAndDeleteReport(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.
Child reports do not get deleted here when there is an error because we do not pass shouldDeleteChildReports
to true
here. More details here #45625
Details
This PR switches from creating chats with
fetchOrCreateChat
toOpenReport
. It also makes creating a chat optimistic, and provides feedback / errors. The implementation is very similar to creating a new workspace, because it is basically the same thing.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/229484
Tests
Test creating a chat optimistically
Test creating a failed chat optimistically
Test that we push the report to all users involved, including personal details
account1
)account1
on a different platformaccount2
) thataccount1
does not have a chat with yet on a 3rd different platformaccount1
withaccount2
account1
on both platforms, andaccount2
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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 */
this
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
Test creating a chat optimistically
Test creating a failed chat optimistically
Test that we push the report to all users involved, including personal details
account1
)account1
on a different platformaccount2
) thataccount1
does not have a chat with yet on a 3rd different platformaccount1
withaccount2
account1
on both platforms, andaccount2
Screenshots
Web
2022-10-06_15-16-17.mp4
Mobile Web - Chrome
Mobile Web - Safari
Desktop
2022-10-06_15-19-13.mp4
iOS
2022-10-06_16-20-28.mp4
Android
2022-10-06_16-50-50.mp4