fix: remove empty variants when changing tabs (#5850)

This PR makes a change to how variants work in the new setup. Variants will now: * Be removed if you change tab or unmount the component and it has no name * Moved StrategyVariants into a separate component to isolate this change * Add error handling around onSubmit and only trigger feedback if it's successful
2024-12-22 19:07:54 +01:00 · 2024-01-11 10:43:29 +01:00 · 2024-01-11 10:43:29 +01:00 · 2a723ea9e8
commit 2a723ea9e8
parent 3a2d4ae60b
7 changed files with 251 additions and 116 deletions
--- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx
+++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx
@ -25,7 +25,7 @@ import { useSegments } from 'hooks/api/getters/useSegments/useSegments';
 import { useUiFlag } from 'hooks/useUiFlag';
 import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
 import { NewFeatureStrategyForm } from 'component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm';
-import { StrategyVariants } from 'component/feature/StrategyTypes/StrategyVariants';
+import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants';

 interface IEditChangeProps {
    change: IChangeRequestAddStrategy | IChangeRequestUpdateStrategy;
@ -174,7 +174,7 @@ export const NewEditChange = ({
                            tab={tab}
                            setTab={setTab}
                            StrategyVariants={
-                                <StrategyVariants
+                                <NewStrategyVariants
                                    strategy={strategy}
                                    setStrategy={setStrategy}
                                    environment={environment}
--- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx
@ -47,6 +47,7 @@ import EnvironmentIcon from 'component/common/EnvironmentIcon/EnvironmentIcon';
 import { useFeedback } from 'component/feedbackNew/useFeedback';
 import { useUserSubmittedFeedback } from 'hooks/useSubmittedFeedback';
 import { useUiFlag } from 'hooks/useUiFlag';
+import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants';

 interface IFeatureStrategyFormProps {
    feature: IFeatureToggle;
@ -330,10 +331,14 @@ export const NewFeatureStrategyForm = ({
    };

    const onSubmitWithFeedback = async () => {
-        await onSubmit();
+        try {
+            await onSubmit();

-        if (newStrategyConfigurationFeedback && !hasSubmittedFeedback) {
-            createFeedbackContext();
+            if (newStrategyConfigurationFeedback && !hasSubmittedFeedback) {
+                createFeedbackContext();
+            }
+        } catch (e) {
+            console.error(e);
        }
    };

--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
@ -199,6 +199,38 @@ describe('NewFeatureStrategyCreate', () => {
        const addVariantEl = await screen.findByText('Add variant');
        fireEvent.click(addVariantEl);

+        const inputElement = screen.getAllByRole('textbox')[0];
+        fireEvent.change(inputElement, {
+            target: { value: expectedVariantName },
+        });
+
+        const targetingEl = await screen.findByText('Targeting');
+        fireEvent.click(targetingEl);
+
+        const addConstraintEl = await screen.findByText('Add constraint');
+        expect(addConstraintEl).toBeInTheDocument();
+
+        fireEvent.click(variantsEl);
+        const inputElement2 = screen.getAllByRole('textbox')[0];
+
+        expect(inputElement2).not.toBeDisabled();
+    });
+
+    test('should remove empty variants when changing tabs', async () => {
+        setupComponent();
+
+        const titleEl = await screen.findByText('Gradual rollout');
+        expect(titleEl).toBeInTheDocument();
+
+        const variantsEl = screen.getByText('Variants');
+        fireEvent.click(variantsEl);
+
+        const addVariantEl = await screen.findByText('Add variant');
+        fireEvent.click(addVariantEl);
+
+        const variants = screen.queryAllByTestId('VARIANT');
+        expect(variants.length).toBe(1);
+
        const targetingEl = await screen.findByText('Targeting');
        fireEvent.click(targetingEl);

@ -207,12 +239,8 @@ describe('NewFeatureStrategyCreate', () => {

        fireEvent.click(variantsEl);

-        const inputElement = screen.getAllByRole('textbox')[0];
-        fireEvent.change(inputElement, {
-            target: { value: expectedVariantName },
-        });
-
-        expect(screen.getByText(expectedVariantName)).toBeInTheDocument();
+        const variants2 = screen.queryAllByTestId('VARIANT');
+        expect(variants2.length).toBe(0);
    });

    test('Should autosave constraint settings when navigating between tabs', async () => {
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx
@ -34,7 +34,7 @@ import useQueryParams from 'hooks/useQueryParams';
 import { useSegments } from 'hooks/api/getters/useSegments/useSegments';
 import { useDefaultStrategy } from '../../../project/Project/ProjectSettings/ProjectDefaultStrategySettings/ProjectEnvironment/ProjectEnvironmentDefaultStrategy/EditDefaultStrategy';
 import { NewFeatureStrategyForm } from 'component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm';
-import { StrategyVariants } from 'component/feature/StrategyTypes/StrategyVariants';
+import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants';

 export const NewFeatureStrategyCreate = () => {
    const [tab, setTab] = useState(0);
@ -211,7 +211,7 @@ export const NewFeatureStrategyCreate = () => {
                tab={tab}
                setTab={setTab}
                StrategyVariants={
-                    <StrategyVariants
+                    <NewStrategyVariants
                        strategy={strategy}
                        setStrategy={setStrategy}
                        environment={environmentId}
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx
@ -29,7 +29,7 @@ import { useChangeRequestApi } from 'hooks/api/actions/useChangeRequestApi/useCh
 import { usePendingChangeRequests } from 'hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests';
 import { usePlausibleTracker } from 'hooks/usePlausibleTracker';
 import { NewFeatureStrategyForm } from 'component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm';
-import { StrategyVariants } from 'component/feature/StrategyTypes/StrategyVariants';
+import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants';

 const useTitleTracking = () => {
    const [previousTitle, setPreviousTitle] = useState<string>('');
@ -233,7 +233,7 @@ export const NewFeatureStrategyEdit = () => {
                tab={tab}
                setTab={setTab}
                StrategyVariants={
-                    <StrategyVariants
+                    <NewStrategyVariants
                        strategy={strategy}
                        setStrategy={setStrategy}
                        environment={environmentId}
--- a/frontend/src/component/feature/StrategyTypes/NewStrategyVariants.tsx
+++ b/frontend/src/component/feature/StrategyTypes/NewStrategyVariants.tsx
@ -0,0 +1,202 @@
+import { VariantForm } from '../FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsModal/VariantForm/VariantForm';
+import { updateWeightEdit } from '../../common/util';
+import React, { FC, useEffect, useState } from 'react';
+import { IFeatureVariantEdit } from '../FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsModal/EnvironmentVariantsModal';
+import PermissionButton from '../../common/PermissionButton/PermissionButton';
+import { UPDATE_FEATURE_ENVIRONMENT_VARIANTS } from '../../providers/AccessProvider/permissions';
+import { v4 as uuidv4 } from 'uuid';
+import { WeightType } from '../../../constants/variantTypes';
+import { Box, styled, Typography, useTheme } from '@mui/material';
+import { IFeatureStrategy } from 'interfaces/strategy';
+import SplitPreviewSlider from './SplitPreviewSlider/SplitPreviewSlider';
+import { HelpIcon } from '../../common/HelpIcon/HelpIcon';
+import { StrategyVariantsUpgradeAlert } from '../../common/StrategyVariantsUpgradeAlert/StrategyVariantsUpgradeAlert';
+import { usePlausibleTracker } from 'hooks/usePlausibleTracker';
+import { useUiFlag } from 'hooks/useUiFlag';
+import { Add } from '@mui/icons-material';
+import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
+
+const StyledVariantForms = styled('div')({
+    display: 'flex',
+    flexDirection: 'column',
+});
+
+const StyledHelpIconBox = styled(Box)(({ theme }) => ({
+    display: 'flex',
+    alignItems: 'center',
+    marginTop: theme.spacing(1),
+    marginBottom: theme.spacing(1),
+}));
+
+const StyledVariantsHeader = styled('div')(({ theme }) => ({
+    color: theme.palette.text.secondary,
+    marginTop: theme.spacing(1.5),
+}));
+
+export const NewStrategyVariants: FC<{
+    setStrategy: React.Dispatch<
+        React.SetStateAction<Partial<IFeatureStrategy>>
+    >;
+    strategy: Partial<IFeatureStrategy>;
+    projectId: string;
+    environment: string;
+    editable?: boolean;
+}> = ({ strategy, setStrategy, projectId, environment, editable }) => {
+    const { trackEvent } = usePlausibleTracker();
+    const [variantsEdit, setVariantsEdit] = useState<IFeatureVariantEdit[]>([]);
+    const theme = useTheme();
+
+    const stickiness =
+        strategy?.parameters && 'stickiness' in strategy?.parameters
+            ? String(strategy.parameters.stickiness)
+            : 'default';
+
+    useEffect(() => {
+        return () => {
+            setStrategy((prev) => ({
+                ...prev,
+                variants: variantsEdit.filter((variant) =>
+                    Boolean(variant.name),
+                ),
+            }));
+        };
+    }, [JSON.stringify(variantsEdit)]);
+
+    useEffect(() => {
+        setVariantsEdit(
+            (strategy.variants || []).map((variant) => ({
+                ...variant,
+                new: editable || false,
+                isValid: true,
+                id: uuidv4(),
+                overrides: [],
+            })),
+        );
+    }, []);
+
+    useEffect(() => {
+        setStrategy((prev) => ({
+            ...prev,
+            variants: variantsEdit.map((variant) => ({
+                stickiness,
+                name: variant.name,
+                weight: variant.weight,
+                payload: variant.payload,
+                weightType: variant.weightType,
+            })),
+        }));
+    }, [stickiness, JSON.stringify(variantsEdit)]);
+
+    const updateVariant = (updatedVariant: IFeatureVariantEdit, id: string) => {
+        setVariantsEdit((prevVariants) =>
+            updateWeightEdit(
+                prevVariants.map((prevVariant) =>
+                    prevVariant.id === id ? updatedVariant : prevVariant,
+                ),
+                1000,
+            ),
+        );
+    };
+
+    const addVariant = () => {
+        const id = uuidv4();
+        setVariantsEdit((variantsEdit) => [
+            ...variantsEdit,
+            {
+                name: '',
+                weightType: WeightType.VARIABLE,
+                weight: 0,
+                stickiness,
+                new: true,
+                isValid: false,
+                id,
+            },
+        ]);
+        trackEvent('strategy-variants', {
+            props: {
+                eventType: 'variant added',
+            },
+        });
+    };
+
+    return (
+        <>
+            <StyledVariantsHeader>
+                Variants enhance a feature flag by providing a version of the
+                feature to be enabled
+            </StyledVariantsHeader>
+            <StyledHelpIconBox>
+                <Typography>Variants</Typography>
+                <HelpIcon
+                    htmlTooltip
+                    tooltip={
+                        <Box>
+                            <Typography variant='body2'>
+                                Variants in feature toggling allow you to serve
+                                different versions of a feature to different
+                                users. This can be used for A/B testing, gradual
+                                rollouts, and canary releases. Variants provide
+                                a way to control the user experience at a
+                                granular level, enabling you to test and
+                                optimize different aspects of your features.
+                                Read more about variants{' '}
+                                <a
+                                    href='https://docs.getunleash.io/reference/strategy-variants'
+                                    target='_blank'
+                                    rel='noopener noreferrer'
+                                >
+                                    here
+                                </a>
+                            </Typography>
+                        </Box>
+                    }
+                />
+            </StyledHelpIconBox>
+            <StyledVariantForms>
+                <ConditionallyRender
+                    condition={variantsEdit.length > 0}
+                    show={<StrategyVariantsUpgradeAlert />}
+                />
+
+                {variantsEdit.map((variant, i) => (
+                    <VariantForm
+                        disableOverrides={true}
+                        key={variant.id}
+                        variant={variant}
+                        variants={variantsEdit}
+                        updateVariant={(updatedVariant) =>
+                            updateVariant(updatedVariant, variant.id)
+                        }
+                        removeVariant={() =>
+                            setVariantsEdit((variantsEdit) =>
+                                updateWeightEdit(
+                                    variantsEdit.filter(
+                                        (v) => v.id !== variant.id,
+                                    ),
+                                    1000,
+                                ),
+                            )
+                        }
+                        decorationColor={
+                            theme.palette.variants[
+                                i % theme.palette.variants.length
+                            ]
+                        }
+                    />
+                ))}
+            </StyledVariantForms>
+            <PermissionButton
+                onClick={addVariant}
+                variant='outlined'
+                permission={UPDATE_FEATURE_ENVIRONMENT_VARIANTS}
+                projectId={projectId}
+                environmentId={environment}
+                data-testid='ADD_STRATEGY_VARIANT_BUTTON'
+                startIcon={<Add />}
+            >
+                Add variant
+            </PermissionButton>
+            <SplitPreviewSlider variants={variantsEdit} />
+        </>
+    );
+};
--- a/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx
+++ b/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx
@ -6,33 +6,18 @@ import PermissionButton from '../../common/PermissionButton/PermissionButton';
 import { UPDATE_FEATURE_ENVIRONMENT_VARIANTS } from '../../providers/AccessProvider/permissions';
 import { v4 as uuidv4 } from 'uuid';
 import { WeightType } from '../../../constants/variantTypes';
-import { Box, Link, styled, Typography, useTheme } from '@mui/material';
+import { Link, styled, Typography, useTheme } from '@mui/material';
 import { IFeatureStrategy } from 'interfaces/strategy';
 import SplitPreviewSlider from './SplitPreviewSlider/SplitPreviewSlider';
 import { HelpIcon } from '../../common/HelpIcon/HelpIcon';
 import { StrategyVariantsUpgradeAlert } from '../../common/StrategyVariantsUpgradeAlert/StrategyVariantsUpgradeAlert';
 import { usePlausibleTracker } from 'hooks/usePlausibleTracker';
-import { useUiFlag } from 'hooks/useUiFlag';
-import { Add } from '@mui/icons-material';
-import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';

 const StyledVariantForms = styled('div')({
    display: 'flex',
    flexDirection: 'column',
 });

-const StyledHelpIconBox = styled(Box)(({ theme }) => ({
-    display: 'flex',
-    alignItems: 'center',
-    marginTop: theme.spacing(1),
-    marginBottom: theme.spacing(1),
-}));
-
-const StyledVariantsHeader = styled('div')(({ theme }) => ({
-    color: theme.palette.text.secondary,
-    marginTop: theme.spacing(1.5),
-}));
-
 export const StrategyVariants: FC<{
    setStrategy: React.Dispatch<
        React.SetStateAction<Partial<IFeatureStrategy>>
@ -45,7 +30,6 @@ export const StrategyVariants: FC<{
    const { trackEvent } = usePlausibleTracker();
    const [variantsEdit, setVariantsEdit] = useState<IFeatureVariantEdit[]>([]);
    const theme = useTheme();
-    const newStrategyConfiguration = useUiFlag('newStrategyConfiguration');

    const stickiness =
        strategy?.parameters && 'stickiness' in strategy?.parameters
@ -109,90 +93,6 @@ export const StrategyVariants: FC<{
        });
    };

-    if (newStrategyConfiguration) {
-        return (
-            <>
-                <StyledVariantsHeader>
-                    Variants enhance a feature flag by providing a version of
-                    the feature to be enabled
-                </StyledVariantsHeader>
-                <StyledHelpIconBox>
-                    <Typography>Variants</Typography>
-                    <HelpIcon
-                        htmlTooltip
-                        tooltip={
-                            <Box>
-                                <Typography variant='body2'>
-                                    Variants in feature toggling allow you to
-                                    serve different versions of a feature to
-                                    different users. This can be used for A/B
-                                    testing, gradual rollouts, and canary
-                                    releases. Variants provide a way to control
-                                    the user experience at a granular level,
-                                    enabling you to test and optimize different
-                                    aspects of your features. Read more about
-                                    variants{' '}
-                                    <a
-                                        href='https://docs.getunleash.io/reference/strategy-variants'
-                                        target='_blank'
-                                        rel='noopener noreferrer'
-                                    >
-                                        here
-                                    </a>
-                                </Typography>
-                            </Box>
-                        }
-                    />
-                </StyledHelpIconBox>
-                <StyledVariantForms>
-                    <ConditionallyRender
-                        condition={variantsEdit.length > 0}
-                        show={<StrategyVariantsUpgradeAlert />}
-                    />
-
-                    {variantsEdit.map((variant, i) => (
-                        <VariantForm
-                            disableOverrides={true}
-                            key={variant.id}
-                            variant={variant}
-                            variants={variantsEdit}
-                            updateVariant={(updatedVariant) =>
-                                updateVariant(updatedVariant, variant.id)
-                            }
-                            removeVariant={() =>
-                                setVariantsEdit((variantsEdit) =>
-                                    updateWeightEdit(
-                                        variantsEdit.filter(
-                                            (v) => v.id !== variant.id,
-                                        ),
-                                        1000,
-                                    ),
-                                )
-                            }
-                            decorationColor={
-                                theme.palette.variants[
-                                    i % theme.palette.variants.length
-                                ]
-                            }
-                        />
-                    ))}
-                </StyledVariantForms>
-                <PermissionButton
-                    onClick={addVariant}
-                    variant='outlined'
-                    permission={UPDATE_FEATURE_ENVIRONMENT_VARIANTS}
-                    projectId={projectId}
-                    environmentId={environment}
-                    data-testid='ADD_STRATEGY_VARIANT_BUTTON'
-                    startIcon={<Add />}
-                >
-                    Add variant
-                </PermissionButton>
-                <SplitPreviewSlider variants={variantsEdit} />
-            </>
-        );
-    }
-
    return (
        <>
            <Typography