From 7a436347cbea9d1e8e0dd84231ee8bf94bd5b3db Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 12 Dec 2024 09:31:39 +0100 Subject: [PATCH] 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. --- .../ManageBulkTagsDialog.test.tsx | 89 +++++++++++++++++++ .../ManageTagsDialog/ManageBulkTagsDialog.tsx | 23 +++-- .../ManageTagsDialog/TagTypeSelect.tsx | 10 ++- .../ManageTagsDialog/TagsInput.tsx | 7 +- 4 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.test.tsx diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.test.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.test.tsx new file mode 100644 index 0000000000..42e9768721 --- /dev/null +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.test.tsx @@ -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: [], + }); + }); +}); diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.tsx index 9881a6bfb4..0323cb3ddc 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/ManageTagsDialog/ManageBulkTagsDialog.tsx @@ -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 { type AutocompleteProps, @@ -46,7 +46,7 @@ const mergeTags = (tags: ITag[], newTag: ITag) => [ const filterTags = (tags: ITag[], tag: ITag) => tags.filter((x) => !(x.value === tag.value && x.type === tag.type)); -const payloadReducer = ( +export const payloadReducer = ( state: Payload, action: | { @@ -56,7 +56,8 @@ const payloadReducer = ( | { type: 'clear'; payload: ITag[]; - }, + } + | { type: 'reset' }, ) => { switch (action.type) { case 'add': @@ -76,6 +77,11 @@ const payloadReducer = ( addedTags: [], removedTags: action.payload, }; + case 'reset': + return { + addedTags: [], + removedTags: [], + }; default: return state; } @@ -87,7 +93,7 @@ const emptyTagType = { icon: '', }; -export const ManageBulkTagsDialog: VFC = ({ +export const ManageBulkTagsDialog: FC = ({ open, initialValues, initialIndeterminateValues, @@ -106,6 +112,11 @@ export const ManageBulkTagsDialog: VFC = ({ removedTags: [], }); + const submitAndReset = () => { + onSubmit(payload); + dispatch({ type: 'reset' }); + }; + const resetTagType = ( tagType: ITagType = tagTypes.length > 0 ? tagTypes[0] : emptyTagType, ) => { @@ -230,7 +241,7 @@ export const ManageBulkTagsDialog: VFC = ({ secondaryButtonText='Cancel' primaryButtonText='Save tags' title='Update feature flag tags' - onClick={() => onSubmit(payload)} + onClick={submitAndReset} disabledPrimaryButton={ payload.addedTags.length === 0 && payload.removedTags.length === 0 @@ -244,7 +255,7 @@ export const ManageBulkTagsDialog: VFC = ({ > Tags allow you to group features together -
onSubmit(payload)}> + option.name} - renderOption={(props, option) => ( + renderOption={( + { + key, + ...props + }: JSX.IntrinsicAttributes & HTMLAttributes, + option, + ) => ( & React.LiHTMLAttributes, option: TagOption, @@ -64,7 +67,7 @@ export const TagsInput = ({ indeterminateOption.title === option.title, ) ?? false; return ( -
  • +
  • theme.spacing(0.5) }} />}