Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Bookmark fixes for 0.12.5 #4756

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Bookmark fixes for 0.12.5 #4756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions js/about/bookmarks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Copy link
Member

@bbondy bbondy Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have another check somewhere that checks all decedents but I don't remember where off hand. Is this a strong enough check? I.e. could someone do something like this?

A
--- B
     --- C

And then move A into C to create a circular ref?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have something like this in the moveSite code; I'll make sure that code is rock solid by working on some unit tests 😄

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this still return -1? If so won't a .filter() throw?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye 😄 The folderId of -1 is reserved for the "Other Bookmarks" folder (just like 0 is reserved for "Bookmarks Toolbar"). This binding creates an item for each child by matching folderId with the parentFolderId bound to this control. This does match successfully for "Other Bookmarks", but not at the root level (the one below which has the isRoot check, which is where "Other Bookmarks" is isolated to, the bottom)

Sorry that was an awfully big mouthful 😄 (hopefully it makes sense!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use -1 for "Other Bookmarks" in importer too.

.filter((bookmarkFolder) => (bookmarkFolder.get('parentFolderId') || 0) === this.props.bookmarkFolder.get('folderId'))
return <div>
<div role='listitem'
Expand Down Expand Up @@ -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}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is so that folders that are children of "Other Bookmarks" don't show under "Bookmarks Toolbar". Since the above check (line 50) was removed, it will now bind ALL bookmarks to this control.

selected={!this.props.search && this.props.selectedFolderId === bookmarkFolder.get('folderId')}
selectedFolderId={this.props.selectedFolderId}
Expand Down
3 changes: 2 additions & 1 deletion js/state/siteUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'))) {
Expand Down
66 changes: 43 additions & 23 deletions test/unit/state/siteUtilTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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 () {
Expand Down