From d32a1b4a2c34e632432edd1d6413a9c15daeb91f Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Sat, 12 Nov 2016 03:15:02 -0700 Subject: [PATCH 1/3] Refactoring, Bug fixes, and tests for top sites on about:newtab Bookmarking items does not affect position Fixes https://github.com/brave/browser-laptop/issues/5413 Sites are ordered by most visited (count) DESC Fixes https://github.com/brave/browser-laptop/issues/5322 Groundwork laid for https://github.com/brave/browser-laptop/issues/5565, but the task is unfinished as-is. ----- These commits moving existing logic from newtab.js into the session helper. Tests were then added and I manually tested each scenario and tried to make sure tests cover those. Things which are not covered are marked with TODO(bsclifton) The important thing: appState.about.newtab.sites no longer persists a separate copy of the sites array. It instead uses the location/partion of items saved to lookup the real object from appState.sites. ----- Auditors: @cezaraugusto, @bbondy --- app/common/state/aboutNewTabState.js | 127 +++++------ js/about/newtab.js | 80 ++----- js/stores/appStore.js | 6 +- .../unit/common/state/aboutNewTabStateTest.js | 211 ++++++------------ 4 files changed, 157 insertions(+), 267 deletions(-) diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 25d86ba5a3c..9b1aa3774bf 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -5,92 +5,89 @@ const Immutable = require('immutable') const {makeImmutable} = require('./immutableUtil') const siteUtil = require('../../../js/state/siteUtil') +const aboutNewTabMaxEntries = 100 -const excludeSiteDetail = (siteDetail) => { - return !siteUtil.isBookmark(siteDetail) && !siteUtil.isHistoryEntry(siteDetail) +const compareSites = (site1, site2) => { + if (!site1 || !site2) return false + return site1.get('location') === site2.get('location') && + site1.get('partitionNumber') === site2.get('partitionNumber') +} +const pinnedTopSites = (state) => { + return (state.getIn(['about', 'newtab', 'pinnedTopSites']) || Immutable.List()).setSize(18) +} +const ignoredTopSites = (state) => { + return state.getIn(['about', 'newtab', 'ignoredTopSites']) || Immutable.List() +} +const isPinned = (state, siteProps) => { + return pinnedTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0 +} +const isIgnored = (state, siteProps) => { + return ignoredTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0 +} +const sortCountDescending = (left, right) => { + if (left.get('count') < right.get('count')) return 1 + if (left.get('count') > right.get('count')) return -1 + return 0 } -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')) +/** + * topSites are defined by users. Pinned sites are attached to their positions + * in the grid, and the non pinned indexes are populated with newly accessed sites + */ +const getTopSites = (state) => { + // remove folders; sort by visit count; enforce a max limit + const sites = (state.get('sites') || new Immutable.List()) + .filter((site) => !siteUtil.isFolder(site)) + .sort(sortCountDescending) + .slice(-aboutNewTabMaxEntries) + + // Filter out pinned and ignored sites + let unpinnedSites = sites.filter((site) => !(isPinned(state, site) || isIgnored(state, site))) + + // TODO(bsclifton): de-dupe here + // .. + + // Merge the pinned and unpinned lists together + // Pinned items have priority because the position is important + const gridSites = pinnedTopSites(state).map((pinnedSite) => { + // Fetch latest siteDetail objects from appState.sites using location/partition + if (pinnedSite) { + const matches = sites.filter((site) => compareSites(site, pinnedSite)) + if (matches.size > 0) return matches.first() + } + // Default to unpinned items + const firstSite = unpinnedSites.first() + unpinnedSites = unpinnedSites.shift() + return firstSite }) + + return gridSites.filter((site) => site != null) } const aboutNewTabState = { - mergeDetails: (state, props) => { - state = makeImmutable(state) - if (!props) { - return state - } - - state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail) - return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) + getGridLayoutSize: (state) => { + return state.getIn(['about', 'newtab', 'gridLayoutSize']) }, - addSite: (state, props) => { - state = makeImmutable(state) - 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()) + getSites: (state) => { + return state.getIn(['about', 'newtab', 'sites']) }, - removeSite: (state, props) => { + mergeDetails: (state, props) => { state = makeImmutable(state) if (!props) { return state } - // Only bookmarks and history items should be considered - let siteDetail = makeImmutable(props.siteDetail) - if (excludeSiteDetail(siteDetail)) { - return state - } - - // Remove tags if this is a history item. - // NOTE: siteUtil.removeSite won't delete the entry unless tags are missing - if (siteDetail.get('tags') && siteDetail.get('tags').size === 0) { - siteDetail = siteDetail.delete('tags') - } - - const sites = state.getIn(['about', 'newtab', 'sites']) - state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, siteDetail, undefined)) + state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) }, - updateSiteFavicon: (state, props) => { + setSites: (state) => { 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.getIn(['frameProps', 'location']), props.get('favicon')) - state = state.setIn(['about', 'newtab', 'sites'], sitesWithFavicon) + // return a filtered version of the sites array + state = state.setIn(['about', 'newtab', 'sites'], getTopSites(state)) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) } } diff --git a/js/about/newtab.js b/js/about/newtab.js index 9e289848d0f..3d3e99e0bc8 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -51,86 +51,49 @@ class NewTabPage extends React.Component { }) }) } - get randomBackgroundImage () { const image = Object.assign({}, backgrounds[Math.floor(Math.random() * backgrounds.length)]) image.style = {backgroundImage: 'url(' + image.source + ')'} return image } - get fallbackImage () { const image = Object.assign({}, config.newtab.fallbackImage) const pathToImage = path.join(__dirname, '..', '..', image.source) image.style = {backgroundImage: 'url(' + `${pathToImage}` + ')'} return image } - - get sites () { - return this.state.newTabData.getIn(['newTabDetail', 'sites']) + get topSites () { + return this.state.newTabData.getIn(['newTabDetail', 'sites']) || Immutable.List() } - get pinnedTopSites () { - return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']).setSize(18) + return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']) || Immutable.List().setSize(18) } - get ignoredTopSites () { - return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites']) + return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites']) || Immutable.List() } - get gridLayoutSize () { - return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize']) + return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize']) || 'small' } - isPinned (siteProps) { - return this.pinnedTopSites.includes(siteProps) - } - - isIgnored (siteProps) { - return this.ignoredTopSites.includes(siteProps) + return this.pinnedTopSites.filter((site) => { + if (!site || !site.get) return false + return site.get('location') === siteProps.get('location') && + site.get('partitionNumber') === siteProps.get('partitionNumber') + }).size > 0 } - isBookmarked (siteProps) { return siteUtil.isSiteBookmarked(this.topSites, siteProps) } - - /** - * topSites are defined by users. Pinned sites are attached to their positions - * in the grid, and the non pinned indexes are populated with newly accessed sites - */ - get topSites () { - const pinnedTopSites = this.pinnedTopSites - const sites = this.sites - let unpinnedTopSites = sites.filter((site) => !this.isPinned(site) ? site : null) - let gridSites - - const getUnpinned = () => { - const firstSite = unpinnedTopSites.first() - unpinnedTopSites = unpinnedTopSites.shift() - return firstSite - } - - gridSites = pinnedTopSites.map(pinned => pinned || getUnpinned()) - - // Remove from grid all ignored sites - gridSites = gridSites.filter((site) => !this.isIgnored(site)) - - return gridSites.filter(site => site != null) - } - get gridLayout () { - const gridLayoutSize = this.gridLayoutSize - const gridSizes = {large: 18, medium: 12, small: 6} - const sitesRow = gridSizes[gridLayoutSize] - - return this.topSites.take(sitesRow) + const sizeToCount = {large: 18, medium: 12, small: 6} + const count = sizeToCount[this.gridLayoutSize] + return this.topSites.take(count) } - showSiteRemovalNotification () { this.setState({ showSiteRemovalNotification: true }) } - hideSiteRemovalNotification () { this.setState({ showSiteRemovalNotification: false @@ -160,8 +123,6 @@ class NewTabPage extends React.Component { onDraggedSite (currentId, finalId) { let gridSites = this.topSites - let pinnedTopSites = this.pinnedTopSites - const currentPosition = gridSites.filter((site) => site.get('location') === currentId).get(0) const finalPosition = gridSites.filter((site) => site.get('location') === finalId).get(0) @@ -173,6 +134,7 @@ class NewTabPage extends React.Component { gridSites = gridSites.splice(finalPositionIndex, 0, currentPosition) // If the reordered site is pinned, update pinned order as well + let pinnedTopSites = this.pinnedTopSites pinnedTopSites = pinnedTopSites.splice(currentPositionIndex, 1) pinnedTopSites = pinnedTopSites.splice(finalPositionIndex, 0, currentPosition) @@ -199,13 +161,11 @@ class NewTabPage extends React.Component { } onPinnedTopSite (siteProps) { - const gridSites = this.topSites - const currentPosition = gridSites.filter((site) => siteProps.get('location') === site.get('location')).get(0) - const currentPositionIndex = gridSites.indexOf(currentPosition) + const currentPosition = this.topSites.filter((site) => siteProps.get('location') === site.get('location')).get(0) + const currentPositionIndex = this.topSites.indexOf(currentPosition) // If pinned, leave it null. Otherwise stores site on ignoredTopSites list, retaining the same position - let pinnedTopSites = this.pinnedTopSites - pinnedTopSites = pinnedTopSites.splice(currentPositionIndex, 1, this.isPinned(siteProps) ? null : siteProps) + let pinnedTopSites = this.pinnedTopSites.splice(currentPositionIndex, 1, this.isPinned(siteProps) ? null : siteProps) aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites}) } @@ -213,6 +173,8 @@ class NewTabPage extends React.Component { onIgnoredTopSite (siteProps) { this.showSiteRemovalNotification() + // TODO: this is not working :( + // If a pinnedTopSite is ignored, remove it from the pinned list as well const newTabState = {} if (this.isPinned(siteProps)) { @@ -261,14 +223,14 @@ class NewTabPage extends React.Component { return null } - const gridLayout = this.gridLayout - const getLetterFromUrl = (url) => { const hostname = urlutils.getHostname(url.get('location'), true) const name = url.get('title') || hostname || '?' return name.charAt(0).toUpperCase() } + const gridLayout = this.gridLayout + return
{ this.state.backgroundImage diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 1dff646f1dc..848b3784174 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -448,11 +448,11 @@ const handleAppAction = (action) => { if (oldSiteSize !== appState.get('sites').size) { filterOutNonRecents() } - appState = aboutNewTabState.addSite(appState, action) + appState = aboutNewTabState.setSites(appState, action) break case AppConstants.APP_REMOVE_SITE: appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag)) - appState = aboutNewTabState.removeSite(appState, action) + appState = aboutNewTabState.setSites(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 = aboutNewTabState.updateSiteFavicon(appState, action) + appState = aboutNewTabState.setSites(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 index e45236607b6..f10f2580262 100644 --- a/test/unit/common/state/aboutNewTabStateTest.js +++ b/test/unit/common/state/aboutNewTabStateTest.js @@ -21,6 +21,7 @@ const arbitraryTimeInThePast = 1450000000000 const assertTimeUpdated = (state) => { const updatedStamp = state.getIn(['about', 'newtab', 'updatedStamp']) assert.equal(typeof updatedStamp === 'number' && updatedStamp > arbitraryTimeInThePast, true) + return updatedStamp } const assertNoChange = (state) => { @@ -30,40 +31,77 @@ const assertNoChange = (state) => { } describe('aboutNewTabState', function () { - const testTime = 1478213227349 - const bookmarkFolderAction = { - siteDetail: { - location: 'https://brave.com', - tags: [siteTags.BOOKMARK_FOLDER], - customTitle: 'folder 1', + describe('BSC]] setSites', function () { + const site1 = Immutable.fromJS({ + location: 'https://example1.com/', + title: 'sample 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 - } - const historyAction = { - siteDetail: { - title: 'Brave', - location: 'https://brave.com', - lastAccessedTime: testTime - }, - tag: undefined - } + count: 10 + }) + const site2 = Immutable.fromJS({ + location: 'https://example2.com', + title: 'sample 2', + parentFolderId: 0, + count: 5 + }) + const site3 = Immutable.fromJS({ + location: 'https://example3.com', + title: 'sample 3', + parentFolderId: 0, + count: 23 + }) + const site4 = Immutable.fromJS({ + location: 'https://example4.com', + title: 'sample 4', + parentFolderId: 0, + count: 0 + }) + const folder1 = Immutable.fromJS({ + customTitle: 'folder1', + parentFolderId: 0, + tags: [siteTags.BOOKMARK_FOLDER] + }) + it('returns a list of the top sites', function () { + const stateWithSites = defaultAppState.set('sites', + Immutable.List().push(site1).push(site2).push(site3).push(folder1)) + let expectedState = stateWithSites.setIn(['about', 'newtab', 'sites'], + Immutable.List().push(site3).push(site1).push(site2)) + + // verify timestamp was updated + const actualState = aboutNewTabState.setSites(stateWithSites) + const ts = assertTimeUpdated(actualState) + expectedState = expectedState.setIn(['about', 'newtab', 'updatedStamp'], ts) + + // checks: + // - sorts by count DESC + // - no folders included + assert.deepEqual(actualState.toJS(), expectedState.toJS()) + }) + it('respect position of pinned items', function () { + let stateWithPinnedSites = defaultAppState.set('sites', + Immutable.List().push(site1).push(site2).push(site3).push(folder1).push(site4)) + + const allPinned = Immutable.fromJS([null, null, site1, null, null, null, null, null, site4]) + + stateWithPinnedSites = stateWithPinnedSites.setIn(['about', 'newtab', 'pinnedTopSites'], + allPinned) + let expectedState = stateWithPinnedSites.setIn(['about', 'newtab', 'sites'], + Immutable.List().push(site3).push(site2).push(site1).push(site4)) + + // verify timestamp was updated + const actualState = aboutNewTabState.setSites(stateWithPinnedSites) + const ts = assertTimeUpdated(actualState) + expectedState = expectedState.setIn(['about', 'newtab', 'updatedStamp'], ts) + + // checks: + // - pinned item are in their expected order + // - unpinned items fill the rest of the spots (starting w/ highest # visits first) + assert.deepEqual(actualState.toJS(), expectedState.toJS()) + }) + // TODO(bsclifton): test ignored sites + // TODO(bsclifton): check that max# entries is enforced + // TODO(bsclifton): test de-duping + }) describe('mergeDetails', function () { it('updates the `updatedStamp` value on success', function () { @@ -84,111 +122,4 @@ describe('aboutNewTabState', function () { 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('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, historyAction) - assert.equal(stateWithSite.size, 1) - - const state = aboutNewTabState.removeSite(stateWithSite, historyAction) - 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) - }) - }) }) From 0ce7149a7923ae49bc9c0a4ee9fbe027eec5a679 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Sun, 13 Nov 2016 14:40:12 -0700 Subject: [PATCH 2/3] More tests; now returns more than 18 results Auditors: @cezaraugusto --- app/common/state/aboutNewTabState.js | 12 +++-- .../unit/common/state/aboutNewTabStateTest.js | 48 +++++++++++++++++-- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 9b1aa3774bf..a3fe3ac6d37 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -49,7 +49,7 @@ const getTopSites = (state) => { // Merge the pinned and unpinned lists together // Pinned items have priority because the position is important - const gridSites = pinnedTopSites(state).map((pinnedSite) => { + let gridSites = pinnedTopSites(state).map((pinnedSite) => { // Fetch latest siteDetail objects from appState.sites using location/partition if (pinnedSite) { const matches = sites.filter((site) => compareSites(site, pinnedSite)) @@ -61,13 +61,17 @@ const getTopSites = (state) => { return firstSite }) + // Include up to [aboutNewTabMaxEntries] entries so that folks + // can ignore sites and have new items fill those empty spaces + if (unpinnedSites.size > 0) { + gridSites = gridSites.concat(unpinnedSites) + } + return gridSites.filter((site) => site != null) } const aboutNewTabState = { - getGridLayoutSize: (state) => { - return state.getIn(['about', 'newtab', 'gridLayoutSize']) - }, + maxSites: aboutNewTabMaxEntries, getSites: (state) => { return state.getIn(['about', 'newtab', 'sites']) diff --git a/test/unit/common/state/aboutNewTabStateTest.js b/test/unit/common/state/aboutNewTabStateTest.js index f10f2580262..bcb43a6e682 100644 --- a/test/unit/common/state/aboutNewTabStateTest.js +++ b/test/unit/common/state/aboutNewTabStateTest.js @@ -31,7 +31,16 @@ const assertNoChange = (state) => { } describe('aboutNewTabState', function () { - describe('BSC]] setSites', function () { + describe('getSites', function () { + it('returns the contents of about.newtab.sites', function () { + const expectedSites = Immutable.List().push(1).push(2).push(3) + const stateWithSites = defaultAppState.setIn(['about', 'newtab', 'sites'], expectedSites) + const actualSites = aboutNewTabState.getSites(stateWithSites) + assert.deepEqual(actualSites.toJS(), expectedSites.toJS()) + }) + }) + + describe('setSites', function () { const site1 = Immutable.fromJS({ location: 'https://example1.com/', title: 'sample 1', @@ -67,7 +76,7 @@ describe('aboutNewTabState', function () { let expectedState = stateWithSites.setIn(['about', 'newtab', 'sites'], Immutable.List().push(site3).push(site1).push(site2)) - // verify timestamp was updated + // account for timestamp being updated in results const actualState = aboutNewTabState.setSites(stateWithSites) const ts = assertTimeUpdated(actualState) expectedState = expectedState.setIn(['about', 'newtab', 'updatedStamp'], ts) @@ -88,7 +97,7 @@ describe('aboutNewTabState', function () { let expectedState = stateWithPinnedSites.setIn(['about', 'newtab', 'sites'], Immutable.List().push(site3).push(site2).push(site1).push(site4)) - // verify timestamp was updated + // account for timestamp being updated in results const actualState = aboutNewTabState.setSites(stateWithPinnedSites) const ts = assertTimeUpdated(actualState) expectedState = expectedState.setIn(['about', 'newtab', 'updatedStamp'], ts) @@ -98,8 +107,37 @@ describe('aboutNewTabState', function () { // - unpinned items fill the rest of the spots (starting w/ highest # visits first) assert.deepEqual(actualState.toJS(), expectedState.toJS()) }) - // TODO(bsclifton): test ignored sites - // TODO(bsclifton): check that max# entries is enforced + it('only returns `maxSites` results', function () { + const maxSites = aboutNewTabState.maxSites + let tooManySites = Immutable.List() + for (let i = 0; i < maxSites + 1; i++) { + tooManySites = tooManySites.push( + site1.set('location', 'https://example' + i + '.com').set('title', 'sample ' + i)) + } + const stateWithTooManySites = defaultAppState.set('sites', tooManySites) + const actualState = aboutNewTabState.setSites(stateWithTooManySites) + assert.equal(actualState.getIn(['about', 'newtab', 'sites']).size, maxSites) + }) + + it('does not include items marked as ignored', function () { + let stateWithIgnoredSites = defaultAppState.set('sites', + Immutable.List().push(site1).push(site2).push(site3).push(site4)) + + const ignoredSites = Immutable.List().push(site1).push(site3) + stateWithIgnoredSites = stateWithIgnoredSites.setIn(['about', 'newtab', 'ignoredTopSites'], ignoredSites) + + let expectedState = stateWithIgnoredSites.setIn(['about', 'newtab', 'sites'], + Immutable.List().push(site2).push(site4)) + + // account for timestamp being updated in results + const actualState = aboutNewTabState.setSites(stateWithIgnoredSites) + const ts = assertTimeUpdated(actualState) + expectedState = expectedState.setIn(['about', 'newtab', 'updatedStamp'], ts) + + // checks: + // - ignored items are not included + assert.deepEqual(actualState.toJS(), expectedState.toJS()) + }) // TODO(bsclifton): test de-duping }) From c65f27145280c419b6e3ae3cc95f21f8b143165b Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Sun, 13 Nov 2016 15:27:25 -0700 Subject: [PATCH 3/3] Really close to appearing to be bug free Last remaining known issue(s): 1. ignoring sites - pin some items (position 1, position 5) - ignore by hitting X on item 2 or 3 - items become unpinned :( 2. default top sites - we need to populate this (like we talked about) - brave.com, etc --- js/about/aboutActions.js | 5 +++-- js/about/newtab.js | 11 ++++++----- js/stores/appStore.js | 3 +++ 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/js/about/aboutActions.js b/js/about/aboutActions.js index 91a9cb96d1a..26f97ef92a4 100644 --- a/js/about/aboutActions.js +++ b/js/about/aboutActions.js @@ -166,10 +166,11 @@ const aboutActions = { ipc.send(messages.SET_CLIPBOARD, text) }, - setNewTabDetail: function (newTabPageDetail) { + setNewTabDetail: function (newTabPageDetail, refresh) { aboutActions.dispatchAction({ actionType: appConstants.APP_CHANGE_NEW_TAB_DETAIL, - newTabPageDetail + newTabPageDetail, + refresh }) }, diff --git a/js/about/newtab.js b/js/about/newtab.js index 3d3e99e0bc8..97cb2a82c25 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -173,8 +173,6 @@ class NewTabPage extends React.Component { onIgnoredTopSite (siteProps) { this.showSiteRemovalNotification() - // TODO: this is not working :( - // If a pinnedTopSite is ignored, remove it from the pinned list as well const newTabState = {} if (this.isPinned(siteProps)) { @@ -183,16 +181,19 @@ class NewTabPage extends React.Component { const currentPositionIndex = gridSites.indexOf(currentPosition) const pinnedTopSites = this.pinnedTopSites.splice(currentPositionIndex, 1, null) newTabState.pinnedTopSites = pinnedTopSites + + // TODO: ignoring an item sometimes was removing a pin for a different site + // I think the merge is not working properly. } newTabState.ignoredTopSites = this.ignoredTopSites.push(siteProps) - aboutActions.setNewTabDetail(newTabState) + aboutActions.setNewTabDetail(newTabState, true) } onUndoIgnoredTopSite () { // Remove last List's entry const ignoredTopSites = this.ignoredTopSites.splice(-1, 1) - aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites}) + aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites}, true) this.hideSiteRemovalNotification() } @@ -200,7 +201,7 @@ class NewTabPage extends React.Component { * Clear ignoredTopSites and pinnedTopSites list */ onRestoreAll () { - aboutActions.setNewTabDetail({ignoredTopSites: [], pinnedTopSites: []}) + aboutActions.setNewTabDetail({ignoredTopSites: []}, true) this.hideSiteRemovalNotification() } diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 848b3784174..664b57d6124 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -431,6 +431,9 @@ const handleAppAction = (action) => { break case AppConstants.APP_CHANGE_NEW_TAB_DETAIL: appState = aboutNewTabState.mergeDetails(appState, action) + if (action.refresh) { + appState = aboutNewTabState.setSites(appState, action) + } break case AppConstants.APP_ADD_SITE: const oldSiteSize = appState.get('sites').size