Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Improves verified-caching #12287

Merged
merged 1 commit into from
Jan 8, 2018
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Dec 14, 2017

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 #12272

Auditors:

Test Plan:

Plan 1

  1. enable wallet
  2. close preference page
  3. visit some pages (stay on this pages more then 15s), some of the verified as well (for example clifton, coinmarket, ddg, etc)
  4. open preference page and go to payments
  5. make sure that all sites that should be verified are verified (to trigger table refresh just visit another site)

Plan 2

  1. enable wallet
  2. every hour there should be call to the server to get publisher timestamp

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

@NejcZdovc
Copy link
Contributor Author

blocked on brave-intl/bat-client#33

@bsclifton bsclifton modified the milestones: 0.19.x Hotfix 9, 0.21.x (Developer Channel) Dec 20, 2017
@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Dec 21, 2017
@codecov-io
Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #12287 into master will increase coverage by 0.01%.
The diff coverage is 81.69%.

@@            Coverage Diff             @@
##           master   #12287      +/-   ##
==========================================
+ Coverage   55.95%   55.97%   +0.01%     
==========================================
  Files         278      278              
  Lines       27029    27034       +5     
  Branches     4373     4372       -1     
==========================================
+ Hits        15125    15133       +8     
+ Misses      11904    11901       -3
Flag Coverage Δ
#unittest 55.97% <81.69%> (+0.01%) ⬆️
Impacted Files Coverage Δ
js/actions/appActions.js 19.22% <ø> (+0.1%) ⬆️
app/browser/reducers/ledgerReducer.js 47.54% <50%> (-0.5%) ⬇️
app/browser/api/ledger.js 52.93% <83.58%> (+1.16%) ⬆️
app/renderer/reducers/frameReducer.js 54.71% <0%> (-2.79%) ⬇️
js/dnd.js 27.17% <0%> (-1.55%) ⬇️
app/browser/api/topSites.js 93.25% <0%> (-1.29%) ⬇️
...r/components/preferences/payment/enabledContent.js 74.87% <0%> (-0.97%) ⬇️
js/actions/windowActions.js 4.95% <0%> (-0.05%) ⬇️
app/browser/reducers/autoplayReducer.js 97.89% <0%> (-0.03%) ⬇️
... and 16 more

if (verifiedTimeoutId) {
clearInterval(verifiedTimeoutId)
}
verifiedTimeoutId = setInterval(getPublisherTimestamp, 1 * ledgerUtil.milliseconds.hour)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to call clearInterval and set to false when payments is turned off?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just for safety, because we trigger boot via action, so there could be race problem if we trigger interval in other place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed base on our slack conversation

assert(checkVerifiedStatusSpy.withArgs(sinon.match.any, 'clifton.io', 20).calledOnce)
})

it('check mulitple publishers', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mulitple/multiple 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two items noted (concern about clearing interval and misspelling)

Changes work great as expected- Code changes look good too 😄 👍

Resolves brave#12272

Auditors:

Test Plan:
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Would be nice to have getPublisherTimestamp return if payments is disabled... but that is an edge case which seems pretty minimal (and would only happen once an hour until closed).

@bsclifton bsclifton merged commit 9f70216 into brave:master Jan 8, 2018
bsclifton added a commit that referenced this pull request Jan 8, 2018
bsclifton added a commit that referenced this pull request Jan 8, 2018
@bsclifton
Copy link
Member

master 9f70216
0.21.x acda5e0
0.20.x 1e14d81

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve verified-caching
3 participants