From 049c5b9afadac1b260b4d5d92a539524108440f9 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Tue, 2 Jan 2024 13:53:04 +0100 Subject: [PATCH] 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 --- .../Changes/Change/EditChange.tsx | 9 + .../Changes/Change/NewEditChange.tsx | 9 + .../ConstraintAccordionList.tsx | 1 - .../NewFeatureStrategyForm.tsx | 30 +-- .../NewFeatureStrategyCreate.test.tsx | 128 +++++-------- .../NewFeatureStrategyCreate.tsx | 10 + .../featureStrategyFormTestSetup.ts | 99 ++++++++++ .../NewFeatureStrategyEdit.test.tsx | 172 ++++++++++++++---- .../NewFeatureStrategyEdit.tsx | 9 + .../StrategyTypes/StrategyVariants.tsx | 5 +- frontend/src/hooks/usePlausibleTracker.ts | 3 +- 11 files changed, 342 insertions(+), 133 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/featureStrategyFormTestSetup.ts diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EditChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EditChange.tsx index c6dc7bd24f..aefb5140d8 100644 --- 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={ + + } /> } elseShow={ diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx index dfe8d4e7f8..a32590e840 100644 --- 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={ + + } /> } elseShow={ diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList.tsx index 7c4845a01a..b86433a968 100644 --- 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< {constraints.map((constraint, index) => ( - {console.log(state.get(constraint))} 0} show={} diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/NewFeatureStrategyForm.tsx index 660a5bc427..865ad2a296 100644 --- 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>; + 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={ - - } + show={StrategyVariants} /> } /> diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx index ac54195445..ce5b49688c 100644 --- 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(); + }); }); diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx index 89db37944d..7a1bde224c 100644 --- 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={ + + } /> {staleDataNotification} diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/featureStrategyFormTestSetup.ts b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/featureStrategyFormTestSetup.ts new file mode 100644 index 0000000000..84e8fa3210 --- /dev/null +++ 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', + }, + ]); +}; diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.test.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.test.tsx index 02c20c547e..c3384cf136 100644 --- 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( + + } + /> + , + { + 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(); + }); }); diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyEdit/NewFeatureStrategyEdit.tsx index 627ec11ee1..27447f0d5f 100644 --- 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(''); @@ -231,6 +232,14 @@ export const NewFeatureStrategyEdit = () => { isChangeRequest={isChangeRequestConfigured(environmentId)} tab={tab} setTab={setTab} + StrategyVariants={ + + } /> {staleDataNotification} diff --git a/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx b/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx index 7f319aea13..dac3d11473 100644 --- 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; projectId: string; environment: string; -}> = ({ strategy, setStrategy, projectId, environment }) => { + editable?: boolean; +}> = ({ strategy, setStrategy, projectId, environment, editable }) => { const { trackEvent } = usePlausibleTracker(); const [variantsEdit, setVariantsEdit] = useState([]); 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: [], diff --git a/frontend/src/hooks/usePlausibleTracker.ts b/frontend/src/hooks/usePlausibleTracker.ts index 39ee0f0351..5587bebfb5 100644 --- 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);