-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Enable switching from Old Dot to New Dot on web when copiloted in #59323
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
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 |
src/libs/actions/Delegate.ts
Outdated
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
src/libs/actions/Delegate.ts
Outdated
.catch((error) => { | ||
Log.alert('[Delegate] Error connecting as delegate', {error}); | ||
Onyx.update(failureData); | ||
}); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh
Yeah
I was a bit hasty with the changes and didn't notice that I deleted this 😅
I'll fix it now
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.
@suneox
Done !
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 |
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.
Works as expected
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.
Thank you both!
✋ 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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
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