Merge pull request #15779 from brave/news-desktop-v2-enable

Enable news v2 by default for Desktop

components/brave_today/browser/channels_controller.cc

@@ -34,6 +34,20 @@ bool IsChannelSubscribedInLocale(const base::Value::Dict& subscriptions,
 }
 }  // namespace
 
+// static
+void ChannelsController::SetChannelSubscribedPref(PrefService* prefs,
+                                                  const std::string& locale,
+                                                  const std::string& channel_id,
+                                                  bool subscribed) {
+  DictionaryPrefUpdate update(prefs, prefs::kBraveNewsChannels);
+  auto* locale_dict = update->GetDict().EnsureDict(locale);
+  if (!subscribed) {
+    locale_dict->Remove(channel_id);
+  } else {
+    locale_dict->Set(channel_id, true);
+  }
+}
+
 ChannelsController::ChannelsController(
     PrefService* prefs,
     PublishersController* publishers_controller)
@@ -111,18 +125,11 @@ mojom::ChannelPtr ChannelsController::SetChannelSubscribed(
     const std::string& locale,
     const std::string& channel_id,
     bool subscribed) {
-  // Note: DictionaryPrefUpdate is nested here so the update happens before
-  // we call |GetChannelLocales|.
-  {
-    DictionaryPrefUpdate update(prefs_, prefs::kBraveNewsChannels);
-    auto* locale_dict = update->GetDict().EnsureDict(locale);
-    if (!subscribed) {
-      locale_dict->Remove(channel_id);
-    } else {
-      locale_dict->Set(channel_id, true);
-    }
-  }
+  // Persist the pref
+  ChannelsController::SetChannelSubscribedPref(prefs_, locale, channel_id,
+                                               subscribed);
 
+  // Provide an updated entity
   auto result = mojom::Channel::New();
   result->channel_name = channel_id;
   result->subscribed_locales = GetChannelLocales(channel_id);

components/brave_today/browser/channels_controller.h

@@ -47,6 +47,11 @@ class ChannelsController {
                             const std::string& channel_id);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(BraveNewsFeedBuildingTest, BuildFeedV2);
+  static void SetChannelSubscribedPref(PrefService* prefs,
+                                       const std::string& locale,
+                                       const std::string& channel_id,
+                                       bool subscribed);
   raw_ptr<PrefService> prefs_;
   raw_ptr<PublishersController> publishers_controller_;
 };

components/brave_today/browser/feed_building.cc

@@ -267,14 +267,16 @@ bool ShouldDisplayFeedItem(const mojom::FeedItemPtr& feed_item,
       // it belongs to are subscribed to.
       for (const auto& locale_info : publisher->locales) {
         for (const auto& channel_id : locale_info->channels) {
-          const auto& channel = channels.at(channel_id);
-          if (base::Contains(channel->subscribed_locales,
-                             locale_info->locale)) {
-            VLOG(2) << "Showing article because publisher "
-                    << data->publisher_id << ": " << publisher->publisher_name
-                    << " is in channel " << locale_info->locale << "."
-                    << channel_id << " which is subscribed to.";
-            return true;
+          if (channels.contains(channel_id)) {
+            const auto& channel = channels.at(channel_id);
+            if (base::Contains(channel->subscribed_locales,
+                               locale_info->locale)) {
+              VLOG(2) << "Showing article because publisher "
+                      << data->publisher_id << ": " << publisher->publisher_name
+                      << " is in channel " << locale_info->locale << "."
+                      << channel_id << " which is subscribed to.";
+              return true;
+            }
           }
         }
       }

components/brave_today/browser/feed_building_unittest.cc

@@ -81,7 +81,7 @@ std::string GetFeedJson() {
           "score": 13.96799592432192
         },
         {
-          "category": "Technology",
+          "category": "Top News",
           "publish_time": "2021-09-01 07:01:28",
           "url": "https://www.digitaltrends.com/computing/logi-bolt-secure-wireless-connectivity/",
           "title": "Expecting Second Logitech built Bolt to make wireless mice and keyboards work better",
@@ -92,7 +92,7 @@ std::string GetFeedJson() {
           "creative_instance_id": "",
           "url_hash": "523b9f2091474c2a082c06ec17965f8c2392f871917407228bbeb51d8a55d6be",
           "padded_img": "https://pcdn.brave.com/brave-today/cache/052e832456e00a3cee51c68eee206fe71c32cba35d5e53dee2777dd132e01364.jpg.pad",
-          "score": 13.91160989810695
+          "score": 13.97160989810695
         }
       ]
     )";
@@ -114,13 +114,13 @@ std::vector<mojom::LocaleInfoPtr> CreateLocales(
 void PopulatePublishers(Publishers* publisher_list) {
   auto publisher1 = mojom::Publisher::New(
       "111", mojom::PublisherType::COMBINED_SOURCE, "First Publisher",
-      "Top News", true, CreateLocales({"en_US"}, {"Top News"}),
+      "Top News", true, CreateLocales({"en_US"}, {"Top News", "Top Sources"}),
       GURL("https://www.example.com"), absl::nullopt, absl::nullopt,
       absl::nullopt, GURL("https://first-publisher.com/feed.xml"),
       mojom::UserEnabled::NOT_MODIFIED);
   auto publisher2 = mojom::Publisher::New(
       "222", mojom::PublisherType::COMBINED_SOURCE, "Second Publisher",
-      "Top News", true, CreateLocales({"en_US"}, {"Top News"}),
+      "Top News", true, CreateLocales({"en_US"}, {"Top News", "Top Sources"}),
       GURL("https://www.example.com"), absl::nullopt, absl::nullopt,
       absl::nullopt, GURL("https://second-publisher.com/feed.xml"),
       mojom::UserEnabled::NOT_MODIFIED);
@@ -153,7 +153,11 @@ class BraveNewsFeedBuildingTest : public testing::Test {
   TestingProfile profile_;
 };
 
-TEST_F(BraveNewsFeedBuildingTest, BuildFeed) {
+TEST_F(BraveNewsFeedBuildingTest, BuildFeedV1) {
+  // Use v1 feed strategy
+  base::test::ScopedFeatureList features;
+  features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
+
   Publishers publisher_list;
   PopulatePublishers(&publisher_list);
 
@@ -183,10 +187,50 @@ TEST_F(BraveNewsFeedBuildingTest, BuildFeed) {
             "live-transfer-deadline-day-will-real-madrid-land-psg-star-mbappe");
   ASSERT_EQ(feed.pages[0]->items[1]->items.size(), 1u);
   ASSERT_EQ(feed.pages[0]->items[1]->items[0]->get_article()->data->url,
-            "https://www.digitaltrends.com/computing/"
-            "logi-bolt-secure-wireless-connectivity/");
+            "https://www.example.com/an-article/");
   ASSERT_EQ(feed.pages[0]->items[2]->items.size(), 1u);
   ASSERT_EQ(feed.pages[0]->items[2]->items[0]->get_article()->data->url,
+            "https://www.digitaltrends.com/computing/"
+            "logi-bolt-secure-wireless-connectivity/");
+}
+
+TEST_F(BraveNewsFeedBuildingTest, BuildFeedV2) {
+  // Use v2 feed strategy by subscribing to a channel
+  base::test::ScopedFeatureList features;
+  features.InitAndEnableFeature(brave_today::features::kBraveNewsV2Feature);
+
+  ChannelsController::SetChannelSubscribedPref(profile_.GetPrefs(), "en_US",
+                                               "Top Sources", true);
+
+  Publishers publisher_list;
+  PopulatePublishers(&publisher_list);
+
+  std::unordered_set<std::string> history_hosts = {"www.espn.com"};
+
+  std::vector<mojom::FeedItemPtr> feed_items;
+  ParseFeedItems(GetFeedJson(), &feed_items);
+
+  mojom::Feed feed;
+
+  ASSERT_TRUE(BuildFeed(feed_items, history_hosts, &publisher_list, &feed,
+                        profile_.GetPrefs()));
+  ASSERT_EQ(feed.pages.size(), 1u);
+  // Validate featured article is top news
+  ASSERT_TRUE(feed.featured_item->is_article());
+  ASSERT_EQ(feed.featured_item->get_article()->data->url.spec(),
+            "https://www.digitaltrends.com/computing/"
+            "logi-bolt-secure-wireless-connectivity/");
+  // Validate sorted by score descending
+  ASSERT_GE(feed.pages[0]->items.size(), 2u);
+  // Because we cannot access a flat list, then select the items from each card
+  // (some cards have 1 item, some have 2, etc). If the page_content_order
+  // changes, then also change here which items we access in which order.
+  ASSERT_EQ(feed.pages[0]->items[0]->items.size(), 1u);
+  ASSERT_EQ(feed.pages[0]->items[0]->items[0]->get_article()->data->url,
+            "https://www.espn.com/soccer/blog-transfer-talk/story/4465789/"
+            "live-transfer-deadline-day-will-real-madrid-land-psg-star-mbappe");
+  ASSERT_EQ(feed.pages[0]->items[1]->items.size(), 1u);
+  ASSERT_EQ(feed.pages[0]->items[1]->items[0]->get_article()->data->url,
             "https://www.example.com/an-article/");
 }
 
@@ -221,6 +265,10 @@ TEST_F(BraveNewsFeedBuildingTest, DirectFeedsShouldAlwaysBeDisplayed) {
 }
 
 TEST_F(BraveNewsFeedBuildingTest, RemovesDefaultOffItems) {
+  // Use v1 feed strategy
+  base::test::ScopedFeatureList features;
+  features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
+
   Publishers publisher_list;
   PopulatePublishers(&publisher_list);
   std::unordered_set<std::string> history_hosts = {};

components/brave_today/browser/urls_unittest.cc

@@ -15,12 +15,20 @@
 
 class BraveNewsUrlsTest : public testing::Test {};
 
-TEST_F(BraveNewsUrlsTest, BraveNewsV2IsDisabled) {
+TEST_F(BraveNewsUrlsTest, BraveNewsV2FeatureFlag) {
+#if BUILDFLAG(IS_ANDROID)
   EXPECT_FALSE(
       base::FeatureList::IsEnabled(brave_today::features::kBraveNewsV2Feature));
+#else
+  EXPECT_TRUE(
+      base::FeatureList::IsEnabled(brave_today::features::kBraveNewsV2Feature));
+#endif
 }
 
-TEST_F(BraveNewsUrlsTest, BraveNewsUsesV1ByDefault) {
+TEST_F(BraveNewsUrlsTest, BraveNewsV1UsesLocalUrl) {
+  base::test::ScopedFeatureList features;
+  features.InitAndDisableFeature(brave_today::features::kBraveNewsV2Feature);
+
   {
     brave_l10n::test::ScopedDefaultLocale scoped_default_locale{"en_US"};
     std::string region = brave_today::GetRegionUrlPart();
@@ -44,7 +52,7 @@ TEST_F(BraveNewsUrlsTest, BraveNewsUsesV1ByDefault) {
   }
 }
 
-TEST_F(BraveNewsUrlsTest, BraveNewsV2FlagUsesGlobalFeeds) {
+TEST_F(BraveNewsUrlsTest, BraveNewsUsesGlobalFeedsWithV2) {
   base::test::ScopedFeatureList features;
   features.InitAndEnableFeature(brave_today::features::kBraveNewsV2Feature);
 

components/brave_today/common/features.cc

@@ -12,8 +12,16 @@ namespace features {
 
 const base::Feature kBraveNewsFeature{"BraveNews",
                                       base::FEATURE_ENABLED_BY_DEFAULT};
-const base::Feature kBraveNewsV2Feature{"BraveNewsV2",
-                                        base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::Feature kBraveNewsV2Feature {
+  "BraveNewsV2",
+#if BUILDFLAG(IS_ANDROID)
+      base::FEATURE_DISABLED_BY_DEFAULT
+#else
+      base::FEATURE_ENABLED_BY_DEFAULT
+#endif
+};
+
 const base::Feature kBraveNewsCardPeekFeature{"BraveNewsCardPeek",
                                               base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kBraveNewsSubscribeButtonFeature{