Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug: Ensure usage pings sent on browser upgrade #5635

Merged
merged 1 commit into from
May 23, 2020
Merged

bug: Ensure usage pings sent on browser upgrade #5635

merged 1 commit into from
May 23, 2020

Conversation

kkuehlz
Copy link
Contributor

@kkuehlz kkuehlz commented May 23, 2020 •
edited by bsclifton
Loading

Fixes brave/brave-browser#9921

Usage pings block on initialization. Since we changed the preference for
referral initialization, users upgrading to the newest version won't
initialize with referral server since the kReferralInitialization will
always be set to false. Fallback and also send usage ping if
kReferralCheckedForPromoCodeFile is set.

Resolves

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@kkuehlz
bug: Ensure usage pings sent on browser upgrade
5aaaf64 
Usage pings block on initialization. Since we changed the preference for
referral initialization, users upgrading to the newest version won't
initialize with referral server since the kReferralInitialization will
always be set to false. Fallback and also send usage ping if
kReferralCheckedForPromoCodeFile is set.
emerick
emerick approved these changes May 23, 2020
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@bsclifton bsclifton added this to the 1.11.x - Nightly milestone May 23, 2020
bsclifton
bsclifton approved these changes May 23, 2020
View reviewed changes
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.

++

@bsclifton bsclifton merged commit 0e67d89 into master May 23, 2020
@bsclifton bsclifton deleted the usage_pings_upgrading_users branch May 23, 2020 06:25
@kkuehlz kkuehlz mentioned this pull request May 23, 2020
Closed
brave-builds pushed a commit that referenced this pull request May 23, 2020
@brave-browser-releases
Uplift of #5635 (squashed) to beta
ae4d8d6
brave-builds pushed a commit that referenced this pull request May 23, 2020
@brave-browser-releases
Uplift of #5635 (squashed) to release
8c022e8
This was referenced May 23, 2020
Merged
Merged
bsclifton
bsclifton reviewed May 26, 2020
View reviewed changes
browser/brave_stats_updater.cc
@@ -79,7 +79,8 @@ void BraveStatsUpdater::Start() {
DCHECK(!server_ping_startup_timer_);
server_ping_startup_timer_ = std::make_unique<base::OneShotTimer>();
#if BUILDFLAG(ENABLE_BRAVE_REFERRALS)
if (pref_service_->GetBoolean(kReferralInitialization)) {
if (pref_service_->GetBoolean(kReferralInitialization) ||
pref_service_->GetBoolean(kReferralCheckedForPromoCodeFile)) {
Copy link
Member

Choose a reason for hiding this comment

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

@simonhong does this look correct? @keur was mentioning you had found this in the original PR

Copy link
Member

Choose a reason for hiding this comment

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

#5144 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this change seems correct because kReferralInitialization is only set to true for new installs.
If kReferralCheckedForPromoCodeFile is set, kReferralInitialization is remained as false.
As description stated, all installs before kReferralInitialization is introduce, we should check kReferralCheckedForPromoCodeFile.

@LaurenWags
Copy link
Member

Verified using:

Brave | 1.10.74 Chromium: 81.0.4044.138 (Official Build) dev (64-bit)
-- | --
Revision | 8c6c7ba89cc9453625af54f11fd83179e23450fa-refs/branch-heads/4044@{#999}
OS | macOS Version 10.14.6 (Build 18G3020)

Scenario 1 (Upgrade case w/ non-default promo code):

  1. Launch Fiddler.
  2. Installed 1.8.96
  3. Launch 1.8.96 using promoCode KAM253
  4. Confirmed I saw ping in Fiddler:

Screen Shot 2020-05-26 at 7 31 36 PM

5. Close Brave, copy profile.

  1. Move computer clock ahead one day.

  2. Launch Brave using 1.9.72.

  3. Confirmed I could reproduce the issue of not seeing a ping like above image.

  4. Close Brave.

  5. Rename copied profile to be for dev channel.

  6. Launch Dev.

  7. Confirmed I saw ping in Fiddler:

Screen Shot 2020-05-28 at 2 07 31 PM

Scenario 2 (Upgrade case with default promo code):

  1. Launch Fiddler.
  2. Installed 1.8.96
  3. Launch 1.8.96 without a promoCode (uses default BRV001)
  4. Confirmed I saw ping in Fiddler.
  5. Close Brave, copy profile.
  6. Move computer clock ahead one day.
  7. Launch Brave using 1.9.72
  8. Confirmed I could reproduce the issue of not seeing a ping like above image.
  9. Close Brave.
  10. Rename copied profile to be for dev channel.
  11. Launch Dev.
  12. Confirmed I saw ping in Fiddler.

Scenario 3 (Clean install of 1.10.x):

  1. Launch Fiddler.
  2. Installed dev 1.10.74
  3. Launch 1.10.74 without a promoCode (uses default BRV001)
  4. Confirmed I saw ping in Fiddler.
  5. Close Brave.
  6. Move computer clock ahead one day.
  7. Launch Dev 1.10.74
  8. Confirmed I saw ping in Fiddler.

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

@bsclifton bsclifton bsclifton approved these changes

@simonhong simonhong simonhong left review comments

@emerick emerick emerick approved these changes

Assignees

@kkuehlz kkuehlz

Labels
None yet
Projects
None yet
Milestone
1.11.x - Release #1
Development

Successfully merging this pull request may close these issues.

Stats ping not being sent for users who updated to 1.9.72
5 participants
@kkuehlz @LaurenWags @bsclifton @emerick @simonhong