From 8d1309558e7e152485a579bb194ad15cfaf03041 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 3 Nov 2016 02:27:43 -0700 Subject: [PATCH] Pulled code from about:history into a new historyUtil module Fixes https://github.com/brave/browser-laptop/issues/5353 This has regressed at least once before; we need a way besides web driver to test the code. (usually, problem is immutable vs mutable; ex: ".get method not found", etc) Auditors: @bbondy Test Plan: 1. Launch Brave and visit about:history 2. Double click any entry 3. Enjoy :) --- app/common/lib/historyUtil.js | 56 +++++++++++++++++++ js/about/history.js | 60 +++++---------------- test/unit/lib/historyUtilTest.js | 92 ++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 47 deletions(-) create mode 100644 app/common/lib/historyUtil.js create mode 100644 test/unit/lib/historyUtilTest.js diff --git a/app/common/lib/historyUtil.js b/app/common/lib/historyUtil.js new file mode 100644 index 00000000000..df232cc35e4 --- /dev/null +++ b/app/common/lib/historyUtil.js @@ -0,0 +1,56 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public * 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/. */ + +'use strict' +const Immutable = require('immutable') + +const getDayString = (entry, locale) => { + const lastAccessedTime = entry.get('lastAccessedTime') + return lastAccessedTime + ? new Date(lastAccessedTime).toLocaleDateString(locale, { weekday: 'long', month: 'long', day: 'numeric', year: 'numeric' }) + : '' +} + +module.exports.groupEntriesByDay = (history, locale) => { + const reduced = history.reduce((previousValue, currentValue, currentIndex, array) => { + const result = currentIndex === 1 ? [] : previousValue + if (currentIndex === 1) { + const firstDate = getDayString(currentValue, locale) + result.push({date: firstDate, entries: [previousValue]}) + } + const date = getDayString(currentValue, locale) + const dateIndex = result.findIndex((entryByDate) => entryByDate.date === date) + if (dateIndex !== -1) { + result[dateIndex].entries.push(currentValue) + } else { + result.push({date: date, entries: [currentValue]}) + } + return result + }) + if (reduced) { + return Immutable.fromJS( + Array.isArray(reduced) + ? reduced + : [{date: getDayString(reduced, locale), entries: [reduced]}] + ) + } + return Immutable.fromJS([]) +} + +/** + * Return an array with ALL entries. + * Format is expected to be array containing one array per day. + */ +module.exports.totalEntries = (entriesByDay) => { + let result = new Immutable.List() + entriesByDay.forEach((entry) => { + result = result.push(entry.get('entries')) + }) + return result +} + +module.exports.sortTimeDescending = (left, right) => { + if (left.get('lastAccessedTime') < right.get('lastAccessedTime')) return 1 + if (left.get('lastAccessedTime') > right.get('lastAccessedTime')) return -1 + return 0 +} diff --git a/js/about/history.js b/js/about/history.js index bf33de86f7a..a9d066c735c 100644 --- a/js/about/history.js +++ b/js/about/history.js @@ -14,6 +14,8 @@ const getSetting = require('../settings').getSetting const SortableTable = require('../components/sortableTable') const Button = require('../components/button') const siteUtil = require('../state/siteUtil') +const {makeImmutable} = require('../../app/common/state/immutableUtil') +const historyUtil = require('../../app/common/lib/historyUtil') const ipc = window.chrome.ipc @@ -57,6 +59,7 @@ class HistoryTimeCell extends ImmutableComponent { class HistoryDay extends ImmutableComponent { navigate (entry) { + entry = makeImmutable(entry) aboutActions.newFrame({ location: entry.get('location'), partitionNumber: entry.get('partitionNumber') @@ -79,7 +82,7 @@ class HistoryDay extends ImmutableComponent { urlutils.getHostname(entry.get('location'), true) ])} rowObjects={this.props.entries} - totalRowObjects={this.props.totalEntries} + totalRowObjects={this.props.totalEntries.toJS()} tableID={this.props.tableID} columnClassNames={['time', 'title', 'domain']} addHoverClass @@ -93,53 +96,19 @@ class HistoryDay extends ImmutableComponent { } class GroupedHistoryList extends ImmutableComponent { - getDayString (locale, item) { - const lastAccessedTime = item.get('lastAccessedTime') - return lastAccessedTime - ? new Date(lastAccessedTime).toLocaleDateString(locale, { weekday: 'long', month: 'long', day: 'numeric', year: 'numeric' }) - : '' - } - groupEntriesByDay (locale) { - const reduced = this.props.history.reduce((previousValue, currentValue, currentIndex, array) => { - const result = currentIndex === 1 ? [] : previousValue - if (currentIndex === 1) { - const firstDate = this.getDayString(locale, currentValue) - result.push({date: firstDate, entries: [previousValue]}) - } - const date = this.getDayString(locale, currentValue) - const dateIndex = result.findIndex((entryByDate) => entryByDate.date === date) - if (dateIndex !== -1) { - result[dateIndex].entries.push(currentValue) - } else { - result.push({date: date, entries: [currentValue]}) - } - return result - }) - if (reduced) { - return Array.isArray(reduced) - ? reduced - : [{date: this.getDayString(locale, reduced), entries: [reduced]}] - } - return [] - } - totalEntries (entriesByDay) { - const result = [] - entriesByDay.forEach((entry) => { - result.push(entry.entries) - }) - return result - } render () { const defaultLanguage = this.props.languageCodes.find((lang) => lang.includes(navigator.language)) || 'en-US' - const userLanguage = getSetting(settings.LANGUAGE, this.props.settings) - const entriesByDay = this.groupEntriesByDay(userLanguage || defaultLanguage) + const userLanguage = getSetting(settings.LANGUAGE, this.props.settings) || defaultLanguage + const entriesByDay = historyUtil.groupEntriesByDay(this.props.history, userLanguage) + const totalEntries = historyUtil.totalEntries(entriesByDay) let index = 0 return { entriesByDay.map((groupedEntry) => - ) @@ -189,11 +158,8 @@ class AboutHistory extends React.Component { } get historyDescendingOrder () { 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 - return 0 - }).slice(-500) + .sort(historyUtil.sortTimeDescending) + .slice(-500) } clearBrowsingDataNow () { aboutActions.clearBrowsingDataNow({browserHistory: true}) diff --git a/test/unit/lib/historyUtilTest.js b/test/unit/lib/historyUtilTest.js new file mode 100644 index 00000000000..d0580813d7c --- /dev/null +++ b/test/unit/lib/historyUtilTest.js @@ -0,0 +1,92 @@ +/* global describe, it */ +const assert = require('assert') +const Immutable = require('immutable') +const historyUtil = require('../../../app/common/lib/historyUtil') +require('../braveUnit') + +describe('historyUtil', function () { + const historyDayOne = Immutable.fromJS({ + lastAccessedTime: 1477944718876, + location: 'https://brave.com/page1', + title: 'sample 1', + tags: [] + }) + const historyDayTwo = Immutable.fromJS({ + lastAccessedTime: 1478079042097, + location: 'https://brave.com/page2', + title: 'sample 2', + tags: [] + }) + const historyDayThree = Immutable.fromJS([{ + lastAccessedTime: 1478157051910, + location: 'https://brave.com/page3', + title: 'sample 3', + tags: [] + }, { + lastAccessedTime: 1478157051921, + location: 'https://brave.com/page4', + title: 'sample 4', + tags: [] + }, { + lastAccessedTime: 1478157051932, + location: 'https://brave.com/page5', + title: 'sample 5', + tags: [] + }]) + const historyMultipleDays = historyDayThree.push(historyDayTwo, historyDayOne) + + describe('groupEntriesByDay', function () { + it('returns the result as an Immutable.List', function () { + const result = historyUtil.groupEntriesByDay(historyDayThree) + assert.equal(Immutable.List.isList(result), true) + }) + it('has one object for each day', function () { + const result = historyUtil.groupEntriesByDay(historyDayThree) + assert.equal(result.size, 1) + }) + it('can handle multiple days', function () { + const result = historyUtil.groupEntriesByDay(historyMultipleDays) + assert.equal(result.size, 3) + }) + describe('with the object representing a day', function () { + it('formats a readable `date` field', function () { + const result = historyUtil.groupEntriesByDay(historyDayThree, 'en-US') + assert.equal(result.getIn([0, 'date']), 'Thursday, November 3, 2016') + }) + it('has an entry for each history item', function () { + const result = historyUtil.groupEntriesByDay(historyDayThree, 'en-US') + const entries = result.getIn([0, 'entries']) + assert.equal(entries && entries.size, historyDayThree.size) + }) + }) + }) + + describe('totalEntries', function () { + it('returns the result as an Immutable.List', function () { + const result1 = historyUtil.groupEntriesByDay(historyMultipleDays) + const result2 = historyUtil.totalEntries(result1) + assert.equal(Immutable.List.isList(result2), true) + }) + it('combines entries for multiple days into one response', function () { + const result1 = historyUtil.groupEntriesByDay(historyMultipleDays) + const result2 = historyUtil.totalEntries(result1) + const expectedResult = [ + historyDayThree.toJS(), + [historyDayTwo.toJS()], + [historyDayOne.toJS()] + ] + assert.deepEqual(result2.toJS(), expectedResult) + }) + }) + + describe('sortTimeDescending', function () { + it('sorts the items by date/time DESC', function () { + const result = historyMultipleDays.sort(historyUtil.sortTimeDescending) + const expectedResult = historyDayThree.toJS().reverse() + expectedResult.push(historyDayTwo.toJS()) + expectedResult.push(historyDayOne.toJS()) + + assert.deepEqual(result.toJS(), expectedResult) + }) + }) +})