Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: deleted workspace with invoices is accessible by url #49509

Merged
merged 16 commits into from
Nov 19, 2024
Merged

fix: deleted workspace with invoices is accessible by url #49509

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8768859
fix: deleted workspace with invoices is accessible by url
gijoe0295 Sep 20, 2024
4ff0385
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Sep 20, 2024
db89d9f
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Sep 23, 2024
dc1e317
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Sep 23, 2024
5ebef5e
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Sep 26, 2024
fe57efb
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Sep 27, 2024
729b67b
use prevPolicy
gijoe0295 Sep 27, 2024
093f361
remove redundant changes
gijoe0295 Sep 27, 2024
365c004
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Oct 4, 2024
d8607f7
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Oct 9, 2024
b382b17
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Oct 11, 2024
93d97d1
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Nov 7, 2024
9056d2d
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Nov 15, 2024
e441bc6
do not reuseConnection in withPolicy to avoid infinite loading
gijoe0295 Nov 15, 2024
b6fbe5e
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/49093
gijoe0295 Nov 18, 2024
649137c
reference issue
gijoe0295 Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/pages/workspace/WorkspaceInitialPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, route}: Workspac
const [isCurrencyModalOpen, setIsCurrencyModalOpen] = useState(false);
const hasPolicyCreationError = !!(policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && !isEmptyObject(policy.errors));
const [connectionSyncProgress] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policy?.id}`);
const [currentUserLogin] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.email});
const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${route.params?.policyID ?? '-1'}`);
const hasSyncError = PolicyUtils.hasSyncError(policy, isConnectionInProgress(connectionSyncProgress, policy));
const waitForNavigate = useWaitForNavigation();
Expand Down Expand Up @@ -296,11 +297,11 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, route}: Workspac
const prevProtectedMenuItems = usePrevious(protectedCollectPolicyMenuItems);
const enabledItem = protectedCollectPolicyMenuItems.find((curItem) => !prevProtectedMenuItems.some((prevItem) => curItem.routeName === prevItem.routeName));

const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);
const prevShouldShowPolicy = usePrevious(shouldShowPolicy);
Copy link
Contributor

@ishpaul777 ishpaul777 Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you do it like this it seems to be not working sometimes and sometime i still see workspacepages we want something like mentioned in this proposal #49093 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see in this vid. workspace is deleted but it still navigates to the page

Screen.Recording.2024-09-27.at.12.20.38.AM-1.mov

Copy link
Contributor Author

@gijoe0295 gijoe0295 Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem on main: If we navigated to an invalid link with 3-pane layout, we would have duplicated not found pages in LHP and central pane. And I haven't seen it get reported:

Screenshot 2024-09-28 at 01 26 20

cc @kosmydel Can you take a look since you worked on #41665?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for me your solution works only sometimes, mainly on small screens it often does not work @gijoe0295

Copy link
Contributor Author

@gijoe0295 gijoe0295 Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishpaul777 Besides the above issue, I pushed the approach mentioned in your linked proposal.

Screen.Recording.2024-09-28.at.01.43.58.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay thanks! i'll test again in my morning, off for the day today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gijoe0295, thanks for reaching out. Unfortunately, I no longer work for Expensify. If you need help with this issue, you can try asking C+, who was responsible for this issue, or someone else who participated in the discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops thank you. Sorry for the inconvenience.

// We check shouldShowPolicy and prevShouldShowPolicy to prevent the NotFound view from showing right after we delete the workspace
// eslint-disable-next-line rulesdir/no-negated-variables
const shouldShowNotFoundPage =
isEmptyObject(policy) ||
// We check isPendingDelete for both policy and prevPolicy to prevent the NotFound view from showing right after we delete the workspace
(PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy));
const shouldShowNotFoundPage = isEmptyObject(policy) || (!shouldShowPolicy && !prevShouldShowPolicy);

useEffect(() => {
if (isEmptyObject(prevPolicy) || PolicyUtils.isPendingDeletePolicy(prevPolicy) || !PolicyUtils.isPendingDeletePolicy(policy)) {
Expand Down Expand Up @@ -350,7 +351,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, route}: Workspac
onBackButtonPress={Navigation.dismissModal}
onLinkPress={Navigation.resetToHome}
shouldShow={shouldShowNotFoundPage}
subtitleKey={isEmptyObject(policy) ? undefined : 'workspace.common.notAuthorized'}
subtitleKey={shouldShowPolicy ? 'workspace.common.notAuthorized' : undefined}
>
<HeaderWithBackButton
title={policyName}
Expand Down
18 changes: 9 additions & 9 deletions src/pages/workspace/WorkspacePageWithSections.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,11 @@ function WorkspacePageWithSections({
}: WorkspacePageWithSectionsProps) {
const styles = useThemeStyles();
const policyID = route.params?.policyID ?? '-1';
useNetwork({onReconnect: () => fetchData(policyID, shouldSkipVBBACall)});
const {isOffline} = useNetwork({onReconnect: () => fetchData(policyID, shouldSkipVBBACall)});

const [user] = useOnyx(ONYXKEYS.USER);
const [reimbursementAccount = CONST.REIMBURSEMENT_ACCOUNT.DEFAULT_DATA] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT);
const [currentUserLogin] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.email});

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const isLoading = (reimbursementAccount?.isLoading || isPageLoading) ?? true;
Expand All @@ -138,7 +139,7 @@ function WorkspacePageWithSections({
const {shouldUseNarrowLayout} = useResponsiveLayout();
const firstRender = useRef(showLoadingAsFirstRender);
const isFocused = useIsFocused();
const prevPolicy = usePrevious(policy);

useEffect(() => {
// Because isLoading is false before merging in Onyx, we need firstRender ref to display loading page as well before isLoading is change to true
firstRender.current = false;
Expand All @@ -150,19 +151,18 @@ function WorkspacePageWithSections({
}, [policyID, shouldSkipVBBACall]),
);

const shouldShowPolicy = useMemo(() => PolicyUtils.shouldShowPolicy(policy, isOffline, currentUserLogin), [policy, isOffline, currentUserLogin]);
const prevShouldShowPolicy = usePrevious(shouldShowPolicy);
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 isPendingDelete for both policy and prevPolicy to prevent the NotFound view from showing right after we delete the workspace
return (
(!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) ||
(PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy))
);
// 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 && !prevShouldShowPolicy);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [policy, shouldShowNonAdmin]);
}, [policy, shouldShowNonAdmin, shouldShowPolicy, prevShouldShowPolicy]);

return (
<ScreenWrapper
Expand All @@ -176,7 +176,7 @@ function WorkspacePageWithSections({
onBackButtonPress={Navigation.dismissModal}
onLinkPress={Navigation.resetToHome}
shouldShow={shouldShow}
subtitleKey={isEmptyObject(policy) ? undefined : 'workspace.common.notAuthorized'}
subtitleKey={shouldShowPolicy ? 'workspace.common.notAuthorized' : undefined}
shouldForceFullScreen
>
<HeaderWithBackButton
Expand Down
Loading