Properly generating patch diffs from two blocks (#1861)

* Properly generating patch diffs

* Ading unit tests

* Remove unneeded function

* Fixing tests

webapp/src/blocks/__snapshots__/block.test.ts.snap

@@ -0,0 +1,63 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`block tests correctly generate patches from two blocks should add fields on the new fields added and remove it in the undo 1`] = `
+Array [
+  Object {
+    "deletedFields": Array [],
+    "updatedFields": Object {
+      "newField": "new field",
+    },
+  },
+  Object {
+    "deletedFields": Array [
+      "newField",
+    ],
+    "updatedFields": Object {},
+  },
+]
+`;
+
+exports[`block tests correctly generate patches from two blocks should generate two empty patches for the same block 1`] = `
+Array [
+  Object {
+    "deletedFields": Array [],
+    "updatedFields": Object {},
+  },
+  Object {
+    "deletedFields": Array [],
+    "updatedFields": Object {},
+  },
+]
+`;
+
+exports[`block tests correctly generate patches from two blocks should remove field on the new block added and add it again in the undo 1`] = `
+Array [
+  Object {
+    "deletedFields": Array [
+      "test",
+    ],
+    "updatedFields": Object {},
+  },
+  Object {
+    "deletedFields": Array [],
+    "updatedFields": Object {
+      "test": "test",
+    },
+  },
+]
+`;
+
+exports[`block tests correctly generate patches from two blocks should update propertie on the main object and revert it back on the undo 1`] = `
+Array [
+  Object {
+    "deletedFields": Array [],
+    "parentId": "new-parent-id",
+    "updatedFields": Object {},
+  },
+  Object {
+    "deletedFields": Array [],
+    "parentId": "old-parent-id",
+    "updatedFields": Object {},
+  },
+]
+`;

webapp/src/blocks/block.test.ts

@@ -0,0 +1,43 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+import {TestBlockFactory} from '../test/testBlockFactory'
+
+import {createPatchesFromBlocks, createBlock} from './block'
+
+describe('block tests', () => {
+    const board = TestBlockFactory.createBoard()
+    const card = TestBlockFactory.createCard(board)
+
+    describe('correctly generate patches from two blocks', () => {
+        it('should generate two empty patches for the same block', () => {
+            const textBlock = TestBlockFactory.createText(card)
+            const result = createPatchesFromBlocks(textBlock, textBlock)
+            expect(result).toMatchSnapshot()
+        })
+
+        it('should add fields on the new fields added and remove it in the undo', () => {
+            const oldBlock = TestBlockFactory.createText(card)
+            const newBlock = createBlock(oldBlock)
+            newBlock.fields.newField = 'new field'
+            const result = createPatchesFromBlocks(newBlock, oldBlock)
+            expect(result).toMatchSnapshot()
+        })
+
+        it('should remove field on the new block added and add it again in the undo', () => {
+            const oldBlock = TestBlockFactory.createText(card)
+            const newBlock = createBlock(oldBlock)
+            oldBlock.fields.test = 'test'
+            const result = createPatchesFromBlocks(newBlock, oldBlock)
+            expect(result).toMatchSnapshot()
+        })
+
+        it('should update propertie on the main object and revert it back on the undo', () => {
+            const oldBlock = TestBlockFactory.createText(card)
+            const newBlock = createBlock(oldBlock)
+            oldBlock.parentId = 'old-parent-id'
+            newBlock.parentId = 'new-parent-id'
+            const result = createPatchesFromBlocks(newBlock, oldBlock)
+            expect(result).toMatchSnapshot()
+        })
+    })
+})

webapp/src/blocks/block.ts

@@ -1,6 +1,8 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
+import difference from 'lodash/difference'
+
 import {Utils} from '../utils'
 
 const contentBlockTypes = ['text', 'image', 'divider', 'checkbox'] as const
@@ -63,43 +65,42 @@ function createBlock(block?: Block): Block {
 // contains the delta to update the block and another one for the undo
 // action, in case it happens
 function createPatchesFromBlocks(newBlock: Block, oldBlock: Block): BlockPatch[] {
-    const oldDeletedFields = [] as string[]
-    const newUpdatedFields = Object.keys(newBlock.fields).reduce((acc, val): Record<string, any> => {
-        // the field is in both old and new, so it is part of the new
-        // patch
-        if (val in oldBlock.fields) {
-            acc[val] = newBlock.fields[val]
-        } else {
-            // the field is only in the new block, so we set it to be
-            // removed in the undo patch
-            oldDeletedFields.push(val)
+    const newDeletedFields = difference(Object.keys(newBlock.fields), Object.keys(oldBlock.fields))
+    const newUpdatedFields: Record<string, any> = {}
+    const newUpdatedData: Record<string, any> = {}
+    Object.keys(newBlock.fields).forEach((val) => {
+        if (oldBlock.fields[val] !== newBlock.fields[val]) {
+            newUpdatedFields[val] = newBlock.fields[val]
         }
-        return acc
-    }, {} as Record<string, any>)
-
-    const newDeletedFields = [] as string[]
-    const oldUpdatedFields = Object.keys(oldBlock.fields).reduce((acc, val): Record<string, any> => {
-        // the field is in both, so in this case we set the old one to
-        // be applied for the undo patch
-        if (val in newBlock.fields) {
-            acc[val] = oldBlock.fields[val]
-        } else {
-            // the field is only on the old block, which means the
-            // update patch should remove it
-            newDeletedFields.push(val)
+    })
+    Object.keys(newBlock).forEach((val) => {
+        if (val !== 'fields' && (oldBlock as any)[val] !== (newBlock as any)[val]) {
+            newUpdatedData[val] = (newBlock as any)[val]
         }
-        return acc
-    }, {} as Record<string, any>)
+    })
+
+    const oldDeletedFields = difference(Object.keys(oldBlock.fields), Object.keys(newBlock.fields))
+    const oldUpdatedFields: Record<string, any> = {}
+    const oldUpdatedData: Record<string, any> = {}
+    Object.keys(oldBlock.fields).forEach((val) => {
+        if (oldBlock.fields[val] !== newBlock.fields[val]) {
+            oldUpdatedFields[val] = oldBlock.fields[val]
+        }
+    })
+    Object.keys(oldBlock).forEach((val) => {
+        if (val !== 'fields' && (oldBlock as any)[val] !== (newBlock as any)[val]) {
+            oldUpdatedData[val] = (oldBlock as any)[val]
+        }
+    })
 
-    // ToDo: add tests
     return [
         {
-            ...newBlock as BlockPatch,
+            ...newUpdatedData,
             updatedFields: newUpdatedFields,
             deletedFields: oldDeletedFields,
         },
         {
-            ...oldBlock as BlockPatch,
+            ...oldUpdatedData,
             updatedFields: oldUpdatedFields,
             deletedFields: newDeletedFields,
         },

webapp/src/components/content/checkboxElement.tsx

@@ -27,6 +27,8 @@ const CheckboxElement = React.memo((props: Props) => {
     const titleRef = useRef<Focusable>(null)
     const cardDetail = useCardDetailContext()
     const [addedBlockId, setAddedBlockId] = useState(cardDetail.lastAddedBlock.id)
+    const [active, setActive] = useState(Boolean(block.fields.value))
+    const [title, setTitle] = useState(block.title)
 
     useEffect(() => {
         if (block.id === addedBlockId) {
@@ -35,8 +37,9 @@ const CheckboxElement = React.memo((props: Props) => {
         }
     }, [block, addedBlockId, titleRef])
 
-    const [active, setActive] = useState(Boolean(block.fields.value))
-    const [title, setTitle] = useState(block.title)
+    useEffect(() => {
+        setActive(Boolean(block.fields.value))
+    }, [Boolean(block.fields.value)])
 
     return (
         <div className='CheckboxElement'>