[HOLD for payment 2024-08-02] [$500] "Go back to home page" in LHN doesn't work when WS deleted from secondary device

[kavimuru]

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.4.39-0
Reproducible in staging?: y
Reproducible in production?: y
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:

Prerequisite: Main and secondary devices logged in to the same user in New Expensify

1. [Main device] Create new Workspace
2. [Main device] Navigate to Members
3. [Secondary device] Delete Workspace created in Main device
4. [Main device] Click on "Go back to home page" in LHN

Expected Result:

User should be redirected to Workspaces settings.

Actual Result:

Nothing happens when user clicks on "Go back to home page" in LHN

Workaround:

unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6373526_1707483142078.2024-02-08_23-53-05.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0170390297c07d44ce
• Upwork Job ID: 1755993628416561152
• Last Price Increase: 2024-02-16

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[kavimuru]

We think that this bug might be related to #wave8-collect-admins

[jeremy-croff]

Proposal

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

The Go back to home page link is not taking you back home

What is the root cause of that problem?

The link actions is defined to dismiss the modal it's expected to be in. The Component is FullPageNotFoundView. however we are not in a modal. This is part of navigation previous navigation changes.

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

We should update the link action from onLinkPress = () => Navigation.dismissModal() to() => Navigation.goBack(ROUTES.HOME)
This way the action semantically matches the user text, and can resolve the navigation action without erroring.

What alternative solutions did you explore? (Optional)

[ghost]

Proposal

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

"Go back to home page" in LHN doesn't work when WS deleted from

What is the root cause of that problem?

As discussed in the below comment.

Firstly, we need to change text Go back to home page i.e. in the en.ts file here:

https://github.com/godofoutcasts94/App/blob/72084da8c578e19537789456700a133b59568599/src/languages/en.ts#L1282

In the FullPageNotFoundView.tsx, file i.e. here

App/src/components/BlockingViews/FullPageNotFoundView.tsx

Line 55 in e6147ae

onLinkPress = () => Navigation.dismissModal(),

we, are dismissing modal, instead of doing that, we need to route back to Workspace Settings Page.
Also, in the WorkspaceMembersPage here:

App/src/pages/workspace/WorkspaceMembersPage.js

Line 428 in e6147ae

onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)}

we are just moving back to workspace settings page.

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

Firstly, we need to add Go back in the en.ts file i.e. here :

https://github.com/godofoutcasts94/App/blob/72084da8c578e19537789456700a133b59568599/src/languages/en.ts#L1282

in place of Go back to home page and after that to move to Workspace Settings page. We need to add

    onLinkPress = () => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES),

in the FullPageNotFoundView.tsx. So, that we are routed back to Workspace Settings Page only.

And also, we need to add this :

    const goBack = () => {
        Navigation.goBack(ROUTES.SETTINGS_WORKSPACES);

        // Needed when workspace with given policyID does not exist
        Navigation.navigateWithSwitchPolicyID({route: ROUTES.ALL_SETTINGS});
    };

only, this in the WorkspaceMembersPage.

Working Video of the Solution

Screen.Recording.2024-02-19.at.6.33.41.PM.mp4

What alternative solutions did you explore? (Optional)

N/A

[jeremy-croff]

Proposal

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

The Go back to home page link is not taking you back to settings

What is the root cause of that problem?

The link is not doing anything because the button action is trying to dismiss a modal. The Component is this logic is in is, FullPageNotFoundView. however we are not in a modal. This is part of navigation previous navigation changes.

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

If we want to navigate back to the workspace settings, we want it to be specific to this flow as not all FullPageNotFoundView will want to go to settings.

We should update the link action from onLinkPress = () => Navigation.dismissModal() to () => Navigation.goBack(ROUTES.HOME, true),
and add a new notFound link text with copy to navigate back generically to the previous screen (and not have it say home page) if we want to go back the the previous screen, like settings.
This true boolean on goBack uses ROUTES.HOME as a fallback if there's no previous stack to goBack to.

[ghost]

@jeremy-croff Just wanted to put it that, he just updated his new proposal after, I have put mine.

[jeremy-croff]

@jeremy-croff Just wanted to put it that, he just updated his new proposal after, I have put mine.

I added a second proposal for navigating to settings, since I believe yours is lacking. Instead of updating the original, to accurately reflect the order and not take advantage of you!

[ghost]

@jeremy-croff Just wanted to put it that, he just updated his new proposal after, I have put mine.

I added a second proposal for navigating to settings, since I believe yours is lacking. Instead of updated the original, to accurately reflect the order and not take advantage of you!

My proposal is lacking or not that should be rather decided by Expensify / Proposal Reviewing Team, but for the matter of fact, I believe that inital comments on the second proposal speaks something different, but I am not here to take up or hold any things against anyone. As the the reviewers think is right. I will accept it.

[jeremy-croff]

@jeremy-croff Just wanted to put it that, he just updated his new proposal after, I have put mine.

I added a second proposal for navigating to settings, since I believe yours is lacking. Instead of updated the original, to accurately reflect the order and not take advantage of you!

My proposal is lacking or not that should be rather decided by Expensify / Proposal Reviewing Team, but for the matter of fact, I believe that inital comments on the second proposal speaks something different, but I am not here to take up or hold any things against anyone. As the the reviewers think is right. I will accept it.

I have posted a second proposal after yours that is different from my original one because I am unsure of the expected behaviour to go back home, as the current text indicates, or if should go back to work space settings, like the github issue expects.
In the case that it should be workspace settings, i wanted to write a proposal seperately because it will be handled differently. This is independent of your proposal. If your proposal's solution matched mine I would've not posted mine.

My second proposal is different from yours, as we would avoid regression for a commonly re-used screen, as hardcoding a navigation action to settings ( like you proposed ) is specific to this flow, but not other error screens. I hope you agree that this is a key difference in our proposals and it is listed after yours, so no worry about first come first serve. This is purely a matter of solution chosen.

I did not edit/add an alternative solution to my first proposal, precisely to respect the order and give you the first proposal regarding navigating to workspace-settings.

[eucool]

Proposal

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

"Go back to home page" in LHN doesn't work when WS deleted from secondary device

What is the root cause of that problem?

We do not have a onLinkPress prop defined for the Workspace settings page:

App/src/pages/workspace/WorkspaceInitialPage.tsx

Lines 177 to 181 in bfae5b2

	<FullPageNotFoundView
	onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)}
	shouldShow={shouldShowNotFoundPage}
	subtitleKey={isEmptyObject(policy) ? undefined : 'workspace.common.notAuthorized'}
	>

We also don't have it passed to the WorkspaceMembersPage:

App/src/pages/workspace/WorkspaceMembersPage.js

Lines 425 to 429 in bfae5b2

	<FullPageNotFoundView
	shouldShow={(_.isEmpty(props.policy) && !props.isLoadingReportData) || !PolicyUtils.isPolicyAdmin(props.policy) || PolicyUtils.isPendingDeletePolicy(props.policy)}
	subtitleKey={_.isEmpty(props.policy) ? undefined : 'workspace.common.notAuthorized'}
	onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)}
	>

If we don't pass any prop to the FullPageNotFoundView component, then by default it calls dismiss modal:

App/src/components/BlockingViews/FullPageNotFoundView.tsx

Line 55 in e6147ae

onLinkPress = () => Navigation.dismissModal(),

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

We need to pass the onLinkPress prop to the FullPageNotFoundView component,

Now as we wish to have same functionality for onBackButtonPress and onLinkPress props we can define a new const in order to avoid repeatability, we already do this in WorkspacePageWithSections Page:

App/src/pages/workspace/WorkspacePageWithSections.tsx

Lines 149 to 151 in bfae5b2

	<FullPageNotFoundView
	onBackButtonPress={goBack}
	onLinkPress={goBack}

So in WorkspaceInitialPage and WorkspaceMembersPage, we would define a const goBack as follows:

const goBack = () => {
        Navigation.goBack(ROUTES.SETTINGS_WORKSPACES);

        // Needed when workspace with given policyID does not exist
        Navigation.navigateWithSwitchPolicyID({route: ROUTES.ALL_SETTINGS});
    };

And then use the const in the FullPageNotFoundView component in both pages as follows:

<FullPageNotFoundView 
    onBackButtonPress={goBack} 
     onLinkPress={goBack}

Note

If we want the subtitile to be Go back to workspace setting page then simply pass the prop subtitleKey in both components (We also need to add the same text as a value in the languages folder), this won't be a issue, will follow the instructions from design team and C+ reviewer

The implementation below are totally optional (I think we are having a confusion here as this issue only deals with the above proposed solution :)

Also from the latest comments from @shawnborton , we need to have only one page not found and not two:

We just need to wrap the ScreenWrapper in FullPageNotFoundView

App/src/pages/workspace/WorkspaceInitialPage.tsx

Lines 170 to 171 in bfae5b2

	return (
	<ScreenWrapper

[DylanDylann]

Hi everyone, I see that we have two not-found pages in this case:

1. Not found page on LHN
2. Not found page on the main screen

Screen.Recording.2024-02-11.at.15.27.27.mov

The expected should be: When the user clicks "Go back to home page" on each of them, the App will redirect to the Workspace Setting page totally like this

Some contributors suggest that we should change the subtitle on both Not Found Page from "Go back to home page" to another one like "Go back to workspace setting page". But I think It fine to keep the old one

@Expensify/design Could you help to confirm the expected?

[ghost]

@DylanDylann, my solution doesn't focus on changing the subtitle or anything. My solution more focuses on

Hi everyone, I see that we have two not-found pages in this case:

1. Not found page on LHN

2. Not found page on the main screen

Screen.Recording.2024-02-11.at.15.27.27.mov

The expected should be: When the user clicks "Go back to home page" on each of them, the App will redirect to the Workspace Setting page totally like this

Some contributors suggest that we should change the subtitle on both Not Found Page from "Go back to home page" to another one like "Go back to workspace setting page". But I think It fine to keep the old one

@Expensify/design Could you help to confirm the expected?

@DylanDylann, My solution doesn't focuses on changing the subtitle on the page from "Go back to home page" to
"Go back to workspace setting page". Rather my solution focuses more on redirection to Workspace Settings Page.
Wherever you click among the two not-found pages:

1. Not found page on LHN.
2. Not found page on the main screen.

In both the cases you will be redirected to Workspace Settings Page. If you need more clarity I can share a detailed video of my solution in the thread.

[DylanDylann]

@godofoutcasts94 Where do you apply this change

    onLinkPress = () => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES),

[ghost]

@godofoutcasts94 Where do you apply this change

    onLinkPress = () => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES),

I am planning to apply this change here (that's just an idea maybe I can be right or wrong. Just wanted more clarity if I am wrong as well) :

App/src/components/BlockingViews/FullPageNotFoundView.tsx

Line 55 in bfae5b2

onLinkPress = () => Navigation.dismissModal(),

[eucool]

Proposal Updated

Updated my proposal, to match the expectations from the comments #36266 (comment) by @DylanDylann

[jeremy-croff]

Depending on which way the copy goes my proposal still holds, but will have to follow copy guidance. Main focus of my proposal is to use the Navigation stack to go back to the non error screen, so that this screen generically goes back. With a fallback to HOME if there is no stack.

[MitchExpensify]

Not overdue, progressing

[DylanDylann]

@Expensify/design Please help to check my comment here

[shawnborton]

First of all, I think we are fixing this view so that only one "Not found" view can show at all times. Is that right?

Second, we could just use something way more generic like "Get me out of here" for the link.

[trjExpensify]

Do you think this is wave 8 worthy given its focus on an admin action on workspaces (which will all be Collect when we deprecate free)? @zanyrenney

Yeah, this is wave8 -- an issue born from ideal nav.

[DylanDylann]

@godofoutcasts94 Sorry, It is my mistake. Please update the proposal to cover the case as mentioned here

[ghost]

@godofoutcasts94 Sorry, It is my mistake. Please update the proposal to cover the case as mentioned here

Sure! Updated proposal will be ready in sometime for review. @DylanDylann

[kosmydel]

Hey, I'm from SWM expert's agency.

This issue will be fixed in this PR, as those are closely related.

button.fix.mov

[eucool]

@kosmydel , this works fine for the profile page(even with the current code base), the bug occurs for the members page as we change the component, can you test this again with members page with youe current code?

[kosmydel]

Here you are:

members-button.mov

[ghost]

Here you are:

members-button.mov

@trjExpensify and @DylanDylann is this the required behaviour?

[ghost]

Updated Proposal, i dont know links are not working on the proposal pointing to specific points in the codebase.

[trjExpensify]

Yeah, I think @kosmydel has resolved this issue by the looks of things. So @DylanDylann, this can be closed right?

[DylanDylann]

@trjExpensify Yes. Let's close this one

[trjExpensify]

Cool, sounds good!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[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 9.0.12-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: three not found view v2 #41665

If no regressions arise, payment will be issued on 2024-08-02. 🎊

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

• @DylanDylann requires payment (Needs manual offer from BZ)

[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:

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