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.98k stars 2.98k forks source link

[$125] Workspace - Not found page is displayed only on LHN when deleting a workspace from OD #55122

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks 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.84-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5441953 Email or phone of affected tester (no customers): expensify416+da2@gmail.com Issue reported by: Applause - Internal Team Component: Workspace Settings

Action Performed:

  1. Open one account in new dot and old dot
  2. In ND, create a workspace and navigate to workspace profile page (don't close this page, it should be remained opened)
  3. In OD, navigate to Settings > Workspaces > Group and delete the workspace

Expected Result:

In ND, full screen not found page should be displayed immediately.

Actual Result:

Not found page is only displayed on LHN for a while, then moves to full page.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/84a323e3-49e2-4f78-bb9e-2bcb14ecb51c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021878269152575163326
  • Upwork Job ID: 1878269152575163326
  • Last Price Increase: 2025-01-19
  • Automatic offers:
    • nkdengineer | Contributor | 105780491
Issue OwnerCurrent Issue Owner: @sobitneupane
melvin-bot[bot] commented 2 weeks 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.

jliexpensify commented 2 weeks ago

Able to repro:

image

I think this is a #quality bug but it's low priority.

Changed the results to:

Expected Result:

In ND, full screen not found page should be displayed immediately.

Actual Result:

Not found page is only displayed on LHN for a while, then moves to full page.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Upwork job price has been updated to $125

jliexpensify commented 2 weeks ago

@sobitneupane this kind of seems like a small/easy fix to me, but definitely LMK if this turns out to be more complex and we should bump up the price 🤷‍♂️

ishakakkad commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2025-01-12 06:20:18 UTC.

Proposal

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

Not found page is displayed only on LHN when deleting a workspace from OD

What is the root cause of that problem?

Currently, we are show Not found page on full page if (!prevShouldShowPolicy && !shouldShowPolicy) condition fullfill from here. When we first delete the workspace shouldShowPolicy is true. After rerender component shouldShowPolicy become false. So after sometime Not found page shows on full page.

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

To fix this issue we need to change this condition like below.

return (!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) || !shouldShowPolicy;

And also need to remove logic related to prevShouldShowPolicy https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspacePageWithSections.tsx#L155C11-L155C31

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA for bug

What alternative solutions did you explore? (Optional)

NA

ijmalik commented 2 weeks ago

Hi @jliexpensify,

The issue no longer persists on my local main branch (version: "9.0.84-1"). However, it is still present on staging.

main branch (version: "9.0.84-1")

https://github.com/user-attachments/assets/8f1bc56e-2439-485a-9ec9-9f1af91070bd

Staging:

https://github.com/user-attachments/assets/cdb01e9a-bf0b-4a39-abbe-592a9c7ce8d7

ishakakkad commented 2 weeks ago

Issue is still reproducible in local and staging both the places.

image

nkdengineer commented 1 week ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-20 07:38:59 UTC.

Proposal

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

Not found page is only displayed on LHN for a while, then moves to full page.

What is the root cause of that problem?

  1. When we delete the WS in OD, the policy is changed only once. So shouldShowPolicy is true and prevShouldShowPolicy is false then the not found page doesn't show in WorkspaceProfilePage.

https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/workspace/WorkspacePageWithSections.tsx#L163

  1. After we delete the WS, the logic to navigate back will be handled here and we will not navigate back if the policy is pending delete. So after the API is complete, the pending action is cleared and not found page will display briefly here

https://github.com/Expensify/App/blob/14663690594a436f3a8b5c82697432f9213e576f/src/pages/workspace/WorkspaceInitialPage.tsx#L349-L354

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

  1. (!shouldShowPolicy && !prevShouldShowPolicy) this condition aims to prevent the not found page appears when we delete the workspace in WorkspaceProfilePage but it's not the correct way. We should only not show the not found page if the policy is pending delete and prevPolicy is not. Based on that we can update this condition to show not found page if shouldShowPolicy is true and policy is pending delete and prevPolicy is not.
const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);
const isPendingDelete = policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
const prevIsPendingDelete = prevPolicy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
const shouldShow = useMemo(() => {
    // If the policy object doesn't exist or contains only error data, we shouldn't display it.
    if (((isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
        return true;
    }

    // We check shouldShowPolicy and prevShouldShowPolicy to prevent the NotFound view from showing right after we delete the workspace
    return (!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) || (!shouldShowPolicy && (!isPendingDelete || prevIsPendingDelete));
    // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [policy, shouldShowNonAdmin, shouldShowPolicy, isPendingDelete, prevIsPendingDelete]);

https://github.com/Expensify/App/blob/fb487a59dada9d0d0f8215408287ba86f0eac98b/src/pages/workspace/WorkspacePageWithSections.tsx#L163

  1. We should navigate back immediately after we delete the WS here and we can remove the logic here since I can't reproduce the bug mentioned and the PR introduce this change.
PolicyUtils.goBackFromInvalidPolicy();

https://github.com/Expensify/App/blob/14663690594a436f3a8b5c82697432f9213e576f/src/pages/workspace/WorkspaceProfilePage.tsx#L173

And I think we can also update the condition to show the page that was not found here with the same fix above. https://github.com/Expensify/App/blob/14663690594a436f3a8b5c82697432f9213e576f/src/pages/workspace/WorkspaceInitialPage.tsx#L347

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This is a minor logic bug in the component then I think we don't need to add a test.

Optional: if we want a test, we can create a util for shouldShow in WorkspacePageWithSections and add some test cases for this function.

What alternative solutions did you explore? (Optional)

For point 2, we can update the logic here to call the navigate if the policy is pending delete.

useEffect(() => {
    if (isEmptyObject(prevPolicy) || !PolicyUtils.isPendingDeletePolicy(prevPolicy) || !PolicyUtils.isPendingDeletePolicy(policy)) {
        return;
    }
    console.log("back here");
    PolicyUtils.goBackFromInvalidPolicy();
}, [policy, prevPolicy]);

https://github.com/Expensify/App/blob/14663690594a436f3a8b5c82697432f9213e576f/src/pages/workspace/WorkspaceInitialPage.tsx#L349-L354

melvin-bot[bot] commented 1 week ago

@sobitneupane, @jliexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

jliexpensify commented 1 week ago

Bumping @sobitneupane for reviews

sobitneupane commented 1 week ago
return ... || (!shouldShowPolicy && (!isPendingDelete || prevIsPendingDelete));

@nkdengineer Why would we need prevIsPendingDelete as well in the condition? Wouldn't (!shouldShowPolicy && !isPendingDelete) be enough?

I also noticed that when I delete a workspace, Not Found Page is displayed briefly in LHN. I believe we would not want that.

https://github.com/user-attachments/assets/88794abc-57e4-4bb5-9962-ec2d0d6ea630

nkdengineer commented 1 week ago
!isPendingDelete || prevIsPendingDelete

@sobitneupane this condition to prevent the not found page display briefly when we delete the workspace in workspace profile page.

ishakakkad commented 1 week ago

@nkdengineer I have checked by removing prevShouldShowPolicy and delete the workspace from workspace profile. I can not see no found page briefly. So just removing prevIsPendingDelete works fine, no need to add extra conditions.

sobitneupane commented 1 week ago

@nkdengineer Even with the condition(!isPendingDelete || prevIsPendingDelete), I can reproduce the issue mentioned in above comment.

ishakakkad commented 1 week ago

@sobitneupane my proposal solve the issue I think. Are you facing any challenges in my proposal?

melvin-bot[bot] commented 6 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

nkdengineer commented 6 days ago

@sobitneupane I updated proposal with the RCA and solution to fix the bug here.

melvin-bot[bot] commented 5 days ago

@sobitneupane, @jliexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

jliexpensify commented 5 days ago

Waiting on @sobitneupane to review

sobitneupane commented 5 days ago

my proposal solve the issue I think. Are you facing any challenges in my proposal?

@ishakakkad Thanks for your proposal. Not Found Page is shown briefly if we go with your proposal.

ishakakkad commented 5 days ago

@sobitneupane where are you see "Not Found Page" briefly? I have tested by deleting, I didn't see "Not Found Page" briefly.

https://github.com/user-attachments/assets/c1035878-5660-43a8-9eff-5480f5a75830

sobitneupane commented 5 days ago

@ishakakkad You need to go to the workspace Details/Profile Page and delete it from the profile.

sobitneupane commented 5 days ago

Thanks for the update @nkdengineer. The proposed solution will recreate the issue discussed in 2nd point in RCA in this proposal.

nkdengineer commented 5 days ago

 can remove the logic here since I can't reproduce the bug mentioned and the PR introduce this change.

@sobitneupane This is an optional, we can keep the current logic and only add the new logic in WorkspaceProfilePage.

sobitneupane commented 5 days ago

Thanks for the update @nkdengineer

Proposal from @nkdengineer looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 5 days ago

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

chiragsalian commented 5 days ago

Proposal LGTM. Feel free to create the PR @nkdengineer.

melvin-bot[bot] commented 5 days ago

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

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

nkdengineer commented 4 days ago

@sobitneupane The PR is ready for review.