From 2ba6fbb406dba676dcd3d5014688e903d048ee33 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Wed, 31 May 2023 19:16:16 -0700 Subject: [PATCH] Fix news P3A enabled metric --- .../browser/brave_news_controller.cc | 8 +++++++- .../browser/brave_news_controller.h | 1 + .../brave_news/browser/brave_news_p3a.cc | 20 +++++++++++++++++-- .../browser/brave_news_p3a_unittest.cc | 4 ++++ components/brave_news/common/pref_names.h | 1 + 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/components/brave_news/browser/brave_news_controller.cc b/components/brave_news/browser/brave_news_controller.cc index 37e37bbaf05c..5da75b5ed220 100644 --- a/components/brave_news/browser/brave_news_controller.cc +++ b/components/brave_news/browser/brave_news_controller.cc @@ -18,6 +18,7 @@ #include "base/functional/bind.h" #include "base/functional/callback_forward.h" #include "base/functional/callback_helpers.h" +#include "base/location.h" #include "base/time/time.h" #include "base/values.h" #include "brave/components/api_request_helper/api_request_helper.h" @@ -56,6 +57,9 @@ namespace { // The favicon size we desire. The favicons are rendered at 24x24 pixels but // they look quite a bit nicer if we get a 48x48 pixel icon and downscale it. constexpr uint32_t kDesiredFaviconSizePixels = 48; +// Since we have two boolean prefs for the News enabled status, a delay +// will be used so that we only report the histogram once for both pref updates. +constexpr base::TimeDelta kP3AEnabledReportTimeDelay = base::Seconds(3); } // namespace bool GetIsEnabled(PrefService* prefs) { @@ -627,7 +631,9 @@ void BraveNewsController::Prefetch() { } void BraveNewsController::OnOptInChange() { - p3a::RecordFeatureEnabledChange(prefs_); + p3a_enabled_report_timer_.Start( + FROM_HERE, kP3AEnabledReportTimeDelay, + base::BindOnce(&p3a::RecordFeatureEnabledChange, prefs_)); ConditionallyStartOrStopTimer(); } diff --git a/components/brave_news/browser/brave_news_controller.h b/components/brave_news/browser/brave_news_controller.h index 59dc9e9b9543..5012d1ece4f1 100644 --- a/components/brave_news/browser/brave_news_controller.h +++ b/components/brave_news/browser/brave_news_controller.h @@ -158,6 +158,7 @@ class BraveNewsController : public KeyedService, PrefChangeRegistrar pref_change_registrar_; base::OneShotTimer timer_prefetch_; + base::OneShotTimer p3a_enabled_report_timer_; base::RepeatingTimer timer_feed_update_; base::RepeatingTimer timer_publishers_update_; base::CancelableTaskTracker task_tracker_; diff --git a/components/brave_news/browser/brave_news_p3a.cc b/components/brave_news/browser/brave_news_p3a.cc index fc1cebaad84a..195b4f1e15d0 100644 --- a/components/brave_news/browser/brave_news_p3a.cc +++ b/components/brave_news/browser/brave_news_p3a.cc @@ -38,6 +38,11 @@ constexpr char kUsageDailyHistogramName[] = "Brave.Today.UsageDaily"; namespace { +bool IsNewsEnabled(PrefService* prefs) { + return prefs->GetBoolean(prefs::kBraveNewsOptedIn) && + prefs->GetBoolean(prefs::kNewTabPageShowToday); +} + uint64_t AddToWeeklyStorageAndGetSum(PrefService* prefs, const char* pref_name, int change) { @@ -131,8 +136,14 @@ void RecordTotalCardViews(PrefService* prefs, uint64_t count_delta) { } void RecordFeatureEnabledChange(PrefService* prefs) { - bool enabled = prefs->GetBoolean(prefs::kBraveNewsOptedIn) && - prefs->GetBoolean(prefs::kNewTabPageShowToday); + bool enabled = IsNewsEnabled(prefs); + bool was_ever_enabled = prefs->GetBoolean(prefs::kBraveNewsWasEverEnabled); + if (!enabled && !was_ever_enabled) { + // If the user clicked "no thanks" on the NTP, then we don't want to record + // this as an opt-out, since they were never opted in. + return; + } + prefs->SetBoolean(prefs::kBraveNewsWasEverEnabled, true); UMA_HISTOGRAM_BOOLEAN(kIsEnabledHistogramName, enabled); } @@ -145,6 +156,10 @@ void RecordAtInit(PrefService* prefs) { RecordWeeklySessionCount(prefs, false); RecordWeeklyDisplayAdsViewedCount(prefs, false); RecordTotalCardViews(prefs, 0); + + if (IsNewsEnabled(prefs)) { + prefs->SetBoolean(prefs::kBraveNewsWasEverEnabled, true); + } } void RegisterProfilePrefs(PrefRegistrySimple* registry) { @@ -156,6 +171,7 @@ void RegisterProfilePrefs(PrefRegistrySimple* registry) { registry, prefs::kBraveNewsFirstSessionTime, prefs::kBraveNewsLastSessionTime, prefs::kBraveNewsUsedSecondDay, nullptr, nullptr); + registry->RegisterBooleanPref(prefs::kBraveNewsWasEverEnabled, false); } void RegisterProfilePrefsForMigration(PrefRegistrySimple* registry) { diff --git a/components/brave_news/browser/brave_news_p3a_unittest.cc b/components/brave_news/browser/brave_news_p3a_unittest.cc index d3817467e9ce..220b74141e07 100644 --- a/components/brave_news/browser/brave_news_p3a_unittest.cc +++ b/components/brave_news/browser/brave_news_p3a_unittest.cc @@ -326,6 +326,10 @@ TEST_F(BraveNewsP3ATest, TestNewUserReturningNotFollowingDay) { TEST_F(BraveNewsP3ATest, TestIsEnabled) { PrefService* prefs = GetPrefs(); + // should not record "disabled" if never opted in + prefs->SetBoolean(prefs::kNewTabPageShowToday, false); + histogram_tester_.ExpectTotalCount(kIsEnabledHistogramName, 0); + prefs->SetBoolean(prefs::kBraveNewsOptedIn, true); prefs->SetBoolean(prefs::kNewTabPageShowToday, true); RecordFeatureEnabledChange(prefs); diff --git a/components/brave_news/common/pref_names.h b/components/brave_news/common/pref_names.h index eb1079e49c57..a8d59d8cf742 100644 --- a/components/brave_news/common/pref_names.h +++ b/components/brave_news/common/pref_names.h @@ -37,6 +37,7 @@ constexpr char kBraveNewsFirstSessionTime[] = constexpr char kBraveNewsUsedSecondDay[] = "brave.today.p3a_used_second_day"; constexpr char kBraveNewsLastSessionTime[] = "brave.today.p3a_last_session_time"; +constexpr char kBraveNewsWasEverEnabled[] = "brave.today.p3a_was_ever_enabled"; // Dictionary value keys constexpr char kBraveNewsDirectFeedsKeyTitle[] = "title";