-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Onboarding - Stage 2] Onboarding Flow - enabled #39687
[Onboarding - Stage 2] Onboarding Flow - enabled #39687
Conversation
…d-on merge wave9/onboarding-flow into wave9/onboarding-flow-turned-on
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.
The changes look good, lets get this tested well now 🚀
This comment has been minimized.
This comment has been minimized.
@hoangzinh are you able to do a round of testing on this one today please? |
@filip-solecki I got an intermittent issue after sign up with a new account, sometimes it displays the onboarding modal, sometimes it shows infinite loading. Do you face same issue as me? |
Yes, I am investigating this issue, but if you have any idea don't hesitate to DM me or write here |
Hmm, it is reproducible also on main in my case, how about you @hoangzinh ? |
I believe so. But I think it's better if we can fix it. I'm still trying to find root cause. |
But this is something along the way I pickup
But even we do above, the issue is still reproducable. |
@hoangzinh this issue has started to appear after the new architecture got merged into main, it doesn't only happen on this branch. Here's the steps to fix it:
|
I think something went wrong with Onyx. OpenApp API returns a lot of data including App/src/libs/actions/Welcome.ts Lines 44 to 52 in 7d19f82
Screen.Recording.2024-04-11.at.21.58.00.mov |
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:
src/libs/actions/Welcome.ts
Outdated
/** | ||
* Check if user dismissed modal and if report data are loaded | ||
*/ | ||
function checkServerReady() { |
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.
function checkServerReady() { | |
function checkServerDataReady() { |
src/libs/actions/Welcome.ts
Outdated
const hasRequiredOnyxKeysBeenLoaded = [isFirstTimeNewExpensifyUser, hasDismissedModal].every((value) => value !== undefined); | ||
|
||
if (isLoadingReportData || !hasRequiredOnyxKeysBeenLoaded) { | ||
function checkOnboardingReady() { |
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.
function checkOnboardingReady() { | |
function checkOnboardingDataReady() { |
src/libs/actions/Welcome.ts
Outdated
* - Whether we are a first time new expensify user | ||
* - Whether we have loaded all policies the server knows about | ||
* - Whether we have loaded all reports the server knows about | ||
* Check if onboarding object is not undefined so user can see onboarding modal |
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.
* Check if onboarding object is not undefined so user can see onboarding modal | |
* Check if onboarding data is ready in order to check if the user has completed onboarding or not |
@filip-solecki When I completed onboarding and then re-login new account, the onboarding modal still shows again Screen.Recording.2024-04-23.at.20.49.15.mov |
It is not reproducible while using staging server, @mountiny is it expected? |
@filip-solecki I found the root cause, if I fill either first name or last name as an empty string, the API CompleteGuidedSetup returns error |
Hmm, we should probably fix validation to not accept empty strings, right? |
I recall we allow to fill in empty strings on the personal details modal |
Sorry I mean we can fill in a " " (space) as described in test step 10 in this PR description #37733 |
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.
Tested again and all goods except this bug #39687 (comment). I think it's not a blocker if we would like to merge this PR asap. Moreover, it should be fixed from BE, therefore I'm good with this PR. All your @mountiny @pecanoro
I think that instruction does not mean you can fill in name as whitespace, but that we should accept names like |
function isOnboardingFlowCompleted({onCompleted, onNotCompleted}: HasCompletedOnboardingFlowProps) { | ||
isOnboardingFlowStatusKnownPromise.then(() => { | ||
// Remove once Stage 1 Onboarding Flow is ready | ||
if (!isFirstTimeNewExpensifyUser) { | ||
if (Array.isArray(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) { |
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: Can you please add a comment to some follow up PR to explain that the onboarding
is an empty array when the user hasnt completed the flow yet
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.
Will add in some follow up
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.
One NAB, but changes are looking great, lets kick this off
✋ 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/mountiny in version: 1.4.65-0 🚀
|
I don't think so @mountiny Based on above requirements in design doc, I think we would like to maintain the same behavior when we update personal details in user profile. Screen.Recording.2024-04-24.at.14.22.25.mov |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.65-5 🚀
|
Hey @pecanoro @mountiny. FYI deploying this before the Auth PR caused this issue on prod and staging. It looked pretty bad for new users. |
@Julesssss What do you mean by this? The PR was tested so I am wondering how these were triggered. Or maybe something that was modified after new small changes were committed? Or is it happening only with some specific case? |
I'm not certain, but it looks like this Auth PR should have been deployed before the full onboarding changes, as the above bug was deployed to staging/prod. Perhaps this PR just modified the onboarding flow and the issue lies with this issue instead, which had the deploy blocker label removed. I'm not sure and don't really have time to investigate further. |
Details
Fixed Issues
$ #39969
$ #39918
$ #39925
PROPOSAL:
Tests
Note: To test with an existing user, you can initiate the flow from Settings > Troubleshoot > Onboarding Flow.
With the creation of a workspace (Track business expenses..., Manage my team’s...)
Without the creation of a workspace (others)
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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.mov
Android: mWeb Chrome
Android.chrome.mov
iOS: Native
ios.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web.1.mov
MacOS: Desktop
desktop.mov