From 820ed5b769378e379cee06226f0c06c08bb888d4 Mon Sep 17 00:00:00 2001 From: Nejc Zdovc Date: Mon, 23 Oct 2017 21:02:07 +0200 Subject: [PATCH] Merge pull request #11592 from NejcZdovc/refactor/#11575-pageData Fixes publisher not added to the ledger --- app/browser/api/ledger.js | 166 +++++---- app/browser/reducers/ledgerReducer.js | 96 ++++- app/browser/reducers/pageDataReducer.js | 67 ---- app/common/lib/ledgerUtil.js | 39 +- app/common/state/pageDataState.js | 66 +--- .../brave/content/scripts/idleHandler.js | 1 + app/sessionStore.js | 5 +- docs/state.md | 40 +- .../app/browser/reducers/ledgerReducerTest.js | 35 +- .../browser/reducers/pageDataReducerTest.js | 344 ------------------ test/unit/app/common/lib/ledgerUtilTest.js | 97 +++-- .../app/common/state/pageDataStateTest.js | 148 -------- 12 files changed, 282 insertions(+), 822 deletions(-) diff --git a/app/browser/api/ledger.js b/app/browser/api/ledger.js index af6249e43a1..18f972917b4 100644 --- a/app/browser/api/ledger.js +++ b/app/browser/api/ledger.js @@ -36,11 +36,14 @@ const tabs = require('../../browser/tabs') const locale = require('../../locale') const appConfig = require('../../../js/constants/appConfig') const getSetting = require('../../../js/settings').getSetting -const {fileUrl, getSourceAboutUrl} = require('../../../js/lib/appUrlUtil') +const {fileUrl, getSourceAboutUrl, isSourceAboutUrl} = require('../../../js/lib/appUrlUtil') const urlParse = require('../../common/urlParse') const ruleSolver = require('../../extensions/brave/content/scripts/pageInformation') const request = require('../../../js/lib/request') const ledgerUtil = require('../../common/lib/ledgerUtil') +const tabState = require('../../common/state/tabState') +const pageDataUtil = require('../../common/lib/pageDataUtil') +const {getWebContents} = require('../../browser/webContentsCache') // Caching let locationDefault = 'NOOP' @@ -734,12 +737,11 @@ const synopsisNormalizer = (state, changedPublisher, returnState) => { return newData } -const updatePublisherInfo = (state, changedPublisher) => { - if (!getSetting(settings.PAYMENTS_ENABLED)) { +const updatePublisherInfo = (state, changedPublisher, refresh = false) => { + if (!refresh && !getSetting(settings.PAYMENTS_ENABLED)) { return state } - // const options = synopsis.options state = synopsisNormalizer(state, changedPublisher, true) return state @@ -857,16 +859,18 @@ const excludeP = (publisherKey, callback) => { }) } -const setLocation = (state, timestamp, tabId) => { +const addVisit = (state, startTimestamp, location, tabId) => { if (!synopsis) { return state } - const locationData = ledgerState.getLocation(state, currentUrl) + location = pageDataUtil.getInfoKey(location) + const locationData = ledgerState.getLocation(state, location) + const timestamp = new Date().getTime() if (_internal.verboseP) { console.log( - `locations[${currentUrl}]=${JSON.stringify(locationData, null, 2)} ` + - `duration=${(timestamp - currentTimestamp)} msec tabId= ${tabId}` + `locations[${location}]=${JSON.stringify(locationData, null, 2)} ` + + `duration=${(timestamp - startTimestamp)} msec tabId= ${tabId}` ) } if (locationData.isEmpty() || !tabId) { @@ -882,21 +886,21 @@ const setLocation = (state, timestamp, tabId) => { visitsByPublisher[publisherKey] = {} } - if (!visitsByPublisher[publisherKey][currentUrl]) { - visitsByPublisher[publisherKey][currentUrl] = { + if (!visitsByPublisher[publisherKey][location]) { + visitsByPublisher[publisherKey][location] = { tabIds: [] } } - const revisitP = visitsByPublisher[publisherKey][currentUrl].tabIds.indexOf(tabId) !== -1 + const revisitP = visitsByPublisher[publisherKey][location].tabIds.indexOf(tabId) !== -1 if (!revisitP) { - visitsByPublisher[publisherKey][currentUrl].tabIds.push(tabId) + visitsByPublisher[publisherKey][location].tabIds.push(tabId) } - let duration = timestamp - currentTimestamp + let duration = timestamp - startTimestamp if (_internal.verboseP) { - console.log('\nadd publisher ' + publisherKey + ': ' + duration + ' msec' + ' revisitP=' + revisitP + ' state=' + - JSON.stringify(underscore.extend({location: currentUrl}, visitsByPublisher[publisherKey][currentUrl]), + console.log('\nadd publisher ' + publisherKey + ': ' + (duration / 1000) + ' sec' + ' revisitP=' + revisitP + ' state=' + + JSON.stringify(underscore.extend({location: location}, visitsByPublisher[publisherKey][location]), null, 2)) } @@ -913,23 +917,36 @@ const setLocation = (state, timestamp, tabId) => { return state } -const addVisit = (state, location, timestamp, tabId) => { +const addNewLocation = (state, location, tabId = tabState.TAB_ID_NONE, keepInfo = false) => { + // We always want to have the latest active tabId + const currentTabId = pageDataState.getLastActiveTabId(state) + state = pageDataState.setLastActiveTabId(state, tabId) if (location === currentUrl) { return state } - state = setLocation(state, timestamp, tabId) + // Save previous recorder page + if (currentUrl !== locationDefault && currentTabId != null && currentTabId !== tabState.TAB_ID_NONE) { + const tab = getWebContents(currentTabId) + const isPrivate = !tab || + tab.isDestroyed() || + !tab.session.partition.startsWith('persist:') - const lastUrl = pageDataState.getLastUrl(state) - const aboutUrl = getSourceAboutUrl(lastUrl) || lastUrl - if (aboutUrl && aboutUrl.match(/^about/)) { - state = pageDataState.resetInfo(state) + const tabFromState = tabState.getByTabId(state, currentTabId) + + // Add visit to the ledger when we are not in a private tab + if (!isPrivate && tabFromState != null && ledgerUtil.shouldTrackView(tabFromState)) { + state = addVisit(state, currentTimestamp, currentUrl, currentTabId) + } } - location = getSourceAboutUrl(location) || location + if (location === locationDefault && !keepInfo) { + state = pageDataState.resetInfo(state) + } - currentUrl = (location && location.match(/^about/)) ? locationDefault : location - currentTimestamp = timestamp + // Update to the latest view + currentUrl = location + currentTimestamp = new Date().getTime() return state } @@ -1069,20 +1086,39 @@ const updateLocation = (state, location, publisherKey) => { return state } -const pageDataChanged = (state) => { - // NB: in theory we have already seen every element in info except for (perhaps) the last one... +const pageDataChanged = (state, viewData = {}, keepInfo = false) => { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + return state + } + let info = pageDataState.getLastInfo(state) + const tabId = viewData.tabId || pageDataState.getLastActiveTabId(state) + const location = viewData.location || locationDefault - if (!synopsis || info.isEmpty()) { + if (!synopsis) { + state = addNewLocation(state, locationDefault, tabId) return state } - if (info.get('url', '').match(/^about/)) { + const realUrl = getSourceAboutUrl(location) || location + if ( + info.isEmpty() && + !isSourceAboutUrl(realUrl) && + viewData && + viewData.tabId != null && + viewData.location != null + ) { + // we need to add visit even when you switch from about page to a normal site + state = addNewLocation(state, location, tabId) + return state + } else if (info.isEmpty() || isSourceAboutUrl(realUrl)) { + // we need to log empty visit + state = addNewLocation(state, locationDefault, tabId, keepInfo) return state } - const location = info.get('key') - const locationData = ledgerState.getLocation(state, location) + let locationKey = info.get('key') + const locationData = ledgerState.getLocation(state, locationKey) let publisherKey = locationData.get('publisher') let publisher = ledgerState.getPublisher(state, publisherKey) if (!publisher.isEmpty()) { @@ -1090,15 +1126,23 @@ const pageDataChanged = (state) => { state = getFavIcon(state, publisherKey, info) } - state = updateLocation(state, location, publisherKey) + state = updateLocation(state, locationKey, publisherKey) } else { - try { - publisherKey = ledgerPublisher.getPublisher(location, _internal.ruleset.raw) - if (!publisherKey || (publisherKey && ledgerUtil.blockedP(state, publisherKey))) { - publisherKey = null + const infoPublisher = info.get('publisher') + if (infoPublisher != null) { + publisherKey = infoPublisher + } else { + try { + // TODO this is only filled when you have ledger on, which means that this whole pageDataChanged + // can be ignored if ledger is disabled? + publisherKey = ledgerPublisher.getPublisher(locationKey, _internal.ruleset.raw) + } catch (ex) { + console.error('getPublisher error for ' + locationKey + ': ' + ex.toString()) } - } catch (ex) { - console.error('getPublisher error for ' + location + ': ' + ex.toString()) + } + + if (!publisherKey || (publisherKey && ledgerUtil.blockedP(state, publisherKey))) { + publisherKey = null } state = ledgerState.setLocationProp(state, info.get('key'), 'publisher', publisherKey) @@ -1113,30 +1157,22 @@ const pageDataChanged = (state) => { } if (initP) { - excludeP(publisherKey, (unused, exclude) => { - if (!getSetting(settings.PAYMENTS_SITES_AUTO_SUGGEST)) { - exclude = true - } - appActions.onPublisherOptionUpdate(publisherKey, 'exclude', exclude) - savePublisherOption(publisherKey, 'exclude', exclude) - }) + if (!getSetting(settings.PAYMENTS_SITES_AUTO_SUGGEST)) { + appActions.onPublisherOptionUpdate(publisherKey, 'exclude', true) + savePublisherOption(publisherKey, 'exclude', true) + } else { + excludeP(publisherKey, (unused, exclude) => { + appActions.onPublisherOptionUpdate(publisherKey, 'exclude', exclude) + savePublisherOption(publisherKey, 'exclude', exclude) + }) + } } - state = updateLocation(state, location, publisherKey) + state = updateLocation(state, locationKey, publisherKey) state = getFavIcon(state, publisherKey, info) } - const pageLoad = pageDataState.getLoad(state) - const view = pageDataState.getView(state) - - if (ledgerUtil.shouldTrackView(view, pageLoad)) { - state = addVisit( - state, - view.get('url', locationDefault), - view.get('timestamp', new Date().getTime()), - view.get('tabId') - ) - } + state = addNewLocation(state, location, tabId, keepInfo) return state } @@ -1230,7 +1266,7 @@ const onWalletRecovery = (state, error, result) => { const quit = (state) => { quitP = true - state = addVisit(state, locationDefault, new Date().getTime(), null) + state = addNewLocation(state, locationDefault) if (!getSetting(settings.PAYMENTS_ENABLED) && getSetting(settings.SHUTDOWN_CLEAR_HISTORY)) { state = ledgerState.resetSynopsis(state, true) @@ -1311,7 +1347,7 @@ const enable = (state, paymentsEnabled) => { } if (synopsis) { - return updatePublisherInfo(state) + return updatePublisherInfo(state, null, true) } if (!ledgerPublisher) { @@ -1848,10 +1884,15 @@ const onCallback = (state, result, delayTime) => { const publishers = ledgerState.getPublishers(state) for (let item of publishers) { const publisherKey = item[0] - excludeP(publisherKey, (unused, exclude) => { - appActions.onPublisherOptionUpdate(publisherKey, 'exclude', exclude) - savePublisherOption(publisherKey, 'exclude', exclude) - }) + if (!getSetting(settings.PAYMENTS_SITES_AUTO_SUGGEST)) { + appActions.onPublisherOptionUpdate(publisherKey, 'exclude', true) + savePublisherOption(publisherKey, 'exclude', true) + } else { + excludeP(publisherKey, (unused, exclude) => { + appActions.onPublisherOptionUpdate(publisherKey, 'exclude', exclude) + savePublisherOption(publisherKey, 'exclude', exclude) + }) + } } }) } @@ -2348,7 +2389,6 @@ module.exports = { backupKeys, recoverKeys, quit, - addVisit, pageDataChanged, init, initialize, diff --git a/app/browser/reducers/ledgerReducer.js b/app/browser/reducers/ledgerReducer.js index dd3f07f2a0f..ec414d428ba 100644 --- a/app/browser/reducers/ledgerReducer.js +++ b/app/browser/reducers/ledgerReducer.js @@ -3,6 +3,7 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const Immutable = require('immutable') +const {BrowserWindow} = require('electron') // Constants const appConstants = require('../../../js/constants/appConstants') @@ -11,6 +12,7 @@ const settings = require('../../../js/constants/settings') // State const ledgerState = require('../../common/state/ledgerState') +const pageDataState = require('../../common/state/pageDataState') // Utils const ledgerApi = require('../../browser/api/ledger') @@ -65,12 +67,6 @@ const ledgerReducer = (state, action, immutableAction) => { } break } - case appConstants.APP_IDLE_STATE_CHANGED: - { - state = ledgerApi.pageDataChanged(state) - state = ledgerApi.addVisit(state, 'NOOP', new Date().getTime(), null) - break - } case appConstants.APP_CHANGE_SETTING: { switch (action.get('key')) { @@ -198,15 +194,6 @@ const ledgerReducer = (state, action, immutableAction) => { state = ledgerState.setInfoProp(state, 'hasBitcoinHandler', hasBitcoinHandler) break } - case 'event-set-page-info': - case appConstants.APP_WINDOW_BLURRED: - case appConstants.APP_CLOSE_WINDOW: - case windowConstants.WINDOW_SET_FOCUSED_FRAME: - case windowConstants.WINDOW_GOT_RESPONSE_DETAILS: - { - state = ledgerApi.pageDataChanged(state) - break - } case appConstants.APP_ON_FAVICON_RECEIVED: { state = ledgerState.setPublishersProp(state, action.get('publisherKey'), 'faviconURL', action.get('blob')) @@ -335,6 +322,85 @@ const ledgerReducer = (state, action, immutableAction) => { state = ledgerState.saveQRCode(state, action.get('currency'), action.get('image')) break } + case appConstants.APP_IDLE_STATE_CHANGED: + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + if (action.has('idleState') && action.get('idleState') !== 'active') { + state = ledgerApi.pageDataChanged(state) + } + break + } + case windowConstants.WINDOW_SET_FOCUSED_FRAME: + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + if (action.get('location')) { + state = ledgerApi.pageDataChanged(state, { + location: action.get('location'), + tabId: action.get('tabId') + }) + } + break + } + case appConstants.APP_WINDOW_BLURRED: + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + let windowCount = BrowserWindow.getAllWindows().filter((win) => win.isFocused()).length + if (windowCount === 0) { + state = ledgerApi.pageDataChanged(state, {}, true) + } + break + } + case 'event-set-page-info': + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + state = ledgerApi.pageDataChanged(state, { + location: action.getIn(['pageInfo', 'url']) + }) + break + } + case appConstants.APP_CLOSE_WINDOW: + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + state = ledgerApi.pageDataChanged(state) + break + } + case windowConstants.WINDOW_GOT_RESPONSE_DETAILS: + { + if (!getSetting(settings.PAYMENTS_ENABLED)) { + break + } + + // Only capture response for the page (not sub resources, like images, JavaScript, etc) + if (action.getIn(['details', 'resourceType']) === 'mainFrame') { + const pageUrl = action.getIn(['details', 'newURL']) + + // create a page view event if this is a page load on the active tabId + const lastActiveTabId = pageDataState.getLastActiveTabId(state) + const tabId = action.get('tabId') + if (!lastActiveTabId || tabId === lastActiveTabId) { + state = ledgerApi.pageDataChanged(state, { + location: pageUrl, + tabId + }) + } + } + break + } } return state } diff --git a/app/browser/reducers/pageDataReducer.js b/app/browser/reducers/pageDataReducer.js index 798bdf342ef..6c7de512b24 100644 --- a/app/browser/reducers/pageDataReducer.js +++ b/app/browser/reducers/pageDataReducer.js @@ -2,87 +2,20 @@ * 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 electron = require('electron') -const BrowserWindow = electron.BrowserWindow - -// Constants -const appConstants = require('../../../js/constants/appConstants') -const windowConstants = require('../../../js/constants/windowConstants') - // State const pageDataState = require('../../common/state/pageDataState') // Utils const {makeImmutable} = require('../../common/state/immutableUtil') -const {isSourceAboutUrl, getTargetAboutUrl} = require('../../../js/lib/appUrlUtil') -const {responseHasContent} = require('../../common/lib/httpUtil') const pageDataReducer = (state, action, immutableAction) => { action = immutableAction || makeImmutable(action) switch (action.get('actionType')) { - case windowConstants.WINDOW_SET_FOCUSED_FRAME: - { - if (action.get('location')) { - state = pageDataState.addView(state, action.get('location'), action.get('tabId')) - } - break - } - case appConstants.APP_WINDOW_BLURRED: - { - let windowCount = BrowserWindow.getAllWindows().filter((win) => win.isFocused()).length - if (windowCount === 0) { - state = pageDataState.addView(state) - } - break - } - // TODO check if this is used anymore - case appConstants.APP_IDLE_STATE_CHANGED: - { - if (action.has('idleState') && action.get('idleState') !== 'active') { - state = pageDataState.addView(state) - } - break - } - case appConstants.APP_WINDOW_CLOSED: - { - state = pageDataState.addView(state) - break - } case 'event-set-page-info': { - // retains all past pages, not really sure that's needed... [MTR] state = pageDataState.addInfo(state, action.get('pageInfo')) break } - case windowConstants.WINDOW_GOT_RESPONSE_DETAILS: - { - // Only capture response for the page (not subresources, like images, JavaScript, etc) - if (action.getIn(['details', 'resourceType']) === 'mainFrame') { - const pageUrl = action.getIn(['details', 'newURL']) - - // create a page view event if this is a page load on the active tabId - const lastActiveTabId = pageDataState.getLastActiveTabId(state) - const tabId = action.get('tabId') - if (!lastActiveTabId || tabId === lastActiveTabId) { - state = pageDataState.addView(state, pageUrl, tabId) - } - - const responseCode = action.getIn(['details', 'httpResponseCode']) - const isNewTab = getTargetAboutUrl('about:new-tab') === pageUrl - if (isSourceAboutUrl(pageUrl) || isNewTab || !responseHasContent(responseCode)) { - break - } - - const pageLoadEvent = makeImmutable({ - timestamp: new Date().getTime(), - url: pageUrl, - tabId: tabId, - details: action.get('details') - }) - state = pageDataState.addLoad(state, pageLoadEvent) - } - break - } } return state diff --git a/app/common/lib/ledgerUtil.js b/app/common/lib/ledgerUtil.js index 02692db93dc..38550ec2db5 100644 --- a/app/common/lib/ledgerUtil.js +++ b/app/common/lib/ledgerUtil.js @@ -18,49 +18,24 @@ const settings = require('../../../js/constants/settings') // Utils const {responseHasContent} = require('./httpUtil') const urlUtil = require('../../../js/lib/urlutil') -const {makeImmutable} = require('../state/immutableUtil') const getSetting = require('../../../js/settings').getSetting /** * Is page an actual page being viewed by the user? (not an error page, etc) * If the page is invalid, we don't want to collect usage info. - * @param {Map} view - an entry from ['pageData', 'view'] - * @param {List} responseList - full ['pageData', 'load'] List + * @param {Map} tabValue - data about provided tab * @return {boolean} true if page should have usage collected, false if not */ -const shouldTrackView = (view, responseList) => { - if (view == null) { +const shouldTrackView = (tabValue) => { + if (tabValue == null) { return false } - view = makeImmutable(view) - const tabId = view.get('tabId') - const url = view.get('url') + const aboutError = tabValue.has('aboutDetails') + const activeEntry = tabValue.getIn(['navigationState', 'activeEntry']) || {} + const response = activeEntry.httpStatusCode === 0 || responseHasContent(activeEntry.httpStatusCode) - if (!url || !tabId) { - return false - } - - responseList = makeImmutable(responseList) - if (!responseList || responseList.size === 0) { - return false - } - - for (let i = (responseList.size - 1); i > -1; i--) { - const response = responseList.get(i) - - if (!response) { - continue - } - - const responseUrl = response.getIn(['details', 'newURL'], null) - - if (url === responseUrl && response.get('tabId') === tabId) { - return responseHasContent(response.getIn(['details', 'httpResponseCode'])) - } - } - - return false + return !aboutError && response } const batToCurrencyString = (bat, ledgerData) => { diff --git a/app/common/state/pageDataState.js b/app/common/state/pageDataState.js index 457744fe081..28feb291af8 100644 --- a/app/common/state/pageDataState.js +++ b/app/common/state/pageDataState.js @@ -4,39 +4,14 @@ const Immutable = require('immutable') +// States +const tabState = require('./tabState') + // Utils const pageDataUtil = require('../lib/pageDataUtil') -const {getWebContents} = require('../../browser/webContentsCache') -const {isSourceAboutUrl} = require('../../../js/lib/appUrlUtil') const {makeImmutable} = require('./immutableUtil') const pageDataState = { - addView: (state, url = null, tabId = null) => { - const tab = getWebContents(tabId) - const isPrivate = !tab || - tab.isDestroyed() || - !tab.session.partition.startsWith('persist:') - - state = pageDataState.setLastActiveTabId(state, tabId) - - if ((url && isSourceAboutUrl(url)) || isPrivate) { - url = null - } - - const lastView = pageDataState.getView(state) - if (lastView.get('url') === url) { - return state - } - - let pageViewEvent = makeImmutable({ - timestamp: new Date().getTime(), - url, - tabId - }) - state = state.setIn(['pageData', 'last', 'url'], url) - return state.setIn(['pageData', 'view'], pageViewEvent) - }, - addInfo: (state, data) => { if (data == null) { return state @@ -55,24 +30,6 @@ const pageDataState = { return state.setIn(['pageData', 'last', 'info'], '') }, - addLoad: (state, data) => { - if (data == null) { - return state - } - - // select only last 100 loads - const newLoad = state.getIn(['pageData', 'load'], Immutable.List()).slice(-100).push(data) - return state.setIn(['pageData', 'load'], newLoad) - }, - - getView: (state) => { - return state.getIn(['pageData', 'view']) || Immutable.Map() - }, - - getLastUrl: (state) => { - return state.getIn(['pageData', 'last', 'url']) - }, - getLastInfo: (state) => { const key = state.getIn(['pageData', 'last', 'info']) @@ -83,33 +40,22 @@ const pageDataState = { return state.getIn(['pageData', 'info', key], Immutable.Map()) }, - getLoad: (state) => { - return state.getIn(['pageData', 'load'], Immutable.List()) - }, - getLastActiveTabId: (state) => { return state.getIn(['pageData', 'last', 'tabId']) }, setLastActiveTabId: (state, tabId) => { - return state.setIn(['pageData', 'last', 'tabId'], tabId) - }, - - setPublisher: (state, key, publisher) => { - if (key == null) { - return state + if (tabId == null) { + tabId = tabState.TAB_ID_NONE } - return state.setIn(['pageData', 'info', key, 'publisher'], publisher) + return state.setIn(['pageData', 'last', 'tabId'], tabId) }, resetPageData: (state) => { return state - .setIn(['pageData', 'load'], Immutable.List()) .setIn(['pageData', 'info'], Immutable.Map()) - .setIn(['pageData', 'view'], Immutable.Map()) .setIn(['pageData', 'last', 'info'], null) - .setIn(['pageData', 'last', 'url'], null) .setIn(['pageData', 'last', 'tabId'], null) } } diff --git a/app/extensions/brave/content/scripts/idleHandler.js b/app/extensions/brave/content/scripts/idleHandler.js index 1675af5efb5..2d434999ec2 100644 --- a/app/extensions/brave/content/scripts/idleHandler.js +++ b/app/extensions/brave/content/scripts/idleHandler.js @@ -1,5 +1,6 @@ chrome.idle.setDetectionInterval(15 * 60) chrome.idle.onStateChanged.addListener((idleState) => { + // Uses appConstants.APP_IDLE_STATE_CHANGED action constant. chrome.ipcRenderer.send('dispatch-action', JSON.stringify([{ actionType: 'app-idle-state-changed', idleState diff --git a/app/sessionStore.js b/app/sessionStore.js index 98c5d60a4fb..a40103944b1 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -816,11 +816,8 @@ module.exports.defaultAppState = () => { info: {}, last: { info: '', - url: '', tabId: -1 - }, - load: [], - view: {} + } }, ledger: { about: { diff --git a/docs/state.md b/docs/state.md index 54be54129f1..f73d835ac98 100644 --- a/docs/state.md +++ b/docs/state.md @@ -251,37 +251,19 @@ AppStore enabled: boolean // enable noscript }, pageData: { - info: [{ - faviconURL: string, - protocol: string, - publisher: string, - timestamp: number, - url: string, - }], + info: { + [urlKey]: { + faviconURL: string, + protocol: string, + publisher: string, + timestamp: number, + url: string + } + }, last: { info: string, // last added info - tabId: number, // last active tabId - url: string // last active URL - }, - load: [{ - timestamp: number, - url: string, - tabId: number, - details: { - status: boolean, - newURL: string, - originalURL: string, - httpResponseCode: number, - requestMethod: string, - referrer: string, - resourceType: string - } - }], - view: { - timestamp: number, - url: string, - tabId: number - } // we save only the last view + tabId: number // last active tabId + } }, passwords: [{ // not being used anymore diff --git a/test/unit/app/browser/reducers/ledgerReducerTest.js b/test/unit/app/browser/reducers/ledgerReducerTest.js index f8cc84d70e3..bf7232f6799 100644 --- a/test/unit/app/browser/reducers/ledgerReducerTest.js +++ b/test/unit/app/browser/reducers/ledgerReducerTest.js @@ -1,4 +1,4 @@ -/* global describe, it, before, after */ +/* global describe, it, before, beforeEach, after, afterEach */ const Immutable = require('immutable') const assert = require('assert') const mockery = require('mockery') @@ -215,25 +215,38 @@ describe('ledgerReducer unit tests', function () { describe('APP_IDLE_STATE_CHANGED', function () { let pageDataChangedSpy - let addVisitSpy - before(function () { + beforeEach(function () { pageDataChangedSpy = sinon.spy(fakeLedgerApi, 'pageDataChanged') - addVisitSpy = sinon.spy(fakeLedgerApi, 'addVisit') + }) + + afterEach(function () { + pageDataChangedSpy.restore() + }) + it('does not calls ledgerApi.pageDataChanged when no idle state is provided', function () { returnedState = ledgerReducer(appState, Immutable.fromJS({ actionType: appConstants.APP_IDLE_STATE_CHANGED })) + assert(pageDataChangedSpy.notCalled) }) - after(function () { - pageDataChangedSpy.restore() - addVisitSpy.restore() - }) - it('calls ledgerApi.pageDataChanged', function () { + it('calls ledgerApi.pageDataChanged when not in idleState', function () { + returnedState = ledgerReducer(appState, Immutable.fromJS({ + actionType: appConstants.APP_IDLE_STATE_CHANGED, + idleState: 'notActive' + })) assert(pageDataChangedSpy.withArgs(appState).calledOnce) }) - it('calls ledgerApi.addVisit', function () { - assert(addVisitSpy.calledOnce) + it('does not calls ledgerApi.pageDataChanged when in idleState', function () { + returnedState = ledgerReducer(appState, Immutable.fromJS({ + actionType: appConstants.APP_IDLE_STATE_CHANGED, + idleState: 'active' + })) + assert(pageDataChangedSpy.notCalled) }) it('returns a modified state', function () { + returnedState = ledgerReducer(appState, Immutable.fromJS({ + actionType: appConstants.APP_IDLE_STATE_CHANGED, + idleState: 'notActive' + })) assert.notDeepEqual(returnedState, appState) }) }) diff --git a/test/unit/app/browser/reducers/pageDataReducerTest.js b/test/unit/app/browser/reducers/pageDataReducerTest.js index 29f47783af1..e9a38e4376d 100644 --- a/test/unit/app/browser/reducers/pageDataReducerTest.js +++ b/test/unit/app/browser/reducers/pageDataReducerTest.js @@ -8,9 +8,6 @@ const Immutable = require('immutable') const assert = require('assert') const sinon = require('sinon') -const appConstants = require('../../../../../js/constants/appConstants') -const windowConstants = require('../../../../../js/constants/windowConstants') - describe('pageDataReducer unit tests', function () { let pageDataReducer, pageDataState, isFocused @@ -69,159 +66,6 @@ describe('pageDataReducer unit tests', function () { this.clock.restore() }) - describe('WINDOW_SET_FOCUSED_FRAME', function () { - let spy - - afterEach(function () { - spy.restore() - }) - - it('null case', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: windowConstants.WINDOW_SET_FOCUSED_FRAME - }) - - assert.equal(spy.notCalled, true) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('data is ok', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: windowConstants.WINDOW_SET_FOCUSED_FRAME, - location: 'https://brave.com', - tabId: 1 - }) - - const expectedState = state - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], 'https://brave.com') - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: 'https://brave.com', - tabId: 1 - })) - - assert.equal(spy.calledOnce, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) - - describe('APP_WINDOW_BLURRED', function () { - let spy - - afterEach(function () { - spy.restore() - }) - - it('there is one focused window', function () { - isFocused = true - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_WINDOW_BLURRED - }) - - assert.equal(spy.notCalled, true) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('there is no focused windows', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_WINDOW_BLURRED - }) - - const expectedState = state - .setIn(['pageData', 'last', 'tabId'], null) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: null, - tabId: null - })) - - assert.equal(spy.calledOnce, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) - - describe('APP_IDLE_STATE_CHANGED', function () { - let spy - - afterEach(function () { - spy.restore() - }) - - it('null case', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_IDLE_STATE_CHANGED - }) - - assert.equal(spy.notCalled, true) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('idleState is active', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_IDLE_STATE_CHANGED, - idleState: 'active' - }) - - assert.equal(spy.notCalled, true) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('idleState is not active', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_IDLE_STATE_CHANGED, - idleState: 'nonactive' - }) - - const expectedState = state - .setIn(['pageData', 'last', 'tabId'], null) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: null, - tabId: null - })) - - assert.equal(spy.calledOnce, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) - - describe('APP_WINDOW_CLOSED', function () { - let spy - - afterEach(function () { - spy.restore() - }) - - it('data is ok', function () { - spy = sinon.spy(pageDataState, 'addView') - const result = pageDataReducer(state, { - actionType: appConstants.APP_WINDOW_CLOSED - }) - - const expectedState = state - .setIn(['pageData', 'last', 'tabId'], null) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: null, - tabId: null - })) - - assert.equal(spy.calledOnce, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) - describe('event-set-page-info', function () { let spy @@ -251,192 +95,4 @@ describe('pageDataReducer unit tests', function () { assert.deepEqual(result.toJS(), expectedState.toJS()) }) }) - - describe('WINDOW_GOT_RESPONSE_DETAILS', function () { - let spyView, spyActiveTab, spyLoad - - afterEach(function () { - spyView.restore() - spyActiveTab.restore() - spyLoad.restore() - }) - - it('null case', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - const result = pageDataReducer(state, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS - }) - - assert.equal(spyView.notCalled, true) - assert.equal(spyActiveTab.notCalled, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('add view if we dont have last active tab', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - const result = pageDataReducer(state, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: { - resourceType: 'mainFrame', - newURL: 'https://brave.com' - }, - tabId: 1 - }) - - const expectedState = state - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], 'https://brave.com') - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: 'https://brave.com', - tabId: 1 - })) - - assert.equal(spyView.calledOnce, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - - it('add view if tabId is the same as last active tab', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - - const newState = state - .setIn(['pageData', 'last', 'tabId'], 1) - - const result = pageDataReducer(newState, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: { - resourceType: 'mainFrame', - newURL: 'https://brave.com' - }, - tabId: 1 - }) - - const expectedState = newState - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], 'https://brave.com') - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: 0, - url: 'https://brave.com', - tabId: 1 - })) - - assert.equal(spyView.calledOnce, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - - it('dont add view if tabId is different as last active tab', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - - const newState = state - .setIn(['pageData', 'last', 'tabId'], 2) - - const result = pageDataReducer(newState, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: { - resourceType: 'mainFrame', - newURL: 'https://brave.com' - }, - tabId: 1 - }) - - assert.equal(spyView.notCalled, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), newState.toJS()) - }) - - it('dont add load if response is not successful', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - - const newState = state - .setIn(['pageData', 'last', 'tabId'], 2) - - const result = pageDataReducer(newState, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: { - resourceType: 'mainFrame', - newURL: 'https://brave.com', - httpResponseCode: 500 - }, - tabId: 1 - }) - - assert.equal(spyView.notCalled, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), newState.toJS()) - }) - - it('dont add load if URL is about page', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - - const newState = state - .setIn(['pageData', 'last', 'tabId'], 2) - - const result = pageDataReducer(newState, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: { - resourceType: 'mainFrame', - newURL: 'about:history', - httpResponseCode: 200 - }, - tabId: 1 - }) - - assert.equal(spyView.notCalled, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.notCalled, true) - assert.deepEqual(result.toJS(), newState.toJS()) - }) - - it('add load', function () { - spyView = sinon.spy(pageDataState, 'addView') - spyActiveTab = sinon.spy(pageDataState, 'getLastActiveTabId') - spyLoad = sinon.spy(pageDataState, 'addLoad') - - const details = { - resourceType: 'mainFrame', - newURL: 'https://brave.com', - httpResponseCode: 200 - } - const newState = state - .setIn(['pageData', 'last', 'tabId'], 2) - - const result = pageDataReducer(newState, { - actionType: windowConstants.WINDOW_GOT_RESPONSE_DETAILS, - details: details, - tabId: 1 - }) - - const expectedState = newState - .setIn(['pageData', 'load'], Immutable.fromJS([{ - timestamp: 0, - url: 'https://brave.com', - tabId: 1, - details: details - }])) - - assert.equal(spyView.notCalled, true) - assert.equal(spyActiveTab.calledOnce, true) - assert.equal(spyLoad.calledOnce, true) - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) }) diff --git a/test/unit/app/common/lib/ledgerUtilTest.js b/test/unit/app/common/lib/ledgerUtilTest.js index 3586e169320..3e068702fa6 100644 --- a/test/unit/app/common/lib/ledgerUtilTest.js +++ b/test/unit/app/common/lib/ledgerUtilTest.js @@ -32,68 +32,67 @@ describe('ledgerUtil test', function () { }) describe('shouldTrackView', function () { - const validView = Immutable.fromJS({ - tabId: 1, - url: 'https://brave.com/' + it('null case', function () { + assert.equal(ledgerUtil.shouldTrackView(), false) }) - const validResponseList = Immutable.fromJS([ - { - tabId: validView.get('tabId'), - details: { - newURL: validView.get('url'), - httpResponseCode: 200 - } - } - ]) - const noMatchResponseList = Immutable.fromJS([ - { - tabId: 3, - details: { - newURL: 'https://not-brave.com' - } - } - ]) - const matchButErrored = Immutable.fromJS([ - { - tabId: validView.get('tabId'), - details: { - newURL: validView.get('url'), - httpResponseCode: 404 + + it('we have about error, but dont have tab navigationState', function () { + const param = Immutable.fromJS({ + aboutDetails: { + title: 'error' } - } - ]) + }) + assert.equal(ledgerUtil.shouldTrackView(param), false) + }) - describe('input validation', function () { - it('returns false if view is falsey', function () { - assert.equal(ledgerUtil.shouldTrackView(null, validResponseList), false) + it('we have tab, but dont have active entry', function () { + const param = Immutable.fromJS({ + navigationState: {} }) - it('returns false if view.url is falsey', function () { - assert.equal(ledgerUtil.shouldTrackView({tabId: 1}, validResponseList), false) + assert.equal(ledgerUtil.shouldTrackView(param), false) + }) + + it('we have tab, but active entry dont have httpStatusCode', function () { + const param = Immutable.fromJS({ + navigationState: { + activeEntry: {} + } }) - it('returns false if view.tabId is falsey', function () { - assert.equal(ledgerUtil.shouldTrackView({url: 'https://brave.com/'}, validResponseList), false) + assert.equal(ledgerUtil.shouldTrackView(param), false) + }) + + it('we have tab, but httpStatusCode is 500', function () { + let param = Immutable.fromJS({ + navigationState: {} }) - it('returns false if responseList is falsey', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, null), false) + + param = param.setIn(['navigationState', 'activeEntry'], { + httpStatusCode: 500 }) - it('returns false if responseList is not an array', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, {}), false) + assert.equal(ledgerUtil.shouldTrackView(param), false) + }) + + it('we have tab and httpStatusCode is 200', function () { + let param = Immutable.fromJS({ + navigationState: {} }) - it('returns false if responseList is a 0 length array', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, []), false) + param = param.setIn(['navigationState', 'activeEntry'], { + httpStatusCode: 200 }) + assert.equal(ledgerUtil.shouldTrackView(param), true) }) - describe('when finding a matching response based on tabId and url', function () { - it('returns false if no match found', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, noMatchResponseList), false) - }) - it('returns false if match is found BUT response code is a failure (ex: 404)', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, matchButErrored), false) + it('we have tab and httpStatusCode is 200, but we have aboutDetails', function () { + let param = Immutable.fromJS({ + aboutDetails: { + title: 'error' + }, + navigationState: {} }) - it('returns true when match is found AND response code is a success (ex: 200)', function () { - assert.equal(ledgerUtil.shouldTrackView(validView, validResponseList), true) + param = param.setIn(['navigationState', 'activeEntry'], { + httpStatusCode: 200 }) + assert.equal(ledgerUtil.shouldTrackView(param), false) }) }) diff --git a/test/unit/app/common/state/pageDataStateTest.js b/test/unit/app/common/state/pageDataStateTest.js index f2eef85aaf1..9245b354493 100644 --- a/test/unit/app/common/state/pageDataStateTest.js +++ b/test/unit/app/common/state/pageDataStateTest.js @@ -83,79 +83,6 @@ describe('pageDataState unit tests', function () { clock.restore() }) - describe('addView', function () { - it('null case', function () { - const result = pageDataState.addView(state) - const expectedResult = state - .setIn(['pageData', 'last', 'tabId'], null) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: now.getTime(), - url: null, - tabId: null - })) - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - - it('url is the same as last one', function () { - const newState = state - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: now.getTime(), - url: 'https://brave.com', - tabId: 1 - })) - const result = pageDataState.addView(newState, 'https://brave.com', 1) - const expectedResult = newState - .setIn(['pageData', 'last', 'tabId'], 1) - - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - - it('url is private', function () { - isPrivate = true - - const result = pageDataState.addView(state, 'https://brave.com', 1) - const expectedResult = state - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: now.getTime(), - url: null, - tabId: 1 - })) - - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - - it('url is about page', function () { - const result = pageDataState.addView(state, 'about:history', 1) - const expectedResult = state - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], null) - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: now.getTime(), - url: null, - tabId: 1 - })) - - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - - it('url is ok', function () { - const result = pageDataState.addView(state, 'https://brave.com', 1) - const expectedResult = state - .setIn(['pageData', 'last', 'tabId'], 1) - .setIn(['pageData', 'last', 'url'], 'https://brave.com') - .setIn(['pageData', 'view'], Immutable.fromJS({ - timestamp: now.getTime(), - url: 'https://brave.com', - tabId: 1 - })) - - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - }) - describe('addInfo', function () { it('null case', function () { const result = pageDataState.addInfo(state) @@ -176,56 +103,6 @@ describe('pageDataState unit tests', function () { }) }) - describe('addLoad', function () { - it('null case', function () { - const result = pageDataState.addLoad(state) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('data is ok', function () { - const result = pageDataState.addLoad(state) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('we only take last 100 views', function () { - let newState = state - - for (let i = 0; i < 100; i++) { - const data = Immutable.fromJS([{ - timestamp: now.getTime(), - url: `https://page${i}.com`, - tabId: 1 - }]) - newState = newState.setIn(['pageData', 'load'], newState.getIn(['pageData', 'load']).push(data)) - } - - const newLoad = Immutable.fromJS({ - timestamp: now.getTime(), - url: 'https://brave.com', - tabId: 1 - }) - - const result = pageDataState.addLoad(newState, newLoad) - const expectedResult = newState - .setIn(['pageData', 'load'], newState.getIn(['pageData', 'load']).shift()) - .setIn(['pageData', 'load'], newState.getIn(['pageData', 'load']).push(newLoad)) - - assert.deepEqual(result.toJS(), expectedResult.toJS()) - }) - }) - - describe('getView', function () { - it('null case', function () { - const result = pageDataState.getView(state) - assert.deepEqual(result, Immutable.Map()) - }) - - it('data is ok', function () { - const result = pageDataState.getView(stateWithData) - assert.deepEqual(result.toJS(), stateWithData.getIn(['pageData', 'view']).toJS()) - }) - }) - describe('getLastInfo', function () { it('null case', function () { const result = pageDataState.getLastInfo(state) @@ -261,18 +138,6 @@ describe('pageDataState unit tests', function () { }) }) - describe('getLoad', function () { - it('null case', function () { - const result = pageDataState.getLoad(state) - assert.deepEqual(result, Immutable.List()) - }) - - it('data is there', function () { - const result = pageDataState.getLoad(stateWithData) - assert.deepEqual(result.toJS(), stateWithData.getIn(['pageData', 'load']).toJS()) - }) - }) - describe('getLastActiveTabId', function () { it('null case', function () { const result = pageDataState.getLastActiveTabId(state) @@ -292,17 +157,4 @@ describe('pageDataState unit tests', function () { assert.deepEqual(result.toJS(), expectedState.toJS()) }) }) - - describe('setPublisher', function () { - it('null case', function () { - const result = pageDataState.setPublisher(state) - assert.deepEqual(result.toJS(), state.toJS()) - }) - - it('data is ok', function () { - const result = pageDataState.setPublisher(stateWithData, 'https://brave.com/', 'https://brave.com') - const expectedState = stateWithData.setIn(['pageData', 'info', 'https://brave.com/', 'publisher'], 'https://brave.com') - assert.deepEqual(result.toJS(), expectedState.toJS()) - }) - }) })