From 22a92068b52b432edad8e2630975129400462617 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Wed, 23 Mar 2022 09:51:15 +0100 Subject: [PATCH] Reducing the amount of context needed to change card properties (#2579) Co-authored-by: Mattermod --- webapp/src/blocks/board.ts | 58 ++++++++++++------ webapp/src/components/kanban/kanban.test.tsx | 2 +- webapp/src/components/kanban/kanban.tsx | 4 +- .../components/kanban/kanbanColumnHeader.tsx | 4 +- .../src/components/propertyValueElement.tsx | 12 ++-- webapp/src/components/table/table.tsx | 2 +- .../components/table/tableGroupHeaderRow.tsx | 4 +- webapp/src/mutator.ts | 61 ++++++++++++------- 8 files changed, 90 insertions(+), 57 deletions(-) diff --git a/webapp/src/blocks/board.ts b/webapp/src/blocks/board.ts index 7dc68d511..ee95f3d24 100644 --- a/webapp/src/blocks/board.ts +++ b/webapp/src/blocks/board.ts @@ -186,12 +186,44 @@ function isPropertyEqual(propA: IPropertyTemplate, propB: IPropertyTemplate): bo return true } +// createCardPropertiesPatches creates two BoardPatch instances, one that +// contains the delta to update the board cardProperties and another one for +// the undo action, in case it happens +function createCardPropertiesPatches(newCardProperties: IPropertyTemplate[], oldCardProperties: IPropertyTemplate[]): BoardPatch[] { + const newDeletedCardProperties = getPropertiesDifference(newCardProperties, oldCardProperties) + const oldDeletedCardProperties = getPropertiesDifference(oldCardProperties, newCardProperties) + const newUpdatedCardProperties: IPropertyTemplate[] = [] + newCardProperties.forEach((val) => { + const oldCardProperty = oldCardProperties.find((o) => o.id === val.id) + if (!oldCardProperty || !isPropertyEqual(val, oldCardProperty)) { + newUpdatedCardProperties.push(val) + } + }) + const oldUpdatedCardProperties: IPropertyTemplate[] = [] + oldCardProperties.forEach((val) => { + const newCardProperty = newCardProperties.find((o) => o.id === val.id) + if (!newCardProperty || !isPropertyEqual(val, newCardProperty)) { + oldUpdatedCardProperties.push(val) + } + }) + + return [ + { + updatedCardProperties: newUpdatedCardProperties, + deletedCardProperties: oldDeletedCardProperties, + }, + { + updatedCardProperties: oldUpdatedCardProperties, + deletedCardProperties: newDeletedCardProperties, + }, + ] +} + // createPatchesFromBoards creates two BoardPatch instances, one that // contains the delta to update the board and another one for the undo // action, in case it happens function createPatchesFromBoards(newBoard: Board, oldBoard: Board): BoardPatch[] { const newDeletedProperties = difference(Object.keys(newBoard.properties || {}), Object.keys(oldBoard.properties || {})) - const newDeletedCardProperties = getPropertiesDifference(newBoard.cardProperties, oldBoard.cardProperties) const newDeletedColumnCalculations = difference(Object.keys(newBoard.columnCalculations), Object.keys(oldBoard.columnCalculations)) const newUpdatedProperties: Record = {} @@ -200,13 +232,6 @@ function createPatchesFromBoards(newBoard: Board, oldBoard: Board): BoardPatch[] newUpdatedProperties[val] = newBoard.properties[val] } }) - const newUpdatedCardProperties: IPropertyTemplate[] = [] - newBoard.cardProperties.forEach((val) => { - const oldCardProperty = oldBoard.cardProperties.find((o) => o.id === val.id) - if (!oldCardProperty || !isPropertyEqual(val, oldCardProperty)) { - newUpdatedCardProperties.push(val) - } - }) const newUpdatedColumnCalculations: Record = {} Object.keys(newBoard.columnCalculations).forEach((val) => { if (oldBoard.columnCalculations[val] !== newBoard.columnCalculations[val]) { @@ -225,7 +250,6 @@ function createPatchesFromBoards(newBoard: Board, oldBoard: Board): BoardPatch[] }) const oldDeletedProperties = difference(Object.keys(oldBoard.properties || {}), Object.keys(newBoard.properties || {})) - const oldDeletedCardProperties = getPropertiesDifference(oldBoard.cardProperties, newBoard.cardProperties) const oldDeletedColumnCalculations = difference(Object.keys(oldBoard.columnCalculations), Object.keys(newBoard.columnCalculations)) const oldUpdatedProperties: Record = {} @@ -234,13 +258,6 @@ function createPatchesFromBoards(newBoard: Board, oldBoard: Board): BoardPatch[] oldUpdatedProperties[val] = oldBoard.properties[val] } }) - const oldUpdatedCardProperties: IPropertyTemplate[] = [] - oldBoard.cardProperties.forEach((val) => { - const newCardProperty = newBoard.cardProperties.find((o) => o.id === val.id) - if (!newCardProperty || !isPropertyEqual(val, newCardProperty)) { - oldUpdatedCardProperties.push(val) - } - }) const oldUpdatedColumnCalculations: Record = {} Object.keys(oldBoard.columnCalculations).forEach((val) => { if (newBoard.columnCalculations[val] !== oldBoard.columnCalculations[val]) { @@ -258,22 +275,22 @@ function createPatchesFromBoards(newBoard: Board, oldBoard: Board): BoardPatch[] } }) + const [cardPropertiesPatch, cardPropertiesUndoPatch] = createCardPropertiesPatches(newBoard.cardProperties, oldBoard.cardProperties) + return [ { ...newData, + ...cardPropertiesPatch, updatedProperties: newUpdatedProperties, deletedProperties: oldDeletedProperties, - updatedCardProperties: newUpdatedCardProperties, - deletedCardProperties: oldDeletedCardProperties, updatedColumnCalculations: newUpdatedColumnCalculations, deletedColumnCalculations: oldDeletedColumnCalculations, }, { ...oldData, + ...cardPropertiesUndoPatch, updatedProperties: oldUpdatedProperties, deletedProperties: newDeletedProperties, - updatedCardProperties: oldUpdatedCardProperties, - deletedCardProperties: newDeletedCardProperties, updatedColumnCalculations: oldUpdatedColumnCalculations, deletedColumnCalculations: newDeletedColumnCalculations, }, @@ -324,4 +341,5 @@ export { BoardTypePrivate, createPatchesFromBoards, createPatchesFromBoardsAndBlocks, + createCardPropertiesPatches, } diff --git a/webapp/src/components/kanban/kanban.test.tsx b/webapp/src/components/kanban/kanban.test.tsx index 0c8dbdf6c..755442987 100644 --- a/webapp/src/components/kanban/kanban.test.tsx +++ b/webapp/src/components/kanban/kanban.test.tsx @@ -450,7 +450,7 @@ describe('src/component/kanban/kanban', () => { fireEvent.blur(inputTitle) await waitFor(async () => { - expect(mockedchangePropertyOptionValue).toBeCalledWith(board, groupProperty, optionQ1, 'New Q1') + expect(mockedchangePropertyOptionValue).toBeCalledWith(board.id, board.cardProperties, groupProperty, optionQ1, 'New Q1') }) expect(container).toMatchSnapshot() diff --git a/webapp/src/components/kanban/kanban.tsx b/webapp/src/components/kanban/kanban.tsx index 13e092dba..d55e94223 100644 --- a/webapp/src/components/kanban/kanban.tsx +++ b/webapp/src/components/kanban/kanban.tsx @@ -66,7 +66,7 @@ const Kanban = (props: Props) => { const visibleBadges = activeView.fields.visiblePropertyIds.includes(Constants.badgesColumnId) const propertyNameChanged = useCallback(async (option: IPropertyOption, text: string): Promise => { - await mutator.changePropertyOptionValue(board, groupByProperty!, option, text) + await mutator.changePropertyOptionValue(board.id, board.cardProperties, groupByProperty!, option, text) }, [board, groupByProperty]) const addGroupClicked = useCallback(async () => { @@ -78,7 +78,7 @@ const Kanban = (props: Props) => { color: 'propColorDefault', } - await mutator.insertPropertyOption(board, groupByProperty!, option, 'add group') + await mutator.insertPropertyOption(board.id, board.cardProperties, groupByProperty!, option, 'add group') }, [board, groupByProperty]) const orderAfterMoveToColumn = useCallback((cardIds: string[], columnId?: string): string[] => { diff --git a/webapp/src/components/kanban/kanbanColumnHeader.tsx b/webapp/src/components/kanban/kanbanColumnHeader.tsx index 33c3bd59c..11184c52c 100644 --- a/webapp/src/components/kanban/kanbanColumnHeader.tsx +++ b/webapp/src/components/kanban/kanbanColumnHeader.tsx @@ -172,7 +172,7 @@ export default function KanbanColumnHeader(props: Props): JSX.Element { id='delete' icon={} name={intl.formatMessage({id: 'BoardComponent.delete', defaultMessage: 'Delete'})} - onClick={() => mutator.deletePropertyOption(board, groupByProperty!, group.option)} + onClick={() => mutator.deletePropertyOption(board.id, board.cardProperties, groupByProperty!, group.option)} /> {Object.entries(Constants.menuColors).map(([key, color]) => ( @@ -180,7 +180,7 @@ export default function KanbanColumnHeader(props: Props): JSX.Element { key={key} id={key} name={color} - onClick={() => mutator.changePropertyOptionColor(board, groupByProperty!, group.option, key)} + onClick={() => mutator.changePropertyOptionColor(board.id, board.cardProperties, groupByProperty!, group.option, key)} /> ))} } diff --git a/webapp/src/components/propertyValueElement.tsx b/webapp/src/components/propertyValueElement.tsx index 6ebc2f846..3156a959b 100644 --- a/webapp/src/components/propertyValueElement.tsx +++ b/webapp/src/components/propertyValueElement.tsx @@ -82,7 +82,7 @@ const PropertyValueElement = (props:Props): JSX.Element => { color: 'propColorDefault', } currentValues.push(option) - mutator.insertPropertyOption(board, propertyTemplate, option, 'add property option').then(() => { + mutator.insertPropertyOption(board.id, board.cardProperties, propertyTemplate, option, 'add property option').then(() => { mutator.changePropertyValue(props.board.id, card, propertyTemplate.id, currentValues.map((v: IPropertyOption) => v.id)) }) }, [board, props.board.id, card, propertyTemplate]) @@ -95,8 +95,8 @@ const PropertyValueElement = (props:Props): JSX.Element => { } }, [value, props.board.id, card, propertyTemplate.id]) const onChangeInMultiselect = useCallback((newValue) => mutator.changePropertyValue(props.board.id, card, propertyTemplate.id, newValue), [props.board.id, card, propertyTemplate]) - const onChangeColorInMultiselect = useCallback((option: IPropertyOption, colorId: string) => mutator.changePropertyOptionColor(board, propertyTemplate, option, colorId), [board, propertyTemplate]) - const onDeleteOptionInMultiselect = useCallback((option: IPropertyOption) => mutator.deletePropertyOption(board, propertyTemplate, option), [board, propertyTemplate]) + const onChangeColorInMultiselect = useCallback((option: IPropertyOption, colorId: string) => mutator.changePropertyOptionColor(board.id, board.cardProperties, propertyTemplate, option, colorId), [board, propertyTemplate]) + const onDeleteOptionInMultiselect = useCallback((option: IPropertyOption) => mutator.deletePropertyOption(board.id, board.cardProperties, propertyTemplate, option), [board, propertyTemplate]) const onCreateInSelect = useCallback((newValue) => { const option: IPropertyOption = { @@ -104,14 +104,14 @@ const PropertyValueElement = (props:Props): JSX.Element => { value: newValue, color: 'propColorDefault', } - mutator.insertPropertyOption(board, propertyTemplate, option, 'add property option').then(() => { + mutator.insertPropertyOption(board.id, board.cardProperties, propertyTemplate, option, 'add property option').then(() => { mutator.changePropertyValue(props.board.id, card, propertyTemplate.id, option.id) }) }, [board, props.board.id, card, propertyTemplate.id]) const onChangeInSelect = useCallback((newValue) => mutator.changePropertyValue(props.board.id, card, propertyTemplate.id, newValue), []) - const onChangeColorInSelect = useCallback((option: IPropertyOption, colorId: string) => mutator.changePropertyOptionColor(board, propertyTemplate, option, colorId), [board, propertyTemplate]) - const onDeleteOptionInSelect = useCallback((option: IPropertyOption) => mutator.deletePropertyOption(board, propertyTemplate, option), [board, propertyTemplate]) + const onChangeColorInSelect = useCallback((option: IPropertyOption, colorId: string) => mutator.changePropertyOptionColor(board.id, board.cardProperties, propertyTemplate, option, colorId), [board, propertyTemplate]) + const onDeleteOptionInSelect = useCallback((option: IPropertyOption) => mutator.deletePropertyOption(board.id, board.cardProperties, propertyTemplate, option), [board, propertyTemplate]) const validateProp = useCallback((val: string): boolean => { if (val === '') { diff --git a/webapp/src/components/table/table.tsx b/webapp/src/components/table/table.tsx index e73646478..322f0152c 100644 --- a/webapp/src/components/table/table.tsx +++ b/webapp/src/components/table/table.tsx @@ -180,7 +180,7 @@ const Table = (props: Props): JSX.Element => { }, [activeView, cards, props.selectedCardIds, groupByProperty]) const propertyNameChanged = useCallback(async (option: IPropertyOption, text: string): Promise => { - await mutator.changePropertyOptionValue(board, groupByProperty!, option, text) + await mutator.changePropertyOptionValue(board.id, board.cardProperties, groupByProperty!, option, text) }, [board, groupByProperty]) return ( diff --git a/webapp/src/components/table/tableGroupHeaderRow.tsx b/webapp/src/components/table/tableGroupHeaderRow.tsx index 7426ddf0b..aa9660ada 100644 --- a/webapp/src/components/table/tableGroupHeaderRow.tsx +++ b/webapp/src/components/table/tableGroupHeaderRow.tsx @@ -125,7 +125,7 @@ const TableGroupHeaderRow = (props: Props): JSX.Element => { id='delete' icon={} name={intl.formatMessage({id: 'BoardComponent.delete', defaultMessage: 'Delete'})} - onClick={() => mutator.deletePropertyOption(board, groupByProperty!, group.option)} + onClick={() => mutator.deletePropertyOption(board.id, board.cardProperties, groupByProperty!, group.option)} /> {Object.entries(Constants.menuColors).map(([key, color]) => ( @@ -133,7 +133,7 @@ const TableGroupHeaderRow = (props: Props): JSX.Element => { key={key} id={key} name={color} - onClick={() => mutator.changePropertyOptionColor(board, groupByProperty!, group.option, key)} + onClick={() => mutator.changePropertyOptionColor(board.id, board.cardProperties, groupByProperty!, group.option, key)} /> ))} } diff --git a/webapp/src/mutator.ts b/webapp/src/mutator.ts index fd32864b1..f77762ceb 100644 --- a/webapp/src/mutator.ts +++ b/webapp/src/mutator.ts @@ -3,10 +3,11 @@ import {IntlShape} from 'react-intl' import {batch} from 'react-redux' +import cloneDeep from 'lodash/cloneDeep' import {BlockIcons} from './blockIcons' import {Block, BlockPatch, createPatchesFromBlocks} from './blocks/block' -import {Board, BoardMember, BoardsAndBlocks, IPropertyOption, IPropertyTemplate, PropertyType, createBoard, createPatchesFromBoards, createPatchesFromBoardsAndBlocks} from './blocks/board' +import {Board, BoardMember, BoardsAndBlocks, IPropertyOption, IPropertyTemplate, PropertyType, createBoard, createPatchesFromBoards, createPatchesFromBoardsAndBlocks, createCardPropertiesPatches} from './blocks/board' import {BoardView, ISortOption, createBoardView, KanbanCalculationFields} from './blocks/boardView' import {Card, createCard} from './blocks/card' import {ContentBlock} from './blocks/contentBlock' @@ -559,52 +560,66 @@ class Mutator { // Properties - async insertPropertyOption(board: Board, template: IPropertyTemplate, option: IPropertyOption, description = 'add option') { - Utils.assert(board.cardProperties.includes(template)) + async updateBoardCardProperties(boardId: string, oldProperties: IPropertyTemplate[], newProperties: IPropertyTemplate[], description = 'update card properties') { + const [updatePatch, undoPatch] = createCardPropertiesPatches(newProperties, oldProperties) + await undoManager.perform( + async () => { + await octoClient.patchBoard(boardId, updatePatch) + }, + async () => { + await octoClient.patchBoard(boardId, undoPatch) + }, + description, + this.undoGroupId, + ) + } - const newBoard = createBoard(board) - const newTemplate = newBoard.cardProperties.find((o: IPropertyTemplate) => o.id === template.id)! + async insertPropertyOption(boardId: string, oldCardProperties: IPropertyTemplate[], template: IPropertyTemplate, option: IPropertyOption, description = 'add option') { + Utils.assert(oldCardProperties.includes(template)) + + const newCardProperties: IPropertyTemplate[] = cloneDeep(oldCardProperties) + const newTemplate = newCardProperties.find((o: IPropertyTemplate) => o.id === template.id)! newTemplate.options.push(option) - await this.updateBoard(newBoard, board, description) + await this.updateBoardCardProperties(boardId, oldCardProperties, newCardProperties, description) } - async deletePropertyOption(board: Board, template: IPropertyTemplate, option: IPropertyOption) { - const newBoard = createBoard(board) - const newTemplate = newBoard.cardProperties.find((o: IPropertyTemplate) => o.id === template.id)! + async deletePropertyOption(boardId: string, oldCardProperties: IPropertyTemplate[], template: IPropertyTemplate, option: IPropertyOption) { + const newCardProperties: IPropertyTemplate[] = cloneDeep(oldCardProperties) + const newTemplate = newCardProperties.find((o: IPropertyTemplate) => o.id === template.id)! newTemplate.options = newTemplate.options.filter((o) => o.id !== option.id) - await this.updateBoard(newBoard, board, 'delete option') + await this.updateBoardCardProperties(boardId, oldCardProperties, newCardProperties, 'delete option') } - async changePropertyOptionOrder(board: Board, template: IPropertyTemplate, option: IPropertyOption, destIndex: number) { + async changePropertyOptionOrder(boardId: string, oldCardProperties: IPropertyTemplate[], template: IPropertyTemplate, option: IPropertyOption, destIndex: number) { const srcIndex = template.options.indexOf(option) Utils.log(`srcIndex: ${srcIndex}, destIndex: ${destIndex}`) - const newBoard = createBoard(board) - const newTemplate = newBoard.cardProperties.find((o: IPropertyTemplate) => o.id === template.id)! + const newCardProperties: IPropertyTemplate[] = cloneDeep(oldCardProperties) + const newTemplate = newCardProperties.find((o: IPropertyTemplate) => o.id === template.id)! newTemplate.options.splice(destIndex, 0, newTemplate.options.splice(srcIndex, 1)[0]) - await this.updateBoard(newBoard, board, 'reorder options') + await this.updateBoardCardProperties(boardId, oldCardProperties, newCardProperties, 'reorder option') } - async changePropertyOptionValue(board: Board, propertyTemplate: IPropertyTemplate, option: IPropertyOption, value: string) { - const newBoard = createBoard(board) - const newTemplate = newBoard.cardProperties.find((o: IPropertyTemplate) => o.id === propertyTemplate.id)! + async changePropertyOptionValue(boardId: string, oldCardProperties: IPropertyTemplate[], propertyTemplate: IPropertyTemplate, option: IPropertyOption, value: string) { + const newCardProperties: IPropertyTemplate[] = cloneDeep(oldCardProperties) + const newTemplate = newCardProperties.find((o: IPropertyTemplate) => o.id === propertyTemplate.id)! const newOption = newTemplate.options.find((o) => o.id === option.id)! newOption.value = value - await this.updateBoard(newBoard, board, 'rename option') + await this.updateBoardCardProperties(boardId, oldCardProperties, newCardProperties, 'rename option') - return newBoard + return newCardProperties } - async changePropertyOptionColor(board: Board, template: IPropertyTemplate, option: IPropertyOption, color: string) { - const newBoard = createBoard(board) - const newTemplate = newBoard.cardProperties.find((o: IPropertyTemplate) => o.id === template.id)! + async changePropertyOptionColor(boardId: string, oldCardProperties: IPropertyTemplate[], template: IPropertyTemplate, option: IPropertyOption, color: string) { + const newCardProperties: IPropertyTemplate[] = cloneDeep(oldCardProperties) + const newTemplate = newCardProperties.find((o: IPropertyTemplate) => o.id === template.id)! const newOption = newTemplate.options.find((o) => o.id === option.id)! newOption.color = color - await this.updateBoard(newBoard, board, 'change option color') + await this.updateBoardCardProperties(boardId, oldCardProperties, newCardProperties, 'rename option') } async changePropertyValue(boardId: string, card: Card, propertyId: string, value?: string | string[], description = 'change property') {