diff --git a/app/browser/reducers/bookmarkFoldersReducer.js b/app/browser/reducers/bookmarkFoldersReducer.js index 2c7339fecb8..3c728b3e5e4 100644 --- a/app/browser/reducers/bookmarkFoldersReducer.js +++ b/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 } } diff --git a/app/browser/reducers/bookmarksReducer.js b/app/browser/reducers/bookmarksReducer.js index 62315e60b58..135bfed90be 100644 --- a/app/browser/reducers/bookmarksReducer.js +++ b/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 } } 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/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/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 0f5148a8b5f..21bb994c46c 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,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) + }) }) }) 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)) + }) + }) })