-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Redirect non-micro companies on web #55514
Redirect non-micro companies on web #55514
Conversation
@trjExpensify it seems like we are not able to set NVP In HybridApp scenario we set NVP I'm also curious why button |
@AndrewGable @Julesssss any chance you can help @mateuuszzzzz here?
What was the user's NVP value prior to clicking "Switch to Expensify Classic" in both attempts here? |
What do you mean exactly? Is this because we then migrate away and the request isn't completed in time?
Sounds like bug to me, can't recall a case we'd keep it despite a manual switch... Lets see what Tom is thinking though |
@Julesssss there's basically two "page" variations you see in the Switch to Classic flow.
|
According to your question @Julesssss, now I simply call this function in code Even when I click button (that navigates you to two "page" variations which @trjExpensify mentioned) In other words, it seems that we don’t have an API command on the NewDot side to set it. In fact, on HybridApp, we never set it via NewDot but rather via OldDot. I also double-checked and disabled the redirect to see if waiting for the API commands to finish would change the value of NVP, but it had no effect. |
@trjExpensify I was testing onboarding flow on your emails ![]() |
Just to confirm, is this the flow?
|
On HybridApp it works fine. I only have issue with Web and mWeb So my flow is:
On NewDot web I check it in console with |
If so, that looks like the same bug I discovered with organic new.expensify.com sign-ups. Admittedly, not common to sign-up on NewDot directly, but nevertheless, they should be set to |
Is it going to fix issue with NVP value that remains unchanged after redirect from NewDot web to OldDot web? And one more thing: Should I use This command expects arguments, but they're optional
I'm not convinced that this command changes NVP value 😅 |
That's expected, unless the NVP of the account that switched was set to So Jules' PR is fixing the fact that a newDot sign-up doesn't have a Feel free to follow these steps to check yourself:
2025-01-22_12-12-28.mp4 |
Thanks! I will re-test it with fixes from staging 😅 |
The above video is a prod test. The point being, it's working correctly to update the NVP value only when
I'll let @Julesssss pass judgement on this btw, but it sounds wrong to me to use that API command here as it's not the user driven "Switch to Expensify Classic" flow. |
I think we need to create another API command in this case if we want to change the dismissed value in this new flow |
Yeah, it's a fair assumption.
Yes I agree. I can create something simple and similar to |
I think simple API command that takes no arguments and sets dismissed value to true should be enough 😅 |
Okay, I'll update |
Thanks! I'll test it once it's on staging |
https://github.com/Expensify/Web-Expensify/pull/45472 is on production |
@mateuuszzzzz I see you made some updates today. I'm able to help with any early testing or debugging tomorrow, just let me know! |
I think this is done. I only have two questions regarding API design and task requirements.
|
Hm, we can ask @Julesssss to confirm, but I think we do want to complete onboarding - because otherwise, if they come back to NewDot at some point in the future, we'd end up re-showing the onboarding modal if
It's super unlikely a new user has the desktop app installed, but not impossible. Let's not write logic to disable the redirect, let's just open OldDot in a browser tab. |
Exactly, users would see onboarding modal so we definitely need to complete it somewhere. |
🚧 @Julesssss has triggered a test hybrid app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
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'm having trouble with the initial redirection to NewDot and I always land in OldDot, which makes it impossible to verify these changes. I have a feeling a backend change has broken the initial redirection 😕
I'm looking further into this.
UGH of course. I forgot it's locked behind a beta. -- updating test instructions to remind myself. |
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.
Working nicely
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
✋ 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/Julesssss in version: 9.1.4-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.1.4-4 🚀
|
Explanation of Change
PR adds web part of redirect to Expensify Classic (for non-micro companies, >=11 employees)
Fixed Issues
$ #53906
PROPOSAL:
Tests
Make sure the account you create is on the
hybridAppRedirect
beta! (@applause
may not yet be added, so please try+@trj.chat
if you are not initially being redirected to NewDot after account creationA) Flow for 11+ accounts:
tryNewDot
NVP should have dismissed key set totrue
B) Flow for less than 11 employees:
tryNewDot
NVP should have dismissed key set tofalse
Offline tests
QA Steps
Same as tests, but last step in both flows can be omitted
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
android.demo.micro.mov
android.demo.other.than.micro.mov
Android: mWeb Chrome
android.mweb.+11.mov
android.mweb.11.mov
iOS: Native
demo.iOS.1.mov
iOS.demo.2.mov
iOS: mWeb Safari
ios.mweb.11+.mov
iosmweb.11.mov
MacOS: Chrome / Safari
web.11+.mov
demo.web.less.than.11.mov
MacOS: Desktop
NOTE: Lack of redirect on desktop is intentional
desktop.11+.mov
desktop.11.mov