App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Improve/fix logic for creating a workspace during onboarding flow

Open carlosmiceli opened this issue 1 year ago • 35 comments

We want new users that are taken to the Employee Count screen to have a workspace created automatically (this is currently in place):

image

However, for signups that join via expensify.com and select either vsb or smb as signup qualifier, we want to skip actually creating a workspace at this step because that's going to be created automatically via the API during the account creation. Otherwise we'll be creating one twice. These are the corresponding vsb and smb signup options on expensify.com:

Screenshot 2024-11-29 at 4 03 39 PM

To clarify, the current onboarding flow logic for creating a workspace should still occur for any new user that's taken to the onboarding modal and does NOT have either vsb or smb as a signupQualifier. Those would be:

  • a sign-up on new.expensify.com on web/mWeb
  • a sign-up on the New Expensify mobile app
  • a sign-up on the native Expensify mobile app

Please include videos of all the possible signup cases and confirming that only one workspace is created for each. Also confirm that this bug where we created duplicate workspaces doesn't reoccur (if it's still happening). Finally, we think there may be an extreme edge case where:

  • User downloaded old version of the app
  • Signs up via web
  • Gets deep linked into the app
  • App is on old version
  • Somehow gets two workspaces.

Please include in your proposal a way to prevent this if it could occur.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021862573059516200142
  • Upwork Job ID: 1862573059516200142
  • Last Price Increase: 2024-11-29
Issue OwnerCurrent Issue Owner: @thesahindia

carlosmiceli avatar Nov 29 '24 19:11 carlosmiceli

Job added to Upwork: https://www.upwork.com/jobs/~021862573059516200142

melvin-bot[bot] avatar Nov 29 '24 19:11 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

melvin-bot[bot] avatar Nov 29 '24 19:11 melvin-bot[bot]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Improve/fix logic for creating a workspace during onboarding flow

What is the root cause of that problem?

This is an improvement

What changes do you think we should make in order to solve the problem?

Instead of creating the policy in the employee step or auto-create a new workspace for vsb onboarding, we can only create the new workspace when the user completes the final step.

  1. We can remove the create a new workspace logic here and here

  2. In here, if the onboardingPurposeSelected is newDotManageTeam, create a new workspace via createWorkspace and then get adminsChatReportID, policyID and pass it to completeOnboarding function

let onboardingAdminsChatReportID; let onboardingPolicyID;
if (onboardingPurposeSelected === CONST.ONBOARDING_CHOICES.MANAGE_TEAM) {
    const {adminsChatReportID, policyID} = Policy.createWorkspace(undefined, true, '', Policy.generatePolicyID(), CONST.ONBOARDING_CHOICES.MANAGE_TEAM);
    onboardingAdminsChatReportID =  adminsChatReportID;
    onboardingPolicyID = policyID;
};
Report.completeOnboarding(
    onboardingPurposeSelected,
    CONST.ONBOARDING_MESSAGES[onboardingPurposeSelected],
    undefined,
    undefined,
    onboardingAdminsChatReportID ?? undefined,
    onboardingPolicyID,
    undefined,
    onboardingCompanySize,
    userReportedIntegration,
);

https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/pages/OnboardingAccounting/BaseOnboardingAccounting.tsx#L158

That can make sure the workspace is only created when we complete the onboarding flow with onboardingPurposeSelected as newDotManageTeam

What alternative solutions did you explore? (Optional)

In completeOnboarding function, we can build the optimistic data for new workspace via buildPolicyData function and then add these onyx data in completeOnboarding and from the backend, we will use the onboardingPolicyID to create a new workspace.

nkdengineer avatar Nov 29 '24 19:11 nkdengineer

Proposal

Please re-state the problem that we are trying to solve in this issue.

If user completed the onboarding flow using OD, two workspaces are created.

What is the root cause of that problem?

  1. if signupQualifier is VSB, we are not showing How many employees do you have? or OnboardingEmployees page, hence this useEffect is added to automatically create a workspace for it. https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/pages/OnboardingAccounting/BaseOnboardingAccounting.tsx#L61-L73
  2. if signupQualifier is SMB, it is not considered anywhere in code to skip the workspace creation, workspace is created in any case here. https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/pages/OnboardingEmployees/BaseOnboardingEmployees.tsx#L66-L70

What changes do you think we should make in order to solve the problem?

  1. remove this useEffect, because it is used to create a workspace if signupQualifier is VSB https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/pages/OnboardingAccounting/BaseOnboardingAccounting.tsx#L61-L73

  2. add one more condition here in BaseOnboardingEmployees, if the signupQualifier is SMB, then don't create the workspace. const isSmb = onboardingValues && 'signupQualifier' in onboardingValues && onboardingValues.signupQualifier === CONST.ONBOARDING_SIGNUP_QUALIFIERS.SMB; https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/pages/OnboardingEmployees/BaseOnboardingEmployees.tsx#L66-L70

as : if (!onboardingPolicyID && !isSmb) {

  1. and to cover the edge case, we can add the following conditions in above line:
const filteredPolicies = Object.values(allPolicies ?? {}).filter(PolicyUtils.isPaidGroupPolicy);

as:

                  if (!onboardingPolicyID && !isSmb && filteredPolicies.length <=0 ) {

Shahidullah-Muffakir avatar Nov 29 '24 20:11 Shahidullah-Muffakir

@thesahindia any thoughts on which proposal to go with?

danielrvidal avatar Dec 02 '24 19:12 danielrvidal

Waiting for proposals review.

carlosmiceli avatar Dec 03 '24 01:12 carlosmiceli

@Shahidullah-Muffakir's proposal will work for all the cases.

🎀 👀 🎀 C+ reviewed

thesahindia avatar Dec 03 '24 07:12 thesahindia

Current assignee @carlosmiceli is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Dec 03 '24 07:12 melvin-bot[bot]

@thesahindia I don't think the selected proposal is correct when we sign up from OldDot

  1. It redirects to NewDot and no workspace is created
  2. The employee step will be skipped if we sign with vsb or smb

So we should only create a new workspace at the final step to verify that the workspace is only created when we complete the onboarding flow.

nkdengineer avatar Dec 03 '24 07:12 nkdengineer

@thesahindia I don't think the selected proposal is correct when we sign up from OldDot

  1. It redirects to NewDot and no workspace is created
  2. The employee step will be skipped if we sign with vsb or smb

So we should only create a new workspace at the final step to verify that the workspace is only created when we complete the onboarding flow.

This is the issue we are trying to solve, we don't want to create aother workspace, as mention in the OP, the api will automatically create workspace for vsb and smb

we want to skip actually creating a workspace at this step because that's going to be created automatically via the API during the account creation.

Shahidullah-Muffakir avatar Dec 03 '24 07:12 Shahidullah-Muffakir

we want to skip actually creating a workspace at this step because that's going to be created automatically via the API during the account creation.

Actually, no workspace is created after signing up via old dot.

nkdengineer avatar Dec 03 '24 07:12 nkdengineer

Currently, the NewDot onboarding flow is skipped for accounts created from OldDot

https://github.com/Expensify/App/blob/f12b73c3aff5fea8cb8d41f111d52d5bfd187eb1/src/libs/onboardingSelectors.ts#L13-L17

Then the duplicate workspace case we only need to fix is the user can create multiple workspaces at the employee step if we do not complete the onboarding and login at multiple devices.

nkdengineer avatar Dec 03 '24 08:12 nkdengineer

Actually, no workspace is created after signing up via old dot.

What do you mean, can you elaborate further? Are you saying that there's no way for an OD signup to have a workspace created? There could be a case I think for an individual sign up to still go through the workspace creation, but will double check.

the NewDot onboarding flow is skipped for accounts created from OldDot

If this is true, then we need to change this. We shouldn't skip onboarding for OD signups. cc @mountiny @danielrvidal bringing you in to confirm that this is at least part of the issue besides the work here, since we were talking about this modal not showing yesterday.

carlosmiceli avatar Dec 03 '24 14:12 carlosmiceli

the NewDot onboarding flow is skipped for accounts created from OldDot

Let's clarify this:

  • If an OD account is migrated to ND, they should skip the onboarding.
  • ALL new OD signups that get redirected should see the onboarding.

I'll update that comment to avoid confusions.

Are we good to proceed @thesahindia or should we still evaluate some of @nkdengineer's comments?

carlosmiceli avatar Dec 03 '24 14:12 carlosmiceli

What do you mean, can you elaborate further? Are you saying that there's no way for an OD signup to have a workspace created? There could be a case I think for an individual sign up to still go through the workspace creation, but will double check.

we want to skip actually creating a workspace at this step because that's going to be created automatically via the API during the account creation.

@carlosmiceli In the description, we're saying that a workspace will be created automatically via the API during the account creation. But actually, there's no workspace after the user signs from OD with vsb or smb and redirects to ND. This is a confusing description.

the NewDot onboarding flow is skipped for accounts created from OldDot

I think this is for the old account.

nkdengineer avatar Dec 03 '24 15:12 nkdengineer

there's no workspace after the user signs from OD with vsb or smb and redirects to ND

That's being worked on in the BE, but the FE needs to be ready beforehand or we will create duplicate workspaces. That's the goal of this issue.

carlosmiceli avatar Dec 03 '24 15:12 carlosmiceli

📣 @Shahidullah-Muffakir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] avatar Dec 03 '24 15:12 melvin-bot[bot]

Any updates here @Shahidullah-Muffakir @thesahindia ?

carlosmiceli avatar Dec 06 '24 14:12 carlosmiceli

@carlosmiceli I’m running into an issue that seems to be on the backend. When signing up through expensify.com, the onboarding flow modal on new.expensify.com doesn’t always show up, it’s inconsistent, though I believe it should always appear. Because of this, I haven’t been able to fully test the changes yet.

I’m waiting for this to be fixed so I can create the PR.

Shahidullah-Muffakir avatar Dec 06 '24 15:12 Shahidullah-Muffakir

@Shahidullah-Muffakir are you still seeing this happening? I think this was recently solved, but can you confirm to me that you're seeing this happen now?

carlosmiceli avatar Dec 06 '24 15:12 carlosmiceli

@carlosmiceli Yes, I’m still experiencing this issue.

https://github.com/user-attachments/assets/c4cccfcc-ba81-405a-b89a-2132286dae6e

Shahidullah-Muffakir avatar Dec 06 '24 15:12 Shahidullah-Muffakir

Gotcha, asking about it, I thought it was solved but apparently not. This should be solved soon, though, can we start working on a PR in the meantime? At least to start getting ahead a bit.

carlosmiceli avatar Dec 06 '24 18:12 carlosmiceli

Got it. I’ll start working on the PR in the meantime.

Shahidullah-Muffakir avatar Dec 06 '24 18:12 Shahidullah-Muffakir

@Shahidullah-Muffakir can you please verify if you face the same issue on desktop and native app ?

allgandalf avatar Dec 06 '24 19:12 allgandalf

@Shahidullah-Muffakir can you please verify if you face the same issue on desktop and native app ?

@allgandalf I tested on all supported platforms, and it’s working correctly on mWeb, native, and desktop.

However, on macOS Chrome and Safari, the onboarding flow modal doesn’t appear when the browser window size is large(but if I refresh the page after the signup, then the onboarding flow modal appears). If I decrease the window width(narrowLayout), it works fine on both browsers. Considering this, I think it appears to be a frontend issue. I’ll look into it to see if I can find a possible solution.

Given this update, @carlosmiceli, I’ll go ahead and create the PR now since I can proceed with testing. Thank you.

https://github.com/user-attachments/assets/b5ae5adb-f62a-494c-ad6c-89242e2c9ec7

Shahidullah-Muffakir avatar Dec 06 '24 20:12 Shahidullah-Muffakir

@Shahidullah-Muffakir That's great news! I'll create a separate issue and assign you as well so you can fix the onboarding display too, sounds good?

carlosmiceli avatar Dec 06 '24 22:12 carlosmiceli

@Shahidullah-Muffakir Here: https://github.com/Expensify/App/issues/53724

carlosmiceli avatar Dec 06 '24 22:12 carlosmiceli

@Shahidullah-Muffakir That's great news! I'll create a separate issue and assign you as well so you can fix the onboarding display too, sounds good?

@carlosmiceli Sounds good, thank you! I’ll look into it.

Shahidullah-Muffakir avatar Dec 07 '24 02:12 Shahidullah-Muffakir

@carlosmiceli

When you have time, could you confirm the following:

  1. If a user signs up through OD and selects Manage expenses for a small team (1-9 employees), is the workspace of type team automatically created in the backend? I’ve tested this multiple times, and it seems like no workspace is being created. Alternatively, if the backend is creating the workspace, it might not be sending the newly created workspace in the OpenApp endpoint when the user is redirected to ND.

  2. If a user selects Control expenses for a larger organization (10+ employees) in OD, the user is not redirected to ND. Is this the expected behavior?

Thanks!

Shahidullah-Muffakir avatar Dec 07 '24 03:12 Shahidullah-Muffakir

Also, just to mention, I found the root cause of the issue here: https://github.com/Expensify/App/pull/50759. At that time, the backend changes weren’t ready, so they moved the workspace creation step to the employee step since the first step was being skipped for signups through OD, and the workspace was not being created for users signing up through OD. I think moving the workspace creation step back to the first step is a good idea now because we’re safe with the vsb and smb signupQualifiers skipping that step (Consider it in the PR).

Shahidullah-Muffakir avatar Dec 07 '24 04:12 Shahidullah-Muffakir