[HOLD for payment 2024-04-09] [$500] Workspace - New Workspace button doesn't create WS and shows no default name

lanitochka17 commented 7 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.53-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Exploratory around https://expensify.testrail.io/index.php?/tests/view/4428235 Issue reported by: Applause - Internal Team

Action Performed:

Go to NewDot and login
Click WS dropdown in the top of LHN
Click +
Change WS name and click Save
Click New Workspace button in the top right corner

Expected Result:

New WS created, default name should be displayed

Actual Result:

Default name is not displayed, and WS is not created

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f43e8a9a-90bf-47d0-9409-0ddee4c0e068

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0171b0f044271b7cff
Upwork Job ID: 1769593725439668224
Last Price Increase: 2024-03-18
Automatic offers:

alitoshmatov | Reviewer | 0

Issue Owner

Current Issue Owner: @alitoshmatov

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

lanitochka17 commented 7 months ago

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 7 months ago

We think that this bug might be related to #wave-collect - Release 1

HezekielT commented 7 months ago

Proposal

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

Workspace - New Workspace button doesn't create WS and shows no default name.

What is root cause of that problem?

When we create a new workspace for the first time by clicking the + icon that is found in workspace switcher(on LHN), we call App.createWorkspaceWithPolicyDraftAndNavigateToIt()

https://github.com/Expensify/App/blob/4e599b2cd6061b52d3964dc69e7575e210f0581d/src/pages/WorkspaceSwitcherPage.tsx#L223

Inside createWorkspaceWithPolicyDraftAndNavigateToIt(), we generate a policyID, create a draft initial workspace and then navigate to WORKSPACE_INITIAL which renders the WorkspaceInitialPage component.

Now in WorkspaceInitialPage, we have a useEffect that creates the workspace by calling savePolicyDraftByNewWorkspace() if we have a draft policy id as shown below.

https://github.com/Expensify/App/blob/4e599b2cd6061b52d3964dc69e7575e210f0581d/src/pages/workspace/WorkspaceInitialPage.tsx#L76-L86

The problem is that the above useEffect runs only once when the component mounts and the next time we try to create a second workspace by clicking on the create workspace button on the top right corner of the screen after modifying the first workspace's name, the useEffect will not run again since WorkspaceInitialPage component is already rendered and there are no dependencies that would trigger a rerender => preventing savePolicyDraftByNewWorkspace() from being called for the second workspace even if we have a draft one => leading to the workspace not being created and the default name not being shown.

This is the root cause.

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

We need to update the dependency of the useEffect so that App.savePolicyDraftByNewWorkspace() will be called whenever there is a change in draft policy. We can add policyDraft or policyDraft?.id to the dependency here.

https://github.com/Expensify/App/blob/4e599b2cd6061b52d3964dc69e7575e210f0581d/src/pages/workspace/WorkspaceInitialPage.tsx#L86

Result

https://github.com/Expensify/App/assets/39636266/304f8404-2638-4e48-9016-7b559d533593

ShridharGoel commented 7 months ago

Proposal

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

Workspace lists page opens up when trying to update workspace name, currency etc after creating a new workspace.

What is the root cause of that problem?

We shouldn't be seeing the workspaces list when editing the newly created workspace name.

This logic is unnecessarily pushing the workspaces list page in the central pane.

https://github.com/Expensify/App/blob/e40fba13be231a88c76f789ac94c4a65ec34f280/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L185-L189

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

There's no need of the above logic, we can simply remove it and it will solve the navigation issue. Same issue can be seen at multiple other places which will also get resolved.

This is same as my proposal here: https://github.com/Expensify/App/issues/38436#issuecomment-2002326187

allgandalf commented 7 months ago

Proposal

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

What is the root cause of that problem?

We have a logic added from https://github.com/Expensify/App/pull/37421 Ideal Nav PR which sets the Central Pane to Workspace settings page https://github.com/Expensify/App/blob/e40fba13be231a88c76f789ac94c4a65ec34f280/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L185-L189 This was done to address the condition where RHP was opened from FullScreenNavigator, to adjust to the correct page of central pane.

But over here we only consider the condition when RHP was opened from LHN and not the condition where RHP is opened from Central Pane.

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

We need to consider an additional Condition to check if the RHP was opened from LHN and then only Set the Central Pane to workspace settings Page:


- if (matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
+ if ((matchingRootRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) && isRHPScreenOpenedFromLHN) {
                routes.push(createCentralPaneNavigator({name: SCREENS.SETTINGS.WORKSPACES}));
            }

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

ShridharGoel commented 7 months ago

@GandalfGwaihir Do you have any example of a use-case where this would be needed?

iammudassirali commented 7 months ago

Hi there!

I've carefully reviewed the issue you've reported regarding the default workspace creation problem in the Expensify app. I understand the importance of maintaining a reliable and seamless user experience, especially when it comes to financial transactions.

To address this issue, I propose the following technical solution:

Investigation and Analysis:

I'll start by examining the codebase related to workspace creation in the Expensify app. I'll identify any potential logic errors or misconfigurations that could be causing the default workspace name not to display. Code Refactoring:

If necessary, I'll refactor the relevant code to ensure that the default workspace name is properly displayed. This may involve debugging the existing code and making modifications to improve its functionality. Testing and Validation:

Once the changes are implemented, I'll thoroughly test the workspace creation functionality across different environments, including staging and production. I'll also perform regression testing to ensure that no new issues are introduced as a result of the changes. Documentation and Reporting:

Throughout the process, I'll maintain detailed documentation of the steps taken and the changes made. I'll provide clear reports on the findings, actions taken, and outcomes of the testing phase. Collaboration and Communication:

I'll maintain open communication with the Expensify team, providing regular updates on the progress of the resolution process. I'll actively seek feedback and collaborate with team members to ensure that the solution aligns with the project requirements. I'm confident that my technical expertise and problem-solving skills make me well-suited to address this issue effectively. I'm committed to delivering a high-quality solution that meets the standards of reliability and user experience expected from the Expensify app.

I look forward to the opportunity to contribute to the success of the Expensify project. If you have any questions or need further clarification, please don't hesitate to reach out.

Best regards, Mudassir Ali

P.S. I'm excited about the opportunity to work with the Expensify team and contribute to the ongoing improvement of the app! 😊

melvin-bot[bot] commented 7 months ago

📣 @iammudassirali! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

iammudassirali commented 7 months ago

thisismudassirali@gmail.com https://www.upwork.com/freelancers/~0191fe0bea01c9f099

iammudassirali commented 7 months ago

Contributor details Email Address: thisismudassirali@gmail.com Upwork Profile: https://www.upwork.com/freelancers/~0191fe0bea01c9f099

iammudassirali commented 7 months ago

Contributor details Your Expensify account email: thisismudassirali@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0191fe0bea01c9f099

melvin-bot[bot] commented 7 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

iammudassirali commented 7 months ago

Hi there!

I've carefully reviewed the issue you've reported regarding the default workspace creation problem in the Expensify app. I understand the importance of maintaining a reliable and seamless user experience, especially when it comes to financial transactions.

To address this issue, I propose the following technical solution:

Investigation and Analysis:

I'll start by examining the codebase related to workspace creation in the Expensify app. I'll identify any potential logic errors or misconfigurations that could be causing the default workspace name not to display. Code Refactoring:

If necessary, I'll refactor the relevant code to ensure that the default workspace name is properly displayed. This may involve debugging the existing code and making modifications to improve its functionality. Testing and Validation:

Once the changes are implemented, I'll thoroughly test the workspace creation functionality across different environments, including staging and production. I'll also perform regression testing to ensure that no new issues are introduced as a result of the changes. Documentation and Reporting:

Throughout the process, I'll maintain detailed documentation of the steps taken and the changes made. I'll provide clear reports on the findings, actions taken, and outcomes of the testing phase. Collaboration and Communication:

I'll maintain open communication with the Expensify team, providing regular updates on the progress of the resolution process. I'll actively seek feedback and collaborate with team members to ensure that the solution aligns with the project requirements. I'm confident that my technical expertise and problem-solving skills make me well-suited to address this issue effectively. I'm committed to delivering a high-quality solution that meets the standards of reliability and user experience expected from the Expensify app.

I look forward to the opportunity to contribute to the success of the Expensify project. If you have any questions or need further clarification, please don't hesitate to reach out.

Best regards, Mudassir Ali

P.S. I'm excited about the opportunity to work with the Expensify team and contribute to the ongoing improvement of the app! 😊

allgandalf commented 7 months ago

Welcome to Expensify open source @iammudassirali :wave: , here's the proposal template to follow while posting a proposal, also refer to contribution guidelines , thanks

gieoon commented 7 months ago

Proposal

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

The default workspace name is not defined when creating a workspace from WorkspaceSwitcherPage.tsx.
The workspace is not created or saved.

What is the root cause of that problem?

Workspace is being created from a policy draft rather than a policy. The current logic does not handle policy drafts and the name is undefined in WorkspaceProfilePage.

WorkspaceProfilePage does not accept policyDraft, so policy is undefined and the policyDraft is ignored. https://github.com/Expensify/App/blob/e737a63615e600e5a76a4268d288389318d747ce/src/pages/workspace/WorkspaceProfilePage.tsx#L44

The policy draft is also never saved, so App.savePolicyDraftByNewWorkspace(policyDraft.id, policyDraft.name, '', policyDraft.makeMeAdmin); needs to be called from somewhere.

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

Policy needs to be inherited by WorkspaceProfilePage and handled in the logic when creating a policy.

function WorkspaceProfilePage({policyDraft, policy, currencyList = {}, route}: WorkSpaceProfilePageProps) {

      policy = policyDraft?.id ? policyDraft : policy;

}

App.savePolicyDraftByNewWorkspace also needs to be called from within useEffect in WorkspaceProfilePage.tsx as it is also done in WorkspaceInitialPage.tsx. https://github.com/Expensify/App/blob/e737a63615e600e5a76a4268d288389318d747ce/src/pages/workspace/WorkspaceInitialPage.tsx#L76-L86

Proposed solution in WorkspaceProfilePage.tsx

useEffect(() => {

        ...

        if ( policyDraft ) {
            App.savePolicyDraftByNewWorkspace(policyDraft.id, policyDraft.name, '', policyDraft.makeMeAdmin);
        }

        ...

    }, []);

What alternative solutions did you explore? (Optional)

Changing the props directly in the HOC withPolicy.tsx before they reach the React Component.

melvin-bot[bot] commented 7 months ago

📣 @gieoon! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

gieoon commented 7 months ago

Contributor details Your Expensify account email: jun.a.kagaya@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01506cc9754f8fc72b

melvin-bot[bot] commented 7 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

iammudassirali commented 7 months ago

Please re-state the problem that we are trying to solve in this issue. The issue at hand involves the failure to display the default workspace name when creating a new workspace in the Expensify app. This results in user confusion and impacts the overall user experience, especially for first-time users.

What is the root cause of that problem? Upon analysis, it appears that there is a logic error or misconfiguration in the code related to workspace creation. This prevents the default workspace name from being properly retrieved and displayed to the user during the creation process.

What changes do you think we should make in order to solve the problem? To address this issue, we need to identify and rectify the underlying cause within the codebase. This may involve debugging the existing code and implementing modifications to ensure that the default workspace name is correctly retrieved and displayed to the user.

What alternative solutions did you explore? (Optional) While exploring alternative solutions, we considered the possibility of implementing a workaround to manually input the default workspace name. However, this approach would not address the root cause of the issue and could lead to further user confusion.

In summary, the proposed solution involves identifying and rectifying the logic error or misconfiguration within the codebase to ensure that the default workspace name is correctly displayed during the creation process. This will contribute to a more seamless and user-friendly experience for Expensify app users.

Best regards, Mudassir Ali

kaushiktd commented 7 months ago

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

New Workspace button doesn't create WS and shows no default name

What is the root cause of that problem?

When attempting to create a new workspace after already creating one, the policy object returns undefined, causing issues with the application logic.

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

The policy object received as props is undefined in certain scenarios, particularly during workspace creation. To mitigate this issue, we obtain the policy ID from the route parameters and utilize it to fetch the corresponding policy object. This ensures that the policy is correctly retrieved and resolves the undefined policy error.

add here

  React.useEffect(() => {
        const policyDraftId = route.params?.policyID;

        if (!policyDraftId) {
            return;
        }

        App.savePolicyDraftByNewWorkspace(policyDraftId);

    }, []);

video:-https://drive.google.com/file/d/1VkKQacoakwwl9X2And_f74MjNEu0Ltib/view?usp=sharing

WojtekBoman commented 7 months ago

Hi! I'm Wojtek from Software Mansion, an expert agency, I can take a look at it because it's a regression after merging ideal-nav-v2

WojtekBoman commented 7 months ago

There are two ways to fix this, we have to decide which option is better for us. This issue is strongly related to the flow of opening the workspace settings, and we need to determine which flow is correct:

When we are redirected to WorkspaceInitialScreen from the deeplink (https://dev.new.expensify.com:8082/settings/workspaces/:id) and then click the back button, we'll be navigated to the workspace list. In this flow when user creates a new workspace from the switcher (chat tab), and then goes back to the previous screen, the workspace list will be displayed (profile tab).

https://github.com/Expensify/App/assets/47774969/a0ac81ad-d80a-4243-9d09-97c7bbc80721

When we are redirected to WorkspaceInitialScreen from the deeplink and then click the back button, we'll be navigated to the home screen. In this flow when user creates a new workspace from the switcher, and then goes back to the previous screen, the home page will be displayed.

https://github.com/Expensify/App/assets/47774969/7233acfc-8479-4b0b-97ac-f9e3ddd41dea

I need to know how the app should behave in this case and then I'll be able to fix it :)

cc: @mountiny @hayata-suenaga @adamgrzybowski

mountiny commented 7 months ago

@WojtekBoman I believe the first option is better cc @trjExpensify

trjExpensify commented 7 months ago

Can't we keep the bottom tab position they were on when they opened the switcher? Is that out of the question for some reason?

trjExpensify commented 7 months ago

Chatted to Vit and it seems like (1) is most consistent with our UP navigation, so let's start there. 👍

melvin-bot[bot] commented 7 months ago

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

kaushiktd commented 7 months ago

@WojtekBoman Do you have any feedback for my proposal?

melvin-bot[bot] commented 7 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 7 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.58-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/38734

If no regressions arise, payment will be issued on 2024-04-09. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@WojtekBoman does not require payment (Contractor)
@alitoshmatov requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 7 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@alitoshmatov] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@alitoshmatov] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@alitoshmatov] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@alitoshmatov] Determine if we should create a regression test for this bug.
[ ] [@alitoshmatov] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

mountiny commented 7 months ago

$500 to @alitoshmatov

kadiealexander commented 7 months ago

@alitoshmatov please don't forget about the checklist!

kadiealexander commented 6 months ago

Bumped @alitoshmatov here.

alitoshmatov commented 6 months ago

The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/33280
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/33280/files#r1561250597
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
Determine if we should create a regression test for this bug. No need

kadiealexander commented 6 months ago

Payouts due:

[x] Contributor+: $500 @alitoshmatov

Upwork job is here.

Expensify / App