feat: add warning hints on potential misconfiguration (#2948)

## About the changes Add warnings when we detect something might be wrong with the customer configuration, in particular with regard to variants configuration ## Rationale Moving from variants per feature to variants per environment will allow users to have fine-grained permissions and more control over variants on different environments: #2254 But because this requires an additional step of copying variants to other environments, we identified the potential risk of users forgetting to follow this step. To keep them informed about this, we're introducing a warning sign after a toggle is enabled when we detect that: 1. The environment is enabled without variants 2. Other enabled environments have variants This situation would be a problem if you rely on `getVariant` method from the SDK, because without variants you'll receive the default variant. Probably, not what you'd expect after enabling the toggle, but there are situations where this might be correct. Because of the latter, we thought that adding a warning and letting the user handle the situation was the best solution. ## UI sketches ![image (6)](https://user-images.githubusercontent.com/455064/213676353-112639f0-7781-42c0-8c9d-8c7eba316bae.png) ![Screenshot from 2023-01-19 08-55-10](https://user-images.githubusercontent.com/455064/213664639-7b11ff4b-048a-4a36-aa71-7df2f889adff.png) Co-authored-by: Nuno Góis <github@nunogois.com>
Unleash · Jan 20, 2023 · 70d8f9e · 70d8f9e
1 parent 287d28e
commit 70d8f9e
Show file tree

Hide file tree

Showing 11 changed files with 165 additions and 48 deletions.
diff --git a/frontend/src/component/common/HtmlTooltip/HtmlTooltip.tsx b/frontend/src/component/common/HtmlTooltip/HtmlTooltip.tsx
@@ -7,19 +7,35 @@ const StyledHtmlTooltipBody = styled('div')(({ theme }) => ({
 }));
 
 const StyledHtmlTooltip = styled(
-    ({ className, maxWidth, maxHeight, ...props }: IHtmlTooltipProps) => (
+    ({
+        className,
+        maxWidth,
+        maxHeight,
+        fontSize,
+        ...props
+    }: IHtmlTooltipProps) => (
         <Tooltip
             {...props}
             title={<StyledHtmlTooltipBody>{props.title}</StyledHtmlTooltipBody>}
             classes={{ popper: className }}
         />
     ),
     {
-        shouldForwardProp: prop => prop !== 'maxWidth' && prop !== 'maxHeight',
+        shouldForwardProp: prop =>
+            prop !== 'maxWidth' && prop !== 'maxHeight' && prop !== 'fontSize',
     }
-)<{ maxWidth?: SpacingArgument; maxHeight?: SpacingArgument }>(
-    ({ theme, maxWidth, maxHeight }) => ({
-        maxWidth: maxWidth || theme.spacing(37.5),
+)<{
+    maxWidth?: SpacingArgument;
+    maxHeight?: SpacingArgument;
+    fontSize?: string;
+}>(
+    ({
+        theme,
+        maxWidth = theme.spacing(37.5),
+        maxHeight = theme.spacing(37.5),
+        fontSize = theme.fontSizes.smallerBody,
+    }) => ({
+        maxWidth,
         [`& .${tooltipClasses.tooltip}`]: {
             display: 'flex',
             flexDirection: 'column',
@@ -31,7 +47,8 @@ const StyledHtmlTooltip = styled(
             fontWeight: theme.fontWeight.medium,
             maxWidth: 'inherit',
             border: `1px solid ${theme.palette.lightBorder}`,
-            maxHeight: maxHeight || theme.spacing(37.5),
+            maxHeight,
+            fontSize,
         },
         [`& .${tooltipClasses.arrow}`]: {
             '&:before': {
@@ -45,6 +62,7 @@ const StyledHtmlTooltip = styled(
 export interface IHtmlTooltipProps extends TooltipProps {
     maxWidth?: SpacingArgument;
     maxHeight?: SpacingArgument;
+    fontSize?: string;
 }
 
 export const HtmlTooltip = (props: IHtmlTooltipProps) => (

diff --git a/...atureOverviewSidePanelEnvironmentSwitches/FeatureOverviewSidePanelEnvironmentSwitches.tsx b/...atureOverviewSidePanelEnvironmentSwitches/FeatureOverviewSidePanelEnvironmentSwitches.tsx
@@ -4,6 +4,8 @@ import { useState } from 'react';
 import { FeatureOverviewSidePanelEnvironmentSwitch } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewSidePanel/FeatureOverviewSidePanelEnvironmentSwitches/FeatureOverviewSidePanelEnvironmentSwitch/FeatureOverviewSidePanelEnvironmentSwitch';
 import { Link, styled, Tooltip } from '@mui/material';
 import { Link as RouterLink } from 'react-router-dom';
+import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
+import VariantsWarningTooltip from 'component/feature/FeatureView/FeatureVariants/VariantsTooltipWarning';
 
 const StyledContainer = styled('div')(({ theme }) => ({
     padding: theme.spacing(3),
@@ -21,6 +23,15 @@ const StyledLabel = styled('p')(({ theme }) => ({
 const StyledSubLabel = styled('p')(({ theme }) => ({
     fontSize: theme.fontSizes.smallBody,
     color: theme.palette.text.secondary,
+    display: 'flex',
+    alignItems: 'center',
+}));
+
+const StyledSeparator = styled('span')(({ theme }) => ({
+    padding: theme.spacing(0, 0.5),
+    '::after': {
+        content: '"-"',
+    },
 }));
 
 const StyledLink = styled(Link<typeof RouterLink | 'a'>)(() => ({
@@ -44,7 +55,9 @@ export const FeatureOverviewSidePanelEnvironmentSwitches = ({
 }: IFeatureOverviewSidePanelEnvironmentSwitchesProps) => {
     const [showInfoBox, setShowInfoBox] = useState(false);
     const [environmentName, setEnvironmentName] = useState('');
-
+    const someEnabledEnvironmentHasVariants = feature.environments.some(
+        environment => environment.enabled && environment.variants?.length
+    );
     return (
         <StyledContainer>
             {header}
@@ -58,7 +71,7 @@ export const FeatureOverviewSidePanelEnvironmentSwitches = ({
 
                 const variantsLink = variants.length > 0 && (
                     <>
-                        {' - '}
+                        <StyledSeparator />
                         <Tooltip title="View variants" arrow describeChild>
                             <StyledLink
                                 component={RouterLink}
@@ -73,6 +86,10 @@ export const FeatureOverviewSidePanelEnvironmentSwitches = ({
                     </>
                 );
 
+                const hasWarning =
+                    environment.enabled &&
+                    variants.length == 0 &&
+                    someEnabledEnvironmentHasVariants;
                 return (
                     <FeatureOverviewSidePanelEnvironmentSwitch
                         key={environment.name}
@@ -89,6 +106,15 @@ export const FeatureOverviewSidePanelEnvironmentSwitches = ({
                             <StyledSubLabel>
                                 {strategiesLabel}
                                 {variantsLink}
+                                <ConditionallyRender
+                                    condition={hasWarning}
+                                    show={
+                                        <>
+                                            <StyledSeparator />
+                                            <VariantsWarningTooltip />
+                                        </>
+                                    }
+                                />
                             </StyledSubLabel>
                         </StyledSwitchLabel>
                     </FeatureOverviewSidePanelEnvironmentSwitch>

diff --git a/frontend/src/component/feature/FeatureView/FeatureVariants/VariantsTooltipWarning.tsx b/frontend/src/component/feature/FeatureView/FeatureVariants/VariantsTooltipWarning.tsx
@@ -0,0 +1,33 @@
+import { HtmlTooltip } from 'component/common/HtmlTooltip/HtmlTooltip';
+import { WarningAmber } from '@mui/icons-material';
+import { styled } from '@mui/material';
+
+const StyledWarningAmber = styled(WarningAmber)(({ theme }) => ({
+    color: theme.palette.warning.main,
+    fontSize: theme.fontSizes.bodySize,
+}));
+
+const VariantsWarningTooltip = () => {
+    return (
+        <HtmlTooltip
+            arrow
+            title={
+                <>
+                    This environment has no variants enabled. If you check this
+                    feature's variants in this environment, you will get the{' '}
+                    <a
+                        href="https://docs.getunleash.io/reference/feature-toggle-variants#the-disabled-variant"
+                        target="_blank"
+                    >
+                        disabled variant
+                    </a>
+                    .
+                </>
+            }
+        >
+            <StyledWarningAmber />
+        </HtmlTooltip>
+    );
+};
+
+export default VariantsWarningTooltip;
diff --git a/frontend/src/component/project/Project/ProjectFeatureToggles/ProjectFeatureToggles.tsx b/frontend/src/component/project/Project/ProjectFeatureToggles/ProjectFeatureToggles.tsx
@@ -45,11 +45,28 @@ import { useFavoriteFeaturesApi } from 'hooks/api/actions/useFavoriteFeaturesApi
 import { FeatureTagCell } from 'component/common/Table/cells/FeatureTagCell/FeatureTagCell';
 import { useGlobalLocalStorage } from 'hooks/useGlobalLocalStorage';
 import { useConditionallyHiddenColumns } from 'hooks/useConditionallyHiddenColumns';
+import { flexRow } from 'themes/themeStyles';
+import VariantsWarningTooltip from 'component/feature/FeatureView/FeatureVariants/VariantsTooltipWarning';
 
 const StyledResponsiveButton = styled(ResponsiveButton)(() => ({
     whiteSpace: 'nowrap',
 }));
 
+const StyledSwitchContainer = styled('div', {
+    shouldForwardProp: prop => prop !== 'hasWarning',
+})<{ hasWarning?: boolean }>(({ theme, hasWarning }) => ({
+    flexGrow: 0,
+    ...flexRow,
+    justifyContent: 'center',
+    ...(hasWarning && {
+        '::before': {
+            content: '""',
+            display: 'block',
+            width: theme.spacing(2),
+        },
+    }),
+}));
+
 interface IProjectFeatureTogglesProps {
     features: IProject['features'];
     environments: IProject['environments'];
@@ -64,8 +81,10 @@ type ListItemType = Pick<
         [key in string]: {
             name: string;
             enabled: boolean;
+            variantCount: number;
         };
     };
+    someEnabledEnvironmentHasVariants: boolean;
 };
 
 const staticColumns = ['Actions', 'name', 'favorite'];
@@ -273,15 +292,28 @@ export const ProjectFeatureToggles = ({
                 }: {
                     value: boolean;
                     row: { original: ListItemType };
-                }) => (
-                    <FeatureToggleSwitch
-                        value={value}
-                        projectId={projectId}
-                        featureName={feature?.name}
-                        environmentName={name}
-                        onToggle={onToggle}
-                    />
-                ),
+                }) => {
+                    const hasWarning =
+                        feature.someEnabledEnvironmentHasVariants &&
+                        feature.environments[name].variantCount === 0 &&
+                        feature.environments[name].enabled;
+
+                    return (
+                        <StyledSwitchContainer hasWarning={hasWarning}>
+                            <FeatureToggleSwitch
+                                value={value}
+                                projectId={projectId}
+                                featureName={feature?.name}
+                                environmentName={name}
+                                onToggle={onToggle}
+                            />
+                            <ConditionallyRender
+                                condition={hasWarning}
+                                show={<VariantsWarningTooltip />}
+                            />
+                        </StyledSwitchContainer>
+                    );
+                },
                 sortType: 'boolean',
                 filterName: name,
                 filterParsing: (value: boolean) =>
@@ -311,38 +343,31 @@ export const ProjectFeatureToggles = ({
 
     const featuresData = useMemo(
         () =>
-            features.map(
-                ({
-                    name,
-                    lastSeenAt,
-                    createdAt,
-                    type,
-                    stale,
-                    tags,
-                    favorite,
-                    environments: featureEnvironments,
-                }) => ({
-                    name,
-                    lastSeenAt,
-                    createdAt,
-                    type,
-                    stale,
-                    tags,
-                    favorite,
-                    environments: Object.fromEntries(
-                        environments.map(env => [
+            features.map(feature => ({
+                ...feature,
+                environments: Object.fromEntries(
+                    environments.map(env => {
+                        const thisEnv = feature?.environments.find(
+                            featureEnvironment =>
+                                featureEnvironment?.name === env
+                        );
+                        return [
                             env,
                             {
                                 name: env,
-                                enabled:
-                                    featureEnvironments?.find(
-                                        feature => feature?.name === env
-                                    )?.enabled || false,
+                                enabled: thisEnv?.enabled || false,
+                                variantCount: thisEnv?.variantCount || 0,
                             },
-                        ])
-                    ),
-                })
-            ),
+                        ];
+                    })
+                ),
+                someEnabledEnvironmentHasVariants:
+                    feature.environments?.some(
+                        featureEnvironment =>
+                            featureEnvironment.variantCount > 0 &&
+                            featureEnvironment.enabled
+                    ) || false,
+            })),
         [features, environments]
     );
 

diff --git a/frontend/src/interfaces/featureToggle.ts b/frontend/src/interfaces/featureToggle.ts
@@ -15,6 +15,7 @@ export interface IFeatureToggleListItem {
 export interface IEnvironments {
     name: string;
     enabled: boolean;
+    variantCount: number;
 }
 
 export interface IFeatureToggle {

diff --git a/src/lib/db/feature-strategy-store.ts b/src/lib/db/feature-strategy-store.ts
@@ -392,6 +392,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
             enabled: r.enabled,
             type: r.environment_type,
             sortOrder: r.environment_sort_order,
+            variantCount: r.variants?.length || 0,
         };
     }
 
@@ -469,6 +470,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
             'features.stale as stale',
             'feature_environments.enabled as enabled',
             'feature_environments.environment as environment',
+            'feature_environments.variants as variants',
             'environments.type as environment_type',
             'environments.sort_order as environment_sort_order',
             'ft.tag_value as tag_value',

diff --git a/src/lib/openapi/spec/feature-environment-schema.ts b/src/lib/openapi/spec/feature-environment-schema.ts
@@ -27,6 +27,9 @@ export const featureEnvironmentSchema = {
         sortOrder: {
             type: 'number',
         },
+        variantCount: {
+            type: 'number',
+        },
         strategies: {
             type: 'array',
             items: {

diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts
@@ -91,7 +91,7 @@ export interface FeatureToggleLegacy extends FeatureToggle {
     enabled: boolean;
 }
 
-export interface IEnvironmentDetail extends IEnvironmentOverview {
+export interface IEnvironmentDetail extends IEnvironmentBase {
     strategies: IStrategyConfig[];
     variants: IVariant[];
 }
@@ -152,13 +152,17 @@ export interface IEnvironmentClone {
     clonePermissions?: boolean;
 }
 
-export interface IEnvironmentOverview {
+export interface IEnvironmentBase {
     name: string;
     enabled: boolean;
     type: string;
     sortOrder: number;
 }
 
+export interface IEnvironmentOverview extends IEnvironmentBase {
+    variantCount: number;
+}
+
 export interface IFeatureOverview {
     name: string;
     type: string;

diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap
@@ -1161,6 +1161,9 @@ exports[`should serve the OpenAPI spec 1`] = `
           "type": {
             "type": "string",
           },
+          "variantCount": {
+            "type": "number",
+          },
         },
         "required": [
           "name",

diff --git a/src/test/e2e/services/environment-service.test.ts b/src/test/e2e/services/environment-service.test.ts
@@ -61,6 +61,7 @@ test('Can connect environment to project', async () => {
                 enabled: false,
                 sortOrder: 9999,
                 type: 'production',
+                variantCount: 0,
             },
         ]);
     });
@@ -87,6 +88,7 @@ test('Can remove environment from project', async () => {
                 enabled: false,
                 sortOrder: 9999,
                 type: 'production',
+                variantCount: 0,
             },
         ]);
     });

diff --git a/src/test/fixtures/fake-feature-environment-store.ts b/src/test/fixtures/fake-feature-environment-store.ts
@@ -39,7 +39,7 @@ export default class FakeFeatureEnvironmentStore
             .filter(
                 (fe) =>
                     fe.featureName === featureName &&
-                    environments.indexOf(fe.environment) !== -1,
+                    environments.includes(fe.environment),
             )
             .map((fe) => (fe.variants = variants));
     }