From 11392e43210e5ba951ce4be1599d05c6d62eaeb5 Mon Sep 17 00:00:00 2001 From: yan Date: Tue, 11 Apr 2017 21:58:59 +0000 Subject: [PATCH] fix sync error when default newtab site is visited fix #8217 and refactors syncUtil/urlBarSuggestions to use common siteUtil methods Test Plan: 1. enable sync in a clean profile 2. go to https://brave.com 3. you should not see any console errors --- .../reducers/urlBarSuggestionsReducer.js | 7 +-- app/sync.js | 2 +- js/constants/siteTags.js | 1 + js/state/siteUtil.js | 15 ++++++- js/state/syncUtil.js | 16 +++---- test/unit/state/siteUtilTest.js | 45 ++++++++++++++++++- 6 files changed, 69 insertions(+), 17 deletions(-) diff --git a/app/renderer/reducers/urlBarSuggestionsReducer.js b/app/renderer/reducers/urlBarSuggestionsReducer.js index fcb59ad07dc..7ee16cade21 100644 --- a/app/renderer/reducers/urlBarSuggestionsReducer.js +++ b/app/renderer/reducers/urlBarSuggestionsReducer.js @@ -6,6 +6,7 @@ const windowConstants = require('../../../js/constants/windowConstants') const getSetting = require('../../../js/settings').getSetting +const {isDefaultEntry} = require('../../../js/state/siteUtil') const fetchSearchSuggestions = require('../fetchSearchSuggestions') const {activeFrameStatePath, frameStatePath, getFrameByKey, getFrameKeyByTabId, getActiveFrame} = require('../../../js/state/frameStateUtil') const searchProviders = require('../../../js/data/searchProviders') @@ -128,11 +129,7 @@ const generateNewSuggestionsList = (state) => { let sites = appStoreRenderer.state.get('sites') if (sites) { // Filter out Brave default newtab sites and sites with falsey location - sites = sites.filterNot((site) => - (Immutable.is(site.get('tags'), new Immutable.List(['default'])) && - site.get('lastAccessedTime') === 1) || - !site.get('location') - ) + sites = sites.filter((site) => !isDefaultEntry(site) && site.get('location')) } const searchResults = state.getIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'searchResults'])) const frameSearchDetail = state.getIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'searchDetail'])) diff --git a/app/sync.js b/app/sync.js index 2528b9c2b34..5d880cd8a9f 100644 --- a/app/sync.js +++ b/app/sync.js @@ -53,7 +53,7 @@ const sendSyncRecords = (sender, action, data) => { if (!deviceId) { throw new Error('Cannot build a sync record because deviceId is not set') } - if (!data || !data.length) { + if (!data || !data.length || !data[0]) { return } const category = CATEGORY_MAP[data[0].name] diff --git a/js/constants/siteTags.js b/js/constants/siteTags.js index 082140bcbd3..4a86ba2fa59 100644 --- a/js/constants/siteTags.js +++ b/js/constants/siteTags.js @@ -6,6 +6,7 @@ const mapValuesByKeys = require('../lib/functional').mapValuesByKeys const _ = null const siteTags = { + DEFAULT: _, BOOKMARK: _, BOOKMARK_FOLDER: _, PINNED: _, diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 51cea9bd7a6..face2fb1961 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -11,6 +11,8 @@ const UrlUtil = require('../lib/urlutil') const urlParse = require('../../app/common/urlParse') const {makeImmutable} = require('../../app/common/state/immutableUtil') +const defaultTags = new Immutable.List([siteTags.DEFAULT]) + const isBookmark = (tags) => { if (!tags) { return false @@ -604,8 +606,7 @@ module.exports.isHistoryEntry = function (siteDetail) { if (siteDetail.get('location').startsWith('about:')) { return false } - if (Immutable.is(siteDetail.get('tags'), new Immutable.List(['default'])) && - siteDetail.get('lastAccessedTime') === 1) { + if (module.exports.isDefaultEntry(siteDetail)) { // This is a Brave default newtab site return false } @@ -614,6 +615,16 @@ module.exports.isHistoryEntry = function (siteDetail) { return false } +/** + * Determines if the site detail is a default sites entry. + * @param {Immutable.Map} siteDetail The site detail to check. + * @returns {boolean} if the site detail is a history entry. + */ +module.exports.isDefaultEntry = function (siteDetail) { + return Immutable.is(siteDetail.get('tags'), defaultTags) && + siteDetail.get('lastAccessedTime') === 1 +} + /** * Get a folder by folderId * @returns {Immutable.List.} sites diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js index 80d614d2422..38ea4d8d511 100644 --- a/js/state/syncUtil.js +++ b/js/state/syncUtil.js @@ -396,9 +396,8 @@ module.exports.now = () => { * @returns {boolean} */ module.exports.isSyncable = (type, item) => { - if (type === 'bookmark' && item.get('tags')) { - return (item.get('tags').includes('bookmark') || - item.get('tags').includes('bookmark-folder')) + if (type === 'bookmark') { + return siteUtil.isBookmark(item) || siteUtil.isFolder(item) } else if (type === 'siteSetting') { for (let field in siteSettingDefaults) { if (item.has(field)) { @@ -465,11 +464,12 @@ module.exports.createSiteData = (site, appState) => { siteData[field] = site[field] } } - const siteKey = siteUtil.getSiteKey(Immutable.fromJS(site)) || siteUtil.getSiteKey(Immutable.fromJS(siteData)) + const immutableSite = Immutable.fromJS(site) + const siteKey = siteUtil.getSiteKey(immutableSite) || siteUtil.getSiteKey(Immutable.fromJS(siteData)) if (siteKey === null) { throw new Error('Sync could not create siteKey') } - if (module.exports.isSyncable('bookmark', Immutable.fromJS(site))) { + if (module.exports.isSyncable('bookmark', immutableSite)) { const objectId = site.objectId || module.exports.newObjectId(['sites', siteKey]) const parentFolderObjectId = site.parentFolderObjectId || (site.parentFolderId && findOrCreateFolderObjectId(site.parentFolderId, appState)) @@ -478,18 +478,18 @@ module.exports.createSiteData = (site, appState) => { objectId, value: { site: siteData, - isFolder: site.tags.includes('bookmark-folder'), + isFolder: siteUtil.isFolder(immutableSite), parentFolderObjectId } } - } else if (!site.tags || !site.tags.length || site.tags.includes('pinned')) { + } else if (siteUtil.isHistoryEntry(immutableSite)) { return { name: 'historySite', objectId: site.objectId || module.exports.newObjectId(['sites', siteKey]), value: siteData } } - console.log(`Warning: Can't create site data: ${site}`) + console.log(`Warning: Can't create site data: ${JSON.stringify(site)}`) } /** diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index 5e836f75f2c..482a8038f09 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -1259,6 +1259,14 @@ describe('siteUtil', function () { }) assert.equal(siteUtil.isHistoryEntry(siteDetail), true) }) + it('returns false for a default site', function () { + const siteDetail = Immutable.fromJS({ + location: testUrl1, + tags: [siteTags.DEFAULT], + lastAccessedTime: 1 + }) + assert.equal(siteUtil.isHistoryEntry(siteDetail), false) + }) it('returns false for a bookmark entry with falsey lastAccessedTime', function () { const siteDetail = Immutable.fromJS({ location: testUrl1, @@ -1277,7 +1285,7 @@ describe('siteUtil', function () { it('returns false for a brave default site', function () { const siteDetail = Immutable.fromJS({ location: testUrl1, - tags: ['default'], + tags: [siteTags.DEFAULT], lastAccessedTime: 1 }) assert.equal(siteUtil.isHistoryEntry(siteDetail), false) @@ -1295,6 +1303,41 @@ describe('siteUtil', function () { }) }) + describe('isDefaultEntry', function () { + it('returns false for history entry which has lastAccessedTime', function () { + const siteDetail = Immutable.fromJS({ + location: 'https://brave.com/', + tags: [siteTags.DEFAULT], + lastAccessedTime: 123 + }) + assert.equal(siteUtil.isDefaultEntry(siteDetail), false) + }) + it('returns false for bookmark entry', function () { + const siteDetail = Immutable.fromJS({ + location: 'https://brave.com/', + tags: [siteTags.BOOKMARK], + lastAccessedTime: 1 + }) + assert.equal(siteUtil.isDefaultEntry(siteDetail), false) + }) + it('returns false for entry without lastAccessedTime', function () { + const siteDetail = Immutable.fromJS({ + location: 'https://brave.com/', + tags: [siteTags.DEFAULT] + }) + assert.equal(siteUtil.isDefaultEntry(siteDetail), false) + }) + it('returns true for default entry', function () { + const siteDetail = Immutable.fromJS({ + tags: [siteTags.DEFAULT], + lastAccessedTime: 1, + objectId: [210, 115, 31, 176, 57, 212, 167, 120, 104, 88, 88, 27, 141, 36, 235, 226], + location: testUrl1 + }) + assert.equal(siteUtil.isDefaultEntry(siteDetail), true) + }) + }) + describe('getFolder', function () { const folder = Immutable.fromJS({ customTitle: 'folder1',