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
brave · Apr 12, 2017 · 0fb7fb1 · 0fb7fb1
1 parent ea5a69c
commit 0fb7fb1
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 one of default sites in about:newtab.
+ * @param {Immutable.Map} siteDetail The site detail to check.
+ * @returns {boolean} if the site detail is a default newtab 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.<Immutable.Map>} sites

diff --git 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
@@ -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',