-
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
Fix navigation to Concierge chat after sign-up/sign-in #15788
Conversation
@thesahindia @danieldoglas One of you needs to 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] |
@thesahindia can you please test and do the checklist here? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-03-13.at.8.58.40.PM.movMobile Web - ChromeScreen.Recording.2023-03-13.at.10.05.47.PM.movMobile Web - SafariScreen.Recording.2023-03-13.at.10.10.23.PM.movDesktopScreen.Recording.2023-03-13.at.10.32.18.PM.moviOSScreen.Recording.2023-03-13.at.10.24.38.PM.movAndroidScreen.Recording.2023-03-13.at.10.03.42.PM.mov |
Tests went well with an existing account but not with a new account. I didn't get navigated to concierge. The fab menu opens for the new account. Screen.Recording.2023-03-13.at.10.22.02.PM.movcc: @marcochavezf |
Closing it in favor of this proposal. |
Hi @thesahindia, I didn't see your previous comments and didn't notice you already reviewed the PR, sorry about that 🙇🏽 I think I didn't refresh the PR when I posted this comment. I'm re-opening this PR because there is a regression with the other proposal. I will update the steps and add some videos covering the regression (only reproducible on native). Could you test it again? Thanks |
Btw, I tested again and it's working with new accounts when I open the app from the deep link. |
|
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.
Looks good to me, in sake of moving the deploy forward, I will merge this to get it to staging. Tests seem good to me in the PR description
Fix navigation to Concierge chat after sign-up/sign-in (cherry picked from commit 278502c)
…-15788 🍒 Cherry pick PR #15788 to staging 🍒
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 1.2.87-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
Retest for this CP was a pass in the following platforms:
Note: For iOS, the deep linking is broken so we were blocked from testing on that platform - issue here #15158 Checking it off the list |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.87-1 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.87-1 🚀
|
🚀 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 🚀
|
} | ||
if (route === ROUTES.CONCIERGE) { | ||
navigateToConciergeChat(); | ||
} |
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 #23745
We are not handling the navigation from other URLs except the report or concierge here.
Navigation.navigate(ROUTES.getReportRoute(reportID)); | ||
} | ||
if (route === ROUTES.CONCIERGE) { | ||
navigateToConciergeChat(); |
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 #27042.
Another bug occurs due to this change. When a user logs out and then deep links to the /concierge
using a different account, the previous user's concierge report is opened. This happens because we store the concierge reportID in a local variable in Report.js, more context in #27042 (comment)
Details
This PR fixes the navigation to Concierge right a user signs up or signs in after opening the link https://new.expensify.com/concierge
Fixed Issues
$ #15785
$ #16080
Tests
To navigate to https://new.expensify.com/concierge on dev:
To navigate to https://new.expensify.com/concierge on staging/production:
On web just paste the URL in the browser (i.e. https://new.expensify.com/concierge) and click on open this link in your browser
On desktop also paste the URL in the browser but click on Open Electron from the alert dialog
On native open the report chat from an external link (for example send https://staging.new.expensify.com/concierge to your email and open the link from the phone).
Verify that no errors appear in the JS console
Offline tests
N/A
QA Steps
Same as the test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting 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)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-03-09.at.14.34.09.mov
Mobile Web - Chrome
Screen.Recording.2023-03-09.at.14.46.27.mov
Mobile Web - Safari
Screen.Recording.2023-03-09.at.14.36.52.mov
Desktop
Screen.Recording.2023-03-09.at.14.59.34.mov
After signing out without closing the application:
Screen.Recording.2023-03-20.at.19.06.31.mov
iOS
Screen.Recording.2023-03-09.at.14.57.13.mov
After signing out without closing the application:
Screen.Recording.2023-03-20.at.19.00.02.mov
Android
Screen.Recording.2023-03-09.at.14.55.33.mov
After signing out without closing the application:
Screen.Recording.2023-03-20.at.19.24.54.mov