Other Bookmarks in about:bookmarks now displays sub-folders properly

Fixes #4685 Bookmark folders can no longer be moved into themselves (which causes them to disappear). (This was a long-standing bug and this commit adds a test to ensure it's not possible) Fixes #4751
brave · Oct 14, 2016 · 592e3f5 · 592e3f5
1 parent 7807cd8
commit 592e3f5
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 28 deletions.
diff --git a/js/about/bookmarks.js b/js/about/bookmarks.js
@@ -37,14 +37,17 @@ class BookmarkFolderItem extends ImmutableComponent {
 
   onDrop (e) {
     const bookmark = dndData.getDragData(e.dataTransfer, dragTypes.BOOKMARK)
+
     if (bookmark) {
+      // Don't allow a bookmark folder to be moved into itself
+      if (bookmark.get('folderId') === this.props.bookmarkFolder.get('folderId')) {
+        return
+      }
       aboutActions.moveSite(bookmark.toJS(), this.props.bookmarkFolder.toJS(), dndData.shouldPrependVerticalItem(e.target, e.clientY), true)
     }
   }
   render () {
-    const childBookmarkFolders = this.props.bookmarkFolder.get('folderId') === -1
-      ? []
-      : this.props.allBookmarkFolders
+    const childBookmarkFolders = this.props.allBookmarkFolders
           .filter((bookmarkFolder) => (bookmarkFolder.get('parentFolderId') || 0) === this.props.bookmarkFolder.get('folderId'))
     return <div>
       <div role='listitem'
@@ -106,7 +109,9 @@ class BookmarkFolderList extends ImmutableComponent {
       }
       {
         this.props.bookmarkFolders.map((bookmarkFolder) =>
-          <BookmarkFolderItem bookmarkFolder={bookmarkFolder}
+          this.props.isRoot && bookmarkFolder.get('parentFolderId') === -1
+          ? null
+          : <BookmarkFolderItem bookmarkFolder={bookmarkFolder}
             allBookmarkFolders={this.props.allBookmarkFolders}
             selected={!this.props.search && this.props.selectedFolderId === bookmarkFolder.get('folderId')}
             selectedFolderId={this.props.selectedFolderId}

diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -240,7 +240,8 @@ function fillParentFolders (parentFolderIds, bookmarkFolder, allBookmarks) {
 module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, destinationIsParent, disallowReparent) {
   // Disallow loops
   let parentFolderIds = []
-  if (destinationDetail.get('parentFolderId') && sourceDetail.get('folderId')) {
+  if (typeof destinationDetail.get('parentFolderId') === 'number' &&
+      typeof sourceDetail.get('folderId') === 'number') {
     fillParentFolders(parentFolderIds, destinationDetail, sites)
     if (sourceDetail.get('folderId') === destinationDetail.get('folderId') ||
         parentFolderIds.includes(sourceDetail.get('folderId'))) {

diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
@@ -5,10 +5,29 @@ const siteUtil = require('../../../js/state/siteUtil')
 const assert = require('assert')
 const Immutable = require('immutable')
 
-const testUrl1 = 'https://brave.com/'
-const testUrl2 = 'http://example.com/'
-
 describe('siteUtil', function () {
+  const testUrl1 = 'https://brave.com/'
+  const testUrl2 = 'http://example.com/'
+  const emptySites = Immutable.fromJS([])
+  const bookmarkAllFields = Immutable.fromJS({
+    lastAccessedTime: 123,
+    tags: [siteTags.BOOKMARK],
+    location: testUrl1,
+    title: 'sample',
+    parentFolderId: 0,
+    partitionNumber: 0
+  })
+  const bookmarkMinFields = Immutable.fromJS({
+    location: testUrl1,
+    title: 'sample',
+    parentFolderId: 0
+  })
+  const folderMinFields = Immutable.fromJS({
+    customTitle: 'folder1',
+    parentFolderId: 0,
+    tags: [siteTags.BOOKMARK_FOLDER]
+  })
+
   describe('getSiteIndex', function () {
     it('returns -1 if sites is falsey', function () {
       const siteDetail = Immutable.fromJS({
@@ -154,26 +173,6 @@ describe('siteUtil', function () {
   })
 
   describe('addSite', function () {
-    const emptySites = Immutable.fromJS([])
-    const bookmarkAllFields = Immutable.fromJS({
-      lastAccessedTime: 123,
-      tags: [siteTags.BOOKMARK],
-      location: testUrl1,
-      title: 'sample',
-      parentFolderId: 0,
-      partitionNumber: 0
-    })
-    const bookmarkMinFields = Immutable.fromJS({
-      location: testUrl1,
-      title: 'sample',
-      parentFolderId: 0
-    })
-    const folderMinFields = Immutable.fromJS({
-      customTitle: 'folder1',
-      parentFolderId: 0,
-      tags: [siteTags.BOOKMARK_FOLDER]
-    })
-
     it('gets the tag from siteDetail if not provided', function () {
       const processedSites = siteUtil.addSite(emptySites, bookmarkAllFields)
       const expectedSites = Immutable.fromJS([bookmarkAllFields])
@@ -428,6 +427,27 @@ describe('siteUtil', function () {
   })
 
   describe('moveSite', function () {
+    it('does not allow you to move a bookmark folder into itself', function () {
+      // Add a new bookmark folder
+      let processedSites = siteUtil.addSite(emptySites, folderMinFields)
+      const folderId = processedSites.getIn([0, 'folderId'])
+      const bookmark = Immutable.fromJS({
+        lastAccessedTime: 123,
+        title: 'bookmark1',
+        parentFolderId: folderId,
+        location: testUrl1,
+        tags: [siteTags.BOOKMARK]
+      })
+      // Add a bookmark into that folder
+      processedSites = siteUtil.addSite(processedSites, bookmark)
+      const bookmarkFolder = processedSites.get(0)
+
+      // Usage taken from Bookmark Manager, which calls aboutActions.moveSite
+      processedSites = siteUtil.moveSite(processedSites, bookmarkFolder, bookmarkFolder, false, true, false)
+      const siteIndex = siteUtil.getSiteIndex(processedSites, bookmarkFolder, siteTags.BOOKMARK_FOLDER)
+
+      assert.notEqual(processedSites.getIn([siteIndex, 'parentFolderId']), processedSites.getIn([siteIndex, 'folderId']))
+    })
   })
 
   describe('getDetailFromFrame', function () {