MD Bookmarks: Remove deleted nodes from state tree

chrome/test/data/webui/md_bookmarks/reducers_test.js

Index: chrome/test/data/webui/md_bookmarks/reducers_test.js
diff --git a/chrome/test/data/webui/md_bookmarks/reducers_test.js b/chrome/test/data/webui/md_bookmarks/reducers_test.js
index 6e9c3dec69b29dcfc83b73d29710837b46e85e79..36e3448e9e0d8d856a7fb0f553b861c356c14679 100644
--- a/chrome/test/data/webui/md_bookmarks/reducers_test.js
+++ b/chrome/test/data/webui/md_bookmarks/reducers_test.js
@@ -78,6 +78,28 @@ suite('selection state', function() {
     state = bookmarks.SelectionState.updateSelection(state, action);
     assertDeepEquals({}, state.items);
   });
+
+  test('deselects items when they are deleted', function() {
+    var nodeList = testTree(createFolder('0', [
+      createFolder(
+          '1',
+          [
+            createItem('2'),
+            createItem('3'),
+            createItem('4'),
+          ]),
+      createItem('5'),
+    ]));
+
+    action = select(['2', '4', '5'], '4', false);
+    state = bookmarks.SelectionState.updateSelection(state, action);
+
+    action = bookmarks.actions.removeBookmark('1', '0', 0, nodeList);
+    state = bookmarks.SelectionState.updateSelection(state, action);
+
+    assertDeepEquals({5: true}, state.items);
+    assertEquals(null, state.anchor);
+  });
 });
 
 suite('closed folder state', function() {
@@ -146,7 +168,11 @@ suite('selected folder', function() {
 
   setup(function() {
     nodes = testTree(createFolder('1', [
-      createFolder('2', []),
+      createFolder(
+          '2',
+          [
+            createFolder('3', []),
+          ]),
     ]));
 
     state = '1';
@@ -169,6 +195,30 @@ suite('selected folder', function() {
         state, action, nodes);
     assertEquals('1', state);
   });
+
+  test('selects ancestor when selected folder is deleted', function() {
+    action = bookmarks.actions.selectFolder('3');
+    state = bookmarks.SelectedFolderState.updateSelectedFolder(
+        state, action, nodes);
+
+    // Delete the selected folder:
+    action = bookmarks.actions.removeBookmark('3', '2', 0, nodes);
+    state = bookmarks.SelectedFolderState.updateSelectedFolder(
+        state, action, nodes);
+
+    assertEquals('2', state);
+
+    action = bookmarks.actions.selectFolder('3');

[calamity, 2017/04/03 06:54:24]

This is technically selecting a deleted folder, but since we're just looking at
selection state, we don't care. However, this is a bit confusing between state
is used elsewhere to mean the actual store.data 'state'.

I might just take a sledgehammer to this test and rename all the 'state's to the
piece of the state they represent. WDYT?

[tsergeant, 2017/04/04 04:42:28]

Seems like a reasonable change.

In the meantime, I've added a new sibling folder to select here to make it a bit
less confusing.

+    state = bookmarks.SelectedFolderState.updateSelectedFolder(
+        state, action, nodes);
+
+    // Delete an ancestor of the selected folder:
+    action = bookmarks.actions.removeBookmark('2', '1', 0, nodes);
+    state = bookmarks.SelectedFolderState.updateSelectedFolder(
+        state, action, nodes);
+
+    assertEquals('1', state);
+  });
 });
 
 suite('node state', function() {
@@ -215,13 +265,20 @@ suite('node state', function() {
   });
 
   test('updates when a node is deleted', function() {
-    action = bookmarks.actions.removeBookmark('3', '1', 1);
+    action = bookmarks.actions.removeBookmark('3', '1', 1, state);
     state = bookmarks.NodeState.updateNodes(state, action);
 
     assertDeepEquals(['2', '4'], state['1'].children);
 
-    // TODO(tsergeant): Deleted nodes should be removed from the nodes map
-    // entirely.
+    assertDeepEquals(['2', '4'], state['1'].children);
+    assertEquals(undefined, state['3']);
+  });
+
+  test('removes all children of deleted nodes', function() {
+    action = bookmarks.actions.removeBookmark('1', '0', 0, state);
+    state = bookmarks.NodeState.updateNodes(state, action);
+
+    assertDeepEquals(['0', '5'], Object.keys(state).sort());
   });
 
   test('updates when a node is moved', function() {
@@ -305,4 +362,19 @@ suite('search state', function() {
     assertEquals('', selectedState.search.term);
     assertDeepEquals([], selectedState.search.results);
   });
+
+  test('removes deleted nodes', function() {
+    var action;
+
+    action = bookmarks.actions.setSearchTerm('test');
+    state = bookmarks.reduceAction(state, action);
+
+    action = bookmarks.actions.setSearchResults(['1', '3', '2']);
+    state = bookmarks.reduceAction(state, action);
+
+    action = bookmarks.actions.removeBookmark('3', '2', 0, state.nodes);
+    state = bookmarks.reduceAction(state, action);
+
+    assertDeepEquals(['1', '2'], state.search.results);

[calamity, 2017/04/03 06:54:24]

Also delete 1 and ensure 2 vanishes due to being a descendant.

[tsergeant, 2017/04/04 04:42:28]

Done, I changed it to delete 2 and check 2 and 3 vanish in one go.

+  });
 });