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 c6dc7bd24f8c..aefb5140d86f 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 dfe8d4e7f8fd..a32590e840eb 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 7c4845a01ad4..b86433a96886 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 660a5bc427b5..865ad2a29685 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 ac541954450f..ce5b49688c42 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 89db37944d63..7a1bde224c43 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 000000000000..84e8fa3210e1 --- /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 02c20c547e7c..c3384cf13625 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 627ec11ee162..27447f0d5f54 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 7f319aea1328..dac3d1147311 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 39ee0f0351e3..5587bebfb521 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);