feat: variant name change on create (#5742)

This PR refactores the StrategyVariants component to be passed in from the outside to the new form component. This allows us to pass in the StrategyVariants with an "editable" property in the create form which we use to determine the editable state of the name input field. If the editable field is not passed in we keep the old behavior. Notable changes: * StrategyVariants is now passed in from the outside, allowing us to define different props at call time * Added tests for the new behavior, and for keeping the old behavior (such as in edit strategy) * Added tracking
2024-12-22 19:07:54 +01:00 · 2024-01-02 13:53:04 +01:00 · 2024-01-02 13:53:04 +01:00 · 049c5b9afa
commit 049c5b9afa
parent f53204c9b2
11 changed files with 342 additions and 133 deletions
--- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EditChange.tsx
+++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EditChange.tsx
@ -25,6 +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';

 interface IEditChangeProps {
    change: IChangeRequestAddStrategy | IChangeRequestUpdateStrategy;
@ -172,6 +173,14 @@ export const EditChange = ({
                            )}
                            tab={tab}
                            setTab={setTab}
+                            StrategyVariants={
+                                <StrategyVariants
+                                    strategy={strategy}
+                                    setStrategy={setStrategy}
+                                    environment={environment}
+                                    projectId={projectId}
+                                />
+                            }
                        />
                    }
                    elseShow={
--- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx
+++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx
@ -25,6 +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';

 interface IEditChangeProps {
    change: IChangeRequestAddStrategy | IChangeRequestUpdateStrategy;
@ -172,6 +173,14 @@ export const NewEditChange = ({
                            )}
                            tab={tab}
                            setTab={setTab}
+                            StrategyVariants={
+                                <StrategyVariants
+                                    strategy={strategy}
+                                    setStrategy={setStrategy}
+                                    environment={environment}
+                                    projectId={projectId}
+                                />
+                            }
                        />
                    }
                    elseShow={
--- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx
+++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx
@ -228,7 +228,6 @@ export const ConstraintList = forwardRef<
        <StyledContainer id={constraintAccordionListId}>
            {constraints.map((constraint, index) => (
                <Fragment key={objectId(constraint)}>
-                    {console.log(state.get(constraint))}
                    <ConditionallyRender
                        condition={index > 0}
                        show={<StrategySeparator text='AND' />}
--- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx
@ -1,4 +1,4 @@
-import React, { useState } from 'react';
+import React, { useEffect, useState } from 'react';
 import { useNavigate } from 'react-router-dom';
 import {
    Alert,
@ -40,13 +40,10 @@ import { usePendingChangeRequests } from 'hooks/api/getters/usePendingChangeRequ
 import { useHasProjectEnvironmentAccess } from 'hooks/useHasAccess';
 import { FeatureStrategyTitle } from './FeatureStrategyTitle/FeatureStrategyTitle';
 import { FeatureStrategyEnabledDisabled } from './FeatureStrategyEnabledDisabled/FeatureStrategyEnabledDisabled';
-import { StrategyVariants } from 'component/feature/StrategyTypes/StrategyVariants';
 import { usePlausibleTracker } from 'hooks/usePlausibleTracker';
 import { formatStrategyName } from 'utils/strategyNames';
 import { Badge } from 'component/common/Badge/Badge';
 import EnvironmentIcon from 'component/common/EnvironmentIcon/EnvironmentIcon';
-import { useProjectEnvironments } from 'hooks/api/getters/useProjectEnvironments/useProjectEnvironments';
-import { useFeature } from 'hooks/api/getters/useFeature/useFeature';

 interface IFeatureStrategyFormProps {
    feature: IFeatureToggle;
@ -66,6 +63,7 @@ interface IFeatureStrategyFormProps {
    errors: IFormErrors;
    tab: number;
    setTab: React.Dispatch<React.SetStateAction<number>>;
+    StrategyVariants: JSX.Element;
 }

 const StyledDividerContent = styled(Box)(({ theme }) => ({
@ -185,6 +183,7 @@ export const NewFeatureStrategyForm = ({
    isChangeRequest,
    tab,
    setTab,
+    StrategyVariants,
 }: IFeatureStrategyFormProps) => {
    const { trackEvent } = usePlausibleTracker();
    const [showProdGuard, setShowProdGuard] = useState(false);
@ -197,6 +196,14 @@ export const NewFeatureStrategyForm = ({
    );
    const { strategyDefinition } = useStrategy(strategy?.name);

+    useEffect(() => {
+        trackEvent('new-strategy-form', {
+            props: {
+                eventType: 'seen',
+            },
+        });
+    });
+
    const foundEnvironment = feature.environments.find(
        (environment) => environment.name === environmentId,
    );
@ -271,6 +278,12 @@ export const NewFeatureStrategyForm = ({
            return;
        }

+        trackEvent('new-strategy-form', {
+            props: {
+                eventType: 'submitted',
+            },
+        });
+
        if (enableProdGuard && !isChangeRequest) {
            setShowProdGuard(true);
        } else {
@ -440,14 +453,7 @@ export const NewFeatureStrategyForm = ({
                                strategy.parameters != null &&
                                'stickiness' in strategy.parameters
                            }
-                            show={
-                                <StrategyVariants
-                                    strategy={strategy}
-                                    setStrategy={setStrategy}
-                                    environment={environmentId}
-                                    projectId={projectId}
-                                />
-                            }
+                            show={StrategyVariants}
                        />
                    }
                />
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx
@ -9,92 +9,17 @@ import {
    UPDATE_FEATURE_STRATEGY,
 } from 'component/providers/AccessProvider/permissions';
 import { NewFeatureStrategyCreate } from './NewFeatureStrategyCreate';
-import { testServerRoute, testServerSetup } from 'utils/testServer';
-
-const server = testServerSetup();
+import {
+    setupProjectEndpoint,
+    setupSegmentsEndpoint,
+    setupStrategyEndpoint,
+    setupFeaturesEndpoint,
+    setupUiConfigEndpoint,
+    setupContextEndpoint,
+} from './featureStrategyFormTestSetup';

 const featureName = 'my-new-feature';

-const setupProjectEndpoint = () => {
-    testServerRoute(server, '/api/admin/projects/default', {
-        environments: [
-            {
-                name: 'development',
-                enabled: true,
-                type: 'development',
-            },
-        ],
-    });
-};
-
-const setupSegmentsEndpoint = () => {
-    testServerRoute(server, '/api/admin/segments', {
-        segments: [
-            {
-                name: 'test',
-                constraints: [],
-            },
-        ],
-    });
-};
-
-const setupStrategyEndpoint = () => {
-    testServerRoute(server, '/api/admin/strategies/flexibleRollout', {
-        displayName: 'Gradual rollout',
-        name: 'flexibleRollout',
-        parameters: [
-            {
-                name: 'rollout',
-            },
-            {
-                name: 'stickiness',
-            },
-            {
-                name: 'groupId',
-            },
-        ],
-    });
-};
-
-const setupFeaturesEndpoint = () => {
-    testServerRoute(
-        server,
-        `/api/admin/projects/default/features/${featureName}`,
-        {
-            environments: [
-                {
-                    name: 'development',
-                    type: 'development',
-                },
-            ],
-            name: featureName,
-            project: 'default',
-        },
-    );
-};
-
-const setupUiConfigEndpoint = () => {
-    testServerRoute(server, '/api/admin/ui-config', {
-        versionInfo: {
-            current: {
-                enterprise: '5.7.0-main',
-            },
-        },
-        environment: 'enterprise',
-        flags: {
-            newStrategyConfiguration: true,
-        },
-    });
-};
-
-const setupContextEndpoint = () => {
-    testServerRoute(server, '/api/admin/context', [
-        {
-            name: 'appName',
-        },
-    ]);
-};
-
 const setupComponent = () => {
    return {
        wrapper: render(
@ -139,7 +64,7 @@ beforeEach(() => {
    setupProjectEndpoint();
    setupSegmentsEndpoint();
    setupStrategyEndpoint();
-    setupFeaturesEndpoint();
+    setupFeaturesEndpoint(featureName);
    setupUiConfigEndpoint();
    setupContextEndpoint();
 });
@ -270,4 +195,39 @@ describe('NewFeatureStrategyCreate', () => {

        expect(screen.getByText(expectedVariantName)).toBeInTheDocument();
    });
+
+    test('should change variant name after changing tab', async () => {
+        const { expectedVariantName } = setupComponent();
+
+        await waitFor(() => {
+            expect(screen.getByText('Gradual rollout')).toBeInTheDocument();
+        });
+
+        const variantsEl = screen.getByText('Variants');
+        fireEvent.click(variantsEl);
+
+        await waitFor(() => {
+            const addVariantEl = screen.getByText('Add variant');
+            fireEvent.click(addVariantEl);
+        });
+
+        await waitFor(() => {
+            const targetingEl = screen.getByText('Targeting');
+            fireEvent.click(targetingEl);
+        });
+
+        await waitFor(() => {
+            const addConstraintEl = screen.getByText('Add constraint');
+            expect(addConstraintEl).toBeInTheDocument();
+        });
+
+        fireEvent.click(variantsEl);
+
+        const inputElement = screen.getAllByRole('textbox')[0];
+        fireEvent.change(inputElement, {
+            target: { value: expectedVariantName },
+        });
+
+        expect(screen.getByText(expectedVariantName)).toBeInTheDocument();
+    });
 });
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx
@ -34,6 +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';

 export const NewFeatureStrategyCreate = () => {
    const [tab, setTab] = useState(0);
@ -209,6 +210,15 @@ export const NewFeatureStrategyCreate = () => {
                isChangeRequest={isChangeRequestConfigured(environmentId)}
                tab={tab}
                setTab={setTab}
+                StrategyVariants={
+                    <StrategyVariants
+                        strategy={strategy}
+                        setStrategy={setStrategy}
+                        environment={environmentId}
+                        projectId={projectId}
+                        editable
+                    />
+                }
            />
            {staleDataNotification}
        </FormTemplate>
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/featureStrategyFormTestSetup.ts
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/featureStrategyFormTestSetup.ts
@ -0,0 +1,99 @@
+import { testServerRoute, testServerSetup } from 'utils/testServer';
+
+const server = testServerSetup();
+
+export const setupProjectEndpoint = () => {
+    testServerRoute(server, '/api/admin/projects/default', {
+        environments: [
+            {
+                name: 'development',
+                enabled: true,
+                type: 'development',
+            },
+        ],
+    });
+};
+
+export const setupSegmentsEndpoint = () => {
+    testServerRoute(server, '/api/admin/segments', {
+        segments: [
+            {
+                name: 'test',
+                constraints: [],
+            },
+        ],
+    });
+};
+
+export const setupStrategyEndpoint = () => {
+    testServerRoute(server, '/api/admin/strategies/flexibleRollout', {
+        displayName: 'Gradual rollout',
+        name: 'flexibleRollout',
+        parameters: [
+            {
+                name: 'rollout',
+            },
+            {
+                name: 'stickiness',
+            },
+            {
+                name: 'groupId',
+            },
+        ],
+    });
+};
+
+export const setupFeaturesEndpoint = (
+    featureName: string,
+    variantName = 'Blue',
+) => {
+    testServerRoute(
+        server,
+        `/api/admin/projects/default/features/${featureName}`,
+        {
+            environments: [
+                {
+                    name: 'development',
+                    type: 'development',
+                    strategies: [
+                        {
+                            id: '1',
+                            constraints: [],
+                            parameters: {
+                                groupId: featureName,
+                                rollout: '50',
+                                stickiness: 'default',
+                            },
+                            name: 'flexibleRollout',
+                            variants: [{ name: variantName, weight: 50 }],
+                        },
+                    ],
+                },
+            ],
+            name: featureName,
+            project: 'default',
+        },
+    );
+};
+
+export const setupUiConfigEndpoint = () => {
+    testServerRoute(server, '/api/admin/ui-config', {
+        versionInfo: {
+            current: {
+                enterprise: '5.7.0-main',
+            },
+        },
+        environment: 'enterprise',
+        flags: {
+            newStrategyConfiguration: true,
+        },
+    });
+};
+
+export const setupContextEndpoint = () => {
+    testServerRoute(server, '/api/admin/context', [
+        {
+            name: 'appName',
+        },
+    ]);
+};
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.test.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.test.tsx
@ -1,42 +1,111 @@
 import { formatUpdateStrategyApiCode } from 'component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit';
 import { IFeatureStrategy, IStrategy } from 'interfaces/strategy';
+import { screen, waitFor, fireEvent } from '@testing-library/react';
+import { render } from 'utils/testRenderer';
+import { Route, Routes } from 'react-router-dom';

-test('formatUpdateStrategyApiCode', () => {
-    const strategy: IFeatureStrategy = {
-        id: 'a',
-        name: 'b',
-        parameters: {
-            c: 1,
-            b: 2,
-            a: 3,
-        },
-        constraints: [],
-    };
+import {
+    CREATE_FEATURE_STRATEGY,
+    UPDATE_FEATURE_ENVIRONMENT_VARIANTS,
+    UPDATE_FEATURE_STRATEGY,
+} from 'component/providers/AccessProvider/permissions';
+import { NewFeatureStrategyEdit } from './NewFeatureStrategyEdit';
+import {
+    setupContextEndpoint,
+    setupFeaturesEndpoint,
+    setupProjectEndpoint,
+    setupSegmentsEndpoint,
+    setupStrategyEndpoint,
+    setupUiConfigEndpoint,
+} from '../NewFeatureStrategyCreate/featureStrategyFormTestSetup';

-    const strategyDefinition: IStrategy = {
-        name: 'c',
-        displayName: 'd',
-        description: 'e',
-        editable: false,
-        deprecated: false,
-        parameters: [
-            { name: 'a', description: '', type: '', required: false },
-            { name: 'b', description: '', type: '', required: false },
-            { name: 'c', description: '', type: '', required: false },
-        ],
-    };
+const featureName = 'my-new-feature';
+const variantName = 'Blue';

-    expect(
-        formatUpdateStrategyApiCode(
-            'projectId',
-            'featureId',
-            'environmentId',
-            'strategyId',
-            strategy,
-            strategyDefinition,
-            'unleashUrl',
+const setupComponent = () => {
+    return {
+        wrapper: render(
+            <Routes>
+                <Route
+                    path={
+                        '/projects/:projectId/features/:featureId/strategies/edit'
+                    }
+                    element={<NewFeatureStrategyEdit />}
+                />
+            </Routes>,
+            {
+                route: `/projects/default/features/${featureName}/strategies/edit?environmentId=development&strategyId=1`,
+                permissions: [
+                    {
+                        permission: CREATE_FEATURE_STRATEGY,
+                        project: 'default',
+                        environment: 'development',
+                    },
+                    {
+                        permission: UPDATE_FEATURE_STRATEGY,
+                        project: 'default',
+                        environment: 'development',
+                    },
+                    {
+                        permission: UPDATE_FEATURE_ENVIRONMENT_VARIANTS,
+                        project: 'default',
+                        environment: 'development',
+                    },
+                ],
+            },
        ),
-    ).toMatchInlineSnapshot(`
+        expectedGroupId: 'newGroupId',
+        expectedVariantName: variantName,
+        expectedSliderValue: '75',
+    };
+};
+
+beforeEach(() => {
+    setupProjectEndpoint();
+    setupSegmentsEndpoint();
+    setupStrategyEndpoint();
+    setupFeaturesEndpoint(featureName, variantName);
+    setupUiConfigEndpoint();
+    setupContextEndpoint();
+});
+
+describe('NewFeatureStrategyEdit', () => {
+    test('formatUpdateStrategyApiCode', () => {
+        const strategy: IFeatureStrategy = {
+            id: 'a',
+            name: 'b',
+            parameters: {
+                c: 1,
+                b: 2,
+                a: 3,
+            },
+            constraints: [],
+        };
+
+        const strategyDefinition: IStrategy = {
+            name: 'c',
+            displayName: 'd',
+            description: 'e',
+            editable: false,
+            deprecated: false,
+            parameters: [
+                { name: 'a', description: '', type: '', required: false },
+                { name: 'b', description: '', type: '', required: false },
+                { name: 'c', description: '', type: '', required: false },
+            ],
+        };
+
+        expect(
+            formatUpdateStrategyApiCode(
+                'projectId',
+                'featureId',
+                'environmentId',
+                'strategyId',
+                strategy,
+                strategyDefinition,
+                'unleashUrl',
+            ),
+        ).toMatchInlineSnapshot(`
      "curl --location --request PUT 'unleashUrl/api/admin/projects/projectId/features/featureId/environments/environmentId/strategies/strategyId' \\
          --header 'Authorization: INSERT_API_KEY' \\
          --header 'Content-Type: application/json' \\
@ -51,4 +120,41 @@ test('formatUpdateStrategyApiCode', () => {
        "constraints": []
      }'"
    `);
+    });
+
+    test('should change general settings', async () => {
+        const { expectedGroupId, expectedSliderValue } = setupComponent();
+
+        await waitFor(() => {
+            expect(screen.getByText('Gradual rollout')).toBeInTheDocument();
+        });
+
+        const slider = await screen.findByRole('slider', { name: /rollout/i });
+        const groupIdInput = await screen.getByLabelText('groupId');
+
+        expect(slider).toHaveValue('50');
+        expect(groupIdInput).toHaveValue(featureName);
+
+        fireEvent.change(slider, { target: { value: expectedSliderValue } });
+        fireEvent.change(groupIdInput, { target: { value: expectedGroupId } });
+
+        expect(slider).toHaveValue(expectedSliderValue);
+        expect(groupIdInput).toHaveValue(expectedGroupId);
+    });
+
+    test('should not change variant names', async () => {
+        const { expectedVariantName } = setupComponent();
+
+        await waitFor(() => {
+            expect(screen.getByText('Gradual rollout')).toBeInTheDocument();
+        });
+
+        const variantsEl = screen.getByText('Variants');
+        fireEvent.click(variantsEl);
+
+        expect(screen.getByText(expectedVariantName)).toBeInTheDocument();
+
+        const inputElement = screen.getAllByRole('textbox')[0];
+        expect(inputElement).toBeDisabled();
+    });
 });
--- a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx
+++ b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx
@ -29,6 +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';

 const useTitleTracking = () => {
    const [previousTitle, setPreviousTitle] = useState<string>('');
@ -231,6 +232,14 @@ export const NewFeatureStrategyEdit = () => {
                isChangeRequest={isChangeRequestConfigured(environmentId)}
                tab={tab}
                setTab={setTab}
+                StrategyVariants={
+                    <StrategyVariants
+                        strategy={strategy}
+                        setStrategy={setStrategy}
+                        environment={environmentId}
+                        projectId={projectId}
+                    />
+                }
            />
            {staleDataNotification}
        </FormTemplate>
--- a/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx
+++ b/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx
@ -39,7 +39,8 @@ export const StrategyVariants: FC<{
    strategy: Partial<IFeatureStrategy>;
    projectId: string;
    environment: string;
-}> = ({ strategy, setStrategy, projectId, environment }) => {
+    editable?: boolean;
+}> = ({ strategy, setStrategy, projectId, environment, editable }) => {
    const { trackEvent } = usePlausibleTracker();
    const [variantsEdit, setVariantsEdit] = useState<IFeatureVariantEdit[]>([]);
    const theme = useTheme();
@ -54,7 +55,7 @@ export const StrategyVariants: FC<{
        setVariantsEdit(
            (strategy.variants || []).map((variant) => ({
                ...variant,
-                new: false,
+                new: editable || false,
                isValid: true,
                id: uuidv4(),
                overrides: [],
--- a/frontend/src/hooks/usePlausibleTracker.ts
+++ b/frontend/src/hooks/usePlausibleTracker.ts
@ -53,7 +53,8 @@ export type CustomEvents =
    | 'playground_token_input_used'
    | 'search-filter'
    | 'scheduled-configuration-changes'
-    | 'search-feature-buttons';
+    | 'search-feature-buttons'
+    | 'new-strategy-form';

 export const usePlausibleTracker = () => {
    const plausible = useContext(PlausibleContext);