Fix leaving room a room when the split bill with multiple participants #23051
Conversation
melvin-bot
bot
requested review from
allroundexperts
and removed request for
a team
|
@allroundexperts 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] |
|
@allroundexperts I've proposed this PR to fix the regression. I think we have to scope the logic to only a report that can be left, the additional logic is adapted from here: App/src/pages/ReportDetailsPage.js Line 114 in 5a07bc0 Please let me know your thoughts |
| // We scope it to only reports that can be left. | ||
| // The nested "if" is intentional and aimed at improving performance. | ||
| const policy = prevProps.policies[`${ONYXKEYS.COLLECTION.POLICY}${prevProps.report.policyID}`]; | ||
| const isThread = ReportUtils.isChatThread(prevProps.report); | ||
| const isUserCreatedPolicyRoom = ReportUtils.isUserCreatedPolicyRoom(prevProps.report); | ||
| const canLeaveRoom = ReportUtils.canLeaveRoom(prevProps.report, !_.isEmpty(policy)); | ||
|
|
||
| if (isUserCreatedPolicyRoom || canLeaveRoom || isThread) { | ||
| Navigation.goBack(); | ||
| Report.navigateToConciergeChat(); | ||
| // isReportRemoved will prevent <FullPageNotFoundView> showing when navigating | ||
| this.setState({isReportRemoved: true}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
@wildan-m This isn't making full sense to me.
Why can't we replace all this with:
if (prevOnyxReportID && !onyxReportID) {
Navigation.goBack();
Report.navigateToConciergeChat();
// isReportRemoved will prevent <FullPageNotFoundView> showing when navigating
this.setState({isReportRemoved: true});
return;
}
There was a problem hiding this comment.
Why can't we replace all this with:
if (prevOnyxReportID && !onyxReportID) {
@allroundexperts Known issue using only that check is when we split the bill and re-join the public room right after we leave it. as I described earlier, it's tricky to determine if a report is removed. when we remove a report from device A, then device B will get this response from the pusher
[
{
"eventType": "onyxApiUpdate",
"data": [
{
"onyxMethod": "set",
"key": "report_7794307233155968",
"value": null
}
]
}
]
The response will completely remove the props.report and make props.reprot equal to defaultProps.report
Here is the tricky part. props.report.reportID === null or _.isEqual(props.report, defaultProps.report) doesn't always mean a report is removed It can also mean:
- User is navigating to another report
- Onyx data for that report is not ready
Since we have a massive call of openReport, then the concurrent situation can happen, there is a chance that a openReport call already filled props.report but another concurrent openReport (e.g when loading report actions, or updateing lastReadReport) thinks props.report is not ready and then in componentDidUpdate condition prevOnyxReportID && !onyxReportID will be true.
In summary, since this ReportScreen is being used by other pages other than the room, then unexpected situations can happen.
Why scoping the condition works?
It actually minimizes the unexpected error, currently, we can't leave split bill chat, so our navigateToConcierge will not be executed.
Is there any other solution?
Yes, if we can modify the backend to send this value instead of null then we can exactly determine isReportRemoved condition and remove the remaining onyx data of a report from the front end. CONST.REPORT.STATUS.REMOVED is a new value, since CONST.REPORT.STATUS.CLOSED can also mean that the report is archived but not removed
[
{
"eventType": "onyxApiUpdate",
"data": [
{
"onyxMethod": "set",
"key": "report_7794307233155968",
value: {
reportID: 7794307233155968
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS.REMOVED,
},
}
]
}
]
There was a problem hiding this comment.
@wildan-m I'm more inclined with fixing this on the backend. But I think that the backend part is not necessarily a regression. That being said, can you post a video of the bug that arises when using:
if (prevOnyxReportID && !onyxReportID) {
There was a problem hiding this comment.
@allroundexperts Sure, here is the video
Recording.44.2.mp4
- split bill issue
- re-join the public room right after leaving it. Notice that the issue not always occurred since
openReportcall not always executed concurrently
There was a problem hiding this comment.
For the split bill issue, I think even without our changes, its redirecting to a 404 page which is incorrect in its own. Is that correct / do you agree?
There was a problem hiding this comment.
That might be a possibility. I'd recommend we bring this conversation to Slack though so we can get more opinions from the team.
Just a heads up that I'll be ooo until Aug 8.
There was a problem hiding this comment.
There was a problem hiding this comment.
@luacmartins, @allroundexperts seems there is no other suggested solution in Slack. can we proceed with this solution?
There was a problem hiding this comment.
I'm not fully satisfied by the solution being proposed here. But at the same time, it looks as if we're short of options. Give me a day or two to think more about this.
There was a problem hiding this comment.
Thanks for starting the discussion in Slack @wildan-m. Let's see what others say about it. I tagged another member of the team to get the conversation going.
|
Hey @allroundexperts @wildan-m 👋, I was working on this PR and noticed this issue -
User should stay on the same chat. User shouldn't be navigated away. Screen.Recording.2023-07-24.at.1.57.00.AM.movI commented out the code from the original PR and this stopped happening. So, do you also plan to fix this in this PR? From what I investigated Might require a backend change like you both said. |
|
@Nikhil-Vats will your activities will cancel/discard report creation when an error occurred? If yes then the redirection issue should be resolved once we can distinguish the type of report removal from the backend. @allroundexperts, can we proceed with the backend change? |
|
@allroundexperts @wildan-m This is the expected behaviour for the issue I am working on, only RHN should close but the user should stay on the same chat. Screen.Recording.2023-07-31.at.8.57.32.AM.mov |
|
Hey @allroundexperts @wildan-m, is it okay with you if we revert the changes in the original PR so that we can get unblocked on this PR? You can add those changes back in this PR with the correct fix. We discussed it here but wanted to confirm with you. |
That's fine @Nikhil-Vats |
|
Thanks for the quick confirmation @allroundexperts. |
|
@allroundexperts I also wanted to confirm if you are also handling this scenario as per your comment here -
We want to stay on the same page after the error but since the whole report is cleared, we are redirected to 404 page. Screen.Recording.2023-08-02.at.21.23.37.movSteps -
Expected - User should stay on the same page, RHN (request money modal) should close and error is shown. This happens on desktop app very frequently, on other platforms this happens sometimes and after a delay. If yes, we will put this issue on hold for this fix. |
@Nikhil-Vats @allroundexperts It seems that 404 has a different root cause and will not be directly fixed here, what we will fix is regression related to navigating to the concierge chat which should only apply after the user leaves a room. (not caused by report creation cancelation) |
|
Moving here: #24407 |
@johncschuster , @allroundexperts
Details
Fix leaving a room from multiple devices and fix regression when splitting the bill
Fixed Issues
$ #21518
$ #22964
PROPOSAL: #21518 (comment)
Tests
Leave Roomon one of the devices (e.g. web)Offline tests
Leave Roomon one of the devices (e.g. web)QA Steps
Leave Roomon one of the devices (e.g. web)PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Recording.36.mp4
Mobile Web - Chrome
Recording.35.mp4
Mobile Web - Safari
Recording.40.mp4
Desktop
Recording.40.2.mp4
iOS
Recording.38.mp4
Android
Recording.34.mp4