diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 1f60c4436aa..741b92e5266 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -366,7 +366,7 @@ const getAncestorFolderIds = (parentFolderIds, bookmarkFolder, allBookmarks) => * Determine if a proposed move is valid * * @param sites The application state's Immutable sites list - * @param siteDetail The site detail to move + * @param sourceDetail The site detail to move * @param destinationDetail The site detail to move to */ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => { @@ -398,24 +398,23 @@ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => { */ module.exports.moveSite = function (sites, sourceKey, destinationKey, prepend, destinationIsParent, disallowReparent) { - const sourceDetail = sites.get(sourceKey) || Immutable.Map() - const destinationDetail = sites.get(destinationKey) || Immutable.Map() + let sourceSite = sites.get(sourceKey) || Immutable.Map() + const destinationSite = sites.get(destinationKey) || Immutable.Map() - if (!module.exports.isMoveAllowed(sites, sourceDetail, destinationDetail)) { + if (!module.exports.isMoveAllowed(sites, sourceSite, destinationSite)) { return sites } - const sourceSiteIndex = sites.getIn([sourceKey, 'order']) + const sourceSiteIndex = sourceSite.get('order') let destinationSiteIndex if (destinationIsParent) { // When the destination is the parent we want to put it at the end destinationSiteIndex = sites.size - 1 prepend = true } else { - destinationSiteIndex = sites.getIn([destinationKey, 'order']) + destinationSiteIndex = destinationSite.get('order') } - let sourceSite = sites.get(sourceKey) if (!sourceSite) { return sites } @@ -423,7 +422,7 @@ module.exports.moveSite = function (sites, sourceKey, destinationKey, prepend, if (destinationSiteIndex > sourceSiteIndex) { --newIndex } - const destinationSite = sites.get(destinationKey) + sites = sites.delete(sourceKey) sites = sites.map((site) => { const siteOrder = site.get('order') @@ -437,11 +436,9 @@ module.exports.moveSite = function (sites, sourceKey, destinationKey, prepend, sourceSite = sourceSite.set('order', newIndex) if (!disallowReparent) { - const destinationFolder = destinationDetail.get('folderId') - - if (destinationIsParent && destinationFolder !== sourceSite.get('folderId')) { - sourceSite = sourceSite.set('parentFolderId', destinationFolder) - } else if (!destinationSite.get('parentFolderId')) { + if (destinationIsParent && destinationSite.get('folderId') !== sourceSite.get('folderId')) { + sourceSite = sourceSite.set('parentFolderId', destinationSite.get('folderId')) + } else if (destinationSite.get('parentFolderId') == null) { sourceSite = sourceSite.set('parentFolderId', 0) } else if (destinationSite.get('parentFolderId') !== sourceSite.get('parentFolderId')) { sourceSite = sourceSite.set('parentFolderId', destinationSite.get('parentFolderId')) diff --git a/test/unit/app/browser/reducers/sitesReducerTest.js b/test/unit/app/browser/reducers/sitesReducerTest.js index bf597fe60dd..4212ae678b0 100644 --- a/test/unit/app/browser/reducers/sitesReducerTest.js +++ b/test/unit/app/browser/reducers/sitesReducerTest.js @@ -112,8 +112,9 @@ describe('sitesReducerTest', function () { describe('APP_MOVE_SITE', function () { it('Moves the specified site', function () { const url = 'https://www.brave.com' + const url2 = 'https://www.brave.com/3' let state = initState - let action = { + let addAction = { actionType: appConstants.APP_ADD_SITE, siteDetail: Immutable.fromJS({ location: url, @@ -121,29 +122,33 @@ describe('sitesReducerTest', function () { }), skipSync: true } - let newState = sitesReducer(state, action) - action.siteDetail = action.siteDetail.set('location', 'https://www.brave.com/2') - newState = sitesReducer(newState, action) - action.siteDetail = action.siteDetail.set('location', 'https://www.brave.com/3') - newState = sitesReducer(newState, action) + + let moveAction = { + actionType: appConstants.APP_MOVE_SITE, + sourceKey: `${url}|0|0`, + destinationKey: `${url2}|0|0` + } + + // Add sites + let newState = sitesReducer(state, addAction) + addAction.siteDetail = addAction.siteDetail.set('location', 'https://www.brave.com/2') + newState = sitesReducer(newState, addAction) + addAction.siteDetail = addAction.siteDetail.set('location', 'https://www.brave.com/3') + newState = sitesReducer(newState, addAction) assert.equal(Object.keys(newState.get('sites').toJS()).length, 3) // Move the site to the 2nd position - action.actionType = appConstants.APP_MOVE_SITE - action.sourceDetail = action.siteDetail - action.destinationDetail = action.siteDetail.set('location', 'https://www.brave.com') - action.siteDetail = undefined - newState = sitesReducer(newState, action).toJS() + newState = sitesReducer(newState, moveAction).toJS() assert.equal(Object.keys(newState.sites).length, 3) - assert.equal(Object.values(newState.sites)[2].location, 'https://www.brave.com/3') - assert.equal(Object.values(newState.sites)[2].order, 1) + assert.equal(Object.values(newState.sites)[2].location, url) + assert.equal(Object.values(newState.sites)[2].order, 2) // Move the site to the 3rd position - action.prepend = true - newState = sitesReducer(Immutable.fromJS(newState), action).toJS() + moveAction.prepend = true + newState = sitesReducer(Immutable.fromJS(newState), moveAction).toJS() assert.equal(Object.keys(newState.sites).length, 3) - assert.equal(Object.values(newState.sites)[2].location, 'https://www.brave.com/3') - assert.equal(Object.values(newState.sites)[2].order, 0) + assert.equal(Object.values(newState.sites)[2].location, url) + assert.equal(Object.values(newState.sites)[2].order, 1) }) }) }) diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index b9035f7b398..1e31ac6367a 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -779,20 +779,15 @@ describe('siteUtil', function () { describe('moveSite', function () { describe('order test', function () { describe('back to front', function () { - const destinationDetail = { - location: testUrl1, - partitionNumber: 0, - parentFolderId: 0, - order: 0 - } - const sourceDetail = { - location: testUrl2 + '4', - partitionNumber: 0, - parentFolderId: 0, - order: 3 - } + const destinationKey = 'https://brave.com/|0|0' + const sourceKey = 'http://example.com/4|0|0' const sites = { - 'https://brave.com/|0|0': destinationDetail, + 'https://brave.com/|0|0': { + location: testUrl1, + partitionNumber: 0, + parentFolderId: 0, + order: 0 + }, 'http://example.com/0|0': { location: testUrl2, partitionNumber: 0, @@ -805,7 +800,12 @@ describe('siteUtil', function () { parentFolderId: 0, order: 2 }, - 'http://example.com/4|0|0': sourceDetail + 'http://example.com/4|0|0': { + location: testUrl2 + '4', + partitionNumber: 0, + parentFolderId: 0, + order: 3 + } } it('prepend target', function () { @@ -836,8 +836,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), true, false, true) + sourceKey, + destinationKey, true, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('not prepend target', function () { @@ -868,26 +868,21 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, true) + sourceKey, + destinationKey, false, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) }) describe('front to back', function () { - const sourceDetail = { - location: testUrl1, - partitionNumber: 0, - parentFolderId: 0, - order: 0 - } - const destinationDetail = { - location: testUrl2 + '4', - partitionNumber: 0, - parentFolderId: 0, - order: 3 - } + const sourceKey = 'https://brave.com/|0|0' + const destinationKey = 'http://example.com/4|0|0' const sites = { - 'https://brave.com/|0|0': sourceDetail, + 'https://brave.com/|0|0': { + location: testUrl1, + partitionNumber: 0, + parentFolderId: 0, + order: 0 + }, 'http://example.com/0|0': { location: testUrl2, partitionNumber: 0, @@ -900,7 +895,12 @@ describe('siteUtil', function () { parentFolderId: 0, order: 2 }, - 'http://example.com/4|0|0': destinationDetail + 'http://example.com/4|0|0': { + location: testUrl2 + '4', + partitionNumber: 0, + parentFolderId: 0, + order: 3 + } } it('prepend target', function () { @@ -931,8 +931,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), true, false, true) + sourceKey, + destinationKey, true, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('not prepend target', function () { @@ -963,13 +963,14 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, true) + sourceKey, + destinationKey, false, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) }) }) it('destination is parent', function () { + const sourceKey = 'https://brave.com/|0|0' const sourceDetail = { location: testUrl1, partitionNumber: 0, @@ -996,8 +997,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, true, false) + sourceKey, + '1', false, true, false) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('reparent', function () { @@ -1027,8 +1028,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, false) + 'https://brave.com/|0|1', + '1', false, false, false) assert.deepEqual(processedSites.toJS(), expectedSites) }) })