From 9143a703a179c1575cf6f7efb90dd715d2056bf0 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Fri, 11 Nov 2016 10:05:46 -0700 Subject: [PATCH] - Added migration for newtab data (and tested manually several times) - Updated tests to be using/expecting a map (not a list) - Updated siteUtil to return empty map where appropriate (instead of empty array) - Removed unused method in siteUtil - Extra error handling in siteUtil.updateSiteFavicon - Added back some of the missing tests for updateSiteFavicon - Update newtab > getUnpinned() to use slice (since it's a map) Auditors: @darkdh, @cezaraugusto --- app/common/state/aboutNewTabState.js | 2 +- app/sessionStore.js | 12 +++ js/about/newtab.js | 2 +- js/state/siteUtil.js | 23 +++-- test/unit/state/siteUtilTest.js | 147 ++++++++++++++------------- 5 files changed, 101 insertions(+), 85 deletions(-) diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 488a163a353..2593a6ed46e 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -37,7 +37,7 @@ const aboutNewTabState = { } // Keep track of the last 18 visited sites - let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List() + let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.Map() sites = sites.take(18) // TODO(cezaraugusto): Sort should respect unshift and don't prioritize bookmarks // | diff --git a/app/sessionStore.js b/app/sessionStore.js index 83c06f93a67..c4bda8a77a2 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -441,6 +441,18 @@ module.exports.loadAppState = () => { }) data.sites = sites } + if (data.about && data.about.newtab && data.about.newtab.sites) { + if (Array.isArray(data.about.newtab.sites) && data.about.newtab.sites.length) { + let sites = {} + data.about.newtab.sites.forEach((site) => { + if (site) { + let key = siteUtil.getSiteKey(Immutable.fromJS(site), site.tags) + sites[key] = site + } + }) + data.about.newtab.sites = sites + } + } // version information (shown on about:brave) const os = require('os') diff --git a/js/about/newtab.js b/js/about/newtab.js index 9e289848d0f..84c307f7004 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -105,7 +105,7 @@ class NewTabPage extends React.Component { const getUnpinned = () => { const firstSite = unpinnedTopSites.first() - unpinnedTopSites = unpinnedTopSites.shift() + unpinnedTopSites = unpinnedTopSites.slice(1) return firstSite } diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 4467e3fd7a2..b30503168c0 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -9,6 +9,7 @@ const settings = require('../constants/settings') const getSetting = require('../settings').getSetting const UrlUtil = require('../lib/urlutil') const urlParse = require('url').parse +const {makeImmutable} = require('../../app/common/state/immutableUtil') const isBookmark = (tags) => { if (!tags) { @@ -120,7 +121,7 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) => lastAccessedTime = newSiteDetail.get('lastAccessedTime') || new Date().getTime() } - let site = Immutable.fromJS({ + let site = makeImmutable({ lastAccessedTime: lastAccessedTime, tags, title: newSiteDetail.get('title'), @@ -350,7 +351,7 @@ module.exports.getDetailFromFrame = function (frame, tag) { location = frame.get('pinnedLocation') } - return Immutable.fromJS({ + return makeImmutable({ location, title: frame.get('title'), partitionNumber: frame.get('partitionNumber'), @@ -370,13 +371,21 @@ module.exports.getDetailFromFrame = function (frame, tag) { * @param favicon favicon URL */ module.exports.updateSiteFavicon = function (sites, location, favicon) { + sites = makeImmutable(sites) + if (UrlUtil.isNotURL(location)) { return sites } + if (!Immutable.Map.isMap(sites)) { + return sites + } const matchingIndices = [] sites.filter((site, index) => { + if (!site) { + return false + } if (isBookmarkFolder(site.get('tags'))) { return false } @@ -537,14 +546,6 @@ module.exports.clearHistory = function (sites) { return bookmarks } -/** - * Determines if the sites list has any sites with no tags - * @param sites The application state's Immutable sites list. - */ -module.exports.hasNoTagSites = function (sites) { - return sites.findIndex((site) => !site.get('tags') || site.get('tags').size === 0) !== -1 -} - /** * Returns all sites that have a bookmark tag. * @param sites The application state's Immutable sites list. @@ -554,7 +555,7 @@ module.exports.getBookmarks = function (sites) { if (sites) { return sites.filter((site) => isBookmarkFolder(site.get('tags')) || isBookmark(site.get('tags'))) } - return [] + return makeImmutable({}) } /** diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index b9ade2e824a..0f6b91af39a 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -118,13 +118,16 @@ describe('siteUtil', function () { describe('getNextFolderId', function () { it('returns the next possible folderId', function () { - const sites = Immutable.fromJS([{ - folderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }, { - folderId: 1, - tags: [siteTags.BOOKMARK_FOLDER] - }]) + const sites = Immutable.fromJS({ + 'key1': { + folderId: 0, + tags: [siteTags.BOOKMARK_FOLDER] + }, + 'key2': { + folderId: 1, + tags: [siteTags.BOOKMARK_FOLDER] + } + }) assert.equal(siteUtil.getNextFolderId(sites), 2) }) it('returns default (0) if sites is falsey', function () { @@ -136,8 +139,7 @@ describe('siteUtil', function () { it('gets the tag from siteDetail if not provided', function () { const processedSites = siteUtil.addSite(emptySites, bookmarkAllFields) const processedKey = siteUtil.getSiteKey(bookmarkAllFields) - const expectedSites = Immutable.fromJS([bookmarkAllFields]) - assert.deepEqual(processedSites.getIn([processedKey, 'tags']), expectedSites.getIn([0, 'tags'])) + assert.deepEqual(processedSites.getIn([processedKey, 'tags']), bookmarkAllFields.get('tags')) }) describe('record count', function () { var processedSites @@ -503,23 +505,47 @@ describe('siteUtil', function () { }) describe('updateSiteFavicon', function () { - it('updates the favicon for all matching entries', function () { + it('updates the favicon for all matching entries (normalizing the URL)', function () { const siteDetail1 = Immutable.fromJS({ tags: [siteTags.BOOKMARK], location: testUrl1, - title: 'bookmarked site' + title: 'bookmarked site', + lastAccessedTime: 123 }) const siteDetail2 = Immutable.fromJS({ tags: [], - location: testUrl1, - title: 'bookmarked site' + location: 'https://www.brave.com', + title: 'history entry', + lastAccessedTime: 456 }) - const sites = Immutable.fromJS([siteDetail1, siteDetail2]) + let sites = siteUtil.addSite(emptySites, siteDetail1, siteTags.BOOKMARK) + sites = siteUtil.addSite(sites, siteDetail2) const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico') const updatedSiteDetail1 = siteDetail1.set('favicon', 'https://brave.com/favicon.ico') const updatedSiteDetail2 = siteDetail2.set('favicon', 'https://brave.com/favicon.ico') - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) - + let expectedSites = siteUtil.addSite(emptySites, updatedSiteDetail1, siteTags.BOOKMARK) + expectedSites = siteUtil.addSite(expectedSites, updatedSiteDetail2) + assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) + }) + it('returns the object unchanged if location is not a URL', function () { + const sites = siteUtil.addSite(emptySites, bookmarkMinFields, siteTags.BOOKMARK) + const processedSites = siteUtil.updateSiteFavicon(sites, 'not-a-url', 'https://brave.com/favicon.ico') + assert.deepEqual(processedSites, sites) + }) + it('returns the object unchanged if it is not an Immutable.Map', function () { + const emptyLegacySites = Immutable.fromJS([]) + const processedSites = siteUtil.updateSiteFavicon(emptyLegacySites, testUrl1, 'https://brave.com/favicon.ico') + assert.deepEqual(processedSites, emptyLegacySites) + }) + it('works even if null/undefined entries are present', function () { + const hasInvalidEntries = Immutable.fromJS({ + 'null': null, + 'undefined': undefined + }) + const sites = siteUtil.addSite(hasInvalidEntries, bookmarkMinFields, siteTags.BOOKMARK) + const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico') + const updatedSiteDetail = bookmarkMinFields.set('favicon', 'https://brave.com/favicon.ico') + const expectedSites = siteUtil.addSite(hasInvalidEntries, updatedSiteDetail, siteTags.BOOKMARK) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) }) @@ -728,91 +754,68 @@ describe('siteUtil', function () { describe('clearHistory', function () { it('does not remove sites which have a valid `tags` property', function () { - const sites = Immutable.fromJS([ - { tags: [siteTags.BOOKMARK_FOLDER] }, - { tags: [siteTags.BOOKMARK] } - ]) + const sites = Immutable.fromJS({ + 'key1': { tags: [siteTags.BOOKMARK_FOLDER] }, + 'key2': { tags: [siteTags.BOOKMARK] } + }) const processedSites = siteUtil.clearHistory(sites) assert.deepEqual(processedSites.toJS(), sites.toJS()) }) it('sets the lastAccessedTime for all entries to null', function () { - const sites = Immutable.fromJS([ - { + const sites = Immutable.fromJS({ + 'key1': { location: 'location1', tags: [], lastAccessedTime: 123 }, - { + 'key2': { location: 'location2', tags: [siteTags.BOOKMARK], lastAccessedTime: 123 } - ]) - const expectedSites = Immutable.fromJS([{ - location: 'location2', - tags: [siteTags.BOOKMARK], - lastAccessedTime: null - }]) + }) + const expectedSites = Immutable.fromJS({ + 'key2': { + location: 'location2', + tags: [siteTags.BOOKMARK], + lastAccessedTime: null + } + }) const processedSites = siteUtil.clearHistory(sites) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) }) - describe('hasNoTagSites', function () { - it('returns true if the ANY sites in the provided list are missing a `tags` property', function () { - const sites = [ - Immutable.fromJS({ - location: 'https://brave.com' - })] - assert.equal(siteUtil.hasNoTagSites(sites), true) - }) - it('returns true if the ANY sites in the provided list have an empty `tags` property', function () { - const sites = [ - Immutable.fromJS({ - tags: [] - })] - assert.equal(siteUtil.hasNoTagSites(sites), true) - }) - it('returns false if all sites have a valid `tags` property', function () { - const sites = [ - Immutable.fromJS({ - tags: [siteTags.BOOKMARK_FOLDER] - }), - Immutable.fromJS({ - tags: [siteTags.BOOKMARK] - })] - assert.equal(siteUtil.hasNoTagSites(sites), false) - }) - }) - describe('getBookmarks', function () { it('returns items which are tagged either `BOOKMARK_FOLDER` or `BOOKMARK`', function () { - const sites = [ - Immutable.fromJS({ + const sites = Immutable.fromJS({ + 'key1': { tags: [siteTags.BOOKMARK_FOLDER] - }), - Immutable.fromJS({ + }, + 'key2': { tags: [siteTags.BOOKMARK] - })] + } + }) const processedSites = siteUtil.getBookmarks(sites) assert.deepEqual(sites, processedSites) }) it('excludes items which are NOT tagged `BOOKMARK_FOLDER` or `BOOKMARK`', function () { - const sites = [ - Immutable.fromJS({ + const sites = Immutable.fromJS({ + 'key1': { tags: ['unknown1'] - }), - Immutable.fromJS({ - tags: ['unknown2'] - })] - const expectedSites = [] + }, + 'key2': { + tags: ['unknown1'] + } + }) + const expectedSites = Immutable.fromJS({}) const processedSites = siteUtil.getBookmarks(sites) - assert.deepEqual(expectedSites, processedSites) + assert.deepEqual(expectedSites.toJS(), processedSites.toJS()) }) - it('returns empty list if input was falsey', function () { + it('returns empty map if input was falsey', function () { const processedSites = siteUtil.getBookmarks(null) - const expectedSites = [] - assert.deepEqual(processedSites, expectedSites) + const expectedSites = Immutable.fromJS({}) + assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) })