Merge pull request #3640 from bsclifton/history-clear-followup

Give more meaningful name to siteUtils method
brave · Sep 3, 2016 · 0ace83f · 0ace83f
2 parents e186986 + 5e6f195
commit 0ace83f
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 24 deletions.
diff --git a/app/sessionStore.js b/app/sessionStore.js
@@ -11,6 +11,7 @@
 // - When all state is collected save it to a JSON file and close the app
 // - NODE_ENV of ‘test’ bypassing session state or else they all fail.
 
+const Immutable = require('immutable')
 const fs = require('fs')
 const path = require('path')
 const electron = require('electron')
@@ -19,6 +20,7 @@ const UpdateStatus = require('../js/constants/updateStatus')
 const settings = require('../js/constants/settings')
 const downloadStates = require('../js/constants/downloadStates')
 const {tabFromFrame} = require('../js/state/frameStateUtil')
+const siteUtil = require('../js/state/siteUtil')
 const sessionStorageVersion = 1
 const filtering = require('./filtering')
 // const tabState = require('./common/state/tabState')
@@ -250,12 +252,7 @@ module.exports.cleanAppData = (data, isShutdown) => {
   if (data.sites) {
     const clearHistory = isShutdown && getSetting(settings.SHUTDOWN_CLEAR_HISTORY) === true
     if (clearHistory) {
-      // TODO - this should the history methods from siteUtils
-      data.sites = data.sites.filter((site) => site && site.tags && site.tags.length)
-      data.sites = data.sites.map((site) => {
-        delete site.lastAccessedTime
-        return site
-      })
+      data.sites = siteUtil.clearHistory(Immutable.fromJS(data.sites)).toJS()
     }
   }
   if (data.downloads) {

diff --git a/docs/appActions.md b/docs/appActions.md
@@ -50,9 +50,9 @@ Adds a site to the site list
 
 
 
-### clearSitesWithoutTags() 
+### clearHistory() 
 
-Clears all sites without tags
+Clears history (all sites without tags). Indirectly called by appActions.clearAppData().
 
 
 

diff --git a/js/actions/appActions.js b/js/actions/appActions.js
@@ -63,11 +63,11 @@ const appActions = {
   },
 
   /**
-   * Clears all sites without tags
+   * Clears history (all sites without tags). Indirectly called by appActions.clearAppData().
    */
-  clearSitesWithoutTags: function () {
+  clearHistory: function () {
     AppDispatcher.dispatch({
-      actionType: AppConstants.APP_CLEAR_SITES_WITHOUT_TAGS
+      actionType: AppConstants.APP_CLEAR_HISTORY
     })
   },
 

diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js
@@ -9,7 +9,7 @@ const AppConstants = {
   APP_NEW_WINDOW: _,
   APP_CLOSE_WINDOW: _,
   APP_ADD_SITE: _,
-  APP_CLEAR_SITES_WITHOUT_TAGS: _,
+  APP_CLEAR_HISTORY: _,
   APP_SET_STATE: _,
   APP_REMOVE_SITE: _,
   APP_MOVE_SITE: _,

diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -375,17 +375,19 @@ module.exports.filterSitesRelativeTo = function (sites, relSite) {
 }
 
 /**
- * Clears out all sites that have no tags.
+ * Clears history by
+ * - filtering out entries which have no tags
+ * - setting lastAccessedTime to null for remaining entries (bookmarks)
  * @param sites The application state's Immutable sites list.
  */
-module.exports.clearSitesWithoutTags = function (sites) {
-  let clearedSites = sites.filter((site) => site.get('tags') && site.get('tags').size > 0)
-  clearedSites.forEach((site, index) => {
+module.exports.clearHistory = function (sites) {
+  let bookmarks = sites.filter((site) => site.get('tags') && site.get('tags').size > 0)
+  bookmarks.forEach((site, index) => {
     if (site.get('lastAccessedTime')) {
-      clearedSites = clearedSites.setIn([index, 'lastAccessedTime'], null)
+      bookmarks = bookmarks.setIn([index, 'lastAccessedTime'], null)
     }
   })
-  return clearedSites
+  return bookmarks
 }
 
 /**

diff --git a/js/stores/appStore.js b/js/stores/appStore.js
@@ -430,8 +430,8 @@ const handleAppAction = (action) => {
         appState = appState.set('downloads', downloads)
       }
       break
-    case AppConstants.APP_CLEAR_SITES_WITHOUT_TAGS:
-      appState = appState.set('sites', siteUtil.clearSitesWithoutTags(appState.get('sites')))
+    case AppConstants.APP_CLEAR_HISTORY:
+      appState = appState.set('sites', siteUtil.clearHistory(appState.get('sites')))
       break
     case AppConstants.APP_SET_DEFAULT_WINDOW_SIZE:
       appState = appState.set('defaultWindowWidth', action.size[0])
@@ -531,7 +531,7 @@ const handleAppAction = (action) => {
       break
     case AppConstants.APP_CLEAR_DATA:
       if (action.clearDataDetail.get('browserHistory')) {
-        handleAppAction({actionType: AppConstants.APP_CLEAR_SITES_WITHOUT_TAGS})
+        handleAppAction({actionType: AppConstants.APP_CLEAR_HISTORY})
         BrowserWindow.getAllWindows().forEach((wnd) => wnd.webContents.send(messages.CLEAR_CLOSED_FRAMES))
         BrowserWindow.getAllWindows().forEach((wnd) => wnd.webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW))
       }

diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
@@ -481,13 +481,13 @@ describe('siteUtil', function () {
   describe('filterSitesRelativeTo', function () {
   })
 
-  describe('clearSitesWithoutTags', 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 processedSites = siteUtil.clearSitesWithoutTags(sites)
+      const processedSites = siteUtil.clearHistory(sites)
       assert.deepEqual(processedSites.toJS(), sites.toJS())
     })
     it('sets the lastAccessedTime for all entries to null', function () {
@@ -508,7 +508,7 @@ describe('siteUtil', function () {
         tags: [siteTags.BOOKMARK],
         lastAccessedTime: null
       }])
-      const processedSites = siteUtil.clearSitesWithoutTags(sites)
+      const processedSites = siteUtil.clearHistory(sites)
       assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
     })
   })