Remove duplicate import bookmarks folder by

1. remove dup folder in addSite 2. fix missing parentFolderId and partionNumber fix #4222 Auditors: @bsclifton, @bbondy Test Plan: Import bookmarks twice, there should not be duplicate folders
brave · Sep 23, 2016 · 3a656bb · 3a656bb
1 parent 5cf9c4b
commit 3a656bb
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 6 deletions.
diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -109,6 +109,16 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
     folderId = module.exports.getNextFolderId(sites)
   }
 
+  // Remve duplicate folder
+  if (!oldSite && tag === siteTags.BOOKMARK_FOLDER) {
+    const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
+      site.get('parentFolderId') === siteDetail.get('parentFolderId') &&
+      site.get('title') === siteDetail.get('title'))
+    if (dupFolder) {
+      sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER)
+    }
+  }
+
   let tags = index !== -1 && sites.getIn([index, 'tags']) || new Immutable.List()
   if (tag) {
     tags = tags.toSet().add(tag).toList()
@@ -118,20 +128,26 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
   let site = Immutable.fromJS({
     lastAccessedTime: siteDetail.get('lastAccessedTime') || new Date().getTime(),
     tags,
-    location: siteDetail.get('location'),
     title: siteDetail.get('title')
   })
+  if (siteDetail.get('location')) {
+    site = site.set('location', siteDetail.get('location'))
+  }
   if (folderId) {
     site = site.set('folderId', Number(folderId))
   }
   if (typeof customTitle === 'string') {
     site = site.set('customTitle', customTitle)
   }
-  if (siteDetail.get('parentFolderId') || oldSite && oldSite.get('parentFolderId')) {
-    site = site.set('parentFolderId', Number(siteDetail.get('parentFolderId') || oldSite.get('parentFolderId')))
+  if (siteDetail.get('parentFolderId') !== undefined || oldSite && oldSite.get('parentFolderId')) {
+    let parentFolderId = siteDetail.get('parentFolderId') !== undefined
+      ? siteDetail.get('parentFolderId') : oldSite.get('parentFolderId')
+    site = site.set('parentFolderId', Number(parentFolderId))
   }
-  if (siteDetail.get('partitionNumber') || oldSite && oldSite.get('partitionNumber')) {
-    site = site.set('partitionNumber', Number(siteDetail.get('partitionNumber') || oldSite.get('partitionNumber')))
+  if (siteDetail.get('partitionNumber') !== undefined || oldSite && oldSite.get('partitionNumber')) {
+    let partitionNumber = siteDetail.get('partitionNumber') !== undefined
+    ? siteDetail.get('partitionNumber') : oldSite.get('partitionNumber')
+    site = site.set('partitionNumber', Number(partitionNumber))
   }
   if (siteDetail.get('favicon') || oldSite && oldSite.get('favicon')) {
     site = site.set('favicon', siteDetail.get('favicon') || oldSite.get('favicon'))

diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
@@ -161,7 +161,9 @@ describe('siteUtil', function () {
           lastAccessedTime: 123,
           tags: [siteTags.BOOKMARK],
           location: testUrl1,
-          title: 'sample'
+          title: 'sample',
+          parentFolderId: 0,
+          partitionNumber: 0
         })
         const processedSites = siteUtil.addSite(sites, siteDetail, siteTags.BOOKMARK)
         const expectedSites = sites.push(siteDetail)
@@ -223,6 +225,45 @@ describe('siteUtil', function () {
         // toJS needed because immutable ownerID :(
         assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
       })
+      it('remove duplicate folder', function () {
+        const sites = Immutable.fromJS([
+          {
+            lastAccessedTime: 123,
+            title: 'folder1',
+            folderId: 1,
+            parentFolderId: 0,
+            tags: [siteTags.BOOKMARK_FOLDER]
+          },
+          {
+            lastAccessedTime: 123,
+            title: 'folder2',
+            folderId: 2,
+            parentFolderId: 1,
+            tags: [siteTags.BOOKMARK_FOLDER]
+          },
+          {
+            lastAccessedTime: 123,
+            title: 'bookmark1',
+            parentFolderId: 1,
+            location: testUrl1,
+            tags: [siteTags.BOOKMARK]
+          },
+          {
+            lastAccessedTime: 123,
+            title: 'bookmark2',
+            parentFolderId: 2,
+            location: testUrl2,
+            tags: [siteTags.BOOKMARK]
+          }
+        ])
+        let processedSites = sites
+        sites.forEach((site) => {
+          processedSites = siteUtil.addSite(processedSites, site)
+        })
+        const expectedSites = sites
+        // toJS needed because immutable ownerID :(
+        assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
+      })
     })
   })