Adds more tests

app/browser/reducers/bookmarkFoldersReducer.js

@@ -74,15 +74,24 @@ const bookmarkFoldersReducer = (state, action, immutableAction) => {
       }
     case appConstants.APP_REMOVE_BOOKMARK_FOLDER:
       {
-        const key = action.get('folderKey')
+        const folderKey = action.get('folderKey')
 
-        if (key == null) {
+        if (folderKey == null) {
           break
         }
 
-        const folder = state.getIn([STATE_SITES.BOOKMARK_FOLDERS, key])
-        state = bookmarkFoldersState.removeFolder(state, key)
-        state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
+        if (Immutable.List.isList(folderKey)) {
+          action.get('folderKey', Immutable.List()).forEach((key) => {
+            const folder = state.getIn([STATE_SITES.BOOKMARK_FOLDERS, key])
+            state = bookmarkFoldersState.removeFolder(state, key)
+            state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
+          })
+        } else {
+          const folder = state.getIn([STATE_SITES.BOOKMARK_FOLDERS, folderKey])
+          state = bookmarkFoldersState.removeFolder(state, folderKey)
+          state = syncUtil.updateObjectCache(state, folder, STATE_SITES.BOOKMARK_FOLDERS)
+        }
+
         break
       }
   }

app/browser/reducers/historyReducer.js

@@ -28,13 +28,18 @@ const historyReducer = (state, action, immutableAction) => {
         const clearData = defaults ? defaults.merge(temp) : temp
         if (clearData.get('browserHistory')) {
           state = historyState.clearSites(state)
+          state = aboutHistoryState.clearHistory(state)
           filtering.clearHistory()
         }
         break
       }
     case appConstants.APP_ADD_HISTORY_SITE:
       {
-        const detail = action.get('siteDetail')
+        const detail = action.get('siteDetail', Immutable.Map())
+
+        if (detail.isEmpty()) {
+          break
+        }
 
         if (Immutable.List.isList(detail)) {
           detail.forEach((item) => {
@@ -53,14 +58,19 @@ const historyReducer = (state, action, immutableAction) => {
 
     case appConstants.APP_REMOVE_HISTORY_SITE:
       {
-        if (Immutable.List.isList(action.get('historyKey'))) {
+        const historyKey = action.get('historyKey')
+        if (historyKey == null) {
+          break
+        }
+
+        if (Immutable.List.isList(historyKey)) {
           action.get('historyKey', Immutable.List()).forEach((key) => {
             state = historyState.removeSite(state, key)
             // TODO: Implement Sync history site removal
             // state = syncUtil.updateObjectCache(state, action.get('siteDetail'), STATE_SITES.HISTORY_SITES)
           })
         } else {
-          state = historyState.removeSite(state, action.get('historyKey'))
+          state = historyState.removeSite(state, historyKey)
           // TODO: Implement Sync history site removal
           // state = syncUtil.updateObjectCache(state, action.get('siteDetail'), STATE_SITES.HISTORY_SITES)
         }

app/browser/reducers/pinnedSitesReducer.js

@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+const Immutable = require('immutable')
+
 // State
 const pinnedSitesState = require('../../common/state/pinnedSitesState')
 const tabState = require('../../common/state/tabState')
@@ -41,7 +43,7 @@ const pinnedSitesReducer = (state, action, immutableAction) => {
       }
     case appConstants.APP_CREATE_TAB_REQUESTED:
       {
-        const createProperties = action.get('createProperties')
+        const createProperties = action.get('createProperties', Immutable.Map())
         if (createProperties.get('pinned')) {
           state = pinnedSitesState.addPinnedSite(state, pinnedSitesUtil.getDetailFromProperties(createProperties))
         }
@@ -50,6 +52,11 @@ const pinnedSitesReducer = (state, action, immutableAction) => {
     case appConstants.APP_ON_PINNED_TAB_REORDER:
       {
         const siteKey = action.get('siteKey')
+
+        if (siteKey == null) {
+          break
+        }
+
         state = pinnedSitesState.reOrderSite(
           state,
           siteKey,

app/common/cache/bookmarkLocationCache.js

@@ -59,6 +59,11 @@ const generateCache = (state) => {
  */
 const getCacheKey = (state, location) => {
   const normalLocation = normalizeLocation(location)
+
+  if (normalLocation == null) {
+    return Immutable.List()
+  }
+
   return state.getIn(['cache', 'bookmarkLocation', normalLocation], Immutable.List())
 }
 

app/common/cache/bookmarkOrderCache.js

@@ -37,8 +37,13 @@ const setOrder = (cache, key, tag, destinationKey, append = true) => {
 }
 
 const addCacheItem = (state, parentId = 0, key, destinationKey, tag, append) => {
+  if (key == null) {
+    return state
+  }
+
   parentId = parentId.toString()
   key = key.toString()
+
   // cache with this parentId doesn't exist yet
   if (!state.hasIn(['cache', 'bookmarkOrder', parentId])) {
     return state.setIn(['cache', 'bookmarkOrder', parentId], Immutable.fromJS([
@@ -53,6 +58,11 @@ const addCacheItem = (state, parentId = 0, key, destinationKey, tag, append) =>
   const cache = state.getIn(['cache', 'bookmarkOrder', parentId])
   // destination key is not provided
   if (destinationKey == null) {
+    const keyExist = cache.some(item => item.get('key') === key)
+    if (keyExist) {
+      return state
+    }
+
     return state.setIn(['cache', 'bookmarkOrder', parentId], cache.push(Immutable.fromJS(
       {
         key: key,
@@ -75,21 +85,26 @@ const addFolderToCache = (state, parentId, key, destinationKey, append) => {
   return addCacheItem(state, parentId, key, destinationKey, siteTags.BOOKMARK_FOLDER, append)
 }
 
-const getFoldersByParentId = (state, parentId) => {
+const getFoldersByParentId = (state, parentId = 0) => {
   return state.getIn(['cache', 'bookmarkOrder', parentId.toString()], Immutable.List())
     .filter(item => bookmarkFoldersUtil.isFolder(item))
 }
 
 const getBookmarksByParentId = (state, parentId = 0) => {
+  const bookmarkUtil = require('../lib/bookmarkUtil')
   return state.getIn(['cache', 'bookmarkOrder', parentId.toString()], Immutable.List())
-    .filter(item => bookmarkFoldersUtil.isFolder(item))
+    .filter(item => bookmarkUtil.isBookmark(item))
 }
 
-const getBookmarksWithFolders = (state, parentId) => {
+const getBookmarksWithFolders = (state, parentId = 0) => {
   return state.getIn(['cache', 'bookmarkOrder', parentId.toString()], Immutable.List())
 }
 
 const removeCacheKey = (state, parentId, key) => {
+  if (parentId == null || key == null) {
+    return state
+  }
+
   parentId = parentId.toString()
   key = key.toString()
   const cache = state.getIn(['cache', 'bookmarkOrder', parentId])
@@ -108,10 +123,20 @@ const removeCacheKey = (state, parentId, key) => {
     }
   }
 
-  return state.setIn(['cache', 'bookmarkOrder', parentId], newCache)
+  if (newCache.size > 0) {
+    state = state.setIn(['cache', 'bookmarkOrder', parentId], newCache)
+  } else {
+    state = state.deleteIn(['cache', 'bookmarkOrder', parentId])
+  }
+
+  return state
 }
 
 const removeCacheParent = (state, parentId) => {
+  if (parentId == null) {
+    return state
+  }
+
   return state.deleteIn(['cache', 'bookmarkOrder', parentId.toString()])
 }
 

app/common/lib/bookmarkFoldersUtil.js

@@ -7,8 +7,8 @@ const isFolderNameValid = (title) => {
   return title != null && title.trim().length > 0
 }
 
-const getNextFolderIdItem = (folders) =>
-  folders.max((folderA, folderB) => {
+const getNextFolderIdItem = (folders) => {
+  return folders.max((folderA, folderB) => {
     const folderIdA = folderA.get('folderId')
     const folderIdB = folderB.get('folderId')
 
@@ -23,12 +23,14 @@ const getNextFolderIdItem = (folders) =>
     }
     return folderIdA > folderIdB
   })
+}
 
 const getNextFolderId = (folders) => {
   const defaultFolderId = 0
   if (!folders) {
     return defaultFolderId
   }
+
   const maxIdItem = getNextFolderIdItem(folders)
   return (maxIdItem ? (maxIdItem.get('folderId') || 0) : 0) + 1
 }
@@ -37,8 +39,8 @@ const getNextFolderName = (folders, name) => {
   if (!folders) {
     return name
   }
-  const site = folders.find((site) => site.get('title') === name)
-  if (!site) {
+  const exist = folders.some((site) => site.get('title') === name)
+  if (!exist) {
     return name
   }
   const filenameFormat = /(.*) \((\d+)\)/
@@ -52,6 +54,10 @@ const getNextFolderName = (folders, name) => {
 }
 
 const isFolder = (folder) => {
+  if (folder == null) {
+    return false
+  }
+
   return folder.get('type') === siteTags.BOOKMARK_FOLDER
 }
 
@@ -85,23 +91,28 @@ const getAncestorFolderIds = (parentFolderIds, bookmarkFolder, allBookmarks) =>
 /**
  * Determine if a proposed move is valid
  *
- * @param sites The application state's Immutable sites list
+ * @param folderList The application state's Immutable folderList list
  * @param sourceDetail The site detail to move
  * @param destinationDetail The site detail to move to
  */
-const isMoveAllowed = (sites, sourceDetail, destinationDetail) => {
+const isMoveAllowed = (folderList, sourceDetail, destinationDetail) => {
+  if (destinationDetail == null || sourceDetail == null) {
+    return false
+  }
+
   if (typeof destinationDetail.get('parentFolderId') === 'number' && typeof sourceDetail.get('folderId') === 'number') {
     // Folder can't be its own parent
     if (sourceDetail.get('folderId') === destinationDetail.get('folderId')) {
       return false
     }
     // Ancestor folder can't be moved into a descendant
     let ancestorFolderIds = []
-    getAncestorFolderIds(ancestorFolderIds, destinationDetail, sites)
+    getAncestorFolderIds(ancestorFolderIds, destinationDetail, folderList)
     if (ancestorFolderIds.includes(sourceDetail.get('folderId'))) {
       return false
     }
   }
+
   return true
 }
 

app/common/lib/bookmarkUtil.js

@@ -171,6 +171,10 @@ const getToolbarBookmarks = (state) => {
 }
 
 const getDetailFromFrame = (frame) => {
+  if (frame == null) {
+    return null
+  }
+
   return Immutable.fromJS({
     location: frame.get('location'),
     title: frame.get('title'),
@@ -204,6 +208,10 @@ const isLocationBookmarked = (state, location) => {
  * @return {Object} A createProperties plain JS object, not ImmutableJS
  */
 const toCreateProperties = (bookmark) => {
+  if (bookmark == null) {
+    return null
+  }
+
   return {
     url: bookmark.get('location'),
     partitionNumber: bookmark.get('partitionNumber')
@@ -225,6 +233,10 @@ const getBookmarksByParentId = (state, folderKey) => {
 }
 
 const isBookmark = (bookmark) => {
+  if (bookmark == null) {
+    return false
+  }
+
   return bookmark.get('type') === siteTags.BOOKMARK
 }
 

app/common/lib/historyUtil.js

@@ -78,9 +78,11 @@ const prepareHistoryEntry = (siteDetail) => {
     objectId: undefined,
     title: siteDetail.get('title'),
     location: siteDetail.get('location'),
+    partitionNumber: ~~siteDetail.get('partitionNumber', 0),
     count: 1,
     themeColor: siteDetail.get('themeColor'),
-    favicon: siteDetail.get('favicon', siteDetail.get('icon'))
+    favicon: siteDetail.get('favicon', siteDetail.get('icon')),
+    key: getKey(siteDetail)
   })
 }
 
@@ -95,6 +97,7 @@ const mergeSiteDetails = (oldDetail, newDetail) => {
     objectId,
     title: newDetail.get('title'),
     location: newDetail.get('location'),
+    partitionNumber: ~~newDetail.get('partitionNumber', 0),
     count: ~~oldDetail.get('count', 0) + 1
   })
 
@@ -111,6 +114,8 @@ const mergeSiteDetails = (oldDetail, newDetail) => {
     site = site.set('favicon', favicon)
   }
 
+  site = site.set('key', getKey(site))
+
   return site
 }
 

app/common/lib/pinnedSitesUtil.js

@@ -20,7 +20,7 @@ const getSitesBySubkey = (sites, siteKey) => {
 
 const getDetailsFromTab = (sites, tab) => {
   let location = tab.get('url')
-  const partitionNumber = tab.get('partitionNumber')
+  const partitionNumber = tab.get('partitionNumber', 0)
   let parentFolderId
 
   // TODO check if needed https://github.com/brave/browser-laptop/pull/8588
@@ -60,8 +60,7 @@ const getDetailsFromTab = (sites, tab) => {
     title: tab.get('title')
   }
 
-  // TODO I think that we don't need this one
-  if (partitionNumber) {
+  if (partitionNumber != null) {
     siteDetail.partitionNumber = partitionNumber
   }
 

app/common/state/bookmarksState.js

@@ -93,8 +93,9 @@ const bookmarksState = {
     if (dataItem.isEmpty()) {
       // check if we have data in tabs
       const tab = tabState.getActiveTab(state) || Immutable.Map()
+      const activeLocation = tab.get('url') || tab.getIn(['frame', 'location'])
 
-      if (!tab.isEmpty() && bookmarkDetail.get('location') === tab.get('url')) {
+      if (!tab.isEmpty() && bookmarkDetail.get('location') === activeLocation) {
         dataItem = makeImmutable({
           partitionNumber: tab.getIn(['frame', 'partitionNumber'], 0),
           favicon: tab.getIn(['frame', 'icon']),

app/common/state/historyState.js

@@ -52,8 +52,6 @@ const historyState = {
   },
 
   removeSite: (state, siteKey) => {
-    // TODO should we remove this only when a tab with this siteKey is not opened
-    // if not, we are deleting data that is use for bookmarking
     return state.deleteIn([STATE_SITES.HISTORY_SITES, siteKey])
   },