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

Jump to bottom

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 7 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
22 changes: 20 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,11 @@
/* eslint-disable rulesdir/no-negated-variables */
import React, {useEffect} from 'react';
import {useIsFocused} from '@react-navigation/native';
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 +35,25 @@ 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 isFocused = useIsFocused();
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);

useEffect(() => {
ZhenjaHorbach marked this conversation as resolved.
Show resolved Hide resolved
if (!isFocused) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ZhenjaHorbach Can you elaborate about this one?
This one will cause regression, because in some feature screens, we still have the modal screen, ie: CategoryDetail.
So if we opened CategoryDetail screen and disabled the category feature, we will only see the NotFoundScreen after we dismiss the modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Evidence:

Screen.Recording.2024-04-22.at.14.36.09.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm
Good catch
Actually I think we can remove this condition
Because it was one of the options to get rid of setTimeout from my previous implementation

But now I tested without this condition
And it seems to work well

Copy link
Contributor

Choose a reason for hiding this comment

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

Still didn't work, please make sure you test it carefully.

Screen.Recording.2024-04-22.at.16.14.35.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned few test cases already, please make sure it will test all these cases.
#39443 (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.

I made the video for all cases

zapis-ekrana-2024-04-22-v-114038_PvYQVrKb.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

}
if (!pendingField || isOffline) {
setIsPolicyFeatureEnabled(isFeatureEnabled);
}
}, [isFocused, pendingField, isOffline, isFeatureEnabled]);

useEffect(() => {
if (!isPolicyIDInRoute || !isEmptyObject(props.policy)) {
Expand Down
46 changes: 40 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,22 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, reimbursementAcc

const protectedCollectPolicyMenuItems: WorkspaceMenuItem[] = [];

if (policy?.areDistanceRatesEnabled) {
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 +210,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 +220,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 +230,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 +239,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 +249,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