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

Commit

Permalink
Merge pull request #7024 from brave/sites-enhancement
Browse files Browse the repository at this point in the history 
Harden addSite and moveSite
  • Loading branch information
@bbondy
bbondy authored Feb 3, 2017
2 parents bfc0cd1 + f9861f9 commit ce14f5d
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 25 deletions.
5 changes: 2 additions & 3 deletions js/components/navigationBar.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class NavigationBar extends ImmutableComponent {
const key = siteUtil.getSiteKey(siteDetail)

if (key !== null) {
siteDetail = siteDetail.set('parentFolderId', this.props.sites.getIn([key]).get('parentFolderId'))
siteDetail = siteDetail.set('parentFolderId', this.props.sites.getIn([key, 'parentFolderId']))
}
windowActions.setBookmarkDetail(siteDetail, siteDetail, null, editing, true)
}
Expand Down Expand Up @@ -93,8 +93,7 @@ class NavigationBar extends ImmutableComponent {
return this.props.activeFrameKey !== undefined &&
siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({
location: this.props.location,
partitionNumber: this.props.partitionNumber,
title: this.props.title
partitionNumber: this.props.partitionNumber
}))
}

Expand Down
50 changes: 29 additions & 21 deletions js/state/siteUtil.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,25 @@ module.exports.getSiteKey = function (siteDetail) {
/**
* Checks if a siteDetail has the specified tag
*
* @param sites The application state's Immutable sites list
* @param sites The application state's Immutable sites map
* @param siteDetail The site to check if it's in the specified tag
* @return true if the location is already bookmarked
*/
module.exports.isSiteBookmarked = function (sites, siteDetail) {
if (!sites) {
return false
}
const key = module.exports.getSiteKey(siteDetail)
if (key === null) {
return false

const site = sites.find((site) =>
isBookmark(site.get('tags')) &&
site.get('location') === siteDetail.get('location') &&
(site.get('partitionNumber') || 0) === (siteDetail.get('partitionNumber') || 0)
)

if (site) {
return true
}
return isBookmark(sites.getIn([key, 'tags']))
return false
}

const getNextFolderIdItem = (sites) =>
Expand Down Expand Up @@ -228,8 +234,11 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
}

let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId, sites.size)
if (oldSite) {
sites = sites.delete(oldKey)
}

const key = originalSiteKey || module.exports.getSiteKey(site)
const key = module.exports.getSiteKey(site)
if (key === null) {
return sites
}
Expand All @@ -239,7 +248,7 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
/**
* Removes the specified tag from a siteDetail
*
* @param sites The application state's Immutable sites list
* @param sites The application state's Immutable sites map
* @param siteDetail The siteDetail to remove a tag from
* @param reorder whether to reorder sites (default with reorder)
* @return The new sites Immutable object
Expand Down Expand Up @@ -306,7 +315,7 @@ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => {
/**
* Moves the specified site from one location to another
*
* @param sites The application state's Immutable sites list
* @param sites The application state's Immutable sites map
* @param siteDetail The site detail to move
* @param destinationDetail The site detail to move to
* @param prepend Whether the destination detail should be prepended or not, not used if destinationIsParent is true
Expand All @@ -320,46 +329,45 @@ module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prep
return sites
}

let sourceKey = module.exports.getSiteKey(sourceDetail)
let destinationKey = module.exports.getSiteKey(destinationDetail)
const sourceKey = module.exports.getSiteKey(sourceDetail)
const destinationKey = module.exports.getSiteKey(destinationDetail)

const sourceSiteIndex = sites.getIn([sourceKey, 'order'])
let destinationSiteIndex
if (destinationIsParent) {
// When the destination is the parent we want to put it at the end
destinationSiteIndex = sites.size - 1
prepend = false
prepend = true
} else {
destinationSiteIndex = sites.getIn([destinationKey, 'order'])
}

let newIndex = destinationSiteIndex + (prepend ? 0 : 1)
const newIndex = destinationSiteIndex + (prepend ? 0 : 1)
let sourceSite = sites.get(sourceKey)
let destinationSite = sites.get(destinationKey)
if (!sourceSite) {
return sites
}
const destinationSite = sites.get(destinationKey)
sites = sites.delete(sourceKey)
sites = sites.map((site) => {
const siteOrder = site.get('order')
if (siteOrder > sourceSiteIndex) {
return site.set('order', siteOrder - 1)
if (siteOrder >= newIndex && siteOrder < sourceSiteIndex) {
return site.set('order', siteOrder + 1)
}
return site
})
if (newIndex > sourceSiteIndex) {
newIndex--
}
sourceSite = sourceSite.set('order', newIndex)

if (!disallowReparent) {
if (destinationIsParent && destinationDetail.get('folderId') !== sourceSite.get('folderId')) {
sourceSite = sourceSite.set('parentFolderId', destinationDetail.get('folderId'))
} else if (!destinationSite.get('parentFolderId')) {
sourceSite = sourceSite.delete('parentFolderId')
sourceSite = sourceSite.set('parentFolderId', 0)
} else if (destinationSite.get('parentFolderId') !== sourceSite.get('parentFolderId')) {
sourceSite = sourceSite.set('parentFolderId', destinationSite.get('parentFolderId'))
}
}
sourceKey = module.exports.getSiteKey(sourceSite)
return sites.set(sourceKey, sourceSite)
return sites.set(module.exports.getSiteKey(sourceSite), sourceSite)
}

module.exports.getDetailFromFrame = function (frame, tag) {
Expand Down
2 changes: 1 addition & 1 deletion js/stores/appStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ const handleAppAction = (action) => {
if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {
action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
appState = appState.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag))
appState = appState.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail))
}
if (action.destinationDetail) {
appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
Expand Down
196 changes: 196 additions & 0 deletions test/unit/state/siteUtilTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,45 @@ describe('siteUtil', function () {
const expectedSites = sites
assert.deepEqual(processedSites.getIn([0, 'lastAccessedTime']), expectedSites.getIn([0, 'lastAccessedTime']))
})
it('move oldSiteDetail to new folder', function () {
const oldSiteDetail = Immutable.fromJS({
tags: [siteTags.BOOKMARK],
location: testUrl1,
title: 'bookmarked site',
customTitle: 'old customTitle',
partitionNumber: 0,
parentFolderId: 0,
order: 0,
favicon: testFavicon1
})
const oldSiteKey = siteUtil.getSiteKey(oldSiteDetail)
const newSiteDetail = Immutable.fromJS({
lastAccessedTime: 456,
tags: [siteTags.BOOKMARK],
location: testUrl1,
partitionNumber: 0,
parentFolderId: 1,
title: 'same entry also acts as history entry'
})
const expectedSiteDetail = Immutable.fromJS({
lastAccessedTime: newSiteDetail.get('lastAccessedTime'),
tags: newSiteDetail.get('tags').toJS(),
location: newSiteDetail.get('location'),
title: newSiteDetail.get('title'),
customTitle: oldSiteDetail.get('customTitle'),
partitionNumber: newSiteDetail.get('partitionNumber'),
parentFolderId: newSiteDetail.get('parentFolderId'),
order: oldSiteDetail.get('order'),
favicon: oldSiteDetail.get('favicon')
})
let sites = {}
sites[oldSiteKey] = oldSiteDetail.toJS()
const processedSites = siteUtil.addSite(Immutable.fromJS(sites), newSiteDetail, siteTags.BOOKMARK, oldSiteDetail)
const expectedSiteKey = siteUtil.getSiteKey(expectedSiteDetail)
let expectedSites = {}
expectedSites[expectedSiteKey] = expectedSiteDetail.toJS()
assert.deepEqual(processedSites.toJS(), expectedSites)
})
})
})

Expand Down Expand Up @@ -555,6 +594,163 @@ describe('siteUtil', function () {
})

describe('moveSite', function () {
describe('order test', function () {
const destinationDetail = {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 0,
order: 0
}
const sourceDetail = {
location: testUrl2 + '4',
partitionNumber: 0,
parentFolderId: 0,
order: 3
}
const sites = {
'https://brave.com/|0|0': destinationDetail,
'http://example.com/0|0': {
location: testUrl2,
partitionNumber: 0,
parentFolderId: 0,
order: 1
},
'https://brave.com/3|0|0': {
location: testUrl1 + '3',
partitionNumber: 0,
parentFolderId: 0,
order: 2
},
'http://example.com/4|0|0': sourceDetail
}

it('prepend target', function () {
const expectedSites = {
'http://example.com/4|0|0': {
location: testUrl2 + '4',
partitionNumber: 0,
parentFolderId: 0,
order: 0
},
'https://brave.com/|0|0': {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 0,
order: 1
},
'http://example.com/0|0': {
location: testUrl2,
partitionNumber: 0,
parentFolderId: 0,
order: 2
},
'https://brave.com/3|0|0': {
location: testUrl1 + '3',
partitionNumber: 0,
parentFolderId: 0,
order: 3
}
}
const processedSites = siteUtil.moveSite(Immutable.fromJS(sites),
Immutable.fromJS(sourceDetail),
Immutable.fromJS(destinationDetail), true, false, true)
assert.deepEqual(processedSites.toJS(), expectedSites)
})
it('not prepend target', function () {
const expectedSites = {
'http://example.com/4|0|0': {
location: testUrl2 + '4',
partitionNumber: 0,
parentFolderId: 0,
order: 1
},
'https://brave.com/|0|0': {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 0,
order: 0
},
'http://example.com/0|0': {
location: testUrl2,
partitionNumber: 0,
parentFolderId: 0,
order: 2
},
'https://brave.com/3|0|0': {
location: testUrl1 + '3',
partitionNumber: 0,
parentFolderId: 0,
order: 3
}
}
const processedSites = siteUtil.moveSite(Immutable.fromJS(sites),
Immutable.fromJS(sourceDetail),
Immutable.fromJS(destinationDetail), false, false, true)
assert.deepEqual(processedSites.toJS(), expectedSites)
})
})
it('destination is parent', function () {
const sourceDetail = {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 0,
order: 1
}
const destinationDetail = {
folderId: 1,
parentFolderId: 0,
order: 0,
tags: [siteTags.BOOKMARK_FOLDER]
}
const sites = {
1: destinationDetail,
'https://brave.com/|0|0': sourceDetail
}
const expectedSites = {
1: destinationDetail,
'https://brave.com/|0|1': {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 1,
order: 1
}
}
const processedSites = siteUtil.moveSite(Immutable.fromJS(sites),
Immutable.fromJS(sourceDetail),
Immutable.fromJS(destinationDetail), false, true, false)
assert.deepEqual(processedSites.toJS(), expectedSites)
})
it('reparent', function () {
const sourceDetail = {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 1,
order: 1
}
const destinationDetail = {
folderId: 1,
parentFolderId: 0,
order: 0,
tags: [siteTags.BOOKMARK_FOLDER]
}
const sites = {
1: destinationDetail,
'https://brave.com/|0|1': sourceDetail
}
const expectedSites = {
1: destinationDetail,
'https://brave.com/|0|0': {
location: testUrl1,
partitionNumber: 0,
parentFolderId: 0,
order: 1
}
}
const processedSites = siteUtil.moveSite(Immutable.fromJS(sites),
Immutable.fromJS(sourceDetail),
Immutable.fromJS(destinationDetail), false, false, false)
assert.deepEqual(processedSites.toJS(), expectedSites)
})
})

describe('updateSiteFavicon', function () {
Expand Down

0 comments on commit ce14f5d

Please sign in to comment.