Merge pull request #14048 from brave/fix/14047-hide-bookmarks-bar-no-…

…items

Hide bookmark bar when removing last item

app/browser/reducers/bookmarkFoldersReducer.js

@@ -15,6 +15,7 @@ const {STATE_SITES} = require('../../../js/constants/stateConstants')
 // 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 bookmarkFoldersReducer = (state, action, immutableAction) => {
@@ -82,6 +83,7 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
 
         const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
         state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARK_FOLDERS)
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
     case appConstants.APP_REMOVE_BOOKMARK_FOLDER:
@@ -103,6 +105,7 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
           state = bookmarkFoldersState.removeFolder(state, folderKey)
           state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
         }
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
   }

app/browser/reducers/bookmarksReducer.js

@@ -89,6 +89,7 @@ const bookmarksReducer = (state, action, immutableAction) => {
 
         const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
         state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARKS)
+        bookmarkUtil.closeToolbarIfEmpty(state)
         break
       }
     case appConstants.APP_REMOVE_BOOKMARK:
@@ -106,6 +107,7 @@ const bookmarksReducer = (state, action, immutableAction) => {
           state = bookmarksState.removeBookmark(state, bookmarkKey)
         }
         state = bookmarkUtil.updateActiveTabBookmarked(state)
+        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
 }

js/settings.js

@@ -61,7 +61,7 @@ const getDefaultSetting = (settingKey, settingsCollection) => {
 }
 
 const resolveValue = (settingKey, settingsCollection) => {
-  if (settingsCollection && settingsCollection.constructor === Immutable.Map &&
+  if (settingsCollection && Immutable.Map.isMap(settingsCollection) &&
     settingsCollection.get(settingKey) !== undefined) {
     return settingsCollection.get(settingKey)
   }

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

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* global describe, it, before, after, afterEach */
+/* global describe, it, before, after, beforeEach, afterEach */
 const mockery = require('mockery')
 const Immutable = require('immutable')
 const assert = require('assert')
@@ -11,7 +11,9 @@ const fakeElectron = require('../../../lib/fakeElectron')
 const fakeAdBlock = require('../../../lib/fakeAdBlock')
 
 const appConstants = require('../../../../../js/constants/appConstants')
+const appActions = require('../../../../../js/actions/appActions')
 const siteTags = require('../../../../../js/constants/siteTags')
+const bookmarkUtil = require('../../../../../app/common/lib/bookmarkUtil')
 require('../../../braveUnit')
 
 describe('bookmarksReducer unit test', function () {
@@ -144,6 +146,8 @@ 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')
@@ -515,23 +519,45 @@ describe('bookmarksReducer unit test', function () {
   })
 
   describe('APP_REMOVE_BOOKMARK', function () {
-    let spy
+    let removeBookmarkSpy
+    let updateActiveTabBookmarkedSpy
+    let closeToolbarIfEmptySpy
+
+    beforeEach(function () {
+      removeBookmarkSpy = sinon.spy(bookmarksState, 'removeBookmark')
+      updateActiveTabBookmarkedSpy = sinon.spy(bookmarkUtil, 'updateActiveTabBookmarked')
+      closeToolbarIfEmptySpy = sinon.spy(bookmarkUtil, 'closeToolbarIfEmpty')
+    })
 
     afterEach(function () {
-      spy.restore()
+      removeBookmarkSpy.restore()
+      updateActiveTabBookmarkedSpy.restore()
+      closeToolbarIfEmptySpy.restore()
     })
 
-    it('null case', function () {
-      spy = sinon.spy(bookmarksState, 'removeBookmark')
-      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 () {
-      spy = sinon.spy(bookmarksState, 'removeBookmark')
+    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'
@@ -546,8 +572,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())
     })
+
+    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))
+    })
+  })
 })