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.

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

[Simplified Collect][CLEAN UP][LOW] Standardize on Workspace wrapper #40598

Merged
109 changes: 109 additions & 0 deletions src/pages/workspace/AccessOrNotFoundWrapper.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/* eslint-disable rulesdir/no-negated-variables */
import React, {useEffect} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView';
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
import NotFoundPage from '@pages/ErrorPage/NotFoundPage';
import * as Policy from '@userActions/Policy';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type * as OnyxTypes from '@src/types/onyx';
import type {PolicyFeatureName} from '@src/types/onyx/Policy';
import callOrReturn from '@src/types/utils/callOrReturn';
import {isEmptyObject} from '@src/types/utils/EmptyObject';

const POLICY_ACCESS_VARIANTS = {
PAID: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPaidGroupPolicy(policy) || !policy?.isPolicyExpenseChatEnabled,
ADMIN: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPolicyAdmin(policy),
} as const satisfies Record<string, (policy: OnyxTypes.Policy) => boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The code here is working well, but I think we need to make it more clean, because while the const name is ACCESS, the return method return true when INACCESS not when ACCESS, so I suggest the following change.

// revert the condition to return true when access
PAID: (policy: OnyxEntry<OnyxTypes.Policy>) => PolicyUtils.isPaidGroupPolicy(policy) && !!policy?.isPolicyExpenseChatEnabled,
ADMIN: (policy: OnyxEntry<OnyxTypes.Policy>) => PolicyUtils.isPolicyAdmin(policy),

// and change its usage here.
// const pageUnaccessible = accessVariants.reduce((acc, variant) => {
//     const accessFunction = POLICY_ACCESS_VARIANTS[variant];
//     return acc || accessFunction(policy);
// }, false);
const isPageAccessible = accessVariants.reduce((acc, variant) => {
    const accessFunction = POLICY_ACCESS_VARIANTS[variant];
    return acc && accessFunction(policy);
}, true);

// const shouldShowNotFoundPage = ... && pageUnaccessible
const shouldShowNotFoundPage = ... && !isPageAccessible

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think using accessVariant name as a const (CONST.POLICY.ACCESS_VARIANTS.PAID) will be more safe than string ('PAID')

const POLICY_ACCESS_VARIANTS = {
    [CONST.POLICY.ACCESS_VARIANTS.PAID]: ....,
    [CONST.POLICY.ACCESS_VARIANTS.ADMIN]: ....
}
<AccessOrNotFoundWrapper accessVariants={[CONST.POLICY.ACCESS_VARIANTS.PAID]}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Sure, I'll apply this, it seems more readable
  2. the idea is that this const is being used only in the context of AccessOrNotFoundWrapper, so splitting its keys to a separate file might be unintuitive, especially since POLICY_ACCESS_VARIANTS is being used as the type source which would be exported to the other places. I'd suggest keeping it this way until we find a case for using POLICY_ACCESS_VARIANTS outside of this component and then move it as a whole

Copy link
Contributor

Choose a reason for hiding this comment

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

the idea is that this const is being used only in the context of AccessOrNotFoundWrapper.

@BrtqKr But it used in many places where AccessOrNotFoundWrapper is used as a prop of it, so I think we need to update it.

Also, this point is a Reviewer Checklist item

[ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such


type PolicyAccessVariant = keyof typeof POLICY_ACCESS_VARIANTS;

type AccessOrNotFoundWrapperOnyxProps = {
/** The report currently being looked at */
policy: OnyxEntry<OnyxTypes.Policy>;

/** Indicated whether the report data is loading */
isLoadingReportData: OnyxEntry<boolean>;
};

type AccessOrNotFoundWrapperProps = AccessOrNotFoundWrapperOnyxProps & {
/** The children to render */
children: ((props: AccessOrNotFoundWrapperOnyxProps) => React.ReactNode) | React.ReactNode;

/** The report currently being looked at */
policyID: string;

/** Defines which types of access should be verified */
accessVariants?: PolicyAccessVariant[];

/** The current feature name that the user tries to get access to */
featureName?: PolicyFeatureName;
};

type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {showFullScreenFallback: boolean};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {showFullScreenFallback: boolean};
type PageNotFoundFallbackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {shouldShowFullScreenFallback: boolean};


function PageNotFoundFallback({policyID, showFullScreenFallback}: PageNotFoundFallackProps) {
return showFullScreenFallback ? (
<FullPageNotFoundView
shouldShow
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)}
shouldForceFullScreen
/>
) : (
<NotFoundPage onBackButtonPress={() => Navigation.goBack(ROUTES.WORKSPACE_PROFILE.getRoute(policyID))} />
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think no need for using both FullPageNotFoundView and NotFoundPage because NotFoundPage is the same as FullPageNotFoundView else if we pass prop shouldForceFullScreen = true.
What do you think about using only FullPageNotFoundView like this?

<FullPageNotFoundView
    shouldShow
    onBackButtonPress={() => Navigation.goBack(!isFeatureEnabled ? ROUTES.SETTINGS_WORKSPACES : ROUTES.WORKSPACE_PROFILE.getRoute(policyID))}
    shouldForceFullScreen={!isFeatureEnabled}
/>

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also use it inline and remove PageNotFoundFallback component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NotFoundPage is inside of a ScreenWrapper, which has a SafeAreaView included. If we replace it with FullPageNotFoundView, it will break the mobile version of the app as shown below. It's been done this way to preserve the logic present in the previous version.
I think we should take the wider approach and unify them both in a separate ticket.


function AccessOrNotFoundWrapper({accessVariants = [], ...props}: AccessOrNotFoundWrapperProps) {
const {policy, policyID, featureName, isLoadingReportData} = props;

const isPolicyIDInRoute = !!policyID?.length;

useEffect(() => {
if (!isPolicyIDInRoute || !isEmptyObject(policy)) {
// If the workspace is not required or is already loaded, we don't need to call the API
return;
}

Policy.openWorkspace(policyID, []);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isPolicyIDInRoute, policyID]);

const shouldShowFullScreenLoadingIndicator = isLoadingReportData !== false && (!Object.entries(policy ?? {}).length || !policy?.id);

const isFeatureEnabled = featureName ? PolicyUtils.isPolicyFeatureEnabled(policy, featureName) : true;
const pageUnaccessible = accessVariants.reduce((acc, variant) => {
const accessFunction = POLICY_ACCESS_VARIANTS[variant];
return acc || accessFunction(policy);
}, false);

const shouldShowNotFoundPage = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id || pageUnaccessible || !isFeatureEnabled;

if (shouldShowFullScreenLoadingIndicator) {
return <FullscreenLoadingIndicator />;
}

if (shouldShowNotFoundPage) {
return (
<PageNotFoundFallback
policyID={policyID}
showFullScreenFallback={!isFeatureEnabled}
/>
);
}

return callOrReturn(props.children, props);
}

export default withOnyx<AccessOrNotFoundWrapperProps, AccessOrNotFoundWrapperOnyxProps>({
policy: {
key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID ?? ''}`,
},
isLoadingReportData: {
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
},
})(AccessOrNotFoundWrapper);
66 changes: 0 additions & 66 deletions src/pages/workspace/AdminPolicyAccessOrNotFoundWrapper.tsx

This file was deleted.

74 changes: 0 additions & 74 deletions src/pages/workspace/FeatureEnabledAccessOrNotFoundWrapper.tsx

This file was deleted.

66 changes: 0 additions & 66 deletions src/pages/workspace/PaidPolicyAccessOrNotFoundWrapper.tsx

This file was deleted.

40 changes: 20 additions & 20 deletions src/pages/workspace/WorkspaceMoreFeaturesPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ import type {TranslationPaths} from '@src/languages/types';
import type SCREENS from '@src/SCREENS';
import type {PendingAction} from '@src/types/onyx/OnyxCommon';
import type IconAsset from '@src/types/utils/IconAsset';
import AdminPolicyAccessOrNotFoundWrapper from './AdminPolicyAccessOrNotFoundWrapper';
import PaidPolicyAccessOrNotFoundWrapper from './PaidPolicyAccessOrNotFoundWrapper';
import AccessOrNotFoundWrapper from './AccessOrNotFoundWrapper';
import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading';
import withPolicyAndFullscreenLoading from './withPolicyAndFullscreenLoading';
import ToggleSettingOptionRow from './workflows/ToggleSettingsOptionRow';
Expand Down Expand Up @@ -196,24 +195,25 @@ function WorkspaceMoreFeaturesPage({policy, route}: WorkspaceMoreFeaturesPagePro
);

return (
<AdminPolicyAccessOrNotFoundWrapper policyID={route.params.policyID}>
<PaidPolicyAccessOrNotFoundWrapper policyID={route.params.policyID}>
<ScreenWrapper
includeSafeAreaPaddingBottom={false}
style={[styles.defaultModalContainer]}
testID={WorkspaceMoreFeaturesPage.displayName}
shouldShowOfflineIndicatorInWideScreen
>
<HeaderWithBackButton
icon={Illustrations.Gears}
title={translate('workspace.common.moreFeatures')}
shouldShowBackButton={isSmallScreenWidth}
/>

<ScrollView contentContainerStyle={styles.pb2}>{sections.map(renderSection)}</ScrollView>
</ScreenWrapper>
</PaidPolicyAccessOrNotFoundWrapper>
</AdminPolicyAccessOrNotFoundWrapper>
<AccessOrNotFoundWrapper
accessVariants={['ADMIN', 'PAID']}
policyID={route.params.policyID}
>
<ScreenWrapper
includeSafeAreaPaddingBottom={false}
style={[styles.defaultModalContainer]}
testID={WorkspaceMoreFeaturesPage.displayName}
shouldShowOfflineIndicatorInWideScreen
>
<HeaderWithBackButton
icon={Illustrations.Gears}
title={translate('workspace.common.moreFeatures')}
shouldShowBackButton={isSmallScreenWidth}
/>

<ScrollView contentContainerStyle={styles.pb2}>{sections.map(renderSection)}</ScrollView>
</ScreenWrapper>
</AccessOrNotFoundWrapper>
);
}

Expand Down
Loading
Loading