From 6159ba015fca4511fb46ce9bb7109e24c78ae869 Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Tue, 5 Sep 2017 17:06:50 +0200 Subject: [PATCH] Removes history and bookmarks when they are removed from suggestions Resolves #9736 Auditors: Test Plan: --- app/browser/reducers/historyReducer.js | 11 ++ .../reducers/urlBarSuggestionsReducer.js | 65 ++++++- app/common/lib/siteSuggestions.js | 10 +- app/common/state/historyState.js | 5 + js/stores/appStore.js | 2 +- .../reducers/urlBarSuggestionsReducerTest.js | 175 ++++++++++++++++-- 6 files changed, 251 insertions(+), 17 deletions(-) diff --git a/app/browser/reducers/historyReducer.js b/app/browser/reducers/historyReducer.js index 57ca841c711..c799358e9ac 100644 --- a/app/browser/reducers/historyReducer.js +++ b/app/browser/reducers/historyReducer.js @@ -16,9 +16,11 @@ const messages = require('../../../js/constants/messages') // Utils const {makeImmutable} = require('../../common/state/immutableUtil') +const {remove} = require('../../common/lib/siteSuggestions') const syncUtil = require('../../../js/state/syncUtil') const filtering = require('../../filtering') const {calculateTopSites} = require('../api/topSites') +const bookmarkLocationCache = require('../../common/cache/bookmarkLocationCache') /** * Helper to pass message to windows to clear closed frames @@ -43,6 +45,15 @@ const historyReducer = (state, action, immutableAction) => { const temp = state.get('tempClearBrowsingData', Immutable.Map()) const clearData = defaults ? defaults.merge(temp) : temp if (clearData.get('browserHistory')) { + let historyList = Immutable.List() + historyState.getSites(state).forEach((site) => { + const bookmarkKey = bookmarkLocationCache.getCacheKey(state, site.get('location')) + if (bookmarkKey.size === 0) { + historyList = historyList.push(site) + } + }) + + remove(historyList) state = historyState.clearSites(state) state = aboutHistoryState.clearHistory(state) filtering.clearHistory() diff --git a/app/browser/reducers/urlBarSuggestionsReducer.js b/app/browser/reducers/urlBarSuggestionsReducer.js index df0bba9eee4..ceb3011bcb5 100644 --- a/app/browser/reducers/urlBarSuggestionsReducer.js +++ b/app/browser/reducers/urlBarSuggestionsReducer.js @@ -7,11 +7,13 @@ const Immutable = require('immutable') const appConstants = require('../../../js/constants/appConstants') const {generateNewSuggestionsList, generateNewSearchXHRResults} = require('../../common/lib/suggestion') -const {init, add} = require('../../common/lib/siteSuggestions') +const {init, add, remove} = require('../../common/lib/siteSuggestions') const {makeImmutable} = require('../../common/state/immutableUtil') const tabState = require('../../common/state/tabState') const historyState = require('../../common/state/historyState') const bookmarksState = require('../../common/state/bookmarksState') +const historyUtil = require('../../common/lib/historyUtil') +const bookmarkLocationCache = require('../../common/cache/bookmarkLocationCache') const urlBarSuggestionsReducer = (state, action) => { switch (action.actionType) { @@ -26,6 +28,67 @@ const urlBarSuggestionsReducer = (state, action) => { add(makeImmutable(action.siteDetail)) } break + case appConstants.APP_REMOVE_BOOKMARK: + { + const bookmarkKey = action.bookmarkKey + if (bookmarkKey == null) { + break + } + + let data = null + if (Array.isArray(bookmarkKey)) { + data = Immutable.List() + bookmarkKey.forEach((key) => { + const bookmark = bookmarksState.getBookmark(state, key) + const historyKey = historyUtil.getKey(bookmark) + if (!historyState.hasSite(state, historyKey)) { + data = data.push(bookmark) + } + }) + } else { + const bookmark = bookmarksState.getBookmark(state, bookmarkKey) + const historyKey = historyUtil.getKey(bookmark) + if (!historyState.hasSite(state, historyKey)) { + data = bookmark + } + } + + if (data != null) { + remove(data) + } + break + } + + case appConstants.APP_REMOVE_HISTORY_SITE: + { + const historyKey = action.historyKey + if (historyKey == null) { + break + } + + let data = null + if (Array.isArray(historyKey)) { + data = Immutable.List() + historyKey.map((key) => { + const site = historyState.getSite(state, key) + const bookmarkKey = bookmarkLocationCache.getCacheKey(state, site.get('location')) + if (bookmarkKey.size === 0) { + data = data.push(site) + } + }) + } else { + const site = historyState.getSite(state, historyKey) + const bookmarkKey = bookmarkLocationCache.getCacheKey(state, site.get('location')) + if (bookmarkKey.size === 0) { + data = site + } + } + + if (data != null) { + remove(data) + } + break + } case appConstants.APP_SET_STATE: const bookmarks = bookmarksState.getBookmarks(action.appState) const history = historyState.getSites(action.appState) diff --git a/app/common/lib/siteSuggestions.js b/app/common/lib/siteSuggestions.js index 2fb837c7add..03fd6a775fc 100644 --- a/app/common/lib/siteSuggestions.js +++ b/app/common/lib/siteSuggestions.js @@ -135,6 +135,13 @@ const add = (data) => { engine.add(data.toJS ? data : Immutable.fromJS(data)) } +const remove = (data) => { + if (!initialized) { + return + } + engine.remove(data.toJS ? data : Immutable.fromJS(data)) +} + const query = (input, options = {}) => { if (!initialized) { return Promise.resolve([]) @@ -163,5 +170,6 @@ module.exports = { init, add, tokenizeInput, - query + query, + remove } diff --git a/app/common/state/historyState.js b/app/common/state/historyState.js index 3185c59d229..4dd8cf41401 100644 --- a/app/common/state/historyState.js +++ b/app/common/state/historyState.js @@ -27,6 +27,11 @@ const historyState = { return state.getIn([STATE_SITES.HISTORY_SITES, key], Immutable.Map()) }, + hasSite: (state, key) => { + state = validateState(state) + return state.hasIn([STATE_SITES.HISTORY_SITES, key], Immutable.Map()) + }, + addSite: (state, siteDetail) => { let sites = historyState.getSites(state) let siteKey = historyUtil.getKey(siteDetail) diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 13a05b0d784..d9b3a647202 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -178,6 +178,7 @@ const handleAppAction = (action) => { // until we have a better way to manage dependencies. // tabsReducer must be above dragDropReducer. require('../../app/browser/reducers/tabsReducer'), + require('../../app/browser/reducers/urlBarSuggestionsReducer'), require('../../app/browser/reducers/bookmarksReducer'), require('../../app/browser/reducers/bookmarkFoldersReducer'), require('../../app/browser/reducers/historyReducer'), @@ -185,7 +186,6 @@ const handleAppAction = (action) => { require('../../app/browser/reducers/windowsReducer'), require('../../app/browser/reducers/syncReducer'), require('../../app/browser/reducers/clipboardReducer'), - require('../../app/browser/reducers/urlBarSuggestionsReducer'), require('../../app/browser/reducers/passwordManagerReducer'), require('../../app/browser/reducers/spellCheckerReducer'), require('../../app/browser/reducers/tabMessageBoxReducer'), diff --git a/test/unit/app/browser/reducers/urlBarSuggestionsReducerTest.js b/test/unit/app/browser/reducers/urlBarSuggestionsReducerTest.js index d0c8254f617..e69fcdce9a7 100644 --- a/test/unit/app/browser/reducers/urlBarSuggestionsReducerTest.js +++ b/test/unit/app/browser/reducers/urlBarSuggestionsReducerTest.js @@ -8,8 +8,22 @@ const appConstants = require('../../../../../js/constants/appConstants') require('../../../braveUnit') const siteTags = require('../../../../../js/constants/siteTags') -describe('urlBarSuggestionsReducer', function () { +describe('urlBarSuggestionsReducer unit test', function () { let urlBarSuggestionsReducer + + const site1 = { + location: 'https://brave.com', + type: siteTags.BOOKMARK, + partitionNumber: 0 + } + + const initState = Immutable.fromJS({ + bookmarks: { + 'key': site1 + }, + historySites: {} + }) + before(function () { mockery.enable({ warnOnReplace: false, @@ -19,7 +33,8 @@ describe('urlBarSuggestionsReducer', function () { this.siteSuggestionsStub = { init: sinon.stub(), - add: sinon.stub() + add: sinon.stub(), + remove: sinon.stub() } mockery.registerMock('../../common/lib/siteSuggestions', this.siteSuggestionsStub) @@ -39,22 +54,11 @@ describe('urlBarSuggestionsReducer', function () { afterEach(function () { this.siteSuggestionsStub.init.reset() this.siteSuggestionsStub.add.reset() + this.siteSuggestionsStub.remove.reset() this.suggestionStub.generateNewSuggestionsList.reset() this.suggestionStub.generateNewSearchXHRResults.reset() }) - const site1 = { - location: 'https://brave.com', - type: siteTags.BOOKMARK - } - - const initState = Immutable.fromJS({ - bookmarks: { - 'key': site1 - }, - historySites: {} - }) - describe('APP_SET_STATE', function () { it('inits the suggestions lib with sites', function () { urlBarSuggestionsReducer(Immutable.Map(), {actionType: appConstants.APP_SET_STATE, appState: initState}) @@ -74,6 +78,149 @@ describe('urlBarSuggestionsReducer', function () { assert.deepEqual(newState, initState) }) }) + describe('APP_REMOVE_BOOKMARK', function () { + let customState + + before(function () { + customState = initState + .setIn(['bookmarks', '1'], Immutable.fromJS({ + location: 'https://brianbondy.com', + type: siteTags.BOOKMARK, + partitionNumber: 0 + })) + .setIn(['bookmarks', '2'], Immutable.fromJS({ + location: 'https://clifton.io', + type: siteTags.BOOKMARK, + partitionNumber: 0 + })) + .setIn(['historySites', 'https://brianbondy.com|0'], Immutable.fromJS({ + location: 'https://brianbondy.com', + partitionNumber: 0 + })) + }) + + it('null case', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_BOOKMARK + }) + assert.equal(this.siteSuggestionsStub.remove.notCalled, true) + }) + + it('bookmark key is list (multiple bookmarks)', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_BOOKMARK, + bookmarkKey: [ + '2', + 'key' + ] + }) + const argument = Immutable.List() + .push(customState.getIn(['bookmarks', '2'])) + .push(customState.getIn(['bookmarks', 'key'])) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + + it('bookmark key is map (single bookmark)', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_BOOKMARK, + bookmarkKey: '2' + }) + const argument = customState.getIn(['bookmarks', '2']) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + + it('bookmark key is map and history with the same site exists', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_BOOKMARK, + bookmarkKey: '1' + }) + assert.equal(this.siteSuggestionsStub.remove.notCalled, true) + }) + + it('bookmark key is list and history with the same site exists', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_BOOKMARK, + bookmarkKey: [ + '1', + 'key' + ] + }) + const argument = Immutable.List() + .push(customState.getIn(['bookmarks', 'key'])) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + }) + + describe('APP_REMOVE_HISTORY_SITE', function () { + let customState + + before(function () { + customState = initState + .setIn(['historySites', '1'], Immutable.fromJS({ + location: 'https://brianbondy.com' + })) + .setIn(['historySites', '2'], Immutable.fromJS({ + location: 'https://clifton.io' + })) + .setIn(['historySites', '3'], Immutable.fromJS({ + location: 'https://brave.com' + })) + .setIn(['cache', 'bookmarkLocation', 'https://brave.com'], Immutable.fromJS([ + 'https://brave.com|0|0' + ])) + }) + + it('null case', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_HISTORY_SITE + }) + assert.equal(this.siteSuggestionsStub.remove.notCalled, true) + }) + + it('history key is list (multiple history sites)', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_HISTORY_SITE, + historyKey: [ + '1', + '2' + ] + }) + const argument = Immutable.List() + .push(customState.getIn(['historySites', '1'])) + .push(customState.getIn(['historySites', '2'])) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + + it('history key is map (single history site)', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_HISTORY_SITE, + historyKey: '1' + }) + const argument = customState.getIn(['historySites', '1']) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + + it('history key is list and bookmark with the same site exists', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_HISTORY_SITE, + historyKey: [ + '1', + '3' + ] + }) + const argument = Immutable.List() + .push(customState.getIn(['historySites', '1'])) + assert.deepEqual(this.siteSuggestionsStub.remove.args[0][0], argument) + }) + + it('history key is map and bookmark with the same site exists', function () { + urlBarSuggestionsReducer(customState, { + actionType: appConstants.APP_REMOVE_HISTORY_SITE, + historyKey: '3' + }) + assert.equal(this.siteSuggestionsStub.remove.notCalled, true) + }) + }) describe('APP_ADD_HISTORY_SITE', function () { it('adds a site in the suggestions lib', function () { const newState = urlBarSuggestionsReducer(initState, {actionType: appConstants.APP_ADD_HISTORY_SITE, siteDetail: site1})