Fixes dnd for bookmark toolbar

Resolves brave#12481 Resolves brave#12506 Auditors: Test Plan:
NejcZdovc · Jan 5, 2018 · 023f4e3 · 023f4e3
1 parent aba789a
commit 023f4e3
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 4 deletions.
diff --git a/app/browser/reducers/bookmarkFoldersReducer.js b/app/browser/reducers/bookmarkFoldersReducer.js
@@ -5,6 +5,7 @@
 const Immutable = require('immutable')
 
 // State
+const bookmarksState = require('../../common/state/bookmarksState')
 const bookmarkFoldersState = require('../../common/state/bookmarkFoldersState')
 const bookmarkToolbarState = require('../../common/state/bookmarkToolbarState')
 
@@ -81,7 +82,7 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
  action.get('moveIntoParent')
  )
 
- const destinationDetail = bookmarkFoldersState.getFolder(state, action.get('destinationKey'))
+ const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
  state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARK_FOLDERS)
  if (
  destinationDetail.get('parentFolderId') === 0 ||

diff --git a/app/browser/reducers/bookmarksReducer.js b/app/browser/reducers/bookmarksReducer.js
@@ -95,7 +95,7 @@ const bookmarksReducer = (state, action, immutableAction) => {
  action.get('moveIntoParent')
  )
 
- const destinationDetail = bookmarksState.getBookmark(state, action.get('destinationKey'))
+ const destinationDetail = bookmarksState.findBookmark(state, action.get('destinationKey'))
  state = syncUtil.updateObjectCache(state, destinationDetail, STATE_SITES.BOOKMARKS)
 
  if (

diff --git a/app/common/state/bookmarkFoldersState.js b/app/common/state/bookmarkFoldersState.js
@@ -173,6 +173,11 @@ const bookmarkFoldersState = {
  ? destinationItem.get('parentFolderId')
  : destinationItem.get('folderId')
 
+ // always use parent ID when we are not moving into the folder
+ if (!moveIntoParent) {
+ parentFolderId = destinationItem.get('parentFolderId')
+ }
+
  if (parentFolderId == null) {
  parentFolderId = destinationKey
  }

diff --git a/app/common/state/bookmarksState.js b/app/common/state/bookmarksState.js
@@ -244,6 +244,11 @@ const bookmarksState = {
  ? destinationItem.get('parentFolderId')
  : destinationItem.get('folderId')
 
+ // always use parent ID when we are not moving into the folder
+ if (!moveIntoParent) {
+ parentFolderId = destinationItem.get('parentFolderId')
+ }
+
  if (parentFolderId == null) {
  parentFolderId = destinationKey
  }

diff --git a/test/unit/app/common/state/bookmarkFoldersStateTest.js b/test/unit/app/common/state/bookmarkFoldersStateTest.js
@@ -665,7 +665,46 @@ describe('bookmarkFoldersState unit test', function () {
  })
 
  it('destination parent ID is different then current parent ID', function () {
- const result = bookmarkFoldersState.moveFolder(moveState, '69', '2')
+ const result = bookmarkFoldersState.moveFolder(moveState, '69', '2', false, false)
+ const expectedState = moveState
+ .setIn(['bookmarkFolders', '69', 'parentFolderId'], 1)
+ .setIn(['cache', 'bookmarkOrder', '0'], Immutable.fromJS([
+ {
+ key: '1',
+ order: 0,
+ type: siteTags.BOOKMARK_FOLDER
+ },
+ {
+ key: '70',
+ order: 1,
+ type: siteTags.BOOKMARK_FOLDER
+ }
+ ]))
+ .setIn(['cache', 'bookmarkOrder', '1'], Immutable.fromJS([
+ {
+ key: '2',
+ order: 0,
+ type: siteTags.BOOKMARK_FOLDER
+ },
+ {
+ key: 'https://brave.com/|0|0',
+ order: 1,
+ type: siteTags.BOOKMARK
+ },
+ {
+ key: '69',
+ order: 2,
+ type: siteTags.BOOKMARK_FOLDER
+ }
+ ]))
+ assert.deepEqual(result.toJS(), expectedState.toJS())
+ assert(removeCacheKeySpy.calledOnce)
+ assert(addFolderToCacheSpy.calledOnce)
+ assert(findBookmarkSpy.calledOnce)
+ })
+
+ it('we want to move folder into the parent', function () {
+ const result = bookmarkFoldersState.moveFolder(moveState, '69', '2', false, true)
  const expectedState = moveState
  .setIn(['bookmarkFolders', '69', 'parentFolderId'], 2)
  .setIn(['cache', 'bookmarkOrder', '0'], Immutable.fromJS([

diff --git a/test/unit/app/common/state/bookmarksStateTest.js b/test/unit/app/common/state/bookmarksStateTest.js
@@ -895,7 +895,52 @@ describe('bookmarkState unit test', function () {
  })
 
  it('destination parent ID is different then current parent ID', function () {
- const result = bookmarksState.moveBookmark(stateWithData, 'https://brave.com/|0|0', '2')
+ const result = bookmarksState.moveBookmark(stateWithData, 'https://brave.com/|0|0', '2', false, false)
+ const expectedState = stateWithData
+ .deleteIn(['bookmarks', 'https://brave.com/|0|0'])
+ .setIn(['bookmarks', 'https://brave.com/|0|1'], Immutable.fromJS(bookmark1))
+ .setIn(['bookmarks', 'https://brave.com/|0|1', 'parentFolderId'], 1)
+ .setIn(['bookmarks', 'https://brave.com/|0|1', 'key'], 'https://brave.com/|0|1')
+ .setIn(['cache', 'bookmarkOrder', '0'], Immutable.fromJS([
+ {
+ key: 'https://clifton.io/|0|0',
+ order: 0,
+ type: siteTags.BOOKMARK
+ },
+ {
+ key: '1',
+ order: 1,
+ type: siteTags.BOOKMARK_FOLDER
+ }
+ ]))
+ .setIn(['cache', 'bookmarkOrder', '1'], Immutable.fromJS([
+ {
+ key: 'https://brianbondy.com/|0|1',
+ order: 0,
+ type: siteTags.BOOKMARK
+ },
+ {
+ key: '2',
+ order: 1,
+ type: siteTags.BOOKMARK_FOLDER
+ },
+ {
+ key: 'https://brave.com/|0|1',
+ order: 2,
+ type: siteTags.BOOKMARK
+ }
+ ]))
+ .setIn(['cache', 'bookmarkLocation', 'https://brave.com/'], Immutable.fromJS([
+ 'https://brave.com/|0|1'
+ ]))
+ assert.deepEqual(result.toJS(), expectedState.toJS())
+ assert(removeCacheKeySpy.calledOnce)
+ assert(addFolderToCacheSpy.calledOnce)
+ assert(findBookmarkSpy.calledOnce)
+ })
+
+ it('we want to move it into the parent folder', function () {
+ const result = bookmarksState.moveBookmark(stateWithData, 'https://brave.com/|0|0', '2', false, true)
  const expectedState = stateWithData
  .deleteIn(['bookmarks', 'https://brave.com/|0|0'])
  .setIn(['bookmarks', 'https://brave.com/|0|2'], Immutable.fromJS(bookmark1))