Refactored bookmarks toolbar closing code + tests

Auditors: @petemill

app/browser/reducers/bookmarkFoldersReducer.js

@@ -11,16 +11,12 @@ const bookmarkFoldersState = require('../../common/state/bookmarkFoldersState')
 // Constants
 const appConstants = require('../../../js/constants/appConstants')
 const {STATE_SITES} = require('../../../js/constants/stateConstants')
-const settings = require('../../../js/constants/settings')
-
-// Actions
-const appActions = require('../../../js/actions/appActions')
 
 // Utils
 const {makeImmutable} = require('../../common/state/immutableUtil')
 const syncUtil = require('../../../js/state/syncUtil')
+const bookmarkUtil = require('../../common/lib/bookmarkUtil')
 const bookmarkFolderUtil = require('../../common/lib/bookmarkFoldersUtil')
-const {getSetting} = require('../../../js/settings')
 
 const bookmarkFoldersReducer = (state, action, immutableAction) => {
   action = immutableAction || makeImmutable(action)
@@ -87,12 +83,7 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
 
         const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
         state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARK_FOLDERS)
-
-        // close bookmark bar when going to 0
-        const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
-        if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
-          appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
-        }
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
     case appConstants.APP_REMOVE_BOOKMARK_FOLDER:
@@ -114,12 +105,7 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
           state = bookmarkFoldersState.removeFolder(state, folderKey)
           state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
         }
-
-        // close bookmark bar when going to 0
-        const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
-        if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
-          appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
-        }
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
   }

app/browser/reducers/bookmarksReducer.js

@@ -10,17 +10,12 @@ const bookmarksState = require('../../common/state/bookmarksState')
 // Constants
 const appConstants = require('../../../js/constants/appConstants')
 const {STATE_SITES} = require('../../../js/constants/stateConstants')
-const settings = require('../../../js/constants/settings')
-
-// Actions
-const appActions = require('../../../js/actions/appActions')
 
 // Utils
 const {makeImmutable} = require('../../common/state/immutableUtil')
 const syncUtil = require('../../../js/state/syncUtil')
 const bookmarkUtil = require('../../common/lib/bookmarkUtil')
 const bookmarkLocationCache = require('../../common/cache/bookmarkLocationCache')
-const {getSetting} = require('../../../js/settings')
 
 const bookmarksReducer = (state, action, immutableAction) => {
   action = immutableAction || makeImmutable(action)
@@ -94,12 +89,7 @@ const bookmarksReducer = (state, action, immutableAction) => {
 
         const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
         state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARKS)
-
-        // close bookmark bar when going to 0
-        const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
-        if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
-          appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
-        }
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
     case appConstants.APP_REMOVE_BOOKMARK:
@@ -117,11 +107,7 @@ const bookmarksReducer = (state, action, immutableAction) => {
           state = bookmarksState.removeBookmark(state, bookmarkKey)
         }
         state = bookmarkUtil.updateActiveTabBookmarked(state)
-        // close bookmark bar when going to 0
-        const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
-        if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
-          appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
-        }
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
   }

app/common/lib/bookmarkUtil.js

@@ -22,6 +22,9 @@ const {getSetting} = require('../../../js/settings')
 const UrlUtil = require('../../../js/lib/urlutil')
 const {makeImmutable} = require('../state/immutableUtil')
 
+// Actions
+const appActions = require('../../../js/actions/appActions')
+
 const bookmarkHangerHeading = (editMode, isAdded) => {
   if (isAdded) {
     return 'bookmarkAdded'
@@ -206,6 +209,13 @@ const buildEditBookmark = (oldBookmark, bookmarkDetail) => {
   return newBookmark.set('key', newKey)
 }
 
+const closeToolbarIfEmpty = (state) => {
+  const bookmarkBarItemCount = bookmarksState.getBookmarksWithFolders(state, 0).size
+  if (bookmarkBarItemCount === 0 && getSetting(settings.SHOW_BOOKMARKS_TOOLBAR, state.get('settings'))) {
+    appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, false)
+  }
+}
+
 module.exports = {
   bookmarkHangerHeading,
   isBookmarkNameValid,
@@ -220,5 +230,6 @@ module.exports = {
   getKey,
   getBookmarksToolbarMode,
   buildBookmark,
-  buildEditBookmark
+  buildEditBookmark,
+  closeToolbarIfEmpty
 }

test/unit/app/browser/reducers/bookmarkFoldersReducerTest.js

@@ -13,6 +13,7 @@ const fakeAdBlock = require('../../../lib/fakeAdBlock')
 const appConstants = require('../../../../../js/constants/appConstants')
 const siteTags = require('../../../../../js/constants/siteTags')
 const {STATE_SITES} = require('../../../../../js/constants/stateConstants')
+const bookmarkUtil = require('../../../../../app/common/lib/bookmarkUtil')
 require('../../../braveUnit')
 
 describe('bookmarkFoldersReducer unit test', function () {
@@ -126,6 +127,7 @@ describe('bookmarkFoldersReducer unit test', function () {
     })
     mockery.registerMock('electron', fakeElectron)
     mockery.registerMock('ad-block', fakeAdBlock)
+    mockery.registerMock('../../common/lib/bookmarkUtil', bookmarkUtil)
     bookmarkFoldersReducer = require('../../../../../app/browser/reducers/bookmarkFoldersReducer')
     bookmarkFoldersState = require('../../../../../app/common/state/bookmarkFoldersState')
   })
@@ -407,5 +409,14 @@ describe('bookmarkFoldersReducer unit test', function () {
       assert.equal(spy.calledTwice, true)
       assert.deepEqual(newState.toJS(), expectedState.toJS())
     })
+
+    it('calls bookmarkUtil.closeToolbarIfEmpty', function () {
+      spy = sinon.spy(bookmarkUtil, 'closeToolbarIfEmpty')
+      bookmarkFoldersReducer(stateWithData, {
+        actionType: appConstants.APP_REMOVE_BOOKMARK_FOLDER,
+        folderKey: '1'
+      })
+      assert.equal(spy.calledOnce, true)
+    })
   })
 })

test/unit/app/browser/reducers/bookmarksReducerTest.js

@@ -12,8 +12,8 @@ const fakeAdBlock = require('../../../lib/fakeAdBlock')
 
 const appConstants = require('../../../../../js/constants/appConstants')
 const appActions = require('../../../../../js/actions/appActions')
-const settingsConstants = require('../../../../../js/constants/settings')
 const siteTags = require('../../../../../js/constants/siteTags')
+const bookmarkUtil = require('../../../../../app/common/lib/bookmarkUtil')
 require('../../../braveUnit')
 
 describe('bookmarksReducer unit test', function () {
@@ -136,10 +136,7 @@ describe('bookmarksReducer unit test', function () {
       }
     },
     historySites: {},
-    tabs: [],
-    settings: {
-      [settingsConstants.SHOW_BOOKMARKS_TOOLBAR]: true
-    }
+    tabs: []
   })
 
   before(function () {
@@ -151,6 +148,7 @@ describe('bookmarksReducer unit test', function () {
     mockery.registerMock('electron', fakeElectron)
     mockery.registerMock('ad-block', fakeAdBlock)
     mockery.registerMock('../../../js/actions/appActions', appActions)
+    mockery.registerMock('../../common/lib/bookmarkUtil', bookmarkUtil)
     bookmarksReducer = require('../../../../../app/browser/reducers/bookmarksReducer')
     bookmarksState = require('../../../../../app/common/state/bookmarksState')
     bookmarkLocationCache = require('../../../../../app/common/cache/bookmarkLocationCache')
@@ -522,28 +520,45 @@ describe('bookmarksReducer unit test', function () {
   })
 
   describe('APP_REMOVE_BOOKMARK', function () {
-    let spy
-    let onChangeSettingSpy
+    let removeBookmarkSpy
+    let updateActiveTabBookmarkedSpy
+    let closeToolbarIfEmptySpy
 
     beforeEach(function () {
-      spy = sinon.spy(bookmarksState, 'removeBookmark')
-      onChangeSettingSpy = sinon.spy(appActions, 'changeSetting')
+      removeBookmarkSpy = sinon.spy(bookmarksState, 'removeBookmark')
+      updateActiveTabBookmarkedSpy = sinon.spy(bookmarkUtil, 'updateActiveTabBookmarked')
+      closeToolbarIfEmptySpy = sinon.spy(bookmarkUtil, 'closeToolbarIfEmpty')
     })
 
     afterEach(function () {
-      spy.restore()
-      onChangeSettingSpy.restore()
+      removeBookmarkSpy.restore()
+      updateActiveTabBookmarkedSpy.restore()
+      closeToolbarIfEmptySpy.restore()
     })
 
-    it('null case', function () {
-      const newState = bookmarksReducer(state, {
-        actionType: appConstants.APP_REMOVE_BOOKMARK
+    describe('when bookmarkKey is null', function () {
+      it('does not call removeBookmark', function () {
+        const newState = bookmarksReducer(state, {
+          actionType: appConstants.APP_REMOVE_BOOKMARK
+        })
+        assert.equal(removeBookmarkSpy.notCalled, true)
+        assert.deepEqual(state, newState)
       })
-      assert.equal(spy.notCalled, true)
-      assert.deepEqual(state, newState)
     })
 
-    it('check if delete is working', function () {
+    describe('when bookmarkKey is a list', function () {
+      // TODO: test that removeBookmark is called multiple times
+    })
+
+    it('calls bookmarksState.removeBookmark', function () {
+      bookmarksReducer(stateWithData, {
+        actionType: appConstants.APP_REMOVE_BOOKMARK,
+        bookmarkKey: 'https://clifton.io/|0|0'
+      })
+      assert.equal(removeBookmarkSpy.calledOnce, true)
+    })
+
+    it('deletes the entry from bookmarks and cache', function () {
       const newState = bookmarksReducer(stateWithData, {
         actionType: appConstants.APP_REMOVE_BOOKMARK,
         bookmarkKey: 'https://clifton.io/|0|0'
@@ -558,21 +573,23 @@ describe('bookmarksReducer unit test', function () {
         ]))
         .deleteIn(['bookmarks', 'https://clifton.io/|0|0'])
         .deleteIn(['cache', 'bookmarkLocation', 'https://clifton.io/'])
-      assert.equal(spy.calledOnce, true)
       assert.deepEqual(newState.toJS(), expectedState.toJS())
-      console.log('settings', newState.get('settings').toJS(), newState.getIn(['settings', settingsConstants.SHOW_BOOKMARKS_TOOLBAR]))
-      assert.ok(onChangeSettingSpy.neverCalledWith(settingsConstants.SHOW_BOOKMARKS_TOOLBAR, false), 'bookmarks toolbar enabled setting is unaffected by removing 1 bookmark')
     })
 
-    it('hides bookmarks toolbar when all toolbar bookmarks are removed', function () {
-      let newState = stateWithData
-      for (const bookmarkKey of stateWithData.get('bookmarks').keys()) {
-        newState = bookmarksReducer(newState, {
-          actionType: appConstants.APP_REMOVE_BOOKMARK,
-          bookmarkKey
-        })
-      }
-      assert.ok(onChangeSettingSpy.calledWith(settingsConstants.SHOW_BOOKMARKS_TOOLBAR, false))
+    it('calls bookmarkUtil.updateActiveTabBookmarked', function () {
+      bookmarksReducer(stateWithData, {
+        actionType: appConstants.APP_REMOVE_BOOKMARK,
+        bookmarkKey: 'https://clifton.io/|0|0'
+      })
+      assert.equal(updateActiveTabBookmarkedSpy.calledOnce, true)
+    })
+
+    it('calls bookmarkUtil.closeToolbarIfEmpty', function () {
+      bookmarksReducer(stateWithData, {
+        actionType: appConstants.APP_REMOVE_BOOKMARK,
+        bookmarkKey: 'https://clifton.io/|0|0'
+      })
+      assert.equal(closeToolbarIfEmptySpy.calledOnce, true)
     })
   })
 })

test/unit/app/common/lib/bookmarkUtilTest.js

@@ -9,8 +9,11 @@ const Immutable = require('immutable')
 const {makeImmutable} = require('../../../../../app/common/state/immutableUtil')
 const {bookmarksToolbarMode} = require('../../../../../app/common/constants/settingsEnums')
 const dragTypes = require('../../../../../js/constants/dragTypes')
+const appActions = require('../../../../../js/actions/appActions')
+const settingsConstants = require('../../../../../js/constants/settings')
 const siteTags = require('../../../../../js/constants/siteTags')
 const tabState = require('../../../../../app/common/state/tabState')
+const bookmarksState = require('../../../../../app/common/state/bookmarksState')
 const sinon = require('sinon')
 
 require('../../../braveUnit')
@@ -121,6 +124,8 @@ describe('bookmarkUtil unit test', function () {
       useCleanCache: true
     })
     mockery.registerMock('../state/tabState', tabState)
+    mockery.registerMock('../../../js/actions/appActions', appActions)
+    // kind of a sloppy hack; set `settingDefaultValue` to value you'd like returned
     mockery.registerMock('../../../js/settings', {
       getSetting: () => {
         return settingDefaultValue
@@ -583,4 +588,49 @@ describe('bookmarkUtil unit test', function () {
       assert.deepEqual(bookmarkUtil.buildEditBookmark(oldBookmark, newBookmark).toJS(), expectedBookmark.toJS())
     })
   })
+
+  describe('closeToolbarIfEmpty', function () {
+    let onChangeSettingSpy
+
+    const removeAllBookmarks = (stateWithBookmarks) => {
+      let newState = stateWithBookmarks
+      for (const bookmarkKey of stateWithBookmarks.get('bookmarks').keys()) {
+        newState = bookmarksState.removeBookmark(newState, bookmarkKey)
+      }
+      return newState
+    }
+
+    beforeEach(function () {
+      onChangeSettingSpy = sinon.spy(appActions, 'changeSetting')
+      settingDefaultValue = true
+    })
+
+    afterEach(function () {
+      onChangeSettingSpy.restore()
+    })
+
+    describe('when SHOW_BOOKMARKS_TOOLBAR is false', function () {
+      beforeEach(function () {
+        settingDefaultValue = false
+      })
+
+      it('does not call appActions.changeSetting if bookmarks toolbar is hidden', function () {
+        let newState = removeAllBookmarks(stateWithData)
+        bookmarkUtil.closeToolbarIfEmpty(newState)
+        assert.equal(onChangeSettingSpy.notCalled, true)
+      })
+    })
+
+    it('does not hide bookmarks toolbar if bookmarks still exist', function () {
+      let newState = bookmarksState.removeBookmark(stateWithData, 'https://brave.com/|0|0')
+      bookmarkUtil.closeToolbarIfEmpty(newState)
+      assert.equal(onChangeSettingSpy.notCalled, true)
+    })
+
+    it('hides bookmarks toolbar when all toolbar bookmarks are removed', function () {
+      let newState = removeAllBookmarks(stateWithData)
+      bookmarkUtil.closeToolbarIfEmpty(newState)
+      assert.ok(onChangeSettingSpy.calledWith(settingsConstants.SHOW_BOOKMARKS_TOOLBAR, false))
+    })
+  })
 })