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

Not here error appears briefly when enabling features #40650

Merged
merged 12 commits into from
Apr 24, 2024
Merged

Not here error appears briefly when enabling features #40650

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0da9f7f
Fix bug with Not here error appears briefly when enabling features
ZhenjaHorbach Apr 20, 2024
3f893fd
Refactor switch
ZhenjaHorbach Apr 20, 2024
0744c28
Refactor changes in FeatureEnabledAccessOrNotFoundComponent
ZhenjaHorbach Apr 20, 2024
224c9ea
Refactor changes in FeatureEnabledAccessOrNotFoundComponent x2
ZhenjaHorbach Apr 20, 2024
b72180c
Add clearPendingFieldsForMoreFeatures and refactor WorkspaceInitialPage
ZhenjaHorbach Apr 21, 2024
578b27e
Update condition for clearPendingFieldsForMoreFeatures
ZhenjaHorbach Apr 21, 2024
cf28c35
Remove clearPendingFieldsForMoreFeatures
ZhenjaHorbach Apr 21, 2024
6a16d6d
Remove isFocuesd from FeatureEnabledAccessOrNotFoundComponent
ZhenjaHorbach Apr 22, 2024
bd426b3
Update condition for FeatureEnabledAccessOrNotFoundComponent
ZhenjaHorbach Apr 22, 2024
dd18fab
Add comments for useEffects
ZhenjaHorbach Apr 22, 2024
29269b7
Update comments
ZhenjaHorbach Apr 23, 2024
7752289
Update comments x3
ZhenjaHorbach Apr 23, 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
12 changes: 9 additions & 3 deletions src/components/Switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useEffect, useRef} from 'react';
import {Animated} from 'react-native';
import {Animated, InteractionManager} from 'react-native';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useNativeDriver from '@libs/useNativeDriver';
Expand Down Expand Up @@ -32,6 +32,12 @@ function Switch({isOn, onToggle, accessibilityLabel, disabled}: SwitchProps) {
const offsetX = useRef(new Animated.Value(isOn ? OFFSET_X.ON : OFFSET_X.OFF));
const theme = useTheme();

const handleSwitchPress = () => {
InteractionManager.runAfterInteractions(() => {
onToggle(!isOn);
});
};

useEffect(() => {
Animated.timing(offsetX.current, {
toValue: isOn ? OFFSET_X.ON : OFFSET_X.OFF,
Expand All @@ -44,8 +50,8 @@ function Switch({isOn, onToggle, accessibilityLabel, disabled}: SwitchProps) {
<PressableWithFeedback
disabled={disabled}
style={[styles.switchTrack, !isOn && styles.switchInactive]}
onPress={() => onToggle(!isOn)}
onLongPress={() => onToggle(!isOn)}
onPress={handleSwitchPress}
onLongPress={handleSwitchPress}
role={CONST.ROLE.SWITCH}
aria-checked={isOn}
accessibilityLabel={accessibilityLabel}
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3806,7 +3806,9 @@ function navigateWhenEnableFeature(policyID: string, featureRoute: Route) {
new Promise<void>((resolve) => {
resolve();
}).then(() => {
Navigation.navigate(featureRoute);
requestAnimationFrame(() => {
Navigation.navigate(featureRoute);
});
});
}

Expand Down
20 changes: 18 additions & 2 deletions src/pages/workspace/FeatureEnabledAccessOrNotFoundWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
/* eslint-disable rulesdir/no-negated-variables */
import React, {useEffect} from 'react';
import React, {useEffect, useState} 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 useNetwork from '@hooks/useNetwork';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as Policy from '@userActions/Policy';
Expand Down Expand Up @@ -33,9 +34,24 @@ type FeatureEnabledAccessOrNotFoundComponentProps = FeatureEnabledAccessOrNotFou
};

function FeatureEnabledAccessOrNotFoundComponent(props: FeatureEnabledAccessOrNotFoundComponentProps) {
const pendingField = props.policy?.pendingFields?.[props.featureName];
const isPolicyIDInRoute = !!props.policyID?.length;
const isFeatureEnabled = PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName);

const [isPolicyFeatureEnabled, setIsPolicyFeatureEnabled] = useState(isFeatureEnabled);
const {isOffline} = useNetwork();

const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !isPolicyFeatureEnabled;
const shouldShowFullScreenLoadingIndicator = props.isLoadingReportData !== false && (!Object.entries(props.policy ?? {}).length || !props.policy?.id);
const shouldShowNotFoundPage = isEmptyObject(props.policy) || !props.policy?.id || !PolicyUtils.isPolicyFeatureEnabled(props.policy, props.featureName);

// If the feature is pending, we should not update the feature state.
// This is due to that during the creation of a workspace the feature state changes several times while we are waiting for a response from the backend, what unexpectedly causes 'Not Found' to be shown.
puneetlath marked this conversation as resolved.
Show resolved Hide resolved
useEffect(() => {
ZhenjaHorbach marked this conversation as resolved.
Show resolved Hide resolved
if (pendingField && !isOffline && !isFeatureEnabled) {
return;
}
setIsPolicyFeatureEnabled(isFeatureEnabled);
}, [pendingField, isOffline, isFeatureEnabled]);

useEffect(() => {
if (!isPolicyIDInRoute || !isEmptyObject(props.policy)) {
Expand Down
48 changes: 42 additions & 6 deletions src/pages/workspace/WorkspaceInitialPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import OfflineWithFeedback from '@components/OfflineWithFeedback';
import ScreenWrapper from '@components/ScreenWrapper';
import ScrollView from '@components/ScrollView';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import usePermissions from '@hooks/usePermissions';
import usePrevious from '@hooks/usePrevious';
import useSingleExecution from '@hooks/useSingleExecution';
Expand All @@ -33,6 +34,7 @@ import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';
import type {PolicyFeatureName} from '@src/types/onyx/Policy';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type IconAsset from '@src/types/utils/IconAsset';
import type {WithPolicyAndFullscreenLoadingProps} from './withPolicyAndFullscreenLoading';
Expand Down Expand Up @@ -71,6 +73,8 @@ type WorkspaceInitialPageOnyxProps = {

type WorkspaceInitialPageProps = WithPolicyAndFullscreenLoadingProps & WorkspaceInitialPageOnyxProps & StackScreenProps<FullScreenNavigatorParamList, typeof SCREENS.WORKSPACE.INITIAL>;

type PolicyFeatureStates = Record<PolicyFeatureName, boolean>;

function dismissError(policyID: string) {
PolicyUtils.goBackFromInvalidPolicy();
Policy.removeWorkspace(policyID);
Expand All @@ -86,6 +90,20 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
const activeRoute = useNavigationState(getTopmostWorkspacesCentralPaneName);
const {translate} = useLocalize();
const {canUseAccountingIntegrations} = usePermissions();
const {isOffline} = useNetwork();

const prevPendingFields = usePrevious(policy?.pendingFields);
const policyFeatureStates = useMemo(
() => ({
[CONST.POLICY.MORE_FEATURES.ARE_DISTANCE_RATES_ENABLED]: policy?.areDistanceRatesEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_WORKFLOWS_ENABLED]: policy?.areWorkflowsEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_CATEGORIES_ENABLED]: policy?.areCategoriesEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_TAGS_ENABLED]: policy?.areTagsEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED]: policy?.tax?.trackingEnabled,
[CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED]: policy?.areConnectionsEnabled,
}),
[policy],
) as PolicyFeatureStates;
Comment on lines +96 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

This cast as PolicyFeatureStates is wrong, we shouldn't cast except for very very very exceptional cases. If you remove the cast, we can clearly see that the type here is wrong:

image

You are saving possibly undefined values into a type that is supposed to have boolean (not boolean | undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, my fault
I agree that this is a mistake


const policyID = policy?.id ?? '';
const policyName = policy?.name ?? '';
Expand Down Expand Up @@ -122,6 +140,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
const shouldShowProtectedItems = PolicyUtils.isPolicyAdmin(policy);
const isPaidGroupPolicy = PolicyUtils.isPaidGroupPolicy(policy);
const isFreeGroupPolicy = PolicyUtils.isFreeGroupPolicy(policy);
const [featureStates, setFeatureStates] = useState(policyFeatureStates);

const protectedFreePolicyMenuItems: WorkspaceMenuItem[] = [
{
Expand Down Expand Up @@ -167,7 +186,24 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc

const protectedCollectPolicyMenuItems: WorkspaceMenuItem[] = [];

if (policy?.areDistanceRatesEnabled) {
// If features are pending, we should not update feature states.
// These changes are made to synchronously change feature states along with FeatureEnabledAccessOrNotFoundComponent.
useEffect(() => {
ZhenjaHorbach marked this conversation as resolved.
Show resolved Hide resolved
setFeatureStates((currentFeatureStates) => {
const newFeatureStates = {} as PolicyFeatureStates;
(Object.keys(policy?.pendingFields ?? {}) as PolicyFeatureName[]).forEach((key) => {
Comment on lines +193 to +194
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 we should always avoid casting (as) since it breaks type safety. Was it really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this case it make sense
Because Object.keys always returns string[]
Which in this case breaks the typing inside the callback

const isFeatureEnabled = PolicyUtils.isPolicyFeatureEnabled(policy, key);
newFeatureStates[key] =
prevPendingFields?.[key] !== policy?.pendingFields?.[key] || isOffline || !policy?.pendingFields?.[key] ? isFeatureEnabled : currentFeatureStates[key];
});
return {
...policyFeatureStates,
...newFeatureStates,
};
});
}, [policy, isOffline, policyFeatureStates, prevPendingFields]);

if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_DISTANCE_RATES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.distanceRates',
icon: Expensicons.Car,
Expand All @@ -176,7 +212,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
});
}

if (policy?.areWorkflowsEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_WORKFLOWS_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.workflows',
icon: Expensicons.Workflows,
Expand All @@ -186,7 +222,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
});
}

if (policy?.areCategoriesEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_CATEGORIES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.categories',
icon: Expensicons.Folder,
Expand All @@ -196,7 +232,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
});
}

if (policy?.areTagsEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_TAGS_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.tags',
icon: Expensicons.Tag,
Expand All @@ -205,7 +241,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
});
}

if (policy?.tax?.trackingEnabled) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_TAXES_ENABLED]) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.taxes',
icon: Expensicons.Tax,
Expand All @@ -215,7 +251,7 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc
});
}

if (policy?.areConnectionsEnabled && canUseAccountingIntegrations) {
if (featureStates?.[CONST.POLICY.MORE_FEATURES.ARE_CONNECTIONS_ENABLED] && canUseAccountingIntegrations) {
protectedCollectPolicyMenuItems.push({
translationKey: 'workspace.common.accounting',
icon: Expensicons.Sync,
Expand Down
Loading