From fc20174c700f1d2a6fd673c0be6737dea8a866c9 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Fri, 3 Feb 2017 19:23:34 +0800 Subject: [PATCH 1/2] Harden addSite and moveSite fix #7014 fix #7016 Auditors: @bbondy, @ayumi Test Plan: covered by automatic test --- js/components/navigationBar.js | 8 +- js/state/siteUtil.js | 49 ++++---- js/stores/appStore.js | 2 +- test/unit/state/siteUtilTest.js | 196 ++++++++++++++++++++++++++++++++ 4 files changed, 227 insertions(+), 28 deletions(-) diff --git a/js/components/navigationBar.js b/js/components/navigationBar.js index fd61cd4f5df..d48e090017d 100644 --- a/js/components/navigationBar.js +++ b/js/components/navigationBar.js @@ -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) } @@ -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})) } get titleMode () { diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 84ee42648a7..90456b36571 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -75,7 +75,7 @@ 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 */ @@ -83,11 +83,16 @@ 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') + ) + + if (site) { + return true } - return isBookmark(sites.getIn([key, 'tags'])) + return false } const getNextFolderIdItem = (sites) => @@ -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 } @@ -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 @@ -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 @@ -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) { diff --git a/js/stores/appStore.js b/js/stores/appStore.js index 16d4382fdce..fcf65cfd487 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -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'), diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index 31283b161a1..218152976f0 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -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) + }) }) }) @@ -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 () { From f9861f9a72359cf5ea5af829e3b273151ef60d2c Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Fri, 3 Feb 2017 21:50:48 +0800 Subject: [PATCH 2/2] Update isSiteBookmarked with partitionNumber --- js/components/navigationBar.js | 5 ++++- js/state/siteUtil.js | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/js/components/navigationBar.js b/js/components/navigationBar.js index d48e090017d..b7c0d513aa1 100644 --- a/js/components/navigationBar.js +++ b/js/components/navigationBar.js @@ -91,7 +91,10 @@ class NavigationBar extends ImmutableComponent { get bookmarked () { return this.props.activeFrameKey !== undefined && - siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({location: this.props.location})) + siteUtil.isSiteBookmarked(this.props.sites, Immutable.fromJS({ + location: this.props.location, + partitionNumber: this.props.partitionNumber + })) } get titleMode () { diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 90456b36571..6114f341e81 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -86,7 +86,8 @@ module.exports.isSiteBookmarked = function (sites, siteDetail) { const site = sites.find((site) => isBookmark(site.get('tags')) && - site.get('location') === siteDetail.get('location') + site.get('location') === siteDetail.get('location') && + (site.get('partitionNumber') || 0) === (siteDetail.get('partitionNumber') || 0) ) if (site) {