Merge pull request #3459 from bsclifton/bookmark-folders-history

History bugfixes

js/about/history.js

@@ -13,6 +13,7 @@ const aboutActions = require('./aboutActions')
 const getSetting = require('../settings').getSetting
 const SortableTable = require('../components/sortableTable')
 const Button = require('../components/button')
+const siteUtil = require('../state/siteUtil')
 
 const ipc = window.chrome.ipc
 
@@ -71,7 +72,11 @@ class GroupedHistoryList extends ImmutableComponent {
       }
       return result
     })
-    if (reduced) return reduced
+    if (reduced) {
+      return Array.isArray(reduced)
+        ? reduced
+        : [{date: this.getDayString(locale, reduced), entries: [reduced]}]
+    }
     return []
   }
   render () {
@@ -125,7 +130,7 @@ class AboutHistory extends React.Component {
     })
   }
   historyDescendingOrder () {
-    return this.state.history.filter((site) => site.get('lastAccessedTime'))
+    return this.state.history.filter((site) => siteUtil.isHistoryEntry(site))
       .sort((left, right) => {
         if (left.get('lastAccessedTime') < right.get('lastAccessedTime')) return 1
         if (left.get('lastAccessedTime') > right.get('lastAccessedTime')) return -1

js/state/siteUtil.js

@@ -302,6 +302,21 @@ module.exports.isFolder = function (siteDetail) {
   return false
 }
 
+/**
+ * Determines if the site detail is a history entry.
+ * @param siteDetail The site detail to check.
+ * @return true if the site detail is a history entry.
+ */
+module.exports.isHistoryEntry = function (siteDetail) {
+  if (siteDetail && typeof siteDetail.get('location') === 'string') {
+    if (siteDetail.get('location').startsWith('about:')) {
+      return false
+    }
+    return !!siteDetail.get('lastAccessedTime') && !module.exports.isFolder(siteDetail)
+  }
+  return false
+}
+
 /**
  * Obtains an array of folders
  */

test/unit/state/siteUtilTest.js

@@ -414,6 +414,50 @@ describe('siteUtil', function () {
     })
   })
 
+  describe('isHistoryEntry', function () {
+    it('returns true for a typical history entry', function () {
+      const siteDetail = Immutable.fromJS({
+        location: testUrl1,
+        lastAccessedTime: 123
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), true)
+    })
+    it('returns true for a bookmark history entry which has lastAccessedTime', function () {
+      const siteDetail = Immutable.fromJS({
+        location: testUrl1,
+        tags: [siteTags.BOOKMARK],
+        lastAccessedTime: 123
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), true)
+    })
+    it('returns false for a bookmark entry with falsey lastAccessedTime', function () {
+      const siteDetail = Immutable.fromJS({
+        location: testUrl1,
+        tags: [siteTags.BOOKMARK]
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), false)
+    })
+    it('returns false for a bookmarks folder', function () {
+      const siteDetail = Immutable.fromJS({
+        location: testUrl1,
+        tags: [siteTags.BOOKMARK_FOLDER],
+        lastAccessedTime: 123
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), false)
+    })
+    it('returns false if input is falsey', function () {
+      assert.equal(siteUtil.isHistoryEntry(null), false)
+      assert.equal(siteUtil.isHistoryEntry(undefined), false)
+    })
+    it('returns false for about: pages', function () {
+      const siteDetail = Immutable.fromJS({
+        location: 'about:fake-page-here',
+        lastAccessedTime: 123
+      })
+      assert.equal(siteUtil.isHistoryEntry(siteDetail), false)
+    })
+  })
+
   describe('getFolders', function () {
   })