From 2a723ea9e82fe52f38ef387875ec931b3e528361 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Thu, 11 Jan 2024 10:43:29 +0100 Subject: [PATCH] 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 --- .../Changes/Change/NewEditChange.tsx | 4 +- .../NewFeatureStrategyForm.tsx | 11 +- .../NewFeatureStrategyCreate.test.tsx | 40 +++- .../NewFeatureStrategyCreate.tsx | 4 +- .../NewFeatureStrategyEdit.tsx | 4 +- .../StrategyTypes/NewStrategyVariants.tsx | 202 ++++++++++++++++++ .../StrategyTypes/StrategyVariants.tsx | 102 +-------- 7 files changed, 251 insertions(+), 116 deletions(-) create mode 100644 frontend/src/component/feature/StrategyTypes/NewStrategyVariants.tsx diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NewEditChange.tsx index a32590e840..b66d96bf24 100644 --- 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={ - { - await onSubmit(); + try { + await onSubmit(); - if (newStrategyConfigurationFeedback && !hasSubmittedFeedback) { - createFeedbackContext(); + if (newStrategyConfigurationFeedback && !hasSubmittedFeedback) { + createFeedbackContext(); + } + } catch (e) { + console.error(e); } }; diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.test.tsx index c22f2288cb..a508719dd9 100644 --- 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 () => { diff --git a/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx b/frontend/src/component/feature/FeatureStrategy/NewFeatureStrategyCreate/NewFeatureStrategyCreate.tsx index 7a1bde224c..b50367c11b 100644 --- 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={ - { const [previousTitle, setPreviousTitle] = useState(''); @@ -233,7 +233,7 @@ export const NewFeatureStrategyEdit = () => { tab={tab} setTab={setTab} StrategyVariants={ - ({ + 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> + >; + strategy: Partial; + projectId: string; + environment: string; + editable?: boolean; +}> = ({ strategy, setStrategy, projectId, environment, editable }) => { + const { trackEvent } = usePlausibleTracker(); + const [variantsEdit, setVariantsEdit] = useState([]); + 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 ( + <> + + Variants enhance a feature flag by providing a version of the + feature to be enabled + + + Variants + + + 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{' '} + + here + + + + } + /> + + + 0} + show={} + /> + + {variantsEdit.map((variant, i) => ( + + 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 + ] + } + /> + ))} + + } + > + Add variant + + + + ); +}; diff --git a/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx b/frontend/src/component/feature/StrategyTypes/StrategyVariants.tsx index e5ded911b6..e5e3656ad9 100644 --- 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> @@ -45,7 +30,6 @@ export const StrategyVariants: FC<{ const { trackEvent } = usePlausibleTracker(); const [variantsEdit, setVariantsEdit] = useState([]); 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 ( - <> - - Variants enhance a feature flag by providing a version of - the feature to be enabled - - - Variants - - - 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{' '} - - here - - - - } - /> - - - 0} - show={} - /> - - {variantsEdit.map((variant, i) => ( - - 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 - ] - } - /> - ))} - - } - > - Add variant - - - - ); - } - return ( <>