From 902debdd92afaeed1f013013823b58fd903ed42d Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Sat, 29 Apr 2017 00:17:19 -0700 Subject: [PATCH] Looking up site using tab details now considers provisionalLocation Result returned now includes parentFolderId Fixes https://github.com/brave/browser-laptop/issues/8477 Auditors: @bbondy, @NejcZdovc cc: @darkdh Includes: Fixes per feedback per @darkdh --- app/browser/reducers/sitesReducer.js | 7 +- js/state/siteUtil.js | 63 +++++++++++++- test/unit/state/siteUtilTest.js | 119 +++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 7 deletions(-) diff --git a/app/browser/reducers/sitesReducer.js b/app/browser/reducers/sitesReducer.js index c95543cf31..b39d5d914d 100644 --- a/app/browser/reducers/sitesReducer.js +++ b/app/browser/reducers/sitesReducer.js @@ -97,11 +97,12 @@ const sitesReducer = (state, action, emitChanges) => { console.warn('Trying to pin a tabId which does not exist:', action.tabId, 'tabs: ', state.get('tabs').toJS()) break } - const siteDetail = siteUtil.getDetailFromTab(tab, siteTags.PINNED) + const sites = state.get('sites') + const siteDetail = siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites) if (action.pinned) { - state = state.set('sites', siteUtil.addSite(state.get('sites'), siteDetail, siteTags.PINNED)) + state = state.set('sites', siteUtil.addSite(sites, siteDetail, siteTags.PINNED)) } else { - state = state.set('sites', siteUtil.removeSite(state.get('sites'), siteDetail, siteTags.PINNED)) + state = state.set('sites', siteUtil.removeSite(sites, siteDetail, siteTags.PINNED)) } if (syncEnabled()) { state = syncUtil.updateSiteCache(state, siteDetail) diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 0bda286dd5..8fd54316d3 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -444,14 +444,69 @@ module.exports.getDetailFromFrame = function (frame, tag) { }) } -module.exports.getDetailFromTab = function (tab, tag) { +const getSitesBySubkey = (sites, siteKey, tag) => { + if (!sites || !siteKey) { + return makeImmutable([]) + } + const splitKey = siteKey.split('|', 2) + const partialKey = splitKey.join('|') + const matches = sites.filter((site, key) => { + if (key.indexOf(partialKey) > -1 && (!tag || (tag && site.get('tags').includes(tag)))) { + return true + } + return false + }) + return matches.toList() +} + +module.exports.getDetailFromTab = function (tab, tag, sites) { + let location = tab.get('url') + const partitionNumber = tab.get('partitionNumber') !== undefined + ? tab.get('partitionNumber') + : undefined + let parentFolderId + + // if site map is available, look up extra information: + // - original url (if redirected) + // - parent folder id + if (sites) { + let results = makeImmutable([]) + + // get all sites matching URL and partition (disregarding parentFolderId) + let siteKey = module.exports.getSiteKey(makeImmutable({location, partitionNumber})) + results = results.merge(getSitesBySubkey(sites, siteKey, tag)) + + // only check for provisional location if entry is not found + if (results.size === 0) { + // if provisional location is different, grab any results which have that URL + // this may be different if the site was redirected + const provisionalLocation = tab.getIn(['frame', 'provisionalLocation']) + if (provisionalLocation && provisionalLocation !== location) { + siteKey = module.exports.getSiteKey(makeImmutable({ + location: provisionalLocation, + partitionNumber + })) + results = results.merge(getSitesBySubkey(sites, siteKey, tag)) + } + } + + // update details which get returned below + if (results.size > 0) { + location = results.getIn([0, 'location']) + parentFolderId = results.getIn([0, 'parentFolderId']) + } + } + const siteDetail = { - location: tab.get('url'), + location: location, title: tab.get('title'), tags: tag ? [tag] : [] } - if (tab.get('partitionNumber') !== undefined) { - siteDetail.partitionNumber = tab.get('partitionNumber') + if (partitionNumber) { + siteDetail.partitionNumber = partitionNumber + } + if (parentFolderId) { + siteDetail.parentFolderId = parentFolderId } return Immutable.fromJS(siteDetail) } diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index 801128e40e..da3b0027cf 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -1081,6 +1081,125 @@ describe('siteUtil', function () { } ) }) + + describe('when considering provisional location', function () { + const originalUrl = 'https://brave.com/oldUrl' + const newUrl = 'https://brave.com/newUrl' + const tab = Immutable.fromJS({ + url: newUrl, + frame: { + src: 'about:blank', + provisionalLocation: originalUrl + }, + title: '41', + partitionNumber: 7 + }) + const oldNotPinned = Immutable.fromJS({ + tags: [], + location: originalUrl, + title: 'site title', + partitionNumber: 7, + lastAccessedTime: 123 + }) + const oldPinned = oldNotPinned.set('tags', [siteTags.PINNED]) + const newNotPinned = oldNotPinned.set('location', newUrl) + const newPinned = newNotPinned.set('tags', [siteTags.PINNED]) + + it('returns `url` if both it and `provisionalLocation` are pinned', function () { + let sites = siteUtil.addSite(emptySites, oldPinned, siteTags.PINNED) + sites = siteUtil.addSite(sites, newPinned, siteTags.PINNED) + assert.deepEqual( + siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites).toJS(), + { + location: newUrl, + title: tab.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber') + } + ) + }) + it('returns `url` if that was pinned (and `provisionalLocation` is not pinned)', function () { + let sites = siteUtil.addSite(emptySites, oldNotPinned) + sites = siteUtil.addSite(sites, newPinned, siteTags.PINNED) + assert.deepEqual( + siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites).toJS(), + { + location: newUrl, + title: tab.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber') + } + ) + }) + it('returns `provisionalLocation` if it was pinned (and `url` (redirected) is not pinned)', function () { + let sites = siteUtil.addSite(emptySites, oldPinned, siteTags.PINNED) + sites = siteUtil.addSite(sites, newNotPinned) + assert.deepEqual( + siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites).toJS(), + { + location: originalUrl, + title: tab.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber') + } + ) + }) + it('returns `url` if `provisionalLocation` is falsey', function () { + const tab2 = tab.setIn(['frame', 'provisionalLocation'], undefined) + let sites = siteUtil.addSite(emptySites, oldPinned, siteTags.PINNED) + sites = siteUtil.addSite(sites, newNotPinned) + assert.deepEqual( + siteUtil.getDetailFromTab(tab2, siteTags.PINNED, sites).toJS(), + { + location: newUrl, + title: tab2.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber') + } + ) + }) + it('returns `url` if `sites` is falsey', function () { + assert.deepEqual( + siteUtil.getDetailFromTab(tab, siteTags.PINNED).toJS(), + { + location: newUrl, + title: tab.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber') + } + ) + }) + }) + + describe('when considering parentFolderId', function () { + const siteUrl = 'https://brave.com/oldUrl' + const tab = Immutable.fromJS({ + url: siteUrl, + title: '41', + partitionNumber: 7 + }) + const siteWithFolder = Immutable.fromJS({ + location: siteUrl, + title: 'site title', + partitionNumber: 7, + lastAccessedTime: 123, + tags: [siteTags.PINNED], + parentFolderId: 10 + }) + it('returns parentFolderId', function () { + const sites = siteUtil.addSite(emptySites, siteWithFolder, siteTags.PINNED) + assert.deepEqual( + siteUtil.getDetailFromTab(tab, siteTags.PINNED, sites).toJS(), + { + location: siteUrl, + title: tab.get('title'), + tags: [siteTags.PINNED], + partitionNumber: tab.get('partitionNumber'), + parentFolderId: siteWithFolder.get('parentFolderId') + } + ) + }) + }) }) describe('getDetailFromCreateProperties', function () {