Skip to content

Commit

Permalink
fix: remove empty variants when changing tabs (#5850)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
FredrikOseberg committed Jan 11, 2024
1 parent 3a2d4ae commit 2a723ea
Show file tree
Hide file tree
Showing 7 changed files with 251 additions and 116 deletions.
Expand Up @@ -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;
Expand Down Expand Up @@ -174,7 +174,7 @@ export const NewEditChange = ({
tab={tab}
setTab={setTab}
StrategyVariants={
<StrategyVariants
<NewStrategyVariants
strategy={strategy}
setStrategy={setStrategy}
environment={environment}
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -330,10 +331,14 @@ export const NewFeatureStrategyForm = ({
};

const onSubmitWithFeedback = async () => {
await onSubmit();

if (newStrategyConfigurationFeedback && !hasSubmittedFeedback) {
createFeedbackContext();
try {
await onSubmit();

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

Expand Down
Expand Up @@ -199,20 +199,48 @@ 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];

const inputElement = screen.getAllByRole('textbox')[0];
fireEvent.change(inputElement, {
target: { value: expectedVariantName },
});
expect(inputElement2).not.toBeDisabled();
});

expect(screen.getByText(expectedVariantName)).toBeInTheDocument();
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);

const addConstraintEl = await screen.findByText('Add constraint');
expect(addConstraintEl).toBeInTheDocument();

fireEvent.click(variantsEl);

const variants2 = screen.queryAllByTestId('VARIANT');
expect(variants2.length).toBe(0);
});

test('Should autosave constraint settings when navigating between tabs', async () => {
Expand Down
Expand Up @@ -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);
Expand Down Expand Up @@ -211,7 +211,7 @@ export const NewFeatureStrategyCreate = () => {
tab={tab}
setTab={setTab}
StrategyVariants={
<StrategyVariants
<NewStrategyVariants
strategy={strategy}
setStrategy={setStrategy}
environment={environmentId}
Expand Down
Expand Up @@ -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>('');
Expand Down Expand Up @@ -233,7 +233,7 @@ export const NewFeatureStrategyEdit = () => {
tab={tab}
setTab={setTab}
StrategyVariants={
<StrategyVariants
<NewStrategyVariants
strategy={strategy}
setStrategy={setStrategy}
environment={environmentId}
Expand Down
202 changes: 202 additions & 0 deletions 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} />
</>
);
};

0 comments on commit 2a723ea

Please sign in to comment.