mirror of
https://github.com/Unleash/unleash.git
synced 2025-07-31 13:47:02 +02:00
fix(1-3173): clear "removed tags" when you bulk update tags (#8952)
This PR fixes a bug wherein the list of tags to remove from a group of tags wouldn't be correctly updated. ## Repro steps - Add a console log line to `frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.tsx`'s `ManagebulkTagsDialog`. Log the value of the`payload` variable. - Pick a flag with no tags. - Add tag A -> before submitting, you should have one added tag and zero removed flags. After submitting, both should be empty. - Now remove tag A -> before submitting, you should have one removed tag and zero added tag. After submitting, both should be empty - Notice that removed flags hasn't been emptied, but still contains tag A. - Now add tab B -> before submitting, you should have tag B in added and nothing in removed. Notice that tag A is still in removed. ## Discussion points This gives us both a `clear` and a `reset` event, which is unfortunate because they sound like they do the same thing. I'd suggest renaming the `clear` event (because it doesn't really clear the state completely), but I'm not sure to what. Happy to do that if you have a suggestion. I have not tested that submission of the form actually resets the state. I spent about 45 minutes looking at it, but couldn't find a way that was sensible and worked (considered spying: couldn't make it work; considered refactoring and extracting components: think that's too much of a change). I think this is benign enough that it can go without a test for that thing actually being called. I did, however, test the different reducer commands.
This commit is contained in:
parent
37a3ec9599
commit
7a436347cb
@ -0,0 +1,89 @@
|
|||||||
|
import { payloadReducer } from './ManageBulkTagsDialog';
|
||||||
|
|
||||||
|
describe('payloadReducer', () => {
|
||||||
|
it('should add a tag to addedTags and remove it from removedTags', () => {
|
||||||
|
const initialState = {
|
||||||
|
addedTags: [{ type: 'simple', value: 'A' }],
|
||||||
|
removedTags: [
|
||||||
|
{ type: 'simple', value: 'B' },
|
||||||
|
{ type: 'simple', value: 'C' },
|
||||||
|
],
|
||||||
|
};
|
||||||
|
|
||||||
|
const action = {
|
||||||
|
type: 'add' as const,
|
||||||
|
payload: { type: 'simple', value: 'B' },
|
||||||
|
};
|
||||||
|
|
||||||
|
const newState = payloadReducer(initialState, action);
|
||||||
|
|
||||||
|
expect(newState).toMatchObject({
|
||||||
|
addedTags: [
|
||||||
|
{ type: 'simple', value: 'A' },
|
||||||
|
{ type: 'simple', value: 'B' },
|
||||||
|
],
|
||||||
|
removedTags: [{ type: 'simple', value: 'C' }],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should remove a tag from addedTags and add it to removedTags', () => {
|
||||||
|
const initialState = {
|
||||||
|
addedTags: [
|
||||||
|
{ type: 'simple', value: 'A' },
|
||||||
|
{ type: 'simple', value: 'B' },
|
||||||
|
],
|
||||||
|
removedTags: [{ type: 'simple', value: 'C' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const action = {
|
||||||
|
type: 'remove' as const,
|
||||||
|
payload: { type: 'simple', value: 'B' },
|
||||||
|
};
|
||||||
|
|
||||||
|
const newState = payloadReducer(initialState, action);
|
||||||
|
|
||||||
|
expect(newState).toMatchObject({
|
||||||
|
addedTags: [{ type: 'simple', value: 'A' }],
|
||||||
|
removedTags: [
|
||||||
|
{ type: 'simple', value: 'C' },
|
||||||
|
{ type: 'simple', value: 'B' },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should empty addedTags and set removedTags to the payload on clear', () => {
|
||||||
|
const initialState = {
|
||||||
|
addedTags: [{ type: 'simple', value: 'A' }],
|
||||||
|
removedTags: [{ type: 'simple', value: 'B' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const action = {
|
||||||
|
type: 'clear' as const,
|
||||||
|
payload: [{ type: 'simple', value: 'C' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const newState = payloadReducer(initialState, action);
|
||||||
|
|
||||||
|
expect(newState).toMatchObject({
|
||||||
|
addedTags: [],
|
||||||
|
removedTags: [{ type: 'simple', value: 'C' }],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should empty both addedTags and removedTags on reset', () => {
|
||||||
|
const initialState = {
|
||||||
|
addedTags: [{ type: 'simple', value: 'test' }],
|
||||||
|
removedTags: [{ type: 'simple', value: 'test2' }],
|
||||||
|
};
|
||||||
|
|
||||||
|
const action = {
|
||||||
|
type: 'reset' as const,
|
||||||
|
};
|
||||||
|
|
||||||
|
const newState = payloadReducer(initialState, action);
|
||||||
|
expect(newState).toMatchObject({
|
||||||
|
addedTags: [],
|
||||||
|
removedTags: [],
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
@ -1,4 +1,4 @@
|
|||||||
import { useEffect, useReducer, useState, type VFC } from 'react';
|
import { type FC, useEffect, useReducer, useState } from 'react';
|
||||||
import { Link as RouterLink } from 'react-router-dom';
|
import { Link as RouterLink } from 'react-router-dom';
|
||||||
import {
|
import {
|
||||||
type AutocompleteProps,
|
type AutocompleteProps,
|
||||||
@ -46,7 +46,7 @@ const mergeTags = (tags: ITag[], newTag: ITag) => [
|
|||||||
const filterTags = (tags: ITag[], tag: ITag) =>
|
const filterTags = (tags: ITag[], tag: ITag) =>
|
||||||
tags.filter((x) => !(x.value === tag.value && x.type === tag.type));
|
tags.filter((x) => !(x.value === tag.value && x.type === tag.type));
|
||||||
|
|
||||||
const payloadReducer = (
|
export const payloadReducer = (
|
||||||
state: Payload,
|
state: Payload,
|
||||||
action:
|
action:
|
||||||
| {
|
| {
|
||||||
@ -56,7 +56,8 @@ const payloadReducer = (
|
|||||||
| {
|
| {
|
||||||
type: 'clear';
|
type: 'clear';
|
||||||
payload: ITag[];
|
payload: ITag[];
|
||||||
},
|
}
|
||||||
|
| { type: 'reset' },
|
||||||
) => {
|
) => {
|
||||||
switch (action.type) {
|
switch (action.type) {
|
||||||
case 'add':
|
case 'add':
|
||||||
@ -76,6 +77,11 @@ const payloadReducer = (
|
|||||||
addedTags: [],
|
addedTags: [],
|
||||||
removedTags: action.payload,
|
removedTags: action.payload,
|
||||||
};
|
};
|
||||||
|
case 'reset':
|
||||||
|
return {
|
||||||
|
addedTags: [],
|
||||||
|
removedTags: [],
|
||||||
|
};
|
||||||
default:
|
default:
|
||||||
return state;
|
return state;
|
||||||
}
|
}
|
||||||
@ -87,7 +93,7 @@ const emptyTagType = {
|
|||||||
icon: '',
|
icon: '',
|
||||||
};
|
};
|
||||||
|
|
||||||
export const ManageBulkTagsDialog: VFC<IManageBulkTagsDialogProps> = ({
|
export const ManageBulkTagsDialog: FC<IManageBulkTagsDialogProps> = ({
|
||||||
open,
|
open,
|
||||||
initialValues,
|
initialValues,
|
||||||
initialIndeterminateValues,
|
initialIndeterminateValues,
|
||||||
@ -106,6 +112,11 @@ export const ManageBulkTagsDialog: VFC<IManageBulkTagsDialogProps> = ({
|
|||||||
removedTags: [],
|
removedTags: [],
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const submitAndReset = () => {
|
||||||
|
onSubmit(payload);
|
||||||
|
dispatch({ type: 'reset' });
|
||||||
|
};
|
||||||
|
|
||||||
const resetTagType = (
|
const resetTagType = (
|
||||||
tagType: ITagType = tagTypes.length > 0 ? tagTypes[0] : emptyTagType,
|
tagType: ITagType = tagTypes.length > 0 ? tagTypes[0] : emptyTagType,
|
||||||
) => {
|
) => {
|
||||||
@ -230,7 +241,7 @@ export const ManageBulkTagsDialog: VFC<IManageBulkTagsDialogProps> = ({
|
|||||||
secondaryButtonText='Cancel'
|
secondaryButtonText='Cancel'
|
||||||
primaryButtonText='Save tags'
|
primaryButtonText='Save tags'
|
||||||
title='Update feature flag tags'
|
title='Update feature flag tags'
|
||||||
onClick={() => onSubmit(payload)}
|
onClick={submitAndReset}
|
||||||
disabledPrimaryButton={
|
disabledPrimaryButton={
|
||||||
payload.addedTags.length === 0 &&
|
payload.addedTags.length === 0 &&
|
||||||
payload.removedTags.length === 0
|
payload.removedTags.length === 0
|
||||||
@ -244,7 +255,7 @@ export const ManageBulkTagsDialog: VFC<IManageBulkTagsDialogProps> = ({
|
|||||||
>
|
>
|
||||||
Tags allow you to group features together
|
Tags allow you to group features together
|
||||||
</Typography>
|
</Typography>
|
||||||
<form id={formId} onSubmit={() => onSubmit(payload)}>
|
<form id={formId} onSubmit={submitAndReset}>
|
||||||
<StyledDialogFormContent>
|
<StyledDialogFormContent>
|
||||||
<TagTypeSelect
|
<TagTypeSelect
|
||||||
key={tagTypesLoading ? 'loading' : tagTypes.length}
|
key={tagTypesLoading ? 'loading' : tagTypes.length}
|
||||||
|
@ -7,6 +7,7 @@ import {
|
|||||||
useTheme,
|
useTheme,
|
||||||
} from '@mui/material';
|
} from '@mui/material';
|
||||||
import type { ITagType } from 'interfaces/tags';
|
import type { ITagType } from 'interfaces/tags';
|
||||||
|
import type { HTMLAttributes } from 'react';
|
||||||
|
|
||||||
interface ITagSelect {
|
interface ITagSelect {
|
||||||
options: ITagType[];
|
options: ITagType[];
|
||||||
@ -37,8 +38,15 @@ export const TagTypeSelect = ({
|
|||||||
disableClearable
|
disableClearable
|
||||||
value={value}
|
value={value}
|
||||||
getOptionLabel={(option) => option.name}
|
getOptionLabel={(option) => option.name}
|
||||||
renderOption={(props, option) => (
|
renderOption={(
|
||||||
|
{
|
||||||
|
key,
|
||||||
|
...props
|
||||||
|
}: JSX.IntrinsicAttributes & HTMLAttributes<HTMLLIElement>,
|
||||||
|
option,
|
||||||
|
) => (
|
||||||
<ListItem
|
<ListItem
|
||||||
|
key={key}
|
||||||
{...props}
|
{...props}
|
||||||
style={{
|
style={{
|
||||||
alignItems: 'flex-start',
|
alignItems: 'flex-start',
|
||||||
|
@ -52,7 +52,10 @@ export const TagsInput = ({
|
|||||||
};
|
};
|
||||||
|
|
||||||
const renderOption = (
|
const renderOption = (
|
||||||
props: JSX.IntrinsicAttributes &
|
{
|
||||||
|
key,
|
||||||
|
...props
|
||||||
|
}: JSX.IntrinsicAttributes &
|
||||||
React.ClassAttributes<HTMLLIElement> &
|
React.ClassAttributes<HTMLLIElement> &
|
||||||
React.LiHTMLAttributes<HTMLLIElement>,
|
React.LiHTMLAttributes<HTMLLIElement>,
|
||||||
option: TagOption,
|
option: TagOption,
|
||||||
@ -64,7 +67,7 @@ export const TagsInput = ({
|
|||||||
indeterminateOption.title === option.title,
|
indeterminateOption.title === option.title,
|
||||||
) ?? false;
|
) ?? false;
|
||||||
return (
|
return (
|
||||||
<li {...props}>
|
<li key={key} {...props}>
|
||||||
<ConditionallyRender
|
<ConditionallyRender
|
||||||
condition={Boolean(option.inputValue)}
|
condition={Boolean(option.inputValue)}
|
||||||
show={<Add sx={{ mr: (theme) => theme.spacing(0.5) }} />}
|
show={<Add sx={{ mr: (theme) => theme.spacing(0.5) }} />}
|
||||||
|
Loading…
Reference in New Issue
Block a user