Fix news P3A enabled metric

brave · Jun 13, 2023 · 2ba6fbb · 2ba6fbb
1 parent 912eda9
commit 2ba6fbb
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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";