Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[WAITING ON CHECKLIST][$250] Workspace picker -Inconsistent Navigation After Creating Workspace on "Workspace picker" #46816

Closed izarutskaya closed 2 months ago

izarutskaya commented 3 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: 9.0.16-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): biruknew45+ees@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to [staging.new.expensify.com].
  2. Click on the "Workspace picker"
  3. Click on the plus icon to create a new workspace.
  4. Click the in-app back button.
  5. Observe that you are not navigated back to the previous page.
  6. Navigate to the inbox.
  7. Click on the "Workspace picker"
  8. Click on the plus icon to create a new workspace.
  9. Click the browser’s back button.

Expected Result:

After creating a workspace from the "Workspace picker" page, the in-app back button should navigate the user back to the "Workspace picker" page, consistent with the behavior of the browser's back button.

Actual Result:

After creating a workspace from the "Workspace picker" page using the plus icon, the in-app back button fails to navigate to the workspace picker page, while the browser’s back button correctly returns the user to the "Workspace picker" page.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/0c002825-6ed1-46d7-b408-65ff36640554

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d2558fbd9a9d9b23
  • Upwork Job ID: 1820632265547564333
  • Last Price Increase: 2024-08-06
Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

izarutskaya commented 3 months ago

We think this issue might be related to the #collect project.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

jliexpensify commented 3 months ago

Able to repro on v9.0.16-5 (staging)

nkdengineer commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-06 06:56:45 UTC.

Proposal

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

After creating a workspace from the "Workspace picker" page using the plus icon, the in-app back button fails to navigate to the workspace picker page, while the browser’s back button correctly returns the user to the "Workspace picker" page.

What is the root cause of that problem?

When going back here, we just dismissModal, so it will only dismiss the workspace profile page and will not go back to the workspace switcher as it should do.

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

We should use the accepted approach of using backTo route param to make sure the navigation is correct in these cases. (We use same approach in various scenarios, examples: there, there and there)

  1. Update https://github.com/Expensify/App/blob/6c3dcc298cad62f69f7440ddec77dfbd647113d8/src/pages/workspace/WorkspaceInitialPage.tsx#L396 to go to the backTo route if there's backTo param defined.

    onBackButtonPress={() => {
    if (route.params?.backTo) {
        Navigation.resetToHome();
        Navigation.navigate(route.params.backTo);
    } else {
        Navigation.dismissModal();
    }
    }}
  2. Update the route definition here to allow a backTo param

    getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo) as const,
  3. Use the backTo param here and in createWorkspaceWithPolicyDraftAndNavigateToIt too

  4. Then in here, pass the current route (or specifically ROUTES.WORKSPACE_SWITCHER) as backTo

What alternative solutions did you explore? (Optional)

In step 1 above, to navigate to backTo route we can use

Navigation.goBack(route.params.backTo, true);
nkdengineer commented 3 months ago

Proposal updated to add an alternation solution

eh2077 commented 3 months ago

@nkdengineer 's proposal looks good to me. Coding details can be discussed in PR.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @carlosmiceli, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 3 months ago

❌ There was an error making the offer to @eh2077 for the Reviewer role. The BZ member will need to manually hire the contributor.

melvin-bot[bot] commented 3 months ago

❌ There was an error making the offer to @nkdengineer for the Contributor role. The BZ member will need to manually hire the contributor.

melvin-bot[bot] commented 2 months ago

This issue has not been updated in over 15 days. @carlosmiceli, @jliexpensify, @eh2077, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

carlosmiceli commented 2 months ago

Not sure what happened here... @jliexpensify did we end up manually hiring the contributors?

@parasharrajat sorry to bring you in here, but am I to understand that this issue will also fix this one?

I'll move this back to weekly since something seems broken.

melvin-bot[bot] commented 2 months ago

Current assignee @eh2077 is eligible for the External assigner, not assigning anyone new.

jliexpensify commented 2 months ago

Hmm looks like the automation broke, I'll manually hire.

jliexpensify commented 2 months ago

Hired @eh2077 and @nkdengineer

parasharrajat commented 2 months ago

Please hold off hiring anyone while I am checking the task.

melvin-bot[bot] commented 2 months ago

@carlosmiceli, @jliexpensify, @eh2077, @nkdengineer Huh... This is 4 days overdue. Who can take care of this?

carlosmiceli commented 2 months ago

What's the status here @parasharrajat?

parasharrajat commented 2 months ago

I noticed that the PR is merged for this issue so there is nothing we need to hold here.

carlosmiceli commented 2 months ago

Gotcha! Can we close this then @jliexpensify ?

jliexpensify commented 2 months ago

Before I do, I just want to confirm: @parasharrajat you had a PR that fixed this, so there's no need to hire and pay anyone?

I notice @nkdengineer and @eh2077 were hired, so will need to cancel these contracts if so.

eh2077 commented 2 months ago

@jliexpensify I believe our work previously hired is legitimate for payment. And it's been overdue for quite a while

jliexpensify commented 2 months ago

Sorry, let's take a step back here! I want to clarify exactly what needs to be done.

Firstly, I am trying to work out what @parasharrajat means by this: https://github.com/Expensify/App/issues/46816#issuecomment-2329178918

Rajat, can you clarify this comment please? Are you implying the PR was not needed to fix this issue?

Separately, @eh2077 - you and @nkdengineer worked on https://github.com/Expensify/App/pull/46939 that resolved this issue? cc @carlosmiceli

eh2077 commented 2 months ago

Yes, https://github.com/Expensify/App/pull/46939 is the PR we worked on to fix this issue.

jliexpensify commented 2 months ago

Great, so it sounds like this is the Payment Summary and job:

https://www.upwork.com/jobs/~01d2558fbd9a9d9b23

So @parasharrajat - I'll wait for your explanation on this before making payment: https://github.com/Expensify/App/issues/46816#issuecomment-2329178918

parasharrajat commented 2 months ago

Actually, the PR for this issue was already merged before I was tagged on this issue. That's why I suggested we don't need to hold this issue if the work is already done here. No PR has been created or reviewed by me for this issue yet.

jliexpensify commented 2 months ago

Ok, thanks for clearing this one up! @eh2077 do we need a checklist here?

jliexpensify commented 2 months ago

Everyone has been paid, just waiting on checklist.

jliexpensify commented 2 months ago

Bump @eh2077 for checklist

eh2077 commented 2 months ago

Checklist

Regression test

  1. Click on the "Workspace picker"
  2. Click on the plus icon to create a new workspace.
  3. Click the in-app back button.
  4. Observe that you are not navigated back to the previous page.
  5. Navigate to the inbox.
  6. Click on the "Workspace picker"
  7. Click on the plus icon to create a new workspace.
  8. Click the browser’s back button.
  9. Verify that: After creating a workspace from the "Workspace picker" page, the in-app back button should navigate the user back to the "Workspace picker" page

Do you agree 👍 or 👎?