Delaying ref headers fetch until after startup #14620

ryanml · 2018-07-01T02:20:22Z

Fixes #14615

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:

Create empty profile folder
Create a file called promoCode in the profile folder with a test promo code, such as ~~kam253~~
Run Brave with LEDGER_VERBOSE=true and LEDGER_ENVIRONMENT=staging
Confirm promotion code is read after 50 - 70 seconds (you should see call in the terminal)
Make sure that promoCode file is deleted when using real promo code

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

NejcZdovc · 2018-07-01T04:55:24Z

app/browser/api/ledger.js

+  // Delay should not occur after client boot has occured
+  const delay = (startupFetchDelay && !bootP) ? startupFetchDelay : 0
+
+  setTimeout(() => {


We need to track timeout and clear it before we add a new one

NejcZdovc · 2018-07-02T04:34:58Z

@bbondy in this PR we are delaying ref request, but we are not delaying initial referral code read (IO). Would you like to delay this as well?

NejcZdovc · 2018-07-06T05:24:58Z

@ryanml let's defer this read as well https://github.com/brave/browser-laptop/blob/master/app/browser/api/ledger.js?utf8=%E2%9C%93#L2346

ryanml · 2018-07-07T00:01:22Z

@NejcZdovc in the current flow, delaying the promocode read on startup in turn will delay the call to fetchReferralHeaders that was occuring on startup, so I was able to condense the two schedules.

cc: @bbondy

NejcZdovc · 2018-07-11T13:00:29Z

@ryanml please add auto-closing tag

NejcZdovc · 2018-07-11T13:05:49Z

app/browser/api/ledger.js

+
+  promoCodeReadTimeoutId = setTimeout(() => {
+    module.exports.firstRunPromoCode()
+  }, ledgerUtil.milliseconds.minute)


this should be random for example 50s to 70s

NejcZdovc · 2018-07-11T13:06:31Z

app/browser/api/ledger.js

-        }
-        fetchReferralHeaders()
-      })
+    schedulePromoCodeRead()
  } else {
    fetchReferralHeaders()


This should be delayed as well

This is now delayed as well

ryanml · 2018-07-12T02:10:18Z

@NejcZdovc changes addressed, ready for review

NejcZdovc · 2018-07-12T04:09:48Z

app/browser/api/ledger.js

+  }
+
+  promoRefFetchTimeoutId = setTimeout(() => {
+    if (updateState.getUpdateProp(state, 'referralDownloadId') == null) {


state in this case could be already outdated. You have up to 70s old state inside

App action has been created to remedy this, a fresh state will be used in these actions now.

NejcZdovc · 2018-07-12T04:10:25Z

@ryanml please add test plan

ryanml · 2018-07-15T00:58:57Z

@jasonrsadler rebased

jasonrsadler

Left one performance related note. Also, when running through the test plan above I get a 400 on the promo PUT. Maybe I'm doing something wrong?

jasonrsadler · 2018-07-15T01:18:05Z

app/browser/api/ledger.js

-    fetchReferralHeaders()
-  }
+  // Schedule promo run and referral header fetch
+  schedulePromoRefFetch(state)


I think we should move this call to the APP_INITIALIZED hook. in here:
https://github.com/brave/browser-laptop/blob/master/app/browser/reducers/ledgerReducer.js#L613
This gets called after most of the browser startup work and ledger.initialize and could save us a little bit on startup :) wdyt?

@jasonrsadler done. Also I can confirm that I am receiving 400 for the promo PUT using the test code on master.

jasonrsadler

-nit- When run with LEDGER_ENVIRONMENT=staging and verbose, gives a 500: Referral check was not successful [object Object]. I think we should check for body.message and display that so we have reason. Also, in DM do we have a working example of success?

ryanml · 2018-07-18T04:55:22Z

@jasonrsadler am receiving a 400 on promo PUT as well on master, still see what's going on with things

@NejcZdovc

NejcZdovc

Test plan is not working for me, promoCode was not deleted. There were no request tot the server

ryanml · 2018-07-19T22:18:35Z

Further inspecting the 400 that occurs on PUT to /promo/initialize/nonua, I am seeing that referralApi (even when running non-staging) is always an empty string. The response from the endpoint is:

"child \"api_key\" fails because [\"api_key\" is required]"

The referralApi key should either be extracted from the buildConfig (you would need to package) or provided via LEDGER_REFERRAL_API_KEY so this is probably why we are running in to issues.

This causes the chain reaction and causes the other 500 onReferralInit

cc: @NejcZdovc @jasonrsadler

…tartup

NejcZdovc · 2018-07-20T03:58:47Z

updated test plan to the real promoCode key, so 500 should go away

promo code was faulty

NejcZdovc

++

Delaying ref headers fetch until after startup

NejcZdovc · 2018-07-24T04:57:19Z

master 2483444
0.24 ed46f98
0.23 bc030f6

ryanml added feature/rewards PR/work-in-progress ⚒ labels Jul 1, 2018

ryanml self-assigned this Jul 1, 2018

NejcZdovc reviewed Jul 1, 2018

View reviewed changes

ryanml force-pushed the fix-14615 branch 2 times, most recently from 1d5945f to 88a876e Compare July 1, 2018 23:26

ryanml requested review from bbondy and NejcZdovc July 2, 2018 04:06

ryanml added PR/pending-review and removed PR/work-in-progress ⚒ labels Jul 2, 2018

ryanml added the PR/needs-rebase ۞ label Jul 6, 2018

ryanml force-pushed the fix-14615 branch from 88a876e to 52595eb Compare July 6, 2018 22:11

ryanml removed the PR/needs-rebase ۞ label Jul 6, 2018

ryanml force-pushed the fix-14615 branch 2 times, most recently from 0fd12d4 to 32adb33 Compare July 6, 2018 23:59

NejcZdovc added this to the 0.23.x Release 3 (Beta Channel) milestone Jul 7, 2018

NejcZdovc requested a review from jasonrsadler July 11, 2018 11:41

NejcZdovc reviewed Jul 11, 2018

View reviewed changes

NejcZdovc modified the milestones: 0.23.x Release 3 (Beta Channel), 0.23.x Release 4 Jul 11, 2018

ryanml force-pushed the fix-14615 branch from 32adb33 to 0c8e79f Compare July 12, 2018 02:08

NejcZdovc reviewed Jul 12, 2018

View reviewed changes

ryanml force-pushed the fix-14615 branch from 0c8e79f to beef0da Compare July 12, 2018 04:37

jasonrsadler previously requested changes Jul 15, 2018

View reviewed changes

ryanml force-pushed the fix-14615 branch from ccdd4cd to b376e2f Compare July 15, 2018 03:42

jasonrsadler reviewed Jul 16, 2018

View reviewed changes

ryanml added PR/work-in-progress ⚒ and removed PR/pending-review labels Jul 16, 2018

alexwykoff modified the milestones: 0.23.x Release 4, 0.23.x w/ Chromium 68 (BETA Channel) Jul 17, 2018

NejcZdovc previously requested changes Jul 18, 2018

View reviewed changes

ryanml added the PR/needs-rebase ۞ label Jul 19, 2018

ryanml force-pushed the fix-14615 branch from b376e2f to 4c8efba Compare July 19, 2018 06:24

ryanml removed the PR/needs-rebase ۞ label Jul 19, 2018

Fixes Issue brave#14615, delaying promocode and referral fetches on s…

037fa51

…tartup

ryanml force-pushed the fix-14615 branch from 4c8efba to 037fa51 Compare July 19, 2018 22:58

ryanml added PR/pending-review and removed PR/work-in-progress ⚒ labels Jul 24, 2018

ryanml requested review from NejcZdovc and jasonrsadler July 24, 2018 02:19

NejcZdovc removed the PR/pending-review label Jul 24, 2018

NejcZdovc approved these changes Jul 24, 2018

View reviewed changes

NejcZdovc merged commit 2483444 into brave:master Jul 24, 2018

NejcZdovc added a commit that referenced this pull request Jul 24, 2018

Merge pull request #14620 from ryanml/fix-14615

ed46f98

Delaying ref headers fetch until after startup

NejcZdovc added a commit that referenced this pull request Jul 24, 2018

Merge pull request #14620 from ryanml/fix-14615

bc030f6

Delaying ref headers fetch until after startup

kjozwiak mentioned this pull request Aug 21, 2018

reducing time for the ref headers fetch at startup #15067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delaying ref headers fetch until after startup #14620

Delaying ref headers fetch until after startup #14620

ryanml commented Jul 1, 2018 •

edited by NejcZdovc

Loading

NejcZdovc Jul 1, 2018

ryanml Jul 1, 2018

NejcZdovc commented Jul 2, 2018

NejcZdovc commented Jul 6, 2018

ryanml commented Jul 7, 2018

NejcZdovc commented Jul 11, 2018

NejcZdovc Jul 11, 2018

ryanml Jul 12, 2018

NejcZdovc Jul 11, 2018

ryanml Jul 12, 2018

ryanml commented Jul 12, 2018

NejcZdovc Jul 12, 2018 •

edited

Loading

ryanml Jul 12, 2018

NejcZdovc commented Jul 12, 2018

ryanml commented Jul 15, 2018

jasonrsadler left a comment

jasonrsadler Jul 15, 2018

ryanml Jul 15, 2018

ryanml Jul 15, 2018

jasonrsadler left a comment •

edited

Loading

ryanml commented Jul 18, 2018

NejcZdovc left a comment

ryanml commented Jul 19, 2018 •

edited

Loading

NejcZdovc commented Jul 20, 2018 •

edited

Loading

NejcZdovc left a comment

NejcZdovc commented Jul 24, 2018

Delaying ref headers fetch until after startup #14620

Delaying ref headers fetch until after startup #14620

Conversation

ryanml commented Jul 1, 2018 • edited by NejcZdovc Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 2, 2018

NejcZdovc commented Jul 6, 2018

ryanml commented Jul 7, 2018

NejcZdovc commented Jul 11, 2018

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 Jul 12, 2018

NejcZdovc Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 12, 2018

ryanml commented Jul 15, 2018

jasonrsadler 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

jasonrsadler left a comment • edited Loading

Choose a reason for hiding this comment

ryanml commented Jul 18, 2018

NejcZdovc left a comment

Choose a reason for hiding this comment

ryanml commented Jul 19, 2018 • edited Loading

NejcZdovc commented Jul 20, 2018 • edited Loading

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 24, 2018

ryanml commented Jul 1, 2018 •

edited by NejcZdovc

Loading

NejcZdovc Jul 12, 2018 •

edited

Loading

jasonrsadler left a comment •

edited

Loading

ryanml commented Jul 19, 2018 •

edited

Loading

NejcZdovc commented Jul 20, 2018 •

edited

Loading