Fixes publisher not added to the ledger #11592

NejcZdovc · 2017-10-18T07:57:32Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Resolves #11553 - Publishers auto-included even with auto-include switch disabled
Resolves #11575 - Page data refactor
Resolves #11273 - Ledger table doesn't auto populate the publishers
Resolves #11274 - Site not added to publisher list even after spending ~10 mins on the site

Auditors:

Test Plan:

for every test plan you need to clean your profile and enable payments

Plan A

open preferences, go to payments
open a new tab
load a site
stay on that site for 15s
switch to preference tab and site should be added

Plan B

open preferences, go to payments
open a new tab
load a site
stay on that site for 15s
open a a new tab
switch to preference tab and site should be added

~~Plan C~~
~~- do plan B~~
~~- switch to the site tab and stay on it for 15s~~
~~- open new tab and visit different site for 15s~~
~~- switch to preference tab and new site should be added and old site should have 2 visits~~
Plan C is not valid, please check plan E

Plan D

open preferences, go to payments
change min visit time to 1min
open new tab
load a site stay on that site for 15s
open another app (so that focus will be out)
click back on opened site in brave and wait for 1m
switch to preference tab and site should be added

Plan E

do plan B
go back to site tab and stay on that site for 15s
switch to preference tab and site should have 1 visit, but 20+ seconds visit time

Plan F

Open preferences, go to payments
Disable auto-include settings
Visit some random sites so that the sites are added to the ledger table
Ensure no site is auto included that gets added to the table

Plan G

open preferences, go to payments
open a new tab
load a site stay on that site for 15s
open a private tab
load a new site stay on that site for 15s
switch to preference tab and table should only contain site visited in normal tab and not from private tab

Plan H

do plan A
disable payments
visit few more sites in new tabs
switch to preference tab and ledger table should only have site from plan A

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-10-18T08:19:43Z

Codecov Report

Merging #11592 into master will decrease coverage by 0.28%.
The diff coverage is 22.78%.

@@            Coverage Diff             @@
##           master   #11592      +/-   ##
==========================================
- Coverage   52.54%   52.26%   -0.29%     
==========================================
  Files         268      268              
  Lines       25296    25311      +15     
  Branches     4032     4034       +2     
==========================================
- Hits        13293    13228      -65     
- Misses      12003    12083      +80

Flag	Coverage Δ
#unittest	`52.26% <22.78%> (-0.29%)`	⬇️

Impacted Files	Coverage Δ
app/sessionStore.js	`76.12% <ø> (ø)`	⬆️
app/browser/reducers/pageDataReducer.js	`100% <ø> (ø)`	⬆️
app/common/state/pageDataState.js	`79.54% <100%> (-8.6%)`	⬇️
app/common/lib/ledgerUtil.js	`84.87% <100%> (-0.74%)`	⬇️
app/browser/api/ledger.js	`31% <15.9%> (-0.3%)`	⬇️
app/browser/reducers/ledgerReducer.js	`43.76% <18.64%> (-4.8%)`	⬇️

NejcZdovc · 2017-10-20T08:04:44Z

I am done with an initial work. I am waiting for some feedback regarding what is visit, then I will do final clenup

NejcZdovc · 2017-10-23T08:06:19Z

PR is ready for the review

Resolves brave#11575 Auditors: Test Plan:

bbondy · 2017-10-23T13:56:04Z

app/browser/reducers/ledgerReducer.js

@@ -3,6 +3,8 @@
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

 const Immutable = require('immutable')
+const electron = require('electron')


this is fine but I think preferred style is const {BrowserWindow} = require('electron')

I see it was moved code, so feel free to leave as is.

bbondy · 2017-10-23T13:57:24Z

app/browser/reducers/ledgerReducer.js

+          if (!lastActiveTabId || tabId === lastActiveTabId) {
+            state = ledgerApi.pageDataChanged(state, {
+              location: pageUrl,
+              tabId: tabId


nit on style:
just tabId instead of tabId: tabId

bbondy · 2017-10-23T14:00:56Z

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


nit: start comment with capital and end in period.

bbondy · 2017-10-23T14:01:36Z

test/unit/app/browser/reducers/ledgerReducerTest.js

+    afterEach(function () {
+      pageDataChangedSpy.restore()
+    })
+    it('doesnt calls ledgerApi.pageDataChanged when no idle state is provided', function () {


nit: does not

bbondy · 2017-10-23T14:01:51Z

test/unit/app/browser/reducers/ledgerReducerTest.js

      assert(pageDataChangedSpy.withArgs(appState).calledOnce)
    })
-    it('calls ledgerApi.addVisit', function () {
-      assert(addVisitSpy.calledOnce)
+    it('doesnt calls ledgerApi.pageDataChanged when in idleState', function () {


nit does not or doesn't

bbondy · 2017-10-23T14:04:09Z

app/common/lib/ledgerUtil.js

 * @return {boolean} true if page should have usage collected, false if not
 */
-const shouldTrackView = (view, responseList) => {
-  if (view == null) {
+const shouldTrackView = (tab) => {


nit: please rename this to tabValue because we usually use tab as the webContents in code.

bbondy · 2017-10-23T14:05:19Z

app/browser/api/ledger.js

  if (location === currentUrl) {
    return state
  }

-  state = setLocation(state, timestamp, tabId)
+  /*
+    Save previous recorder page


nit: * to align with other stars

bbondy · 2017-10-23T14:05:38Z

app/browser/api/ledger.js

-  const aboutUrl = getSourceAboutUrl(lastUrl) || lastUrl
-  if (aboutUrl && aboutUrl.match(/^about/)) {
-    state = pageDataState.resetInfo(state)
+    // add visit to the ledger when we are not in a private tab


nit comment style

bbondy · 2017-10-23T14:05:53Z

app/browser/api/ledger.js


-  currentUrl = (location && location.match(/^about/)) ? locationDefault : location
-  currentTimestamp = timestamp
+  // update to the latest view


nit comment style

Fixes publisher not added to the ledger

NejcZdovc · 2017-10-23T19:05:39Z

master 3cf6a50
0.21 2fa2f88
0.20 820ed5b
0.19 61c92a8

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Oct 18, 2017

NejcZdovc added this to the 0.19.x Hotfix 3 (Release channel) milestone Oct 18, 2017

NejcZdovc self-assigned this Oct 18, 2017

bbondy modified the milestones: 0.19.x Hotfix 3 (Release channel), 0.19.x Hotfix 2 (Release channel) Oct 18, 2017

NejcZdovc force-pushed the refactor/#11575-pageData branch 5 times, most recently from 9c20c0b to cc9b183 Compare October 19, 2017 11:46

NejcZdovc removed the PR/work-in-progress ⚒ label Oct 20, 2017

NejcZdovc force-pushed the refactor/#11575-pageData branch 3 times, most recently from 30e33b5 to f7b1813 Compare October 23, 2017 08:05

NejcZdovc requested review from bbondy and bsclifton October 23, 2017 08:05

Fixes publisher not added to the ledger

07942a0

Resolves brave#11575 Auditors: Test Plan:

NejcZdovc force-pushed the refactor/#11575-pageData branch from f7b1813 to 4d5c436 Compare October 23, 2017 12:55

bbondy reviewed Oct 23, 2017

View reviewed changes

bbondy approved these changes Oct 23, 2017

View reviewed changes

Removes collection when wallet is disabled

eac5a03

NejcZdovc force-pushed the refactor/#11575-pageData branch from 4d5c436 to eac5a03 Compare October 23, 2017 17:50

NejcZdovc merged commit 3cf6a50 into brave:master Oct 23, 2017

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11592 from NejcZdovc/refactor/#11575-pageData

2fa2f88

Fixes publisher not added to the ledger

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11592 from NejcZdovc/refactor/#11575-pageData

820ed5b

Fixes publisher not added to the ledger

NejcZdovc added a commit that referenced this pull request Oct 23, 2017

Merge pull request #11592 from NejcZdovc/refactor/#11575-pageData

61c92a8

Fixes publisher not added to the ledger

This was referenced Oct 24, 2017

Page data refactor #11575

Closed

Ledger table doesnt auto populate the publishers #11273

Closed

This was referenced Oct 24, 2017

Publishers auto-included even with auto-include switch disabled #11553

Closed

Site not added to publisher list even after spending ~10 mins on the site #11274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes publisher not added to the ledger #11592

Fixes publisher not added to the ledger #11592

NejcZdovc commented Oct 18, 2017 •

edited

Loading

codecov-io commented Oct 18, 2017 •

edited

Loading

NejcZdovc commented Oct 20, 2017

NejcZdovc commented Oct 23, 2017

bbondy Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017 •

edited

Loading

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

bbondy Oct 23, 2017

NejcZdovc Oct 23, 2017

NejcZdovc commented Oct 23, 2017

Fixes publisher not added to the ledger #11592

Fixes publisher not added to the ledger #11592

Conversation

NejcZdovc commented Oct 18, 2017 • edited Loading

codecov-io commented Oct 18, 2017 • edited Loading

Codecov Report

NejcZdovc commented Oct 20, 2017

NejcZdovc commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Oct 23, 2017

NejcZdovc commented Oct 18, 2017 •

edited

Loading

codecov-io commented Oct 18, 2017 •

edited

Loading

bbondy Oct 23, 2017 •

edited

Loading