-
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
Move Workspace creation in the onboardingFlow to OnboardingPurpose page and skip Workspace creation for OD signups #53729
base: main
Are you sure you want to change the base?
Move Workspace creation in the onboardingFlow to OnboardingPurpose page and skip Workspace creation for OD signups #53729
Conversation
@thesahindia Please 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 this one is kinda urgent, in case you can prioritize it. Thank you! 🙏 |
Reviewing right now. I was having some issue on native. Even after fixing the issue it wasn't working. Had to restart the mac. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-10.at.11.52.28.PM.movAndroid: mWeb ChromeScreen.Recording.2024-12-10.at.11.54.11.PM.moviOS: NativeScreen.Recording.2024-12-10.at.11.25.03.PM.moviOS: mWeb SafariScreen.Recording.2024-12-11.at.12.13.29.AM.movMacOS: Chrome / SafariScreen.Recording.2024-12-11.at.1.42.08.AM.movScreen.Recording.2024-12-11.at.1.44.53.AM.movMacOS: DesktopScreen.Recording.2024-12-11.at.1.51.10.AM.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.
LGTM!
const {adminsChatReportID, policyID} = Policy.createWorkspace(undefined, true, '', generatePolicyID(), choice); | ||
Welcome.setOnboardingAdminsChatReportID(adminsChatReportID); | ||
Welcome.setOnboardingPolicyID(policyID); | ||
} |
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 confused about why we moved this here.
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.
We moved the workspace creation logic here to BaseOnboardingPurpose
because this page is only shown to users without VSB or SMB signup qualifiers. For users with VSB or SMB, this page is skipped, and the workspace creation won’t happen on the frontend.
I believe the workspace creation was temporarily moved to BaseOnboardingEmployees
in your PR (#50759) until the backend changes are ready. Now that we’ve moved the logic to BaseOnboardingPurpose, the workspace will only be created for users without VSB or SMB signup qualifiers, which is what we need to avoid duplicates.
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.
We have to fix this now because the FE needs will be merged first. Can you please return it to the correct spot?
Just for clarity, it does not do this yet. This PR will need to support the existing flow AND that change when it happens?
I think we have to unwind this part. Something needs the So, App needs to exist in a state where it will maybe create a policy for now and then stop creating one after the backend creates one. At least, I thought that was the plan. @carlosmiceli Let me know if I have this right or if I'm missing something... |
@marcaaron agreed on everything but this line:
I don't think we stated that a policy would get temporarily created (if you do, maybe you can point me to that comment/thread?). Instead it would get created or not in ND depending on the signupQualifier (and it would assume that the BE has created the policy successfully). A policy should be created even before the account, so it's all happening before the user even signs in. |
Please add manual QA steps for the accounting page flow. |
The App code will support the current state of things (no signupQualifier passed or policy created) and then also support the backend code when it gets merged and deployed. Not sure what you mean by "policy would get temporarily created", but I did not use those words 😄 |
Ah, cool, I interpreted that from this sentence:
But seems like we're on the same page now! |
Hi @carlosmiceli and @marcaaron, However, from the discussions above, I understand that we may or may not create a workspace for OD signups in the frontend, depending on whether the backend has already created one. Therefore, we should still support workspace creation for OD signups on the frontend, and only skip it if the backend has already created a workspace for the user. Did I understand this correctly? Thank you! |
This is not totally accurate. The onboarding flow shouldn't need to check for existing workspaces created, it just needs to create a workspace or not depending on the Do we agree @marcaaron? |
Yes, with this PR, we will still create a workspace for signups from ND that don’t have a signup qualifier, such as VSB or SMB.
We are still handling the policyID for ND signups here using |
@luacmartins |
Was this meant for @carlosmiceli? |
Yes I agree with everything here. @carlosmiceli can address the |
Apologies for the confusion, Yes, I meant to tag @carlosmiceli. |
We realized that, since @Shahidullah-Muffakir makes a good point that we need to return the |
@carlosmiceli Currently, we have both checks: I’ll update the PR to remove the |
@carlosmiceli @thesahindia I removed the signupQualifier check for workspace creation, Thanks. |
@Shahidullah-Muffakir great stuff, is this PR ready for review then? |
@carlosmiceli Yes, the PR is ready for review. Thank you! |
Great, @thesahindia all yours! |
@Shahidullah-Muffakir can you help me confirm that we can get the Since the user can't create a workspace until the onboarding is complete, there's no risk of having any other policy other than the personal and the signupBusinessPolicy in the list at this time. Does this make sense? Am I missing something? |
@carlosmiceli If I understand correctly, you’re suggesting that we don’t need the We still need App/src/libs/actions/Report.ts Lines 3590 to 3597 in 13286be
I couldn’t find the |
It's not being sent that way inside One of them should be |
@carlosmiceli I believe we can use that approach. However, currently, I can only see the policy of type |
Right, we'll need to wait for the BE to be merged, it's not yet. But I wanted to confirm that this approach will work, this was the only thing holding back the BE in case we needed to send something different with OpenApp. I'll let you know as soon as it's deployed! |
@Shahidullah-Muffakir ok, we realize that the FE should be merged first or otherwise we risk creating duplicate workspaces. However, the plan remains as we decided on the previous messages (getting the onboarding policy from the policy list). Can you proceed with the plan (hardcode values to test that it works) and once this is merged we can proceed with merging/testing the BE? Let's make sure to test that this flow still creates a workspace for all the other existing cases, it should only skip creating one if the team policy is present in OnyxData after signing up in OD. Thanks for the patience and hard work! |
@carlosmiceli Sure, I’ll update and test the changes, Thanks. |
@carlosmiceli Added the changes to set |
Great! Is it ready for a review by @thesahindia ? |
Yes. |
Will review this in a few hours. |
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!
return; | ||
} | ||
|
||
const {adminsChatReportID, policyID} = Policy.createWorkspace(undefined, true, '', Policy.generatePolicyID(), CONST.ONBOARDING_CHOICES.MANAGE_TEAM); |
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.
If we were creating a policy here I'd assume that maybe we still need to create one until the BE changes are merged.
const {adminsChatReportID, policyID} = Policy.createWorkspace(undefined, true, '', Policy.generatePolicyID(), CONST.ONBOARDING_CHOICES.MANAGE_TEAM); | ||
Welcome.setOnboardingAdminsChatReportID(adminsChatReportID); | ||
Welcome.setOnboardingPolicyID(policyID); | ||
} |
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.
Could be wrong, but I'm pretty sure we still need this to complete the accounting step.
Explanation of Change
With the new BE changes, when a user signs up through OldDot and is redirected to NewDot, the backend now automatically creates a workspace for vsb and smb signup qualifiers.
In this PR, we skip the frontend workspace creation for these cases. We also move the workspace creation logic in the onboarding flow from the
BaseOnboardingEmployees
page to theBaseOnboardingPurpose
page. SinceBaseOnboardingPurpose
is skipped for OD signups, this ensures no duplicate workspace is created.Additionally, we added two conditions:
hasSignupQualifier && filteredPolicies.length === 0
These prevent edge cases where a workspace might be created twice.
Fixed Issues
$ #53326
PROPOSAL: #53326 (comment)
Tests
Test 1
Test 2
Offline tests
Internet connection required.
QA Steps
Test 1
Test 2
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.native.mov
Android: mWeb Chrome
android.chrome.mov
iOS: Native
ios.Native.mov
iOS: mWeb Safari
ios.safari.ND.mov
MacOS: Chrome / Safari
Signup through ND:
macos.20safari.20ND.mp4
Signup through OD:
macos.20safari.20OD.mp4
MacOS: Desktop
Screen.20Recording.202024-12-07.20at.209.mp4