Skip to content

Commit

Permalink
Merge pull request brave#4744 from bsclifton/bookmark-folder-bug-fix
Browse files Browse the repository at this point in the history 
Fixed bug where new folders could destroy existing folders with same name
  • Loading branch information
@darkdh
darkdh authored Oct 13, 2016
2 parents 66774ae + 2b311b1 commit 689b48b
Show file tree
Hide file tree
Showing 2 changed files with 175 additions and 107 deletions.
112 changes: 63 additions & 49 deletions js/state/siteUtil.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,53 @@ module.exports.getNextFolderId = (sites) => {
return (maxIdItem ? (maxIdItem.get('folderId') || 0) : 0) + 1
}

// Some details can be copied from the existing siteDetail if null
// ex: parentFolderId, partitionNumber, and favicon
const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => {
let tags = oldSiteDetail && oldSiteDetail.get('tags') || new Immutable.List()
if (tag) {
tags = tags.toSet().add(tag).toList()
}

const customTitle = typeof newSiteDetail.get('customTitle') === 'string'
? newSiteDetail.get('customTitle')
: (newSiteDetail.get('customTitle') || oldSiteDetail && oldSiteDetail.get('customTitle'))

let site = Immutable.fromJS({
lastAccessedTime: newSiteDetail.get('lastAccessedTime') || new Date().getTime(),
tags,
title: newSiteDetail.get('title')
})

if (newSiteDetail.get('location')) {
site = site.set('location', newSiteDetail.get('location'))
}
if (folderId) {
site = site.set('folderId', Number(folderId))
}
if (typeof customTitle === 'string') {
site = site.set('customTitle', customTitle)
}
if (newSiteDetail.get('parentFolderId') !== undefined || oldSiteDetail && oldSiteDetail.get('parentFolderId')) {
let parentFolderId = newSiteDetail.get('parentFolderId') !== undefined
? newSiteDetail.get('parentFolderId') : oldSiteDetail.get('parentFolderId')
site = site.set('parentFolderId', Number(parentFolderId))
}
if (newSiteDetail.get('partitionNumber') !== undefined || oldSiteDetail && oldSiteDetail.get('partitionNumber')) {
let partitionNumber = newSiteDetail.get('partitionNumber') !== undefined
? newSiteDetail.get('partitionNumber') : oldSiteDetail.get('partitionNumber')
site = site.set('partitionNumber', Number(partitionNumber))
}
if (newSiteDetail.get('favicon') || oldSiteDetail && oldSiteDetail.get('favicon')) {
site = site.set('favicon', newSiteDetail.get('favicon') || oldSiteDetail.get('favicon'))
}
if (newSiteDetail.get('themeColor') || oldSiteDetail && oldSiteDetail.get('themeColor')) {
site = site.set('themeColor', newSiteDetail.get('themeColor') || oldSiteDetail.get('themeColor'))
}

return site
}

/**
* Adds or updates the specified siteDetail in sites.
*
Expand All @@ -101,65 +148,32 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
if (tag === undefined) {
tag = siteDetail.getIn(['tags', 0])
}

const index = module.exports.getSiteIndex(sites, originalSiteDetail || siteDetail, tag)
const oldSite = index !== -1 ? sites.getIn([index]) : null

let folderId = siteDetail.get('folderId')
if (!folderId && tag === siteTags.BOOKMARK_FOLDER) {
folderId = module.exports.getNextFolderId(sites)
}

// Remove 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('customTitle') === siteDetail.get('customTitle'))
if (dupFolder) {
sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER)
if (tag === siteTags.BOOKMARK_FOLDER) {
if (!oldSite && folderId) {
// Remove duplicate folder (needed for import)
const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
site.get('parentFolderId') === siteDetail.get('parentFolderId') &&
site.get('customTitle') === siteDetail.get('customTitle'))
if (dupFolder) {
sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER)
}
} else if (!folderId) {
// Assign an id if this is a new folder
folderId = module.exports.getNextFolderId(sites)
}
}

let tags = index !== -1 && sites.getIn([index, 'tags']) || new Immutable.List()
if (tag) {
tags = tags.toSet().add(tag).toList()
}

const customTitle = typeof siteDetail.get('customTitle') === 'string' ? siteDetail.get('customTitle') : (siteDetail.get('customTitle') || oldSite && oldSite.get('customTitle'))
let site = Immutable.fromJS({
lastAccessedTime: siteDetail.get('lastAccessedTime') || new Date().getTime(),
tags,
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') !== 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') !== 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'))
}
if (siteDetail.get('themeColor') || oldSite && oldSite.get('themeColor')) {
site = site.set('themeColor', siteDetail.get('themeColor') || oldSite.get('themeColor'))
}

let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId)
if (index === -1) {
// Insert new entry
return sites.push(site)
}

// Update existing entry
return sites.setIn([index], site)
}

Expand Down
170 changes: 112 additions & 58 deletions test/unit/state/siteUtilTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,24 +154,122 @@ describe('siteUtil', function () {
})

describe('addSite', function () {
describe('sites list does not have this siteDetail', function () {
it('returns updated site list including the new site', function () {
const sites = Immutable.fromJS([])
const siteDetail = Immutable.fromJS({
lastAccessedTime: 123,
tags: [siteTags.BOOKMARK],
location: testUrl1,
title: 'sample',
parentFolderId: 0,
partitionNumber: 0
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])
assert.deepEqual(processedSites.getIn([0, 'tags']), expectedSites.getIn([0, 'tags']))
})

describe('for new entries (oldSite is null)', function () {
describe('when adding bookmark', function () {
it('preserves existing siteDetail fields', function () {
const processedSites = siteUtil.addSite(emptySites, bookmarkAllFields, siteTags.BOOKMARK)
const expectedSites = Immutable.fromJS([bookmarkAllFields])
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
it('sets default values for lastAccessedTime and tag when they are missing', function () {
const processedSites = siteUtil.addSite(emptySites, bookmarkMinFields, siteTags.BOOKMARK)
assert.equal(!!processedSites.getIn([0, 'lastAccessedTime']), true)
assert.deepEqual(processedSites.getIn([0, 'tags']).toJS(), [siteTags.BOOKMARK])
})
})

describe('when adding bookmark folder', function () {
it('assigns a folderId', function () {
const processedSites = siteUtil.addSite(emptySites, folderMinFields)
const folderId = processedSites.getIn([0, 'folderId'])
assert.equal(folderId, 1)
})
it('allows for new folders to use the same customTitle as an existing folder', 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)
assert.equal(processedSites.size, 2)
assert.equal(processedSites.getIn([1, 'parentFolderId']), folderId)

// Add another bookmark folder with the same name / parentFolderId
processedSites = siteUtil.addSite(processedSites, folderMinFields)
assert.equal(processedSites.size, 3)
const folderId2 = processedSites.getIn([2, 'folderId'])
assert.equal(folderId === folderId2, false)

// Ensure fields for both folders are still in sites array
assert.equal(processedSites.getIn([0, 'customTitle']), processedSites.getIn([2, 'customTitle']))
assert.deepEqual(processedSites.getIn([0, 'tags']), processedSites.getIn([2, 'tags']))
})
it('calls removeSite on bookmark folders which have the same customTitle/parentFolderId', function () {
const sites = Immutable.fromJS([
{
lastAccessedTime: 123,
customTitle: 'folder1',
title: undefined,
folderId: 1,
parentFolderId: 0,
tags: [siteTags.BOOKMARK_FOLDER]
},
{
lastAccessedTime: 123,
customTitle: 'folder2',
title: undefined,
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
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
const processedSites = siteUtil.addSite(sites, siteDetail, siteTags.BOOKMARK)
const expectedSites = sites.push(siteDetail)
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
})

describe('sites list already has this siteDetail', function () {
describe('for existing entries (oldSite is an existing siteDetail)', function () {
it('uses parentFolderId, partitionNumber, and favicon values from old siteDetail if null', function () {
const oldSiteDetail = Immutable.fromJS({
tags: [siteTags.BOOKMARK],
Expand Down Expand Up @@ -201,7 +299,6 @@ describe('siteUtil', function () {
const sites = Immutable.fromJS([oldSiteDetail])
const processedSites = siteUtil.addSite(sites, newSiteDetail, siteTags.BOOKMARK, oldSiteDetail)
const expectedSites = Immutable.fromJS([expectedSiteDetail])
// toJS needed because immutable ownerID :(
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
it('overrides the old title with the new title', function () {
Expand All @@ -222,48 +319,6 @@ describe('siteUtil', function () {
const sites = Immutable.fromJS([oldSiteDetail])
const processedSites = siteUtil.addSite(sites, newSiteDetail, siteTags.BOOKMARK, oldSiteDetail)
const expectedSites = Immutable.fromJS([newSiteDetail])
// toJS needed because immutable ownerID :(
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
it('remove duplicate folder', function () {
const sites = Immutable.fromJS([
{
lastAccessedTime: 123,
customTitle: 'folder1',
title: undefined,
folderId: 1,
parentFolderId: 0,
tags: [siteTags.BOOKMARK_FOLDER]
},
{
lastAccessedTime: 123,
customTitle: 'folder2',
title: undefined,
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())
})
})
Expand Down Expand Up @@ -345,7 +400,6 @@ describe('siteUtil', function () {
tags: Immutable.List([])
}
])
// toJS needed because immutable ownerID :(
assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
})
})
Expand Down

0 comments on commit 689b48b

Please sign in to comment.