From 27bdb96ca1ada5360fbc05758ea71819b0e52637 Mon Sep 17 00:00:00 2001 From: petemill Date: Mon, 7 May 2018 20:23:35 -0700 Subject: [PATCH 1/2] Hide bookmark bar when removing last item Fix #14047 The bookmark bar has always remained 'visible' when removing the last item. It just happened to be reduced to a few pixels tall. This was a defect as it affected the vertical rhythm of the UI. With #13758, the toolbar always remains the set height. With this change, the toolbar is hidden automatically when the last item is removed, just like it already automatically shows when the first item is added. Test Plan: Automated - `npm run unittest -- --grep 'bookmarksReducer'` Manual - Add several bookmark items, including folders - Remove some items - observe the toolbar is still shown - Move some items - observe the toolbar is still shown - Delete all items - observe the toolbar is not shown anymore - Recreate items, and use 'move' to put all items in a folder that is not on the bookmark bar - observe the toolbar is not shown anymore --- .../reducers/bookmarkFoldersReducer.js | 17 +++++++++ app/browser/reducers/bookmarksReducer.js | 16 +++++++++ js/settings.js | 2 +- .../browser/reducers/bookmarksReducerTest.js | 35 ++++++++++++++++--- 4 files changed, 64 insertions(+), 6 deletions(-) diff --git a/app/browser/reducers/bookmarkFoldersReducer.js b/app/browser/reducers/bookmarkFoldersReducer.js index 2c7339fecb8..357b308824b 100644 --- a/app/browser/reducers/bookmarkFoldersReducer.js +++ b/app/browser/reducers/bookmarkFoldersReducer.js @@ -11,11 +11,16 @@ 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 bookmarkFolderUtil = require('../../common/lib/bookmarkFoldersUtil') +const {getSetting} = require('../../../js/settings') const bookmarkFoldersReducer = (state, action, immutableAction) => { action = immutableAction || makeImmutable(action) @@ -82,6 +87,12 @@ 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) + } break } case appConstants.APP_REMOVE_BOOKMARK_FOLDER: @@ -103,6 +114,12 @@ 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) + } break } } diff --git a/app/browser/reducers/bookmarksReducer.js b/app/browser/reducers/bookmarksReducer.js index 62315e60b58..d20f73722c1 100644 --- a/app/browser/reducers/bookmarksReducer.js +++ b/app/browser/reducers/bookmarksReducer.js @@ -10,12 +10,17 @@ 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) @@ -89,6 +94,12 @@ 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) + } break } case appConstants.APP_REMOVE_BOOKMARK: @@ -106,6 +117,11 @@ 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) + } break } } diff --git a/js/settings.js b/js/settings.js index 4eac90fa183..de6d97cd368 100644 --- a/js/settings.js +++ b/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) } diff --git a/test/unit/app/browser/reducers/bookmarksReducerTest.js b/test/unit/app/browser/reducers/bookmarksReducerTest.js index 0f5148a8b5f..d695465593c 100644 --- a/test/unit/app/browser/reducers/bookmarksReducerTest.js +++ b/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,6 +11,8 @@ const fakeElectron = require('../../../lib/fakeElectron') 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') require('../../../braveUnit') @@ -31,7 +33,8 @@ describe('bookmarksReducer unit test', function () { bookmarkLocation: {} }, historySites: {}, - tabs: [] + tabs: [], + settings: {} }) const stateWithData = Immutable.fromJS({ @@ -133,7 +136,10 @@ describe('bookmarksReducer unit test', function () { } }, historySites: {}, - tabs: [] + tabs: [], + settings: { + [settingsConstants.SHOW_BOOKMARKS_TOOLBAR]: true + } }) before(function () { @@ -144,6 +150,7 @@ describe('bookmarksReducer unit test', function () { }) mockery.registerMock('electron', fakeElectron) mockery.registerMock('ad-block', fakeAdBlock) + mockery.registerMock('../../../js/actions/appActions', appActions) bookmarksReducer = require('../../../../../app/browser/reducers/bookmarksReducer') bookmarksState = require('../../../../../app/common/state/bookmarksState') bookmarkLocationCache = require('../../../../../app/common/cache/bookmarkLocationCache') @@ -516,13 +523,19 @@ describe('bookmarksReducer unit test', function () { describe('APP_REMOVE_BOOKMARK', function () { let spy + let onChangeSettingSpy + + beforeEach(function () { + spy = sinon.spy(bookmarksState, 'removeBookmark') + onChangeSettingSpy = sinon.spy(appActions, 'changeSetting') + }) afterEach(function () { spy.restore() + onChangeSettingSpy.restore() }) it('null case', function () { - spy = sinon.spy(bookmarksState, 'removeBookmark') const newState = bookmarksReducer(state, { actionType: appConstants.APP_REMOVE_BOOKMARK }) @@ -531,7 +544,6 @@ describe('bookmarksReducer unit test', function () { }) it('check if delete is working', function () { - spy = sinon.spy(bookmarksState, 'removeBookmark') const newState = bookmarksReducer(stateWithData, { actionType: appConstants.APP_REMOVE_BOOKMARK, bookmarkKey: 'https://clifton.io/|0|0' @@ -548,6 +560,19 @@ describe('bookmarksReducer unit test', function () { .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)) }) }) }) From 9b3379f5a6078b5f9638db41dc88e50cac8b9d83 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 8 May 2018 00:04:08 -0700 Subject: [PATCH 2/2] Refactored bookmarks toolbar closing code + tests Auditors: @petemill --- .../reducers/bookmarkFoldersReducer.js | 20 +---- app/browser/reducers/bookmarksReducer.js | 18 +---- app/common/lib/bookmarkUtil.js | 13 +++- .../reducers/bookmarkFoldersReducerTest.js | 11 +++ .../browser/reducers/bookmarksReducerTest.js | 78 +++++++++++-------- test/unit/app/common/lib/bookmarkUtilTest.js | 50 ++++++++++++ 6 files changed, 125 insertions(+), 65 deletions(-) diff --git a/app/browser/reducers/bookmarkFoldersReducer.js b/app/browser/reducers/bookmarkFoldersReducer.js index 357b308824b..3c728b3e5e4 100644 --- a/app/browser/reducers/bookmarkFoldersReducer.js +++ b/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 } } diff --git a/app/browser/reducers/bookmarksReducer.js b/app/browser/reducers/bookmarksReducer.js index d20f73722c1..135bfed90be 100644 --- a/app/browser/reducers/bookmarksReducer.js +++ b/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 } } diff --git a/app/common/lib/bookmarkUtil.js b/app/common/lib/bookmarkUtil.js index 300ad18fe49..3bec9133119 100644 --- a/app/common/lib/bookmarkUtil.js +++ b/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 } diff --git a/test/unit/app/browser/reducers/bookmarkFoldersReducerTest.js b/test/unit/app/browser/reducers/bookmarkFoldersReducerTest.js index 80a54d9fd92..323df58b85e 100644 --- a/test/unit/app/browser/reducers/bookmarkFoldersReducerTest.js +++ b/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) + }) }) }) diff --git a/test/unit/app/browser/reducers/bookmarksReducerTest.js b/test/unit/app/browser/reducers/bookmarksReducerTest.js index d695465593c..21bb994c46c 100644 --- a/test/unit/app/browser/reducers/bookmarksReducerTest.js +++ b/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 () { @@ -33,8 +33,7 @@ describe('bookmarksReducer unit test', function () { bookmarkLocation: {} }, historySites: {}, - tabs: [], - settings: {} + tabs: [] }) const stateWithData = Immutable.fromJS({ @@ -136,10 +135,7 @@ describe('bookmarksReducer unit test', function () { } }, historySites: {}, - tabs: [], - settings: { - [settingsConstants.SHOW_BOOKMARKS_TOOLBAR]: true - } + tabs: [] }) before(function () { @@ -151,6 +147,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 +519,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 +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()) - 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) }) }) }) diff --git a/test/unit/app/common/lib/bookmarkUtilTest.js b/test/unit/app/common/lib/bookmarkUtilTest.js index 2aca45542a6..1f532138d40 100644 --- a/test/unit/app/common/lib/bookmarkUtilTest.js +++ b/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)) + }) + }) })