diff --git a/app/common/lib/historyUtil.js b/app/common/lib/historyUtil.js index e42e76c8821..c3cade0a637 100644 --- a/app/common/lib/historyUtil.js +++ b/app/common/lib/historyUtil.js @@ -17,7 +17,7 @@ const sortTimeDescending = (left, right) => { } module.exports.getHistory = (sites) => { - sites = makeImmutable(sites) || new Immutable.List() + sites = makeImmutable(sites) ? makeImmutable(sites).toList() : new Immutable.List() return sites.filter((site) => siteUtil.isHistoryEntry(site)) .sort(sortTimeDescending) .slice(0, aboutHistoryMaxEntries) diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 3d46147de5c..0cbb4484d20 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -66,7 +66,7 @@ const removeDuplicateDomains = (list) => { */ const getTopSites = (state) => { // remove folders; sort by visit count; enforce a max limit - const sites = (state.get('sites') || new Immutable.List()) + const sites = (state.get('sites') ? state.get('sites').toList() : new Immutable.List()) .filter((site) => !siteUtil.isFolder(site)) .filter((site) => !isSourceAboutUrl(site.get('location'))) .sort(sortCountDescending) diff --git a/app/importer.js b/app/importer.js index ad937d1c90b..471f204a811 100644 --- a/app/importer.js +++ b/app/importer.js @@ -23,6 +23,7 @@ const locale = require('./locale') var isMergeFavorites = false var isImportingBookmarks = false var hasBookmarks +var importedSites exports.init = () => { importer.initialize() @@ -171,6 +172,7 @@ importer.on('add-bookmarks', (e, bookmarks, topLevelFolder) => { sites.push(site) } } + importedSites = Immutable.fromJS(sites) appActions.addSite(Immutable.fromJS(sites)) }) @@ -187,7 +189,7 @@ importer.on('add-favicons', (e, detail) => { } } }) - let sites = AppStore.getState().get('sites') + let sites = importedSites sites = sites.map((site) => { if ((site.get('favicon') === undefined && site.get('location') !== undefined && faviconMap[site.get('location')] !== undefined) || diff --git a/app/renderer/components/bookmarksToolbar.js b/app/renderer/components/bookmarksToolbar.js index 90db85f268c..e4b5b2cfef5 100644 --- a/app/renderer/components/bookmarksToolbar.js +++ b/app/renderer/components/bookmarksToolbar.js @@ -319,17 +319,18 @@ class BookmarksToolbar extends ImmutableComponent { // Loop through until we fill up the entire bookmark toolbar width let i - for (i = 0; i < noParentItems.size; i++) { + let noParentItemsList = noParentItems.toList() + for (i = 0; i < noParentItemsList.size; i++) { let iconWidth = props.showFavicon ? iconSize : 0 // font-awesome file icons are 3px smaller - if (props.showFavicon && !noParentItems.getIn([i, 'folderId']) && !noParentItems.getIn([i, 'favicon'])) { + if (props.showFavicon && !noParentItemsList.getIn([i, 'folderId']) && !noParentItemsList.getIn([i, 'favicon'])) { iconWidth -= 3 } - const chevronWidth = props.showFavicon && noParentItems.getIn([i, 'folderId']) ? this.chevronWidth : 0 + const chevronWidth = props.showFavicon && noParentItemsList.getIn([i, 'folderId']) ? this.chevronWidth : 0 if (props.showFavicon && props.showOnlyFavicon) { widthAccountedFor += this.padding + iconWidth + chevronWidth } else { - const text = noParentItems.getIn([i, 'customTitle']) || noParentItems.getIn([i, 'title']) || noParentItems.getIn([i, 'location']) + const text = noParentItemsList.getIn([i, 'customTitle']) || noParentItemsList.getIn([i, 'title']) || noParentItemsList.getIn([i, 'location']) widthAccountedFor += Math.min(calculateTextWidth(text, `${this.fontSize} ${this.fontFamily}`) + this.padding + iconWidth + chevronWidth, this.maxWidth) } widthAccountedFor += margin @@ -337,9 +338,18 @@ class BookmarksToolbar extends ImmutableComponent { break } } - this.bookmarksForToolbar = noParentItems.take(i) + const bookmarkSort = (x, y) => { + if (x.get('order') < y.get('order')) { + return -1 + } else if (x.get('order') > y.get('order')) { + return 1 + } else { + return 0 + } + } + this.bookmarksForToolbar = noParentItems.take(i).sort(bookmarkSort) // Show at most 100 items in the overflow menu - this.overflowBookmarkItems = noParentItems.skip(i).take(100) + this.overflowBookmarkItems = noParentItems.skip(i).take(100).sort(bookmarkSort) } componentWillMount () { this.updateBookmarkData(this.props) diff --git a/app/sessionStore.js b/app/sessionStore.js index 65c0f3bffca..059a58374c0 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -34,6 +34,20 @@ const getSetting = require('../js/settings').getSetting const promisify = require('../js/lib/promisify') const sessionStorageName = `session-store-${sessionStorageVersion}` +const getTopSiteMap = () => { + if (Array.isArray(topSites) && topSites.length) { + let siteMap = {} + let order = 0 + topSites.forEach((site) => { + let key = siteUtil.getSiteKey(Immutable.fromJS(site)) + site.order = order++ + siteMap[key] = site + }) + return siteMap + } + return {} +} + const getStoragePath = () => { return path.join(app.getPath('userData'), sessionStorageName) } @@ -464,6 +478,18 @@ module.exports.loadAppState = () => { } } data = setVersionInformation(data) + + // sites refactoring migration + if (Array.isArray(data.sites) && data.sites.length) { + let sites = {} + let order = 0 + data.sites.forEach((site) => { + let key = siteUtil.getSiteKey(Immutable.fromJS(site)) + site.order = order++ + sites[key] = site + }) + data.sites = sites + } } catch (e) { // TODO: Session state is corrupted, maybe we should backup this // corrupted value for people to report into support. @@ -484,7 +510,7 @@ module.exports.loadAppState = () => { module.exports.defaultAppState = () => { return { firstRunTimestamp: new Date().getTime(), - sites: topSites, + sites: getTopSiteMap(), tabs: [], extensions: {}, visits: [], diff --git a/docs/state.md b/docs/state.md index ac78b58a578..946288f88da 100644 --- a/docs/state.md +++ b/docs/state.md @@ -34,19 +34,21 @@ AppStore } } }, - sites: [{ - location: string, - title: string, - customTitle: string, // User provided title for bookmark; overrides title - tags: [string], // empty, 'bookmark', 'bookmark-folder', 'pinned', or 'reader' - favicon: string, // URL of the favicon - themeColor: string, // css compatible color string - lastAccessedTime: number, // datetime.getTime() - creationTime: number, //creation time of bookmark - partitionNumber: number, // Optionally specifies a specific session - folderId: number, // Set for bookmark folders only - parentFolderId: number // Set for bookmarks and bookmark folders only - }], + sites: { + [siteKey]: { // folder: folderId; bookmark/history: location + partitionNumber + parentFolderId + location: string, + title: string, + customTitle: string, // User provided title for bookmark; overrides title + tags: [string], // empty, 'bookmark', 'bookmark-folder', 'pinned', or 'reader' + favicon: string, // URL of the favicon + themeColor: string, // css compatible color string + lastAccessedTime: number, // datetime.getTime() + creationTime: number, //creation time of bookmark + partitionNumber: number, // Optionally specifies a specific session + folderId: number, // Set for bookmark folders only + parentFolderId: number // Set for bookmarks and bookmark folders only + }, + }, downloads: [{ [downloadId]: { startTime: number, // datetime.getTime() diff --git a/js/components/frame.js b/js/components/frame.js index c3fb05fcddf..aeeb20474bd 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -49,6 +49,16 @@ function isTorrentViewerURL (url) { return isEnabled && isSourceMagnetUrl(url) } +const bookmarkSort = (x, y) => { + if (x.get('order') < y.get('order')) { + return -1 + } else if (x.get('order') > y.get('order')) { + return 1 + } else { + return 0 + } +} + class Frame extends ImmutableComponent { constructor () { super() @@ -101,7 +111,7 @@ class Frame extends ImmutableComponent { this.webview.send(messages.BRAVERY_DEFAULTS_UPDATED, this.braveryDefaults) } else if (location === 'about:bookmarks') { this.webview.send(messages.BOOKMARKS_UPDATED, { - bookmarks: this.props.bookmarks.toJS(), + bookmarks: this.props.bookmarks.toList().sort(bookmarkSort).toJS(), bookmarkFolders: this.props.bookmarkFolders.toJS() }) } else if (location === 'about:history') { diff --git a/js/contextMenus.js b/js/contextMenus.js index 4251cbbfbee..78d08632187 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -360,7 +360,8 @@ function showBookmarkFolderInit (allBookmarkItems, parentBookmarkFolder, activeF function bookmarkItemsInit (allBookmarkItems, items, activeFrame) { const btbMode = getSetting(settings.BOOKMARKS_TOOLBAR_MODE) const showFavicon = (btbMode === bookmarksToolbarMode.TEXT_AND_FAVICONS || btbMode === bookmarksToolbarMode.FAVICONS_ONLY) - const template = items.map((site) => { + const itemsList = items.toList() + const template = itemsList.map((site) => { const isFolder = siteUtil.isFolder(site) let faIcon if (showFavicon && !site.get('favicon')) { diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index b85aaaad1ae..4ce59ecc193 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -9,6 +9,7 @@ const settings = require('../constants/settings') const getSetting = require('../settings').getSetting const UrlUtil = require('../lib/urlutil') const urlParse = require('url').parse +const {makeImmutable} = require('../../app/common/state/immutableUtil') const isBookmark = (tags) => { if (!tags) { @@ -25,22 +26,37 @@ const isBookmarkFolder = (tags) => { tags && typeof tags !== 'string' && tags.includes(siteTags.BOOKMARK_FOLDER) } +const reorderSite = (sites, order) => { + sites = sites.map((site) => { + const siteOrder = site.get('order') + if (siteOrder > order) { + return site.set('order', siteOrder - 1) + } + return site + }) + return sites +} + /** - * Obtains the index of the location in sites + * Calculate siteKey for siteDetail * - * @param sites The application state's Immutable sites list - * @param siteDetail The siteDetails entry to get the index of - * @param tags Tag for siteDetail (ex: bookmark). Folders are searched differently than other entries - * @return index of the siteDetail or -1 if not found. + * @param siteDetail The site to to be calculated + * @return key if siteDetail is valid */ -module.exports.getSiteIndex = function (sites, siteDetail, tags) { - if (!sites || !siteDetail) { - return -1 +module.exports.getSiteKey = function (siteDetail) { + if (!siteDetail) { + return null } - if (isBookmarkFolder(tags)) { - return sites.findIndex((site) => isBookmarkFolder(site.get('tags')) && site.get('folderId') === siteDetail.get('folderId')) + const folderId = siteDetail.get('folderId') + const location = siteDetail.get('location') + if (folderId) { + return folderId.toString() + } else if (location) { + return location + + (siteDetail.get('partitionNumber') || 0) + + (siteDetail.get('parentFolderId') || 0) } - return sites.findIndex((site) => site.get('location') === siteDetail.get('location') && (site.get('partitionNumber') || 0) === (siteDetail.get('partitionNumber') || 0)) + return null } /** @@ -51,11 +67,14 @@ module.exports.getSiteIndex = function (sites, siteDetail, tags) { * @return true if the location is already bookmarked */ module.exports.isSiteBookmarked = function (sites, siteDetail) { - const index = module.exports.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK) - if (index === -1) { + if (!sites) { + return false + } + const key = module.exports.getSiteKey(siteDetail) + if (key === null) { return false } - return isBookmark(sites.get(index).get('tags')) + return isBookmark(sites.getIn([key, 'tags'])) } const getNextFolderIdItem = (sites) => @@ -85,7 +104,7 @@ module.exports.getNextFolderId = (sites) => { // Some details can be copied from the existing siteDetail if null // ex: parentFolderId, partitionNumber, and favicon -const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => { +const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) => { let tags = oldSiteDetail && oldSiteDetail.get('tags') || new Immutable.List() if (tag) { tags = tags.toSet().add(tag).toList() @@ -102,12 +121,17 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => { lastAccessedTime = newSiteDetail.get('lastAccessedTime') || new Date().getTime() } - let site = Immutable.fromJS({ + let site = makeImmutable({ lastAccessedTime: lastAccessedTime, tags, - title: newSiteDetail.get('title') + title: newSiteDetail.get('title'), + order }) + if (oldSiteDetail && oldSiteDetail.get('order') !== undefined) { + site = site.set('order', oldSiteDetail.get('order')) + } + if (newSiteDetail.get('location')) { site = site.set('location', newSiteDetail.get('location')) } @@ -162,8 +186,13 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { tag = siteDetail.getIn(['tags', 0]) } - const index = module.exports.getSiteIndex(sites, originalSiteDetail || siteDetail, tag) - const oldSite = index !== -1 ? sites.getIn([index]) : null + let originalSiteKey + if (originalSiteDetail) { + originalSiteKey = module.exports.getSiteKey(originalSiteDetail) + } + + const oldKey = originalSiteKey || module.exports.getSiteKey(siteDetail) + const oldSite = oldKey !== null ? sites.get(oldKey) : null let folderId = siteDetail.get('folderId') if (tag === siteTags.BOOKMARK_FOLDER) { @@ -173,7 +202,7 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { site.get('parentFolderId') === siteDetail.get('parentFolderId') && site.get('customTitle') === siteDetail.get('customTitle')) if (dupFolder) { - sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER) + sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER, true) } } else if (!folderId) { // Assign an id if this is a new folder @@ -181,13 +210,13 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { } } - let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId) - if (index === -1) { - // Insert new entry - return sites.push(site) + let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId, sites.size) + + const key = originalSiteKey || module.exports.getSiteKey(site) + if (key === null) { + return sites } - // Update existing entry - return sites.setIn([index], site) + return sites.set(key, site) } /** @@ -195,38 +224,29 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) { * * @param sites The application state's Immutable sites list * @param siteDetail The siteDetail to remove a tag from + * @param reorder whether to reorder sites (default with reorder) * @return The new sites Immutable object */ -module.exports.removeSite = function (sites, siteDetail, tag) { - const index = module.exports.getSiteIndex(sites, siteDetail, tag) - if (index === -1) { - return sites - } +module.exports.removeSite = function (sites, siteDetail, tag, reorder = true) { + const key = module.exports.getSiteKey(siteDetail) - const tags = sites.getIn([index, 'tags']) + const tags = sites.getIn([key, 'tags']) if (isBookmarkFolder(tags)) { - const folderId = sites.getIn([index, 'folderId']) + const folderId = sites.getIn([key, 'folderId']) const childSites = sites.filter((site) => site.get('parentFolderId') === folderId) childSites.forEach((site) => { const tags = site.get('tags') tags.forEach((tag) => { - sites = module.exports.removeSite(sites, site, tag) + sites = module.exports.removeSite(sites, site, tag, false) }) }) } - if (tags.size === 0 && !tag) { - // If called without tags and entry has no tags, remove the entry - return sites.splice(index, 1) - } else if (tags.size > 0 && !tag) { - // If called without tags BUT entry has tags, null out lastAccessedTime. - // This is a bookmark entry that we want to clear history for (but NOT delete/untag bookmark) - return sites.setIn([index, 'lastAccessedTime'], null) + if (sites.size && reorder) { + const order = sites.getIn([key, 'order']) + sites = reorderSite(sites, order) } - // Else, remove the specified tag - return sites - .setIn([index, 'parentFolderId'], 0) - .deleteIn([index, 'customTitle']) - .setIn([index, 'tags'], tags.toSet().remove(tag).toList()) + + return sites.delete(key) } /** @@ -277,28 +297,40 @@ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => { * @param disallowReparent If set to true, parent folder will not be set * @return The new sites Immutable object */ -module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, destinationIsParent, disallowReparent) { +module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, + destinationIsParent, disallowReparent) { if (!module.exports.isMoveAllowed(sites, sourceDetail, destinationDetail)) { return sites } - const sourceSiteIndex = module.exports.getSiteIndex(sites, sourceDetail, sourceDetail.get('tags')) + let sourceKey = module.exports.getSiteKey(sourceDetail) + let 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 } else { - destinationSiteIndex = module.exports.getSiteIndex(sites, destinationDetail, destinationDetail.get('tags')) + destinationSiteIndex = sites.getIn([destinationKey, 'order']) } let newIndex = destinationSiteIndex + (prepend ? 0 : 1) - let sourceSite = sites.get(sourceSiteIndex) - let destinationSite = sites.get(destinationSiteIndex) - sites = sites.splice(sourceSiteIndex, 1) + let sourceSite = sites.get(sourceKey) + let 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) + } + return site + }) if (newIndex > sourceSiteIndex) { newIndex-- } + sourceSite = sourceSite.set('order', newIndex) if (!disallowReparent) { if (destinationIsParent && destinationDetail.get('folderId') !== sourceSite.get('folderId')) { @@ -309,7 +341,8 @@ module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prep sourceSite = sourceSite.set('parentFolderId', destinationSite.get('parentFolderId')) } } - return sites.splice(newIndex, 0, sourceSite) + sourceKey = module.exports.getSiteKey(sourceSite) + return sites.set(sourceKey, sourceSite) } module.exports.getDetailFromFrame = function (frame, tag) { @@ -318,7 +351,7 @@ module.exports.getDetailFromFrame = function (frame, tag) { location = frame.get('pinnedLocation') } - return Immutable.fromJS({ + return makeImmutable({ location, title: frame.get('title'), partitionNumber: frame.get('partitionNumber'), @@ -338,9 +371,14 @@ module.exports.getDetailFromFrame = function (frame, tag) { * @param favicon favicon URL */ module.exports.updateSiteFavicon = function (sites, location, favicon) { + sites = makeImmutable(sites) + if (UrlUtil.isNotURL(location)) { return sites } + if (!Immutable.Map.isMap(sites)) { + return sites + } const matchingIndices = [] @@ -521,14 +559,6 @@ module.exports.clearHistory = function (sites) { return bookmarks } -/** - * Determines if the sites list has any sites with no tags - * @param sites The application state's Immutable sites list. - */ -module.exports.hasNoTagSites = function (sites) { - return sites.findIndex((site) => !site.get('tags') || site.get('tags').size === 0) !== -1 -} - /** * Returns all sites that have a bookmark tag. * @param sites The application state's Immutable sites list. @@ -538,7 +568,7 @@ module.exports.getBookmarks = function (sites) { if (sites) { return sites.filter((site) => isBookmarkFolder(site.get('tags')) || isBookmark(site.get('tags'))) } - return [] + return makeImmutable({}) } /** diff --git a/js/stores/appStore.js b/js/stores/appStore.js index affd9760b28..35bc134ea11 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -446,10 +446,15 @@ const handleAppAction = (action) => { appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), s, action.tag)) }) } else { - appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), action.siteDetail, action.tag, action.originalSiteDetail)) + let sites = appState.get('sites') + 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)) } if (action.destinationDetail) { - appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.siteDetail, action.destinationDetail, false, false, true)) + appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), + action.siteDetail, action.destinationDetail, false, false, true)) } // If there was an item added then clear out the old history entries if (oldSiteSize !== appState.get('sites').size) { @@ -459,13 +464,17 @@ const handleAppAction = (action) => { appState = aboutHistoryState.setHistory(appState, action) break case AppConstants.APP_REMOVE_SITE: - appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag)) + appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag, true)) appState = aboutNewTabState.setSites(appState, action) appState = aboutHistoryState.setHistory(appState, action) break case AppConstants.APP_MOVE_SITE: - appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.sourceDetail, action.destinationDetail, action.prepend, action.destinationIsParent, false)) - break + { + appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), + action.sourceDetail, action.destinationDetail, action.prepend, + action.destinationIsParent, false)) + break + } case AppConstants.APP_MERGE_DOWNLOAD_DETAIL: if (action.downloadDetail) { appState = appState.mergeIn(['downloads', action.downloadId], action.downloadDetail) diff --git a/test/app/sessionStoreTest.js b/test/app/sessionStoreTest.js index bfa318a107e..70d13d5e475 100644 --- a/test/app/sessionStoreTest.js +++ b/test/app/sessionStoreTest.js @@ -1,8 +1,10 @@ /* global describe, it, before, after */ const Brave = require('../lib/brave') +const Immutable = require('immutable') const {navigator, urlInput, navigatorBookmarked, navigatorNotBookmarked} = require('../lib/selectors') const siteTags = require('../../js/constants/siteTags') +const siteUtil = require('../../js/state/siteUtil') describe('sessionStore', function () { function * setup (client) { @@ -13,6 +15,11 @@ describe('sessionStore', function () { Brave.beforeAllServerSetup(this) before(function * () { const page1Url = Brave.server.url('page1.html') + const site = { + location: page1Url, + title: 'some page' + } + const key = siteUtil.getSiteKey(Immutable.fromJS(site)) yield Brave.startApp() yield setup(Brave.app.client) yield Brave.app.client @@ -22,14 +29,11 @@ describe('sessionStore', function () { .windowParentByUrl(page1Url) .moveToObject(navigator) .waitForExist(navigatorNotBookmarked) - yield Brave.app.client.addSite({ - location: page1Url, - title: 'some page' - }, siteTags.BOOKMARK) + yield Brave.app.client.addSite(site, siteTags.BOOKMARK) .waitUntil(function () { return this.getAppState().then((val) => { let state = val.value - return state.sites.length === 1 && state.sites[0].location === page1Url + return state.sites && state.sites[key].location === page1Url }) }) yield Brave.stopApp(false) diff --git a/test/components/urlBarSuggestionsTest.js b/test/components/urlBarSuggestionsTest.js index 88e6e079ca9..12119b299ea 100644 --- a/test/components/urlBarSuggestionsTest.js +++ b/test/components/urlBarSuggestionsTest.js @@ -1,6 +1,7 @@ /* global describe, it, beforeEach */ const Brave = require('../lib/brave') +const Immutable = require('immutable') const messages = require('../../js/constants/messages') const {urlInput, urlBarSuggestions} = require('../lib/selectors') @@ -26,7 +27,7 @@ describe('urlBarSuggestions', function () { .windowByUrl(Brave.browserWindowUrl) .waitUntil(function () { return this.getAppState().then((val) => { - return !!val.value.sites.find((site) => site.location === page1Url) + return !!Immutable.fromJS(val.value.sites).find((site) => site.get('location') === page1Url) }) }) .tabByIndex(0) @@ -39,7 +40,7 @@ describe('urlBarSuggestions', function () { .waitForElementFocus(urlInput) .waitUntil(function () { return this.getAppState().then((val) => { - return !!val.value.sites.find((site) => site.location === page2Url) + return !!Immutable.fromJS(val.value.sites).find((site) => site.get('location') === page2Url) }) }) }) diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index a323cd47b66..a3a710a2f1b 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -9,7 +9,7 @@ describe('siteUtil', function () { const testUrl1 = 'https://brave.com/' const testUrl2 = 'http://example.com/' const testFavicon1 = 'https://brave.com/favicon.ico' - const emptySites = Immutable.fromJS([]) + const emptySites = Immutable.fromJS({}) const bookmarkAllFields = Immutable.fromJS({ lastAccessedTime: 123, tags: [siteTags.BOOKMARK], @@ -33,143 +33,102 @@ describe('siteUtil', function () { title: 'sample' }) - describe('getSiteIndex', function () { - it('returns -1 if sites is falsey', function () { - const siteDetail = Immutable.fromJS({ - folderId: 0 - }) - const index = siteUtil.getSiteIndex(null, siteDetail, siteTags.BOOKMARK_FOLDER) - assert.equal(index, -1) - }) - it('returns -1 if siteDetail is falsey', function () { - const sites = Immutable.fromJS([{ - folderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }]) - const index = siteUtil.getSiteIndex(sites, null, siteTags.BOOKMARK_FOLDER) - assert.equal(index, -1) + describe('getSiteKey', function () { + it('returns null if siteDetail is falsey', function () { + const key = siteUtil.getSiteKey(null) + assert.equal(key, null) }) describe('matching `BOOKMARK_FOLDER`', function () { - it('returns index if folderId matches', function () { - const sites = Immutable.fromJS([{ - folderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }]) - const siteDetail = Immutable.fromJS({ - folderId: 0 - }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK_FOLDER) - assert.equal(index, 0) - }) - it('returns -1 if folderId does not match', function () { - const sites = Immutable.fromJS([{ - folderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }]) + it('returns key if folderId matches', function () { const siteDetail = Immutable.fromJS({ folderId: 1 }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK_FOLDER) - assert.equal(index, -1) + const key = siteUtil.getSiteKey(siteDetail) + assert.equal(key, 1) + }) + it('returns null if folderId is missing', function () { + const siteDetail = new Immutable.Map() + const key = siteUtil.getSiteKey(siteDetail) + assert.equal(key, null) }) }) describe('matching `BOOKMARK`', function () { - it('returns index if location and partitionNumber match', function () { - const sites = Immutable.fromJS([{ - location: testUrl1, - partitionNumber: 0, - tags: [siteTags.BOOKMARK] - }]) + it('returns key if location and partitionNumber match', function () { const siteDetail = Immutable.fromJS({ location: testUrl1, partitionNumber: 0 }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK) - assert.equal(index, 0) + const key = siteUtil.getSiteKey(siteDetail) + assert.equal(key, testUrl1 + '00') }) - it('returns index if location matches and partitionNumber is NOT present', function () { - const sites = Immutable.fromJS([{ - location: testUrl1, - tags: [siteTags.BOOKMARK] - }]) + it('returns key if location matches and partitionNumber is NOT present', function () { const siteDetail = Immutable.fromJS({ location: testUrl1 }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK) - assert.equal(index, 0) + const key = siteUtil.getSiteKey(siteDetail) + assert.equal(key, testUrl1 + '00') }) - it('returns -1 if location does not match', function () { - const sites = Immutable.fromJS([{ - location: testUrl1, - tags: [siteTags.BOOKMARK] - }]) - const siteDetail = Immutable.fromJS({ - location: testUrl2 - }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK) - assert.equal(index, -1) - }) - it('returns -1 if partitionNumber does not match', function () { - const sites = Immutable.fromJS([{ - location: testUrl1, - partitionNumber: 0, - tags: [siteTags.BOOKMARK] - }]) - const siteDetail = Immutable.fromJS({ - location: testUrl1, - partitionNumber: 1 - }) - const index = siteUtil.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK) - assert.equal(index, -1) + it('returns null if location is missing', function () { + const siteDetail = new Immutable.Map() + const key = siteUtil.getSiteKey(siteDetail) + assert.equal(key, null) }) }) }) describe('isSiteBookmarked', function () { it('returns true if site is bookmarked', function () { - const sites = Immutable.fromJS([{ + const site = { location: testUrl1, tags: [siteTags.BOOKMARK] - }]) - const siteDetail = Immutable.fromJS({ - location: testUrl1 - }) - const result = siteUtil.isSiteBookmarked(sites, siteDetail) + } + const siteDetail = Immutable.fromJS(site) + const key = siteUtil.getSiteKey(siteDetail) + const sites = {} + sites[key] = site + const result = siteUtil.isSiteBookmarked(Immutable.fromJS(sites), siteDetail) assert.equal(result, true) }) it('returns false if site is not bookmarked', function () { - const sites = Immutable.fromJS([{ + const site = { + location: testUrl2, + tags: [siteTags.BOOKMARK] + } + const key = siteUtil.getSiteKey(Immutable.fromJS(site)) + const sites = {} + sites[key] = site + const result = siteUtil.isSiteBookmarked(Immutable.fromJS(sites), Immutable.fromJS({ location: testUrl1, tags: [siteTags.BOOKMARK] - }]) - const siteDetail = Immutable.fromJS({ - location: testUrl2 - }) - const result = siteUtil.isSiteBookmarked(sites, siteDetail) + })) assert.equal(result, false) }) it('returns false if site is a bookmark folder', function () { - const sites = Immutable.fromJS([{ + const site = { folderId: 0, tags: [siteTags.BOOKMARK_FOLDER] - }]) - const siteDetail = Immutable.fromJS({ - folderId: 0 - }) - const result = siteUtil.isSiteBookmarked(sites, siteDetail) + } + const siteDetail = Immutable.fromJS(site) + const key = siteUtil.getSiteKey(siteDetail) + const sites = {} + sites[key] = site + const result = siteUtil.isSiteBookmarked(Immutable.fromJS(sites), siteDetail) assert.equal(result, false) }) }) describe('getNextFolderId', function () { it('returns the next possible folderId', function () { - const sites = Immutable.fromJS([{ - folderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }, { - folderId: 1, - tags: [siteTags.BOOKMARK_FOLDER] - }]) + const sites = Immutable.fromJS({ + 'key1': { + folderId: 0, + tags: [siteTags.BOOKMARK_FOLDER] + }, + 'key2': { + folderId: 1, + tags: [siteTags.BOOKMARK_FOLDER] + } + }) assert.equal(siteUtil.getNextFolderId(sites), 2) }) it('returns default (0) if sites is falsey', function () { @@ -180,43 +139,53 @@ describe('siteUtil', function () { describe('addSite', function () { it('gets the tag from siteDetail if not provided', function () { const processedSites = siteUtil.addSite(emptySites, bookmarkAllFields) - const expectedSites = Immutable.fromJS([bookmarkAllFields]) - assert.deepEqual(processedSites.getIn([0, 'tags']), expectedSites.getIn([0, 'tags'])) + const processedKey = siteUtil.getSiteKey(bookmarkAllFields) + assert.deepEqual(processedSites.getIn([processedKey, 'tags']), bookmarkAllFields.get('tags')) }) describe('record count', function () { var processedSites it('create history record with count', function () { processedSites = siteUtil.addSite(emptySites, siteMinFields) - assert.deepEqual(processedSites.getIn([0, 'count']), 1) + const processedKey = siteUtil.getSiteKey(siteMinFields) + assert.deepEqual(processedSites.getIn([processedKey, 'count']), 1) }) it('increments count for history item', function () { processedSites = siteUtil.addSite(processedSites, siteMinFields) - assert.deepEqual(processedSites.getIn([0, 'count']), 2) + const processedKey = siteUtil.getSiteKey(siteMinFields) + assert.deepEqual(processedSites.getIn([processedKey, 'count']), 2) }) }) describe('for new entries (oldSite is null)', function () { describe('when adding bookmark', function () { it('preserves existing siteDetail fields', function () { const processedSites = siteUtil.addSite(emptySites, bookmarkAllFields, siteTags.BOOKMARK) - const expectedSites = Immutable.fromJS([bookmarkAllFields]) + const processedKey = siteUtil.getSiteKey(bookmarkAllFields) + let sites = {} + sites[processedKey] = bookmarkAllFields.set('order', 0).toJS() + const expectedSites = Immutable.fromJS(sites) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) it('sets 0 for lastAccessedTime if not specified', function () { const processedSites = siteUtil.addSite(emptySites, bookmarkMinFields, siteTags.BOOKMARK) - assert.equal(processedSites.getIn([0, 'lastAccessedTime']), 0) - assert.deepEqual(processedSites.getIn([0, 'tags']).toJS(), [siteTags.BOOKMARK]) + const processedKey = siteUtil.getSiteKey(bookmarkMinFields) + assert.equal(processedSites.getIn([processedKey, 'lastAccessedTime']), 0) + assert.deepEqual(processedSites.getIn([processedKey, 'tags']).toJS(), [siteTags.BOOKMARK]) }) }) describe('when adding bookmark folder', function () { it('assigns a folderId', function () { const processedSites = siteUtil.addSite(emptySites, folderMinFields) - const folderId = processedSites.getIn([0, 'folderId']) + const folderMinFieldsWithId = folderMinFields.set('folderId', 1) + const processedKey = siteUtil.getSiteKey(folderMinFieldsWithId) + const folderId = processedSites.getIn([processedKey, 'folderId']) assert.equal(folderId, 1) }) it('allows for new folders to use the same customTitle as an existing folder', function () { // Add a new bookmark folder let processedSites = siteUtil.addSite(emptySites, folderMinFields) - const folderId = processedSites.getIn([0, 'folderId']) + const folderMinFieldsWithId1 = folderMinFields.set('folderId', 1) + const processedKey1 = siteUtil.getSiteKey(folderMinFieldsWithId1) + const folderId = processedSites.getIn([processedKey1, 'folderId']) const bookmark = Immutable.fromJS({ lastAccessedTime: 123, title: 'bookmark1', @@ -226,65 +195,81 @@ describe('siteUtil', function () { }) // Add a bookmark into that folder processedSites = siteUtil.addSite(processedSites, bookmark) + const processedKey2 = siteUtil.getSiteKey(bookmark) assert.equal(processedSites.size, 2) - assert.equal(processedSites.getIn([1, 'parentFolderId']), folderId) + assert.equal(processedSites.getIn([processedKey2, 'parentFolderId']), folderId) // Add another bookmark folder with the same name / parentFolderId processedSites = siteUtil.addSite(processedSites, folderMinFields) + const folderMinFieldsWithId2 = folderMinFields.set('folderId', 2) + const processedKey3 = siteUtil.getSiteKey(folderMinFieldsWithId2) assert.equal(processedSites.size, 3) - const folderId2 = processedSites.getIn([2, 'folderId']) + const folderId2 = processedSites.getIn([processedKey3, 'folderId']) assert.equal(folderId === folderId2, false) // Ensure fields for both folders are still in sites array - assert.equal(processedSites.getIn([0, 'customTitle']), processedSites.getIn([2, 'customTitle'])) - assert.deepEqual(processedSites.getIn([0, 'tags']), processedSites.getIn([2, 'tags'])) + assert.equal(processedSites.getIn([processedKey1, 'customTitle']), + processedSites.getIn([processedKey3, 'customTitle'])) + assert.deepEqual(processedSites.getIn([processedKey1, 'tags']), processedSites.getIn([processedKey3, 'tags'])) }) it('calls removeSite on bookmark folders which have the same customTitle/parentFolderId', function () { - const sites = Immutable.fromJS([ - { - lastAccessedTime: 123, - customTitle: 'folder1', - title: undefined, - folderId: 1, - parentFolderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }, - { - lastAccessedTime: 123, - customTitle: 'folder2', - title: undefined, - folderId: 2, - parentFolderId: 1, - tags: [siteTags.BOOKMARK_FOLDER] - }, - { - lastAccessedTime: 123, - title: 'bookmark1', - parentFolderId: 1, - location: testUrl1, - tags: [siteTags.BOOKMARK] - }, - { - lastAccessedTime: 123, - title: 'bookmark2', - parentFolderId: 2, - location: testUrl2, - tags: [siteTags.BOOKMARK] - } - ]) - let processedSites = sites - sites.forEach((site) => { + let sites = {} + const site1 = { + lastAccessedTime: 123, + customTitle: 'folder1', + title: undefined, + folderId: 1, + parentFolderId: 0, + order: 0, + tags: [siteTags.BOOKMARK_FOLDER] + } + const site2 = { + lastAccessedTime: 123, + customTitle: 'folder2', + title: undefined, + folderId: 2, + parentFolderId: 1, + order: 1, + tags: [siteTags.BOOKMARK_FOLDER] + } + const site3 = { + lastAccessedTime: 123, + title: 'bookmark1', + parentFolderId: 1, + location: testUrl1, + order: 2, + tags: [siteTags.BOOKMARK] + } + const site4 = { + lastAccessedTime: 123, + title: 'bookmark2', + parentFolderId: 2, + location: testUrl2, + order: 3, + tags: [siteTags.BOOKMARK] + } + const siteKey1 = siteUtil.getSiteKey(Immutable.fromJS(site1)) + const siteKey2 = siteUtil.getSiteKey(Immutable.fromJS(site2)) + const siteKey3 = siteUtil.getSiteKey(Immutable.fromJS(site3)) + const siteKey4 = siteUtil.getSiteKey(Immutable.fromJS(site4)) + sites[siteKey1] = site1 + sites[siteKey2] = site2 + sites[siteKey3] = site3 + sites[siteKey4] = site4 + let processedSites = Immutable.fromJS(sites) + Immutable.fromJS(sites).forEach((site) => { processedSites = siteUtil.addSite(processedSites, site) }) - const expectedSites = sites + const expectedSites = Immutable.fromJS(sites) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) }) describe('when adding history', function () { it('sets default values for lastAccessedTime and tag when they are missing', function () { const processedSites = siteUtil.addSite(emptySites, bookmarkMinFields) - assert.equal(!!processedSites.getIn([0, 'lastAccessedTime']), true) - assert.deepEqual(processedSites.getIn([0, 'tags']).toJS(), []) + const processedKey = siteUtil.getSiteKey(bookmarkMinFields) + assert.equal(!!processedSites.getIn([processedKey, 'lastAccessedTime']), true) + assert.deepEqual(processedSites.getIn([processedKey, 'tags']).toJS(), []) }) }) }) @@ -297,8 +282,10 @@ describe('siteUtil', function () { customTitle: 'old customTitle', partitionNumber: 3, parentFolderId: 8, + order: 0, favicon: testFavicon1 }) + const oldSiteKey = siteUtil.getSiteKey(oldSiteDetail) const newSiteDetail = Immutable.fromJS({ lastAccessedTime: 456, tags: [siteTags.BOOKMARK], @@ -313,12 +300,16 @@ describe('siteUtil', function () { customTitle: oldSiteDetail.get('customTitle'), partitionNumber: oldSiteDetail.get('partitionNumber'), parentFolderId: oldSiteDetail.get('parentFolderId'), + order: oldSiteDetail.get('order'), favicon: oldSiteDetail.get('favicon') }) - const sites = Immutable.fromJS([oldSiteDetail]) - const processedSites = siteUtil.addSite(sites, newSiteDetail, siteTags.BOOKMARK, oldSiteDetail) - const expectedSites = Immutable.fromJS([expectedSiteDetail]) - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) + 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) }) it('overrides the old title with the new title', function () { const oldSiteDetail = Immutable.fromJS({ @@ -326,8 +317,10 @@ describe('siteUtil', function () { tags: [siteTags.BOOKMARK], location: testUrl1, title: 'old title', + order: 0, customTitle: 'old customTitle' }) + const oldSiteKey = siteUtil.getSiteKey(oldSiteDetail) const newSiteDetail = Immutable.fromJS({ lastAccessedTime: 456, tags: [siteTags.BOOKMARK], @@ -335,91 +328,126 @@ describe('siteUtil', function () { title: 'new title', customTitle: 'new customTitle' }) - const sites = Immutable.fromJS([oldSiteDetail]) - const processedSites = siteUtil.addSite(sites, newSiteDetail, siteTags.BOOKMARK, oldSiteDetail) - const expectedSites = Immutable.fromJS([newSiteDetail]) - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) + let sites = {} + sites[oldSiteKey] = oldSiteDetail.toJS() + const processedSites = siteUtil.addSite(Immutable.fromJS(sites), newSiteDetail, siteTags.BOOKMARK, oldSiteDetail) + const expectedSites = {} + const expectedSiteKey = siteUtil.getSiteKey(newSiteDetail) + expectedSites[expectedSiteKey] = newSiteDetail.set('order', 0).toJS() + assert.deepEqual(processedSites.toJS(), expectedSites) }) }) }) describe('removeSite', function () { describe('tag=truthy', function () { - it('removes the tag from the siteDetail', function () { + it('removes the entry', function () { const siteDetail = { tags: [siteTags.BOOKMARK], location: testUrl1 } - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK) - const expectedSites = sites.setIn([0, 'parentFolderId'], 0).setIn([0, 'tags'], Immutable.List([])) + const siteKey = siteUtil.getSiteKey(Immutable.fromJS(siteDetail)) + let sites = {} + sites[siteKey] = siteDetail + const processedSites = siteUtil.removeSite(Immutable.fromJS(sites), Immutable.fromJS(siteDetail), siteTags.BOOKMARK) + const expectedSites = new Immutable.Map() assert.deepEqual(processedSites, expectedSites) }) - it('removes the customTitle', function () { - const siteDetail = { - tags: [siteTags.BOOKMARK], + it('removes folder and its children', function () { + let sites = {} + const site1 = { + folderId: 1, + parentFolderId: 0, + order: 0, + tags: [siteTags.BOOKMARK_FOLDER] + } + const site2 = { + folderId: 2, + parentFolderId: 1, + order: 1, + tags: [siteTags.BOOKMARK_FOLDER] + } + const site3 = { + parentFolderId: 1, location: testUrl1, - customTitle: 'customTitle' + order: 2, + tags: [siteTags.BOOKMARK] + } + const site4 = { + parentFolderId: 2, + location: testUrl2, + order: 3, + tags: [siteTags.BOOKMARK] + } + const siteKey1 = siteUtil.getSiteKey(Immutable.fromJS(site1)) + const siteKey2 = siteUtil.getSiteKey(Immutable.fromJS(site2)) + const siteKey3 = siteUtil.getSiteKey(Immutable.fromJS(site3)) + const siteKey4 = siteUtil.getSiteKey(Immutable.fromJS(site4)) + sites[siteKey1] = site1 + sites[siteKey2] = site2 + sites[siteKey3] = site3 + sites[siteKey4] = site4 + const siteDetail = { + folderId: 1, + parentFolderId: 0, + tags: [siteTags.BOOKMARK_FOLDER] } - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK) - const expectedSites = sites.setIn([0, 'parentFolderId'], 0) - .deleteIn([0, 'customTitle']) - .setIn([0, 'tags'], Immutable.List([])) + const processedSites = siteUtil.removeSite(Immutable.fromJS(sites), Immutable.fromJS(siteDetail), siteTags.BOOKMARK_FOLDER) + const expectedSites = new Immutable.Map() assert.deepEqual(processedSites, expectedSites) }) - it('removes folder and its children', function () { - const sites = Immutable.fromJS([ - { - folderId: 1, - parentFolderId: 0, - tags: [siteTags.BOOKMARK_FOLDER] - }, - { - folderId: 2, - parentFolderId: 1, - tags: [siteTags.BOOKMARK_FOLDER] - }, - { - parentFolderId: 1, - location: testUrl1, - tags: [siteTags.BOOKMARK] - }, - { - parentFolderId: 2, - location: testUrl2, - tags: [siteTags.BOOKMARK] - } - ]) - const siteDetail = { + it('removes with reorder', function () { + let sites = {} + let expectedSites = {} + const site1 = { folderId: 1, parentFolderId: 0, + order: 0, tags: [siteTags.BOOKMARK_FOLDER] } - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail), siteTags.BOOKMARK_FOLDER) - const expectedSites = Immutable.fromJS([ - { - folderId: 1, - parentFolderId: 0, - tags: Immutable.List([]) - }, - { - folderId: 2, - parentFolderId: 0, - tags: Immutable.List([]) - }, - { - parentFolderId: 0, - location: testUrl1, - tags: Immutable.List([]) - }, - { - parentFolderId: 0, - location: testUrl2, - tags: Immutable.List([]) - } - ]) - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) + const site2 = { + folderId: 2, + parentFolderId: 1, + order: 1, + tags: [siteTags.BOOKMARK_FOLDER] + } + const site3 = { + parentFolderId: 1, + location: testUrl1, + order: 2, + tags: [siteTags.BOOKMARK] + } + const site4 = { + parentFolderId: 2, + location: testUrl2, + order: 3, + tags: [siteTags.BOOKMARK] + } + const expectedSite4 = { + parentFolderId: 2, + location: testUrl2, + order: 2, + tags: [siteTags.BOOKMARK] + } + const siteKey1 = siteUtil.getSiteKey(Immutable.fromJS(site1)) + const siteKey2 = siteUtil.getSiteKey(Immutable.fromJS(site2)) + const siteKey3 = siteUtil.getSiteKey(Immutable.fromJS(site3)) + const siteKey4 = siteUtil.getSiteKey(Immutable.fromJS(site4)) + sites[siteKey1] = site1 + sites[siteKey2] = site2 + sites[siteKey3] = site3 + sites[siteKey4] = site4 + expectedSites[siteKey1] = site1 + expectedSites[siteKey2] = site2 + expectedSites[siteKey4] = expectedSite4 + const siteDetail = { + parentFolderId: 1, + location: testUrl1, + tags: [siteTags.BOOKMARK] + } + const processedSites = siteUtil.removeSite(Immutable.fromJS(sites), Immutable.fromJS(siteDetail), + siteTags.BOOKMARK_FOLDER) + assert.deepEqual(processedSites.toJS(), expectedSites) }) }) describe('tag=falsey', function () { @@ -428,20 +456,11 @@ describe('siteUtil', function () { tags: [], location: testUrl1 } - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail)) - assert.deepEqual(processedSites, Immutable.fromJS([])) - }) - it('nulls out the lastAccessedTime for a bookmarked entry (has tag)', function () { - const siteDetail = { - location: testUrl1, - tags: [siteTags.BOOKMARK], - lastAccessedTime: 123 - } - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail)) - const expectedSites = sites.setIn([0, 'lastAccessedTime'], null) - assert.deepEqual(processedSites, expectedSites) + const siteKey = siteUtil.getSiteKey(Immutable.fromJS(siteDetail)) + let sites = {} + sites[siteKey] = siteDetail + const processedSites = siteUtil.removeSite(Immutable.fromJS(sites), Immutable.fromJS(siteDetail)) + assert.deepEqual(processedSites, Immutable.fromJS({})) }) }) }) @@ -451,24 +470,32 @@ describe('siteUtil', function () { it('does not allow you to move a bookmark folder into itself', function () { // Add a new bookmark folder let processedSites = siteUtil.addSite(emptySites, folderMinFields) - const folderId = processedSites.getIn([0, 'folderId']) + const folderMinFieldsWithId = folderMinFields.set('folderId', 1) + const processedKey = siteUtil.getSiteKey(folderMinFieldsWithId) + const folderId = processedSites.getIn([processedKey, 'folderId']) // Add a bookmark into that folder processedSites = siteUtil.addSite(processedSites, bookmarkAllFields.set('parentFolderId', folderId)) - const bookmarkFolder = processedSites.get(0) + const bookmarkFolder = processedSites.get(processedKey) // Should NOT be able to move bookmark folder into itself assert.equal(false, siteUtil.isMoveAllowed(processedSites, bookmarkFolder, bookmarkFolder)) }) it('does not allow you to move an ancestor folder into a descendant folder', function () { // Add a new bookmark folder let processedSites = siteUtil.addSite(emptySites, folderMinFields) - const folderId1 = processedSites.getIn([0, 'folderId']) + const folderMinFieldsWithId1 = folderMinFields.set('folderId', 1) + const processedKey1 = siteUtil.getSiteKey(folderMinFieldsWithId1) + const folderId1 = processedSites.getIn([processedKey1, 'folderId']) // Add a child below that folder processedSites = siteUtil.addSite(processedSites, folderMinFields.set('parentFolderId', folderId1)) - const folderId2 = processedSites.getIn([1, 'folderId']) + const folderMinFieldsWithId2 = folderMinFields.set('folderId', 2) + const processedKey2 = siteUtil.getSiteKey(folderMinFieldsWithId2) + const folderId2 = processedSites.getIn([processedKey2, 'folderId']) // Add a folder below the previous child processedSites = siteUtil.addSite(processedSites, folderMinFields.set('parentFolderId', folderId2)) - const bookmarkFolder1 = processedSites.get(0) - const bookmarkFolder3 = processedSites.get(2) + const folderMinFieldsWithId3 = folderMinFields.set('folderId', 3) + const processedKey3 = siteUtil.getSiteKey(folderMinFieldsWithId3) + const bookmarkFolder1 = processedSites.get(processedKey1) + const bookmarkFolder3 = processedSites.get(processedKey3) // Should NOT be able to move grandparent folder into its grandchild assert.equal(false, siteUtil.isMoveAllowed(processedSites, bookmarkFolder1, bookmarkFolder3)) }) @@ -478,138 +505,49 @@ describe('siteUtil', function () { }) describe('updateSiteFavicon', function () { - it('updates the favicon for all matching entries', function () { - const sites = Immutable.fromJS([bookmarkMinFields, siteMinFields]) + it('updates the favicon for all matching entries (normalizing the URL)', function () { + const siteDetail1 = Immutable.fromJS({ + tags: [siteTags.BOOKMARK], + location: testUrl1, + title: 'bookmarked site', + lastAccessedTime: 123 + }) + const siteDetail2 = Immutable.fromJS({ + tags: [], + location: 'https://www.brave.com', + title: 'history entry', + lastAccessedTime: 456 + }) + let sites = siteUtil.addSite(emptySites, siteDetail1, siteTags.BOOKMARK) + sites = siteUtil.addSite(sites, siteDetail2) const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, testFavicon1) - const updatedSiteDetail1 = bookmarkMinFields.set('favicon', testFavicon1) - const updatedSiteDetail2 = siteMinFields.set('favicon', testFavicon1) - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) + const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1) + const updatedSiteDetail2 = siteDetail2.set('favicon', testFavicon1) + let expectedSites = siteUtil.addSite(emptySites, updatedSiteDetail1, siteTags.BOOKMARK) + expectedSites = siteUtil.addSite(expectedSites, updatedSiteDetail2) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) - - describe('when searching for matches', function () { - it('disregards folders', function () { - const sites = siteUtil.addSite(emptySites, folderMinFields) - const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, testFavicon1) - assert.deepEqual(processedSites.toJS(), sites.toJS()) - }) - it('ensures entry.location is truthy', function () { - const invalidSite = Immutable.fromJS({ - title: 'sample' - }) - const sites = siteUtil.addSite(emptySites, invalidSite) - const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, testFavicon1) - assert.deepEqual(processedSites.toJS(), sites.toJS()) - }) - it('ensures input and entry.location are valid URLs', function () { - const invalidSite = Immutable.fromJS({ - title: 'sample', - location: '......not a real URL' - }) - const sites = siteUtil.addSite(emptySites, invalidSite) - const processedSites = siteUtil.updateSiteFavicon(sites, '......not a real URL', testFavicon1) - assert.deepEqual(processedSites.toJS(), sites.toJS()) - }) - it('ensures input is truthy', function () { - const sites = siteUtil.addSite(emptySites, bookmarkMinFields) - const processedSites = siteUtil.updateSiteFavicon(sites, undefined, testFavicon1) - assert.deepEqual(processedSites.toJS(), sites.toJS()) - }) - }) - - describe('normalizes the URL when searching for matches', function () { - it('normalizes trailing slashes', function () { - const siteDetail1 = Immutable.fromJS({ - tags: [siteTags.BOOKMARK], - location: 'https://brave.com', - title: 'bookmarked site' - }) - const siteDetail2 = Immutable.fromJS({ - tags: [], - location: 'https://brave.com/', - title: 'visited site' - }) - - const sites = Immutable.fromJS([siteDetail1, siteDetail2]) - const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/', testFavicon1) - const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1) - const updatedSiteDetail2 = siteDetail2.set('favicon', testFavicon1) - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) - - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) - it('normalizes port numbers', function () { - const siteDetail1 = Immutable.fromJS({ - tags: [siteTags.BOOKMARK], - location: 'https://brave.com:443', - title: 'bookmarked site' - }) - - const sites = Immutable.fromJS([siteDetail1, siteMinFields]) - const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/', testFavicon1) - const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1) - const updatedSiteDetail2 = siteMinFields.set('favicon', testFavicon1) - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) - - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) - it('strips www', function () { - const siteDetail1 = Immutable.fromJS({ - tags: [siteTags.BOOKMARK], - location: 'https://www.brave.com/', - title: 'bookmarked site' - }) - - const sites = Immutable.fromJS([siteDetail1, siteMinFields]) - const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/', testFavicon1) - const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1) - const updatedSiteDetail2 = siteMinFields.set('favicon', testFavicon1) - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) - - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) - it('removes the fragment', function () { - const siteDetail1 = Immutable.fromJS({ - tags: [siteTags.BOOKMARK], - location: 'https://www.brave.com/#contact', - title: 'bookmarked site' - }) - const siteDetail2 = Immutable.fromJS({ - tags: [], - location: 'https://brave.com/#people', - title: 'visited site' - }) - - const sites = Immutable.fromJS([siteDetail1, siteDetail2]) - const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/#about', testFavicon1) - const updatedSiteDetail1 = siteDetail1.set('favicon', testFavicon1) - const updatedSiteDetail2 = siteDetail2.set('favicon', testFavicon1) - const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2]) - - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) - it('handles malformed URIs gracefully', function () { - const siteDetail = Immutable.fromJS({ - tags: [siteTags.BOOKMARK], - location: 'https://www.foo.com/bar/archive/%3+c', - title: 'bookmarked site' - }) - const sites = Immutable.fromJS([siteDetail]) - const processedSites = siteUtil.updateSiteFavicon(sites, 'https://www.foo.com/bar/archive/%3+c', 'https://www.foo.com/favicon.ico') - const updatedSiteDetail1 = siteDetail.set('favicon', 'https://www.foo.com/favicon.ico') - const expectedSites = Immutable.fromJS([updatedSiteDetail1]) - - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) - it('works even if null/undefined/non-immutable entries are present', function () { - const hasInvalidEntries = Immutable.fromJS([null, undefined, {get: 'test'}]) - const sites = hasInvalidEntries.push(bookmarkMinFields) - const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, testFavicon1) - const updatedSiteDetail = bookmarkMinFields.set('favicon', testFavicon1) - const expectedSites = hasInvalidEntries.push(updatedSiteDetail) - assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) - }) + it('returns the object unchanged if location is not a URL', function () { + const sites = siteUtil.addSite(emptySites, bookmarkMinFields, siteTags.BOOKMARK) + const processedSites = siteUtil.updateSiteFavicon(sites, 'not-a-url', 'https://brave.com/favicon.ico') + assert.deepEqual(processedSites, sites) + }) + it('returns the object unchanged if it is not an Immutable.Map', function () { + const emptyLegacySites = Immutable.fromJS([]) + const processedSites = siteUtil.updateSiteFavicon(emptyLegacySites, testUrl1, 'https://brave.com/favicon.ico') + assert.deepEqual(processedSites, emptyLegacySites) + }) + it('works even if null/undefined entries are present', function () { + const hasInvalidEntries = Immutable.fromJS({ + 'null': null, + 'undefined': undefined + }) + const sites = siteUtil.addSite(hasInvalidEntries, bookmarkMinFields, siteTags.BOOKMARK) + const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico') + const updatedSiteDetail = bookmarkMinFields.set('favicon', 'https://brave.com/favicon.ico') + const expectedSites = siteUtil.addSite(hasInvalidEntries, updatedSiteDetail, siteTags.BOOKMARK) + assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) }) @@ -817,91 +755,68 @@ describe('siteUtil', function () { describe('clearHistory', function () { it('does not remove sites which have a valid `tags` property', function () { - const sites = Immutable.fromJS([ - { tags: [siteTags.BOOKMARK_FOLDER] }, - { tags: [siteTags.BOOKMARK] } - ]) + const sites = Immutable.fromJS({ + 'key1': { tags: [siteTags.BOOKMARK_FOLDER] }, + 'key2': { tags: [siteTags.BOOKMARK] } + }) const processedSites = siteUtil.clearHistory(sites) assert.deepEqual(processedSites.toJS(), sites.toJS()) }) it('sets the lastAccessedTime for all entries to null', function () { - const sites = Immutable.fromJS([ - { + const sites = Immutable.fromJS({ + 'key1': { location: 'location1', tags: [], lastAccessedTime: 123 }, - { + 'key2': { location: 'location2', tags: [siteTags.BOOKMARK], lastAccessedTime: 123 } - ]) - const expectedSites = Immutable.fromJS([{ - location: 'location2', - tags: [siteTags.BOOKMARK], - lastAccessedTime: null - }]) + }) + const expectedSites = Immutable.fromJS({ + 'key2': { + location: 'location2', + tags: [siteTags.BOOKMARK], + lastAccessedTime: null + } + }) const processedSites = siteUtil.clearHistory(sites) assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) }) - describe('hasNoTagSites', function () { - it('returns true if the ANY sites in the provided list are missing a `tags` property', function () { - const sites = [ - Immutable.fromJS({ - location: 'https://brave.com' - })] - assert.equal(siteUtil.hasNoTagSites(sites), true) - }) - it('returns true if the ANY sites in the provided list have an empty `tags` property', function () { - const sites = [ - Immutable.fromJS({ - tags: [] - })] - assert.equal(siteUtil.hasNoTagSites(sites), true) - }) - it('returns false if all sites have a valid `tags` property', function () { - const sites = [ - Immutable.fromJS({ - tags: [siteTags.BOOKMARK_FOLDER] - }), - Immutable.fromJS({ - tags: [siteTags.BOOKMARK] - })] - assert.equal(siteUtil.hasNoTagSites(sites), false) - }) - }) - describe('getBookmarks', function () { it('returns items which are tagged either `BOOKMARK_FOLDER` or `BOOKMARK`', function () { - const sites = [ - Immutable.fromJS({ + const sites = Immutable.fromJS({ + 'key1': { tags: [siteTags.BOOKMARK_FOLDER] - }), - Immutable.fromJS({ + }, + 'key2': { tags: [siteTags.BOOKMARK] - })] + } + }) const processedSites = siteUtil.getBookmarks(sites) assert.deepEqual(sites, processedSites) }) it('excludes items which are NOT tagged `BOOKMARK_FOLDER` or `BOOKMARK`', function () { - const sites = [ - Immutable.fromJS({ + const sites = Immutable.fromJS({ + 'key1': { + tags: ['unknown1'] + }, + 'key2': { tags: ['unknown1'] - }), - Immutable.fromJS({ - tags: ['unknown2'] - })] - const expectedSites = [] + } + }) + const expectedSites = Immutable.fromJS({}) const processedSites = siteUtil.getBookmarks(sites) - assert.deepEqual(expectedSites, processedSites) + assert.deepEqual(expectedSites.toJS(), processedSites.toJS()) }) - it('returns empty list if input was falsey', function () { + it('returns empty map if input was falsey', function () { const processedSites = siteUtil.getBookmarks(null) - const expectedSites = [] - assert.deepEqual(processedSites, expectedSites) + const expectedSites = Immutable.fromJS({}) + assert.deepEqual(processedSites.toJS(), expectedSites.toJS()) }) })