Fix duplicate concierge report was created #19294
Conversation
melvin-bot
bot
requested review from
0xmiros and
mountiny
and removed request for
a team
|
@hellohublot you can still test desktop deep link. With desktop app installed or opened, navigating https://new.expensify.com/concierge link in browser should show popup to open desktop app. And after open, deep link should work. |
|
@0xmiroslav Thank you for your suggestion, I can’t open it in the previous test, maybe I should use the safari browser, I will add screenshots of desktop tomorrow or the day after tomorrow, Thanks |
In order to test chrome browser in dev mode, you need to comment this line to show popup: App/src/components/DeeplinkWrapper/index.website.js Lines 45 to 47 in 9e43cb7 |
|
@0xmiroslav Thanks for your suggestion, but I still can't open it _desktop.mov
So I guess we can give up desktop screenshots as it still has some other issues, but I still providing iOS screenshots, I think it's enough proof that we've fixed the duplicate concierge |
|
@hellohublot does this work in staging? |
|
@0xmiroslav would you be able to give this one a look soon? |
|
@mountiny Yes, you can see that screenshot is from staging environment
|
|
|
|
I finally found and confirmed 2 regressions:
Screen.Recording.2023-05-23.at.3.01.21.PM.mov
53.mov
55.mov@hellohublot please check if you experience the same |
|
Thanks for thorough testing, I agree that both issues are quite important blockers/ regressions. We definitely need the FAB to be opened and the skeleton loading view is better than full loading spinner. @hellohublot could we maybe move the promise more down the stack to where we decide what report to fetch to ensure we only hold the concierge chat fetch call? I am not sure if that is possible to do |
This comment was marked as outdated.
This comment was marked as outdated.
|
@hellohublot How is it looking? Would be great to get this out soon as this is causing errors in our API layer as users are trying to access stale chats. |
|
Here is a further investigation, the full screen loading comes from function navigateToConciergeChat() {
if (!conciergeChatReportID) {
// In order not to delay the report life cycle, we first navigate to the unknow report
if (Navigation.getReportIDFromRoute().length <= 0) {
Navigation.navigate(ROUTES.REPORT);
}
// In order to avoid creating concierge repeatedly,
// we need to ensure that the server data has been successfully pulled
Welcome.serverDataIsReadyPromise().then(() => {
navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]);
})
} else {
Navigation.navigate(ROUTES.getReportRoute(conciergeChatReportID));
}
}I'm still searching for the best solution, you guys can give me some inspiration too, Thanks |
|
@mountiny Yes, this solution can solve the two regressions _output.mp4@0xmiroslav Thank you. It's done. Could you please help me review it again? |
|
@mountiny as seen in video, the url changes multiple times. Is that fine?
54.mov@hellohublot can you investigate why |
|
@0xmiroslav Thanks for your feedback The staging environment will also navigate twice because we don't know the _web.movThe first call chain: The second call chain: |
ok, as it happens on main, can be out of scope |
|
@hellohublot please fix conflict |
|
@0xmiroslav Thanks, done |
|
One thing I found while testing but hopefully out of scope since it's already happening on main Reproducible step:
desktop-bug-branch.mov
|
There was a problem hiding this comment.
@hellohublot let's fix these minor and all good except 2 known issues I mentioned above
src/libs/actions/Report.js
Outdated
| if (!conciergeChatReportID) { | ||
| navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]); | ||
| return; | ||
| // In order not to delay the report life cycle, we first navigate to the unknow report |
src/libs/actions/Report.js
Outdated
| navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]); | ||
| return; | ||
| // In order not to delay the report life cycle, we first navigate to the unknow report | ||
| if (Navigation.getReportIDFromRoute().length <= 0) { |
There was a problem hiding this comment.
| if (Navigation.getReportIDFromRoute().length <= 0) { | |
| if (_.isEmpty(Navigation.getReportIDFromRoute())) { |
|
@0xmiroslav Thanks for the thorough testing, can you report those bugs in bugs channel please. @hellohublot lets fix the comments from @0xmiroslav and I will ship this! |
|
In Web, I added videos of various cases Web
54-new-concierge.mov
56-new-normal.mov
concierge.mov
normal.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.mov |
Reviewer Checklist
Screenshots/VideosC+ already added screenshots WebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
|
Oh I thought @mountiny already filled checklist so I added only screenshots. |
|
np |
…eated Fix duplicate concierge report was created (cherry picked from commit b10f19d)
…-19294 🍒 Cherry pick PR #19294 to staging 🍒
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 1.3.17-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
We are getting Endless spinner on Desktop and blocked from validation. rest platforms are pass. 19294.Desktop.mp4 |
|
I consider the above report by maria as a non blocker since the problem exists on production as well so we're still proceeding with deploy. Is the reported issue by maria something you can investigate @hellohublot? |
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.17-5 🚀
|
@chiragsalian I already reported here - #19294 (comment) |
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
| Welcome.serverDataIsReadyPromise().then(() => { | ||
| // If we don't have a chat with Concierge then create it | ||
| navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]); | ||
| }); |
There was a problem hiding this comment.
In order to avoid creating concierge repeatedly,
What does this mean?
we need to ensure that the server data has been successfully pulled
Server data? What server data?
There was a problem hiding this comment.
When the concierge deeplink has been opened without the report data locally after signing in as in the test steps, this code did not wait to fetch the actual reportID of the chat between user and concierge and instead it created a new optimistic chat. This however is "stale" chat then as that particular user already has existing chat with concierge with different reportID.
| if (!conciergeChatReportID) { | ||
| navigateToAndOpenReport([CONST.EMAIL.CONCIERGE]); | ||
| return; | ||
| // In order not to delay the report life cycle, we first navigate to the unknown report |
There was a problem hiding this comment.
This comment also makes no sense to me.
| if (_.isEmpty(Navigation.getReportIDFromRoute())) { | ||
| Navigation.navigate(ROUTES.REPORT); | ||
| } |
There was a problem hiding this comment.
This code introduced a regression in #26314. Specifically, when deep linking to the concierge from a logged-out user, after login, the "not found" page is displayed while the concierge report is being retrieved from the server.
Details
In order to avoid creating concierge repeatedly, we need to accurately judge the
conciergeChatReportID, so we need to wait for the OpenApp interface request to succeed before making an empty judgmentFixed Issues
$ #17884
$ #17884 (comment)
Tests
Since deep links seem to only work in online environments, looks like it has some issues with the online environment too, and this problem only exists in browser, our condition in
navigateToConciergeChatis also robust enough, so we can skip screenshots for desktop and android/conciergeat the end of the browser url (If you are testing iOS native app, at this step you should enternew-expensify://conciergein mobile safari, and then open the app)Offline tests
QA Steps
Same as others
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
Existing account
_web.mov
New account
_web.mov
Mobile Web - Chrome
_mobile_chrome.mp4
Mobile Web - Safari
_mobile_safari.mp4
Desktop
iOS
_ios.mp4
Android