From c37ed6ede34eb41c733b9fc88266ef2ef0457e93 Mon Sep 17 00:00:00 2001
From: yan <yan@mit.edu>
Date: Fri, 28 Apr 2017 18:19:24 +0000
Subject: [PATCH] fix syncing error when lastAccessedTime is empty

Test Plan: unittests should pass
---
 js/state/siteUtil.js            | 11 +++++------
 js/state/syncUtil.js            | 11 ++++++-----
 test/unit/state/siteUtilTest.js |  7 +++++++
 test/unit/state/syncUtilTest.js | 26 ++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
index baf15fad866..741dcae53cd 100644
--- a/js/state/siteUtil.js
+++ b/js/state/siteUtil.js
@@ -603,14 +603,13 @@ module.exports.isImportedBookmark = function (siteDetail) {
  */
 module.exports.isHistoryEntry = function (siteDetail) {
   if (siteDetail && typeof siteDetail.get('location') === 'string') {
-    if (siteDetail.get('location').startsWith('about:')) {
+    const tags = siteDetail.get('tags')
+    if (siteDetail.get('location').startsWith('about:') ||
+      module.exports.isDefaultEntry(siteDetail) ||
+      isBookmarkFolder(tags)) {
       return false
     }
-    if (module.exports.isDefaultEntry(siteDetail)) {
-      // This is a Brave default newtab site
-      return false
-    }
-    return !!siteDetail.get('lastAccessedTime') && !isBookmarkFolder(siteDetail.get('tags'))
+    return !!siteDetail.get('lastAccessedTime') || !tags || tags.size === 0
   }
   return false
 }
diff --git a/js/state/syncUtil.js b/js/state/syncUtil.js
index 9bbbb5b3f67..aa5be828c6d 100644
--- a/js/state/syncUtil.js
+++ b/js/state/syncUtil.js
@@ -79,7 +79,7 @@ module.exports.getSiteDataFromRecord = (record, appState, records) => {
   let existingObjectData
 
   if (record.action !== writeActions.CREATE) {
-    const existingObject = module.exports.getObjectById(objectId, category,
+    const existingObject = getObjectById(objectId, category,
       appState)
     existingObjectData = existingObject && existingObject[1]
   }
@@ -237,7 +237,7 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
   const AppStore = require('../stores/appStore')
   const appState = AppStore.getState()
   const objectId = new Immutable.List(syncRecord.objectId)
-  const existingObject = module.exports.getObjectById(objectId, categoryName, appState)
+  const existingObject = getObjectById(objectId, categoryName, appState)
   if (!existingObject) { return null }
 
   const existingObjectKeyPath = existingObject[0]
@@ -256,7 +256,8 @@ module.exports.getExistingObject = (categoryName, syncRecord) => {
       throw new Error(`Invalid category: ${categoryName}`)
   }
   if (!item) {
-    throw new Error(`Can't create JS from existingObject! ${existingObjectData}`)
+    console.log(`Warning: Can't create JS from existingObject! ${JSON.stringify(existingObjectData)}`)
+    return null
   }
   return {
     action: writeActions.CREATE,
@@ -314,7 +315,7 @@ module.exports.updateSiteCache = (appState, siteDetail) => {
  * @param {Immutable.Map=} appState
  * @returns {Array} [<Array>, <Immutable.Map>] array is AppStore searchKeyPath e.g. ['sites', 10] for use with updateIn
  */
-module.exports.getObjectById = (objectId, category, appState) => {
+const getObjectById = (objectId, category, appState) => {
   if (!(objectId instanceof Immutable.List)) {
     throw new Error('objectId must be an Immutable.List')
   }
@@ -355,7 +356,7 @@ const getFolderIdByObjectId = (objectId, appState, records) => {
     return folderIdMap.get(objectId)
   }
   let folderId
-  const entry = module.exports.getObjectById(objectId, 'BOOKMARKS', appState)
+  const entry = getObjectById(objectId, 'BOOKMARKS', appState)
   if (entry) {
     folderId = entry[1].get('folderId')
   } else if (records) {
diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
index 482a8038f09..983f09f23a2 100644
--- a/test/unit/state/siteUtilTest.js
+++ b/test/unit/state/siteUtilTest.js
@@ -1274,6 +1274,13 @@ describe('siteUtil', function () {
       })
       assert.equal(siteUtil.isHistoryEntry(siteDetail), false)
     })
+    it('returns true for a history entry with falsey lastAccessedTime', function () {
+      const siteDetail = Immutable.fromJS({
+        location: testUrl1,
+        tags: []
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), true)
+    })
     it('returns false for a bookmarks folder', function () {
       const siteDetail = Immutable.fromJS({
         location: testUrl1,
diff --git a/test/unit/state/syncUtilTest.js b/test/unit/state/syncUtilTest.js
index b728eab0d8a..784c8019b21 100644
--- a/test/unit/state/syncUtilTest.js
+++ b/test/unit/state/syncUtilTest.js
@@ -82,6 +82,32 @@ describe('syncUtil', () => {
       }
       assert.deepEqual(syncUtil.createSiteData(bookmark), expectedBookmark)
     })
+
+    it('site without lastAccessedTime', () => {
+      const site = {
+        order: 1207,
+        count: 15,
+        partitionNumber: 0,
+        location: 'https://parsecpizzadelivery.com/',
+        title: "Parsec Pizza Delivery trailer - A pixelated deliver 'em up",
+        tags: [],
+        objectId: [0, 63, 197, 156, 48, 17, 112, 109, 247, 175, 79, 57, 151, 123, 29, 198],
+        themeColor: 'rgb(5, 5, 5)'
+      }
+      const expectedSite = {
+        name: 'historySite',
+        objectId: [0, 63, 197, 156, 48, 17, 112, 109, 247, 175, 79, 57, 151, 123, 29, 198],
+        value: {
+          creationTime: 0,
+          customTitle: '',
+          favicon: '',
+          lastAccessedTime: 0,
+          location: 'https://parsecpizzadelivery.com/',
+          title: "Parsec Pizza Delivery trailer - A pixelated deliver 'em up"
+        }
+      }
+      assert.deepEqual(syncUtil.createSiteData(site), expectedSite)
+    })
   })
 
   describe('ipcSafeObject()', () => {