Fixes publisher toggle (heart), button enables after time requirements satisfied. #13900

ryanml · 2018-04-23T08:15:06Z

Fixes #12675

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.
Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Enable payments
Select a minimum browsing time and visits in payment settings, (1 visit for ease)
Open web console for error notation.
Navigate to any web page.
Browse the page for the minimum browsing time.
Confirm that the publisher toggle button is enabled (Yellow if site was pinned when included in the ledger, deeper black if merely included in the ledger)
In the same tab, navigate to a different web page
Repeat steps 5 and 6.
Open a new tab, navigate to a new web page
Repeat steps 5 and 6

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

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

Given that there didn't seem to be an appropriate appConstant (or a relatively efficient/telling new one that could be made), it seems to make sense for the Publisher Toggle component to have an attribute of its internal state that can check its ledger status at an interval.

Currently, the interval avoids starting until after APP_TOP_SITE_DATA_AVAILABLE has dispatched. Doing this call to pageDataChanged before that action can break ledger activity recorder.

The interval will clear once the publisher toggle has reached an enabled state (there's no way for a site to become disabled merely by browsing, so this interval does not concern that path), any time the publisherToggle component dismounts, and reset if the tab loads to a new potential publisher.

ryanml · 2018-04-24T04:27:33Z

WIP has been removed, I may have a way to eliminate the use of setTimeout

NejcZdovc

getting this error:

"Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the PublisherToggle component.", source: http://localhost:8080/gen/app.entry.js (5816)

NejcZdovc · 2018-04-25T08:09:29Z

app/renderer/components/navigation/publisherToggle.js

+    // page visit has started recording. It is important that a
+    // page data update is not triggered before this has happened.
+    const topSitesWait = ledgerUtil.milliseconds.second * 6
+    const updateWait = ledgerUtil.milliseconds.second * 3


could we instead of triggering this every 3s, trigger this only after MIN_VISIT_TIME?

also we have min count, so that means if you set this for 10, you need to visit this site 10 times. Maybe we should only add this timer if this visit would hit this minimum. This means that we would not add timer until you have 9 visits already logged and this is your 10 visit. Only in this case we would add timer, which would be min visit time

@ryanml wdyt?

I think this approach would be a lot more efficient yes, especially for minimums with multiple visits. It only would not work for one scenario:

Suppose your minimums are 1 visit for 1 minute, and you've made one visit but only for 41 seconds. Visiting the site again with this approach you'd have to wait 60 seconds for the full MIN_VISIT_TIME to pass.

To get around that, maybe we could calculate the time remaining, (In the above scenario, calculate 60000 - 41000 = 19000 and set the timeout to 19000). Does that sound agreeable?

yes let's do that

ryanml · 2018-04-25T17:50:34Z

app/renderer/components/navigation/publisherToggle.js

@@ -32,6 +32,7 @@ const fundUnverifiedPublisherImage = require('../../../extensions/brave/img/urlb
 class PublisherToggle extends React.Component {
  constructor (props) {
    super(props)
+    this.mounted = false


Thanks for fixing 👍

ryanml · 2018-04-25T17:54:48Z

app/renderer/components/navigation/publisherToggle.js

+    // page visit has started recording. It is important that a
+    // page data update is not triggered before this has happened.
+    const topSitesWait = ledgerUtil.milliseconds.second * 6
+    const updateWait = ledgerUtil.milliseconds.second * 3


I think this approach would be a lot more efficient yes, especially for minimums with multiple visits. It only would not work for one scenario:

Suppose your minimums are 1 visit for 1 minute, and you've made one visit but only for 41 seconds. Visiting the site again with this approach you'd have to wait 60 seconds for the full MIN_VISIT_TIME to pass.

To get around that, maybe we could calculate the time remaining, (In the above scenario, calculate 60000 - 41000 = 19000 and set the timeout to 19000). Does that sound agreeable?

ryanml · 2018-04-25T23:05:14Z

@NejcZdovc changes pushed, ready to review. The correct calculations should now be in place so an update is only triggered when it needs to.

NejcZdovc · 2018-04-26T05:25:06Z

app/common/lib/ledgerUtil.js

@@ -591,6 +592,45 @@ const getMediaProvider = (url, firstPartyUrl, referrer) => {
  return provider
 }

+const shouldSetTimeout = (publisherKey) => {


Please rename this to something more verbose like hasRequiredVisits

NejcZdovc · 2018-04-26T05:25:19Z

app/common/lib/ledgerUtil.js

+  return (visitDifference === 1)
+}
+
+const getTimeoutWait = (publisherKey) => {


Please rename this to something more verbose like getRemainingRequiredTime

NejcZdovc · 2018-04-26T05:26:14Z

app/common/lib/ledgerUtil.js

+  const publisher = ledgerState.getPublisher(appStore.state, publisherKey)
+  const publisherVisits = publisher.get('visits')
+
+  if (publisherVisits === undefined) {


Can we just check here if publisherVisits is number?

NejcZdovc · 2018-04-26T05:27:09Z

app/common/lib/ledgerUtil.js

+  const publisherDuration = publisher.get('duration')
+
+  if (
+    publisherDuration === undefined ||


Can we just check here if publisherDuration is number?

NejcZdovc · 2018-04-26T05:31:34Z

@ryanml please add tests

codecov-io · 2018-04-26T05:37:05Z

Codecov Report

Merging #13900 into master will increase coverage by 0.03%.
The diff coverage is 80.28%.

@@            Coverage Diff             @@
##           master   #13900      +/-   ##
==========================================
+ Coverage   56.45%   56.49%   +0.03%     
==========================================
  Files         284      284              
  Lines       29220    29302      +82     
  Branches     4851     4864      +13     
==========================================
+ Hits        16495    16553      +58     
- Misses      12725    12749      +24

Flag	Coverage Δ
#unittest	`56.49% <80.28%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
js/constants/appConstants.js	`100% <ø> (ø)`	⬆️
app/browser/reducers/ledgerReducer.js	`44.77% <0%> (-0.42%)`	⬇️
app/common/lib/ledgerUtil.js	`96.09% <100%> (+0.38%)`	⬆️
js/actions/appActions.js	`19.26% <100%> (+0.42%)`	⬆️
.../renderer/components/navigation/publisherToggle.js	`81.25% <70%> (-5.42%)`	⬇️
js/stores/appStoreRenderer.js	`91.66% <0%> (-8.34%)`	⬇️
app/renderer/components/reduxComponent.js	`57.75% <0%> (-3.45%)`	⬇️
js/stores/windowStore.js	`28.46% <0%> (-0.3%)`	⬇️
... and 1 more

NejcZdovc

works great, after tests are added we can merge it

ryanml · 2018-04-26T20:32:36Z

@NejcZdovc tests added

…satisfied

NejcZdovc

++ works great

Fixes publisher toggle (heart), button enables after time requirements satisfied.

NejcZdovc · 2018-04-27T08:01:09Z

master 15a6784
0.23 b0e14b2
0.22-release3 f5e4dc6

ryanml added feature/rewards PR/work-in-progress ⚒ labels Apr 23, 2018

ryanml self-assigned this Apr 23, 2018

ryanml removed the PR/work-in-progress ⚒ label Apr 24, 2018

ryanml requested a review from NejcZdovc April 24, 2018 04:02

NejcZdovc suggested changes Apr 25, 2018

View reviewed changes

NejcZdovc reviewed Apr 25, 2018

View reviewed changes

ryanml commented Apr 25, 2018

View reviewed changes

ryanml force-pushed the ledger-heart-toggle-fix branch from 99ce0c3 to 5b1043a Compare April 25, 2018 23:03

ryanml added the PR/pending-review label Apr 25, 2018

ryanml force-pushed the ledger-heart-toggle-fix branch 2 times, most recently from 3e19cb6 to ec69a3c Compare April 26, 2018 04:59

NejcZdovc reviewed Apr 26, 2018

View reviewed changes

NejcZdovc self-requested a review April 26, 2018 05:30

NejcZdovc removed the PR/pending-review label Apr 26, 2018

ryanml force-pushed the ledger-heart-toggle-fix branch from 9965ee5 to 387517a Compare April 26, 2018 06:22

NejcZdovc added the PR/needs-rebase ۞ label Apr 26, 2018

NejcZdovc reviewed Apr 26, 2018

View reviewed changes

NejcZdovc self-requested a review April 26, 2018 09:54

ryanml force-pushed the ledger-heart-toggle-fix branch from 387517a to 3415463 Compare April 26, 2018 19:14

ryanml removed the PR/needs-rebase ۞ label Apr 26, 2018

ryanml force-pushed the ledger-heart-toggle-fix branch from 9c8759e to a78d744 Compare April 26, 2018 20:31

ryanml added the PR/pending-review label Apr 26, 2018

Fixes brave#12675, enabling publisher toggle after time requirements …

486c0d5

…satisfied

NejcZdovc force-pushed the ledger-heart-toggle-fix branch from a78d744 to 486c0d5 Compare April 27, 2018 07:56

NejcZdovc approved these changes Apr 27, 2018

View reviewed changes

NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 27, 2018

NejcZdovc removed the PR/pending-review label Apr 27, 2018

NejcZdovc merged commit 15a6784 into brave:master Apr 27, 2018

NejcZdovc added a commit that referenced this pull request Apr 27, 2018

Merge pull request #13900 from ryanml/ledger-heart-toggle-fix

b0e14b2

Fixes publisher toggle (heart), button enables after time requirements satisfied.

NejcZdovc added a commit that referenced this pull request Apr 27, 2018

Merge pull request #13900 from ryanml/ledger-heart-toggle-fix

f5e4dc6

Fixes publisher toggle (heart), button enables after time requirements satisfied.

This was referenced May 2, 2018

All publishers show same amount of time spent even if the time spent is more #13993

Closed

Fixes extra visit time on first logged visit being ignored #13999

Merged

ryanml mentioned this pull request May 28, 2018

Add to Payments icon in the URL bar is not enabling or turning to orange colour even after media publishers got added to ledger #14253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes publisher toggle (heart), button enables after time requirements satisfied. #13900

Fixes publisher toggle (heart), button enables after time requirements satisfied. #13900

ryanml commented Apr 23, 2018 •

edited by NejcZdovc

Loading

ryanml commented Apr 24, 2018

NejcZdovc left a comment

NejcZdovc Apr 25, 2018

NejcZdovc Apr 25, 2018

ryanml Apr 25, 2018

NejcZdovc Apr 25, 2018

ryanml Apr 25, 2018

ryanml Apr 25, 2018

ryanml commented Apr 25, 2018

NejcZdovc Apr 26, 2018

NejcZdovc Apr 26, 2018 •

edited

Loading

NejcZdovc Apr 26, 2018

NejcZdovc Apr 26, 2018

NejcZdovc commented Apr 26, 2018

codecov-io commented Apr 26, 2018 •

edited

Loading

NejcZdovc left a comment

ryanml commented Apr 26, 2018

NejcZdovc left a comment

NejcZdovc commented Apr 27, 2018

Fixes publisher toggle (heart), button enables after time requirements satisfied. #13900

Fixes publisher toggle (heart), button enables after time requirements satisfied. #13900

Conversation

ryanml commented Apr 23, 2018 • edited by NejcZdovc Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

ryanml commented Apr 24, 2018

NejcZdovc left a 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

ryanml commented Apr 25, 2018

Choose a reason for hiding this comment

NejcZdovc Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 26, 2018

codecov-io commented Apr 26, 2018 • edited Loading

Codecov Report

NejcZdovc left a comment

Choose a reason for hiding this comment

ryanml commented Apr 26, 2018

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 27, 2018

ryanml commented Apr 23, 2018 •

edited by NejcZdovc

Loading

NejcZdovc Apr 26, 2018 •

edited

Loading

codecov-io commented Apr 26, 2018 •

edited

Loading