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

UGP promo should not be checked at startup #14616

Closed
bbondy opened this issue Jun 30, 2018 · 18 comments
Closed

UGP promo should not be checked at startup #14616

bbondy opened this issue Jun 30, 2018 · 18 comments

Comments

@bbondy
Copy link
Member

bbondy commented Jun 30, 2018

Test plan

see #14621 and #14732

Description

ledger.js has code for the UGP promo to be sent up right away on startup.
/v1/grants

We should be avoiding File IO and network requests at startup time in favour of waiting after some initial delay and/or by lazy loading.

In this case I think some initial delay similar to how runtimeUpdateCheckDelay works should be added in the config. Possibly 2 minutes after startup.

Steps to Reproduce

  1. Add console.log logging to app/browser/api/ledger.js
    2.Start the browser
  2. Observe output at startup time

Actual result:
Network requests happens at startup

Expected result:
Network request shouldn't happen directly at startup.

Reproduces how often:
Always

Brave Version

0.23.19

about:brave info:

Brave: 0.23.19
V8: 6.7.288.46
rev: 178c3fb
Muon: 7.1.3
OS Release: 17.5.0
Update Channel: Release
OS Architecture: x64
OS Platform: macOS
Node.js: 7.9.0
Tor: 0.3.3.7 (git-035a35178c92da94)
Brave Sync: v1.4.2
libchromiumcontent: 67.0.3396.87

Reproducible on current live release:
Yes

Additional Information

@jasonrsadler jasonrsadler self-assigned this Jun 30, 2018
@davidtemkin
Copy link

(Note that the UGP promo check is independent of payments opt-in, i.e., we show it to users before they’ve decided to enable payments.)

We have had an ongoing micro-PR problem where we announce a new UGP giveaway; users look at Brave and see nothing; then they tweet and complain. Our standard response has been to tell them to restart the browser, or (more recently) go to Payments prefs and we do the check at that time. (Even a two minute delay would require that we change what we tell people. We could just tell then "go to payments prefs")

In terms of when it occurs, a two-minute delay could work; or we could do something that resembles what happens for users who don't restart -- e.g., we allow no more than 24 hours before checking for the promo. @BrendanEich @bbondy @mrose17

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Jul 1, 2018

@bbondy FYI, the code for checking promos already happens with a delay (albeit only 10 to 15 seconds: https://github.com/brave/browser-laptop/blob/master/app/browser/api/ledger.js#L1343).

I can set this range for minimum of 2 mins and a max of whatever you think we should have it.

@alexwykoff alexwykoff added release/not-blocking priority/P3 Major loss of function. labels Jul 3, 2018
NejcZdovc added a commit that referenced this issue Jul 6, 2018
NejcZdovc added a commit that referenced this issue Jul 6, 2018
NejcZdovc added a commit that referenced this issue Jul 6, 2018
@LaurenWags
Copy link
Member

LaurenWags commented Jul 13, 2018

Asked in slack as well...

I tried this a few times and Test Plan is working as expected (promotion calls seemed to occur around 55s after the browser was launched). However, when I followed the STR below, my actual and expected results were different. can @bsclifton or @jasonrsadler confirm if my expected results are correct?
Launch Brave (I was using a clean profile)
Navigate to Preferences > Payments
Expected Results: at this point I thought the call to the grant server should be made but it wasn’t (I’m basing this off of #12688).
Actual Results: I still had to wait the 55s from browser launch.
cc @kjozwiak

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Jul 13, 2018

Unless I'm mistaken (or mistaking the STR) you shouldn't get the UGP promo notification (top notification in pic related) unless payments is enabled. When enabled, visiting the payments screen will force the notification. Not to be confused with the 'enable payments' notification (bottom) which will show when wallet is not enabled.

tldr; The call to the grants server is delayed on startup if payments is not enabled. If it is enabled it is delayed unless the user goes to the payments screen (and hasn't seen it yet).

screen shot 2018-07-13 at 10 02 30 am

@LaurenWags
Copy link
Member

so then a better way to check might be:
Launch Brave (use a profile that has payments enabled but has not seen the new UGP promo yet)
Navigate to Preferences > Payments
--> should see the UGP notification once you hit Payments page (you don't have to wait the 55s from browser launch)
wdyt @jasonrsadler ?

@jasonrsadler
Copy link
Contributor

@davidtemkin From #12688, we don't force poll the promotions if payments is not enabled. What @LaurenWags is expecting above is that it should force poll when going to the payments section whether payments is enabled or not. How should we proceed?

cc @NejcZdovc

@davidtemkin
Copy link

@jasonrsadler We need to immediately force poll when a user navigates to Payments settings. The delay being discussed here only pertains to the notification check in the event that the user does not navigate to Payments.
cc: @LaurenWags @NejcZdovc

@davidtemkin
Copy link

Also: are you sure we don't do the notification check unless payments are enabled? I was pretty sure we did it irrespective of that cc: @bradleyrichter @NejcZdovc

@jasonrsadler
Copy link
Contributor

Yes,

Looking at 0.22R5 from a clean profile, when going to payments, the promo server is not polled. The promo server is automatically polled after 10 - 15 seconds so it isn't as noticeable as the new 45 to 60 second interval.

I'll work getting the polling immediate on payments tab enter.

@jasonrsadler
Copy link
Contributor

To more clearly answer:
We still check. We just don't force the check unless payments are enabled.

@jasonrsadler
Copy link
Contributor

Reopened on #14732

@jasonrsadler jasonrsadler reopened this Jul 14, 2018
jasonrsadler pushed a commit that referenced this issue Jul 14, 2018
Promotions server should now be polled from payments screen without having payments enabled.
jasonrsadler pushed a commit that referenced this issue Jul 14, 2018
Promotions server should now be polled from payments screen without having payments enabled.
jasonrsadler pushed a commit that referenced this issue Jul 14, 2018
Promotions server should now be polled from payments screen without having payments enabled.
@jasonrsadler
Copy link
Contributor

Fixed on #14732

@jasonrsadler
Copy link
Contributor

@LaurenWags @davidtemkin
Thanks for the good testing Lauren. Actually found that #14621 caused a break in the notifications. Everything should work properly now.

@LaurenWags
Copy link
Member

@jasonrsadler - I noticed that if I have a profile from June (which had already accepted the June UGP), and I launch 0.23.37 with this profile, I am notified about the UGP immediately (no delay). Is this expected?

@jasonrsadler
Copy link
Contributor

It's not expected to see it if you've already accepted UGP. What this PR addresses is the startup network I/O. If the UGP/Promo server has already been polled in a previous session without the notifications showing, then, yes, I would expect it to come up right away if it already has the data.

@davidtemkin
Copy link

@LaurenWags @jasonrsadler there has been a new UGP promo posted since June -- it went live July 6. So, you could claim a grant from that promotion even if you'd already claimed a grant from the June promotion.

@LaurenWags
Copy link
Member

@davidtemkin when working with @jasonrsadler on my above question, we found an issue with that (at least using a particular profile) - it has been logged as #14747 and added to 0.23.x Release 4 milestone. cc @kjozwiak

@LaurenWags
Copy link
Member

LaurenWags commented Jul 16, 2018

Verified with macOS 10.12.6 using

  • 0.23.37 47b1b59
  • Muon 7.1.6
  • libchromiumcontent 67.0.3396.103

Verified on Windows 10 x64 with

  • 0.23.37 47b1b59
  • Muon 7.1.6
  • libchromiumcontent 67.0.3396.103

Verified on Ubuntu 17.10 x64

  • 0.23.37
  • Muon 7.1.6
  • libchromiumcontent 67.0.3396.103

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