[HOLD for payment 2023-11-22] [$500] Workspace - Error page appears for a second after deleting a Workspace

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.87-10
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Log in with any account
2. Open the workspace #admins or #announce cha
3. Press the chat header to open the details
4. Press the workspace name below the chat name. You will be navigated to the workspace page
5. Delete the workspace

Expected Result:

You will be navigated back to the chat details

Actual Result:

Error page appears for a second and then navigated back to the chat details

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• Windows: Chrome
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

Windows: Chrome

Bug6244780_1697826660099.Error.mp4

MacOS: Desktop

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[ZhenjaHorbach]

Proposal

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

Error page appears for a second after deleting a Workspace

What is the root cause of that problem?

When we confirm delete
We call this function

App/src/pages/workspace/WorkspaceInitialPage.js

Lines 77 to 83 in bf8b849

	const confirmDeleteAndHideModal = useCallback(() => {
	const policyReports = _.filter(props.reports, (report) => report && report.policyID === policy.id);
	Policy.deleteWorkspace(policy.id, policyReports, policy.name);
	setIsDeleteModalOpen(false);
	// Pop the deleted workspace page before opening workspace settings.
	Navigation.goBack(ROUTES.SETTINGS_WORKSPACES);
	}, [props.reports, policy]);

As a result PolicyUtils.isPendingDeletePolicy(props.policy) becomes true

App/src/pages/workspace/WorkspaceInitialPage.js

Line 204 in bf8b849

shouldShow={_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy)}

But at this point the animation is not over yet

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

I think the easiest way to fix this is
Will update the condition
So that FullPageNotFoundView is displayed only then when the screen is in focus

shouldShow={(_.isEmpty(props.policy) || !PolicyUtils.isPolicyAdmin(props.policy) || PolicyUtils.isPendingDeletePolicy(props.policy)) && navigation.isFocused()}

What alternative solutions did you explore? (Optional)

An alternative might be that we would first navigate and then call Policy.deleteWorkspace

Or in shouldShow condition we can use previous value for props.policy using usePrevious

Since we will no longer be able to reopen the workspace
This could be a simple alternative

[samilabud]

Proposal

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

Error page appears for a second after deleting a Workspace

What is the root cause of that problem?

When the prop shouldShow accepts any change of policy, like PolicyUtils.isPendingDeletePolicy(policy), the component renders each time the policy change, in this case when pressing the delete workspace button the policy value trigger 3 times.

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

The best option should be to use the react hook (useState) when the policy value change to send a true/false to the component below:

App/src/pages/workspace/WorkspaceInitialPage.js

Line 204 in 4a1e2b3

shouldShow={_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy)}

const [shouldShowErrorPage, setShouldShowErrorPage] = useState(false);

useEffect(()=>{
        return setShouldShowErrorPage(_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy))
    },[policy])

Before changes:

Workspace.when.deleted.before.changes.mp4

After changes:

Workspace.tests.reduced.mp4

[dukenv0307]

Proposal

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

Error page appears for a second and then navigated back to the chat details

What is the root cause of that problem?

In here, we'll show not found page if the workspace is pending delete. So right after we delete the workspace here, it will show not found page briefly.

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

We should not show the not found page in the case of "now the workspace is pending delete, but it's not before", because that means the user just deleted the page and we're pending the navigation.

We can update this line to

shouldShow={_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || (PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy))}

And use the const prevPolicy = usePrevious(policy); to get previous policy.

I also notice another issue, when we open the workspace initial page in 2 tabs, then we delete the workspace in 1 tab, it will show Not Found page in the other tab. In both tabs it should return back to the Workspace List page IMO because if not it will cause confusion for the user. If we want to fix this, we should remove this line and rely on the policy state (if the policy was not deleted and now it's deleted) to navigate instead

useEffect(() => {
    if (PolicyUtils.isPendingDeletePolicy(policy) && !PolicyUtils.isPendingDeletePolicy(prevPolicy)) {
        Navigation.goBack(ROUTES.SETTINGS_WORKSPACES);
    }
}, [policy, prevPolicy]);

(for deleting workspace in 1 device, it syncs to another device, it will require a bit more fix, but as far as I know we consider dynamic updates across devices as bugs now)

What alternative solutions did you explore? (Optional)

Using page focus status: will cause Not Found page to not show up when transitioning in "real" Not Found case.
Relying on PolicyUtils.isPendingDeletePolicy(prevPolicy) only: will cause issues when the delete workspace query fails and we fallback to failure data, we need to check both current and prev policy.

[melvin-bot]

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

[hoangzinh]

reviewing today.

[hoangzinh]

@samilabud Thanks for your proposal #30100 (comment). You pointed out which line of code causes this issue. But your proposal is vague to me. The RCA section should clearly point out what is truly the root cause of this issue, and the solution should be clear enough on how it will solve the issue.

[hoangzinh]

@dukenv0307 Thanks for your proposal #30100 (comment). Your proposal pointed out a correct RC. But your solution is not strong enough for me. It depends on the prevPolicy and current policy value which easily be broken by other props, in my opinion.

[dukenv0307]

@hoangzinh thanks for the review!

It depends on the prevPolicy and current policy value which easily be broken by other props, in my opinion.

@hoangzinh can you clarify what you mean by this?

[samilabud]

Proposal

Updated

@hoangzinh this was what I meant, please verify again

[melvin-bot]

@hoangzinh, @stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

[stephanieelliott]

Hey @hoangzinh can you review this updated proposal when you get a chance: #30100 (comment)

[hoangzinh]

@samilabud Thanks for updates. I think your solution might cause a regression bug when the user accesses a not found workspace case from the beginning. Because your check is only triggered when the user leaves the page (or unmount component).

[hoangzinh]

can you clarify what you mean by this?

@dukenv0307, I mean it's possible that while in animation transition time, there is another prop is updated in that component, it could make your condition here PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy) become true

[hoangzinh]

@ZhenjaHorbach Thanks for your proposal. Your RCA is correct. But your solution causes a regression bug for a real not found workspace case => when the screen is wiped out, the Workspace setting screen is displayed

Screen.Recording.2023-10-31.at.15.33.27.mov

[ZhenjaHorbach]

@hoangzinh
Hello )
What isFocused did you use?
Important to use isFocused() from const navigation = useNavigation()
since this isFocused() does not cause rerendering
there should be no such regression in cases when we do not have updates from (_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy))

const navigation = useNavigation();
shouldShow={(_.isEmpty(policy) || !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy)) && navigation.isFocused()}

Screen.Recording.2023-10-31.at.09.54.49.mov

[dukenv0307]

@hoangzinh The PR is ready for review.

[melvin-bot]

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

[melvin-bot]

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

• Fix not found page display briefly after deleting the workspace #31255

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @hoangzinh requires payment
• @dukenv0307 requires payment

[melvin-bot]

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:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] 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.
• [@trjExpensify / @stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[trjExpensify]

👋 checklist time, @hoangzinh!

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: Fix/27173: Offline deleted workspace allow to open reimburse page #27743
• 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/27743/files#r1403340496
• 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: N/A
• Determine if we should create a regression test for this bug. 🚫 This is a UI improvement, I think we don't need a regression test for this bug.

[trjExpensify]

Yep, agree this was caught by Applause when executing the "core" steps of deleting a workspace.

Okay, payments are as follows by my calculations:

• $500 to @hoangzinh for the C+ review
• $500 to @dukenv0307 for the fix

No urgency bonuses apply as the PR was created after the Oct 24th cutoff.

Sent you both offers!

[hoangzinh]

I haven't received the offer yet, could you check again @trjExpensify? Thanks

[trjExpensify]

@dukenv0307 - paid!

@hoangzinh hm, interesting. Let me check.

[trjExpensify]

Okay, how about now? :)

[hoangzinh]

Accepted. Thanks @trjExpensify

[trjExpensify]

Nice one, paid. Closing!