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

Harden addSite and moveSite #7024

Merged
merged 2 commits into from
Feb 3, 2017
Merged

Harden addSite and moveSite #7024

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc20174
Harden addSite and moveSite
darkdh Feb 3, 2017
f9861f9
Update isSiteBookmarked with partitionNumber
darkdh Feb 3, 2017
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
8 changes: 2 additions & 6 deletions js/components/navigationBar.js
View file
Open in desktop
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 @@ -91,11 +91,7 @@ class NavigationBar extends ImmutableComponent {

get bookmarked () {
return this.props.activeFrameKey !== undefined &&
siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({
location: this.props.location,
partitionNumber: this.props.partitionNumber,
title: this.props.title
}))
siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({location: this.props.location}))
Copy link
Member

Choose a reason for hiding this comment

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

we differentiate on bookmarks with different partitions, at least until today with previous releases. For example a user might want to bookmark twitter.com when logged in as user A and twitter.com when logged in as user B.

}

get titleMode () {
Expand Down
49 changes: 28 additions & 21 deletions js/state/siteUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,24 @@ 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')
Copy link
Member

Choose a reason for hiding this comment

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

ditto lack of partition here

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in f9861f9

)

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

const getNextFolderIdItem = (sites) =>
Expand Down Expand Up @@ -228,8 +233,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 +247,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 +314,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 +328,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
Open in desktop
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
Open in desktop
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