Follow up for sites splits PR brave#10296

Resolves brave#10296
Resolves brave#12378

Auditors:

Test Plan:

Author: NejcZdovc

app/common/lib/historyUtil.js

@@ -15,20 +15,32 @@ const sortTimeDescending = (left, right) => {
 }
 
 const getHistory = (sites) => {
-  sites = makeImmutable(sites) ? makeImmutable(sites).toList() : new Immutable.List()
+  if (sites == null) {
+    return Immutable.List()
+  }
+
+  sites = makeImmutable(sites) ? makeImmutable(sites).toList() : Immutable.List()
   return sites
       .sort(sortTimeDescending)
       .slice(0, aboutHistoryMaxEntries)
 }
 
 const getDayString = (entry, locale) => {
+  if (entry == null) {
+    return ''
+  }
+
   const lastAccessedTime = entry.get('lastAccessedTime')
   return lastAccessedTime
     ? new Date(lastAccessedTime).toLocaleDateString(locale, { weekday: 'long', month: 'long', day: 'numeric', year: 'numeric' })
     : ''
 }
 
 const groupEntriesByDay = (history, locale) => {
+  if (history == null) {
+    return Immutable.List()
+  }
+
   const reduced = history.reduce((previousValue, currentValue, currentIndex) => {
     const result = currentIndex === 1 ? [] : previousValue
     if (currentIndex === 1) {
@@ -44,31 +56,38 @@ const groupEntriesByDay = (history, locale) => {
     }
     return result
   })
+
   if (reduced) {
     return Immutable.fromJS(
       Array.isArray(reduced)
         ? reduced
         : [{date: getDayString(reduced, locale), entries: [reduced]}]
     )
   }
-  return Immutable.fromJS([])
+  return Immutable.List()
 }
 
 /**
  * Return an array with ALL entries.
  * Format is expected to be array containing one array per day.
  */
 const totalEntries = (entriesByDay) => {
-  entriesByDay = makeImmutable(entriesByDay) || new Immutable.List()
+  if (entriesByDay == null) {
+    return Immutable.List()
+  }
 
-  let result = new Immutable.List()
-  entriesByDay.forEach((entry) => {
-    result = result.push(entry.get('entries'))
+  entriesByDay = makeImmutable(entriesByDay)
+
+  return entriesByDay.map((entry) => {
+    return entry.get('entries')
   })
-  return result
 }
 
 const prepareHistoryEntry = (siteDetail) => {
+  if (siteDetail == null) {
+    return Immutable.Map()
+  }
+
   const time = siteDetail.has('lastAccessedTime')
     ? siteDetail.get('lastAccessedTime')
     : new Date().getTime()
@@ -82,13 +101,24 @@ const prepareHistoryEntry = (siteDetail) => {
     count: 1,
     themeColor: siteDetail.get('themeColor'),
     favicon: siteDetail.get('favicon', siteDetail.get('icon')),
-    key: getKey(siteDetail),
+    key: module.exports.getKey(siteDetail),
     skipSync: siteDetail.get('skipSync', null)
   })
 }
 
 const mergeSiteDetails = (oldDetail, newDetail) => {
-  const objectId = newDetail.has('objectId') ? newDetail.get('objectId') : oldDetail.get('objectId', undefined)
+  if (newDetail == null) {
+    return Immutable.Map()
+  }
+
+  if (oldDetail == null) {
+    oldDetail = Immutable.Map()
+  }
+
+  newDetail = makeImmutable(newDetail)
+  oldDetail = makeImmutable(oldDetail)
+
+  const objectId = newDetail.has('objectId') ? newDetail.get('objectId') : oldDetail.get('objectId') || undefined
   const time = newDetail.has('lastAccessedTime')
     ? newDetail.get('lastAccessedTime')
     : new Date().getTime()
@@ -115,16 +145,20 @@ const mergeSiteDetails = (oldDetail, newDetail) => {
     site = site.set('favicon', favicon)
   }
 
-  site = site.set('key', getKey(site))
+  site = site.set('key', module.exports.getKey(site))
 
   return site
 }
 
 const getDetailFromFrame = (frame) => {
+  if (frame == null) {
+    return Immutable.Map()
+  }
+
   return makeImmutable({
     location: frame.get('location'),
     title: frame.get('title'),
-    partitionNumber: frame.get('partitionNumber'),
+    partitionNumber: frame.get('partitionNumber') || 0,
     favicon: frame.get('icon'),
     themeColor: frame.get('themeColor') || frame.get('computedThemeColor')
   })
@@ -142,6 +176,7 @@ const getKey = (siteDetail) => {
     return location + '|' +
       (siteDetail.get('partitionNumber') || 0)
   }
+
   return null
 }
 

app/common/state/bookmarkFoldersState.js

@@ -36,20 +36,39 @@ const bookmarkFoldersState = {
 
   getFolder: (state, folderKey) => {
     state = validateState(state)
+
+    if (folderKey == null) {
+      return Immutable.Map()
+    }
+
     folderKey = folderKey.toString()
     return state.getIn([STATE_SITES.BOOKMARK_FOLDERS, folderKey], Immutable.Map())
   },
 
   getFoldersByParentId: (state, parentFolderId) => {
     state = validateState(state)
 
+    if (parentFolderId == null) {
+      return Immutable.List()
+    }
+
     const folders = bookmarkOrderCache.getFoldersByParentId(state, parentFolderId)
     return folders.map(folder => bookmarkFoldersState.getFolder(state, folder.get('key')))
   },
 
   addFolder: (state, folderDetails, destinationKey) => {
     state = validateState(state)
 
+    if (folderDetails == null) {
+      return state
+    }
+
+    folderDetails = makeImmutable(folderDetails)
+
+    if (folderDetails.get('key') == null) {
+      return state
+    }
+
     state = state.setIn([STATE_SITES.BOOKMARK_FOLDERS, folderDetails.get('key')], folderDetails)
     state = bookmarkOrderCache.addFolderToCache(state, folderDetails.get('parentFolderId'), folderDetails.get('key'), destinationKey)
     return state
@@ -78,24 +97,23 @@ const bookmarkFoldersState = {
   },
 
   removeFolder: (state, folderKey) => {
-    const bookmarksState = require('./bookmarksState')
-    const folders = bookmarkFoldersState.getFolders(state)
+    state = validateState(state)
     const folder = bookmarkFoldersState.getFolder(state, folderKey)
 
     if (folder.isEmpty()) {
       return state
     }
 
+    const bookmarksState = require('./bookmarksState')
+    const folders = bookmarkFoldersState.getFolders(state)
+
     if (getSetting(settings.SYNC_ENABLED) === true) {
       syncActions.removeSites([folder.toJS()])
     }
 
     folders.filter(folder => folder.get('parentFolderId') === Number(folderKey))
       .map(folder => {
-        state = bookmarksState.removeBookmarksByParentId(state, folder.get('folderId'))
         state = bookmarkFoldersState.removeFolder(state, folder.get('folderId'))
-        state = bookmarkOrderCache.removeCacheParent(state, folder.get('folderId'))
-        state = bookmarkOrderCache.removeCacheKey(state, folder.get('parentFolderId'), folderKey)
       })
 
     state = bookmarksState.removeBookmarksByParentId(state, folderKey)
@@ -105,14 +123,15 @@ const bookmarkFoldersState = {
   },
 
   /**
-   * Get all folders except provided folder
+   * Get a list of all folders except provided folder
    * @param state
-   * @param folderKey
+   * @param {number} folderKey
    * @param parentFolderId
    * @param labelPrefix
-   * @returns {Array}
+   * @returns {Array} - each entry with folder id and label (title)
    */
   getFoldersWithoutKey: (state, folderKey, parentFolderId = 0, labelPrefix = '') => {
+    state = validateState(state)
     let folders = []
     const results = bookmarkFoldersState.getFoldersByParentId(state, parentFolderId)
 
@@ -128,27 +147,36 @@ const bookmarkFoldersState = {
         folderId: folder.get('folderId'),
         label
       })
-      const subSites = bookmarkFoldersState.getFoldersWithoutKey(state, folderKey, folder.get('folderId'), (label || '') + ' / ')
-      folders = folders.concat(subSites)
+      const subFolders = bookmarkFoldersState.getFoldersWithoutKey(state, folderKey, folder.get('folderId'), (label || '') + ' / ')
+      folders = folders.concat(subFolders)
     }
 
     return folders
   },
 
   moveFolder: (state, folderKey, destinationKey, append, moveIntoParent) => {
-    const bookmarksState = require('./bookmarksState')
+    state = validateState(state)
     let folder = bookmarkFoldersState.getFolder(state, folderKey)
-    let destinationItem = bookmarksState.findBookmark(state, destinationKey)
-
     if (folder.isEmpty()) {
       return state
     }
 
+    const bookmarksState = require('./bookmarksState')
+    let destinationItem = bookmarksState.findBookmark(state, destinationKey)
+    const numKey = Number(destinationKey)
+    if (destinationItem.isEmpty() && numKey !== -1 && numKey !== 0) {
+      return state
+    }
+
     if (moveIntoParent || destinationItem.get('parentFolderId') !== folder.get('parentFolderId')) {
-      const parentFolderId = destinationItem.get('type') === siteTags.BOOKMARK
+      let parentFolderId = destinationItem.get('type') === siteTags.BOOKMARK
         ? destinationItem.get('parentFolderId')
         : destinationItem.get('folderId')
 
+      if (parentFolderId == null) {
+        parentFolderId = destinationKey
+      }
+
       state = bookmarkOrderCache.removeCacheKey(state, folder.get('parentFolderId'), folderKey)
       folder = folder.set('parentFolderId', Number(parentFolderId))
       const newKey = bookmarkFoldersUtil.getKey(folder)
@@ -169,7 +197,14 @@ const bookmarkFoldersState = {
   },
 
   setWidth: (state, key, width) => {
-    return state.setIn([STATE_SITES.BOOKMARK_FOLDERS, key, 'width'], parseFloat(width))
+    state = validateState(state)
+    width = parseFloat(width)
+
+    if (key == null || isNaN(width)) {
+      return state
+    }
+
+    return state.setIn([STATE_SITES.BOOKMARK_FOLDERS, key, 'width'], width)
   }
 }