Enable switching from Old Dot to New Dot on web when copiloted in #59323
Conversation
|
This is looking great! The loading screen is a little long, but I think that's way better than seeing all the work behind the scenes. I think it's ready to go into review, we can grab another C+ to do some additional testing. Only things to note:
Thank you for tackling this! |
|
@suneox let me know some test emails you'll want to use and I can add them to the beta! |
Could you please add the emails |
|
@suneox those are added; it may take up to an hour but it should be ready to go after that |
Got it! I will review the code changes and complete the checklist after an hour |
|
bump on review @suneox thanks! |
I'm still testing, but when switching from oldDot to newDot both emails I've provided don't have the delegatorEmail parameter in the Screen.Recording.2025-04-02.at.21.52.08.mp4 |
It looks like oldDot doesn't yet support switching Copilot accounts, so I'll continue the checklist by manually adding the |
|
Hrm so it should work with the emails that you shared with me before - the changes to send From what I understand, I think you'll need to:
|
I also used the emails I shared with you earlier and followed the same steps, but the delegatorEmail parameter wasn’t included in the transition route. When I manually added delegatorEmail it will be navigate to sign-in page Screen.Recording.2025-04-02.at.23.45.28.mp4 |
| * @param setIsDelegatorFromOldDotIsReady - An optional callback function that is called | ||
| * when the delegator is authenticated (used in case of switching from OldDot to NewDot). | ||
| */ | ||
| function connect(email: string, setIsDelegatorFromOldDotIsReady?: (isReady: boolean) => void) { |
There was a problem hiding this comment.
NAB: What do you think about making this function return a promise at API.makeRequestWithSideEffects? This way, we can handle the business logic for setIsDelegatorFromOldDotIsReady in AuthScreens, allowing us to separate it from the action logic.
| // or returning from background. If so, we'll assume they have some app data already and we can call reconnectApp() instead of openApp() and connect() for delegator from OldDot. | ||
| if (SessionUtils.didUserLogInDuringSession() || delegatorEmail) { | ||
| if (delegatorEmail) { | ||
| connect(delegatorEmail, setIsDelegatorFromOldDotIsReady); |
There was a problem hiding this comment.
NAB: if we want to return the connect function as a promise func
connect(delegatorEmail).then(() => {
setIsDelegatorFromOldDotIsReady(true);
})There was a problem hiding this comment.
Hmmm
Actually good idea
I will check tomorrow
Strange 🧐 |
|
But wait |
|
Hm it shoudl work either way? But as long as it works on production I guess that's fine (all related code is on prod now) |
Yeah In theory, the functionality should work on all environments |
I still can't get Screen.Recording.2025-04-03.at.21.47.37.mp4 |
Yes on my account is always switching to the main account when chose |
Yeah |
1b3abed to
63b404a
Compare
|
@suneox are you able to allow popups from expensify in your Safari (alternately try Chrome or another browser)? I'm wondering if it's some timing issue with the api commands that it's doing on the back end (for example, it should stay copiloted in in Old Dot now. I just tried on my computer with Chrome and Safari on staging. Chrome works fine, Safari does NOT work until after you allow pop ups, but once it opens the link automatically it includes delegatorEmail |
Oh |
I was using the default Safari settings and wasn't aware of the popup permission. Now it works for me after allow the pop-up. |
| .catch((error) => { | ||
| Log.alert('[Delegate] Error connecting as delegate', {error}); | ||
| Onyx.update(failureData); | ||
| }); |
There was a problem hiding this comment.
I think we need to revert this change to keep updating failureData we can return false after update failureData to handle update on AuthScreen
There was a problem hiding this comment.
Oh
Yeah
I was a bit hasty with the changes and didn't notice that I deleted this 😅
I'll fix it now
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeTest case 1: Screen.Recording.2025-04-04.at.22.46.47.mp4Test case 2: Screen.Recording.2025-04-04.at.22.49.15.mp4iOS: mWeb SafariTest case 1: Screen.Recording.2025-04-04.at.20.21.32.mp4Test case 2: Screen.Recording.2025-04-04.at.22.42.12.mp4MacOS: Chrome / SafariTest case 1: Copilot account Screen.Recording.2025-04-02.at.22.43.50.mp4Original account Screen.Recording.2025-04-04.at.23.00.29.mp4Test case 2: Screen.Recording.2025-04-04.at.20.11.26.mp4Android: NativeScreen.Recording.2025-04-04.at.23.28.52.mp4iOS: NativeScreen.Recording.2025-04-04.at.23.09.08.mp4MacOS: DesktopScreen.Recording.2025-04-04.at.23.16.02.mp4 |
|
✋ 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 https://github.com/dangrous in version: 9.1.24-2 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
Explanation of Change
Enable switching from Old Dot to New Dot on web when copiloted in
Fixed Issues
$ #59306
PROPOSAL:
Tests
Precondition:
Test Steps:
Test case 1
Support → Concierge
Try New Expensify button
Reports → Random report → Take me there button
Test case 2
Offline tests
Precondition:
Test Steps:
Test case 1
Support → Concierge
Try New Expensify button
Reports → Random report → Take me there button
Test case 2
QA Steps
Note: Please ping @dangrous if this doesn't seem to be working, we may have to add additional test emails to the beta; I added the applause.expensifail.com domain. Also, this is specifically for desktop web; this should already work on hybrid app.
Precondition:
Test Steps:
Test case 1
Support → Concierge
Try New Expensify button
Reports → Random report → Take me there button
Test case 2
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
NA
Android: mWeb Chrome
2025-03-31.13.43.39.mov
iOS: Native
NA
iOS: mWeb Safari
2025-03-31.13.35.28.mov
MacOS: Chrome / Safari
2025-03-30.19.11.18.mov
MacOS: Desktop
NA