From 729970f743bb814eb49b4d833bca663d968d7b05 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Wed, 2 Nov 2016 17:49:17 -0700 Subject: [PATCH 1/2] Created session helper for newTabState - site will only be added if it's a bookmark or history item - site will only be removed if it's a bookmark or history item - site moves are now ignored - favicon is now updated for the newTabState (which is a different list than AppState.sites) Fixes https://github.com/brave/browser-laptop/issues/5357 Fixes https://github.com/brave/browser-laptop/issues/5310 Auditors: @bridiver - check out the new session helper; hope I did this right :) @darkdh - this fixes the folder delete issue by ignoring folders @cezaraugusto - move is no longer tracked (which is fine; we don't care about the order of the items with regard to their position in bookmarks toolbar, which is what moveSite is for). Also, see the new session helper (which we can use for ordering by # visits) Test Plan: 1. Launch Brave and visit brave.com 2. Open a new tab (about:newtab) and confirm you see brave.com there (favicon should show also) 3. Visit about:history and delete the visit you just made to brave.com 4. Confirm it's now gone in about:newtab --- app/common/state/newTabState.js | 86 +++++++++++++++++++++++++++++++++ js/stores/appStore.js | 26 ++-------- 2 files changed, 91 insertions(+), 21 deletions(-) create mode 100644 app/common/state/newTabState.js diff --git a/app/common/state/newTabState.js b/app/common/state/newTabState.js new file mode 100644 index 00000000000..e1dde7ccf8a --- /dev/null +++ b/app/common/state/newTabState.js @@ -0,0 +1,86 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * 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/. */ + +const Immutable = require('immutable') +const {makeImmutable} = require('./immutableUtil') +const siteUtil = require('../../../js/state/siteUtil') + +const excludeSiteDetail = (siteDetail) => { + return !siteUtil.isBookmark(siteDetail) && !siteUtil.isHistoryEntry(siteDetail) +} + +const removeDuplicateSites = (sites) => { + // Filter out duplicate entries by location + return sites.filter((element, index, list) => { + if (!element) return false + return index === list.findIndex((site) => site && site.get('location') === element.get('location')) + }) +} + +const newTabState = { + mergeDetails: (state, props) => { + if (!props) { + return state + } + + state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail) + return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + }, + + addSite: (state, props) => { + if (!props) { + return state + } + + // Add timestamp if missing (ex: this is a visit, not a bookmark) + let siteDetail = makeImmutable(props.siteDetail) + siteDetail = siteDetail.set('lastAccessedTime', siteDetail.get('lastAccessedTime') || new Date().getTime()) + + // Only bookmarks and history items should be considered + if (excludeSiteDetail(siteDetail)) { + return state + } + + // Keep track of the last 18 visited sites + let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List() + sites = sites.unshift(siteDetail) + sites = removeDuplicateSites(sites) + sites = sites.take(18) + // TODO(cezaraugusto): Sort should respect unshift and don't prioritize bookmarks + // | + // V + // .sort(suggestion.sortByAccessCountWithAgeDecay) + sites = siteUtil.addSite(sites, siteDetail, props.tag, props.originalSiteDetail) + state = state.setIn(['about', 'newtab', 'sites'], sites) + return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + }, + + removeSite: (state, props) => { + if (!props) { + return state + } + + // Only bookmarks and history items should be considered + if (excludeSiteDetail(props.siteDetail)) { + return state + } + + const sites = state.getIn(['about', 'newtab', 'sites']) + state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, props.siteDetail, props.tag)) + return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + }, + + updateSiteFavicon: (state, props) => { + if (!props) { + return state + } + + const sites = state.getIn(['about', 'newtab', 'sites']) + const sitesWithFavicon = siteUtil.updateSiteFavicon(sites, props.frameProps.get('location'), props.favicon) + state = state.setIn(['about', 'newtab', 'sites'], sitesWithFavicon) + return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + } +} + +module.exports = newTabState diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 089ffce1da6..9e26ccbaa25 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -33,12 +33,12 @@ const path = require('path') const {channel} = require('../../app/channel') const os = require('os') const autofill = require('../../app/autofill') -// const suggestion = require('../lib/suggestion') // state helpers const basicAuthState = require('../../app/common/state/basicAuthState') const extensionState = require('../../app/common/state/extensionState') const tabState = require('../../app/common/state/tabState') +const newTabState = require('../../app/common/state/newTabState') const isDarwin = process.platform === 'darwin' const isWindows = process.platform === 'win32' @@ -430,8 +430,7 @@ const handleAppAction = (action) => { appState = appState.set('passwords', new Immutable.List()) break case AppConstants.APP_CHANGE_NEW_TAB_DETAIL: - appState = appState.mergeIn(['about', 'newtab'], action.newTabPageDetail) - appState = appState.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + appState = newTabState.mergeDetails(appState, action) break case AppConstants.APP_ADD_SITE: const oldSiteSize = appState.get('sites').size @@ -449,30 +448,14 @@ const handleAppAction = (action) => { if (oldSiteSize !== appState.get('sites').size) { filterOutNonRecents() } - let newVisitedSites = appState.getIn(['about', 'newtab', 'sites']) || new Immutable.List() - newVisitedSites = newVisitedSites.unshift(action.siteDetail) - // Filter duplicated entries by its location - newVisitedSites = newVisitedSites.filter((element, index, list) => { - if (!element) return false - return index === list.findIndex((site) => site && site.get('location') === element.get('location')) - }) - newVisitedSites = newVisitedSites.take(18) - // TODO: @cezaraugusto. - // Sort should respect unshift and don't prioritize bookmarks - // | - // V - // .sort(suggestion.sortByAccessCountWithAgeDecay) - - appState = appState.setIn(['about', 'newtab', 'sites'], siteUtil.addSite(newVisitedSites, action.siteDetail, action.tag, action.originalSiteDetail)) - appState = appState.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + appState = newTabState.addSite(appState, action) break case AppConstants.APP_REMOVE_SITE: appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag)) - appState = appState.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(appState.getIn(['about', 'newtab', 'sites']), action.siteDetail, action.tag)) + appState = newTabState.removeSite(appState, action) break case AppConstants.APP_MOVE_SITE: appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.sourceDetail, action.destinationDetail, action.prepend, action.destinationIsParent, false)) - appState = appState.setIn(['about', 'newtab', 'sites'], siteUtil.moveSite(appState.getIn(['about', 'newtab', 'sites']), action.sourceDetail, action.destinationDetail, action.prepend, action.destinationIsParent, false)) break case AppConstants.APP_MERGE_DOWNLOAD_DETAIL: if (action.downloadDetail) { @@ -792,6 +775,7 @@ const handleAppAction = (action) => { break case WindowConstants.WINDOW_SET_FAVICON: appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon)) + appState = newTabState.updateSiteFavicon(appState, action) break case WindowConstants.WINDOW_SET_NAVIGATED: if (!action.isNavigatedInPage) { From a0e182036d190ba4e1c9d8f26d1228758d837faa Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 3 Nov 2016 18:17:51 -0700 Subject: [PATCH 2/2] Added test cases for all methods Auditor: @bridiver --- .../{newTabState.js => aboutNewTabState.js} | 28 ++- js/stores/appStore.js | 10 +- .../unit/common/state/aboutNewTabStateTest.js | 192 ++++++++++++++++++ 3 files changed, 218 insertions(+), 12 deletions(-) rename app/common/state/{newTabState.js => aboutNewTabState.js} (73%) create mode 100644 test/unit/common/state/aboutNewTabStateTest.js diff --git a/app/common/state/newTabState.js b/app/common/state/aboutNewTabState.js similarity index 73% rename from app/common/state/newTabState.js rename to app/common/state/aboutNewTabState.js index e1dde7ccf8a..4e7acd36922 100644 --- a/app/common/state/newTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -18,8 +18,9 @@ const removeDuplicateSites = (sites) => { }) } -const newTabState = { +const aboutNewTabState = { mergeDetails: (state, props) => { + state = makeImmutable(state) if (!props) { return state } @@ -29,6 +30,7 @@ const newTabState = { }, addSite: (state, props) => { + state = makeImmutable(state) if (!props) { return state } @@ -42,6 +44,10 @@ const newTabState = { return state } + // Remove tags since we've verified this is a bookmark/history item + // NOTE: siteUtil.removeSite won't delete the entry unless tags are missing + siteDetail = siteDetail.delete('tags') + // Keep track of the last 18 visited sites let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List() sites = sites.unshift(siteDetail) @@ -51,36 +57,44 @@ const newTabState = { // | // V // .sort(suggestion.sortByAccessCountWithAgeDecay) - sites = siteUtil.addSite(sites, siteDetail, props.tag, props.originalSiteDetail) + sites = siteUtil.addSite(sites, siteDetail, undefined, props.originalSiteDetail) state = state.setIn(['about', 'newtab', 'sites'], sites) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) }, removeSite: (state, props) => { + state = makeImmutable(state) if (!props) { return state } // Only bookmarks and history items should be considered - if (excludeSiteDetail(props.siteDetail)) { + let siteDetail = makeImmutable(props.siteDetail) + if (excludeSiteDetail(siteDetail)) { return state } + // Remove tags since we've verified this is a bookmark/history item + // NOTE: siteUtil.removeSite won't delete the entry unless tags are missing + siteDetail = siteDetail.delete('tags') + const sites = state.getIn(['about', 'newtab', 'sites']) - state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, props.siteDetail, props.tag)) + state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, siteDetail, undefined)) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) }, updateSiteFavicon: (state, props) => { - if (!props) { + state = makeImmutable(state) + props = makeImmutable(props) + if (!props || !props.get('frameProps') || !props.getIn(['frameProps', 'location'])) { return state } const sites = state.getIn(['about', 'newtab', 'sites']) - const sitesWithFavicon = siteUtil.updateSiteFavicon(sites, props.frameProps.get('location'), props.favicon) + const sitesWithFavicon = siteUtil.updateSiteFavicon(sites, props.getIn(['frameProps', 'location']), props.get('favicon')) state = state.setIn(['about', 'newtab', 'sites'], sitesWithFavicon) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) } } -module.exports = newTabState +module.exports = aboutNewTabState diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 9e26ccbaa25..1dff646f1dc 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -38,7 +38,7 @@ const autofill = require('../../app/autofill') const basicAuthState = require('../../app/common/state/basicAuthState') const extensionState = require('../../app/common/state/extensionState') const tabState = require('../../app/common/state/tabState') -const newTabState = require('../../app/common/state/newTabState') +const aboutNewTabState = require('../../app/common/state/aboutNewTabState') const isDarwin = process.platform === 'darwin' const isWindows = process.platform === 'win32' @@ -430,7 +430,7 @@ const handleAppAction = (action) => { appState = appState.set('passwords', new Immutable.List()) break case AppConstants.APP_CHANGE_NEW_TAB_DETAIL: - appState = newTabState.mergeDetails(appState, action) + appState = aboutNewTabState.mergeDetails(appState, action) break case AppConstants.APP_ADD_SITE: const oldSiteSize = appState.get('sites').size @@ -448,11 +448,11 @@ const handleAppAction = (action) => { if (oldSiteSize !== appState.get('sites').size) { filterOutNonRecents() } - appState = newTabState.addSite(appState, action) + appState = aboutNewTabState.addSite(appState, action) break case AppConstants.APP_REMOVE_SITE: appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag)) - appState = newTabState.removeSite(appState, action) + appState = aboutNewTabState.removeSite(appState, action) break case AppConstants.APP_MOVE_SITE: appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.sourceDetail, action.destinationDetail, action.prepend, action.destinationIsParent, false)) @@ -775,7 +775,7 @@ const handleAppAction = (action) => { break case WindowConstants.WINDOW_SET_FAVICON: appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon)) - appState = newTabState.updateSiteFavicon(appState, action) + appState = aboutNewTabState.updateSiteFavicon(appState, action) break case WindowConstants.WINDOW_SET_NAVIGATED: if (!action.isNavigatedInPage) { diff --git a/test/unit/common/state/aboutNewTabStateTest.js b/test/unit/common/state/aboutNewTabStateTest.js new file mode 100644 index 00000000000..ed463385283 --- /dev/null +++ b/test/unit/common/state/aboutNewTabStateTest.js @@ -0,0 +1,192 @@ +/* global describe, it */ +const aboutNewTabState = require('../../../../app/common/state/aboutNewTabState') +const Immutable = require('immutable') +const assert = require('assert') +const siteTags = require('../../../../js/constants/siteTags') + +const defaultAppState = Immutable.fromJS({ + about: { + newtab: { + gridLayoutSize: 'large', + sites: [], + ignoredTopSites: [], + pinnedTopSites: [], + updatedStamp: undefined + } + } +}) + +const arbitraryTimeInThePast = 1450000000000 + +const assertTimeUpdated = (state) => { + const updatedStamp = state.getIn(['about', 'newtab', 'updatedStamp']) + assert.equal(typeof updatedStamp === 'number' && updatedStamp > arbitraryTimeInThePast, true) +} + +const assertNoChange = (state) => { + const updatedStamp = state.getIn(['about', 'newtab', 'updatedStamp']) + assert.deepEqual(state, defaultAppState) + assert.equal(updatedStamp, undefined) +} + +describe('aboutNewTabState', function () { + const testTime = 1478213227349 + const bookmarkFolderAction = { + siteDetail: { + location: 'https://brave.com', + tags: [siteTags.BOOKMARK_FOLDER], + customTitle: 'folder 1', + parentFolderId: 0, + folderId: 1, + lastAccessedTime: testTime + } + } + const aboutPageAction = { + siteDetail: { + location: 'about:preferences', + title: 'preferences', + lastAccessedTime: testTime + } + } + const bookmarkAction = { + siteDetail: { + title: 'Brave', + location: 'https://brave.com', + lastAccessedTime: testTime + }, + tag: siteTags.BOOKMARK + } + + describe('mergeDetails', function () { + it('updates the `updatedStamp` value on success', function () { + const action = {newTabPageDetail: {}} + const state = aboutNewTabState.mergeDetails(defaultAppState, action) + assertTimeUpdated(state) + }) + + it('does not update state or `updatedStamp` if input is falsey', function () { + const state = aboutNewTabState.mergeDetails(defaultAppState, null) + assertNoChange(state) + }) + + it('merges the provided data into about.newtab', function () { + const action = {newTabPageDetail: {testing123: 'TEST STRING'}} + const state = aboutNewTabState.mergeDetails(defaultAppState, action) + const updatedValue = state.getIn(['about', 'newtab', 'testing123']) + assert.equal(updatedValue, 'TEST STRING') + }) + }) + + describe('addSite', function () { + it('updates the `updatedStamp` value on success', function () { + const state = aboutNewTabState.addSite(defaultAppState, bookmarkAction) + assertTimeUpdated(state) + }) + + describe('does not update state or `updatedStamp` if input is invalid', function () { + it('calls with props=falsey', function () { + const state = aboutNewTabState.addSite(defaultAppState, null) + assertNoChange(state) + }) + + it('calls with props=bookmark folder', function () { + const state = aboutNewTabState.addSite(defaultAppState, bookmarkFolderAction) + assertNoChange(state) + }) + + it('calls with props=about page', function () { + const state = aboutNewTabState.addSite(defaultAppState, aboutPageAction) + assertNoChange(state) + }) + }) + + it('adds the entry into the sites list', function () { + const state = aboutNewTabState.addSite(defaultAppState, bookmarkAction) + const updatedValue = state.getIn(['about', 'newtab', 'sites', 0, 'location']) + assert.equal(updatedValue, bookmarkAction.siteDetail.location) + }) + + it('removes the tags when adding to the sites list', function () { + const state = aboutNewTabState.addSite(defaultAppState, bookmarkAction) + const updatedValue = state.getIn(['about', 'newtab', 'sites', 0, 'tags']) + assert.deepEqual(updatedValue.toJS(), []) + }) + + it('will add lastAccessedTime to the siteDetail if missing from history entry', function () { + const action = {siteDetail: {location: 'https://brave.com'}} + const state = aboutNewTabState.addSite(defaultAppState, action) + const updatedValue = state.getIn(['about', 'newtab', 'sites', 0, 'lastAccessedTime']) + assert.equal(typeof updatedValue === 'number' && updatedValue > arbitraryTimeInThePast, true) + }) + }) + + describe('removeSite', function () { + it('updates the `updatedStamp` value on success', function () { + const action = {siteDetail: {location: 'https://brave.com', lastAccessedTime: testTime}} + const state = aboutNewTabState.removeSite(defaultAppState, action) + assertTimeUpdated(state) + }) + + describe('does not update state or `updatedStamp` if input is invalid', function () { + it('calls with props=falsey', function () { + const state = aboutNewTabState.removeSite(defaultAppState, null) + assertNoChange(state) + }) + + it('calls with props=bookmark folder', function () { + const state = aboutNewTabState.removeSite(defaultAppState, bookmarkFolderAction) + assertNoChange(state) + }) + + it('calls with props=about page', function () { + const state = aboutNewTabState.addSite(defaultAppState, aboutPageAction) + assertNoChange(state) + }) + }) + + it('removes the entry from the sites list', function () { + const stateWithSite = aboutNewTabState.addSite(defaultAppState, bookmarkAction) + assert.equal(stateWithSite.size, 1) + + const state = aboutNewTabState.removeSite(stateWithSite, bookmarkAction) + const sites = state.getIn(['about', 'newtab', 'sites']) + assert.equal(sites.size, 0) + }) + }) + + describe('updateSiteFavicon', function () { + it('updates the `updatedStamp` value on success', function () { + const action = {frameProps: {location: 'https://brave.com'}, favicon: 'https://brave.com/favicon.ico'} + const state = aboutNewTabState.updateSiteFavicon(defaultAppState, action) + assertTimeUpdated(state) + }) + + describe('does not update state or `updatedStamp` if input is invalid', function () { + it('calls with props=falsey', function () { + const state = aboutNewTabState.updateSiteFavicon(defaultAppState, null) + assertNoChange(state) + }) + it('calls with props.frameProps=null', function () { + const action = {frameProps: null} + const state = aboutNewTabState.updateSiteFavicon(defaultAppState, action) + assertNoChange(state) + }) + it('calls with props.frameProps.location=null', function () { + const action = {frameProps: {location: null}} + const state = aboutNewTabState.updateSiteFavicon(defaultAppState, action) + assertNoChange(state) + }) + }) + + it('updates the entry into the sites list', function () { + let state = aboutNewTabState.addSite(defaultAppState, bookmarkAction) + let favicon = state.getIn(['about', 'newtab', 'sites', 0, 'favicon']) + assert.equal(favicon, undefined) + + const action = {frameProps: {location: 'https://brave.com'}, favicon: 'https://brave.com/favicon.ico'} + state = aboutNewTabState.updateSiteFavicon(state, action) + favicon = state.getIn(['about', 'newtab', 'sites', 0, 'favicon']) + assert.equal(favicon, action.favicon) + }) + }) +})