From 73048b2cdbf2ff895b3f337e78579f3e27dddd97 Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Tue, 8 Nov 2022 12:30:17 -0800 Subject: [PATCH] Merge pull request #15779 from brave/news-desktop-v2-enable Enable news v2 by default for Desktop --- .../browser/channels_controller.cc | 29 +++++---- .../brave_today/browser/channels_controller.h | 5 ++ .../brave_today/browser/feed_building.cc | 18 +++--- .../browser/feed_building_unittest.cc | 62 ++++++++++++++++--- .../brave_today/browser/urls_unittest.cc | 14 ++++- components/brave_today/common/features.cc | 12 +++- 6 files changed, 109 insertions(+), 31 deletions(-) diff --git a/components/brave_today/browser/channels_controller.cc b/components/brave_today/browser/channels_controller.cc index b0f1f9c818d3..43cada6632ae 100644 --- a/components/brave_today/browser/channels_controller.cc +++ b/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); diff --git a/components/brave_today/browser/channels_controller.h b/components/brave_today/browser/channels_controller.h index ecbe0b1680f1..37962146bfdc 100644 --- a/components/brave_today/browser/channels_controller.h +++ b/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 prefs_; raw_ptr publishers_controller_; }; diff --git a/components/brave_today/browser/feed_building.cc b/components/brave_today/browser/feed_building.cc index 265c9cac8016..7f69ac428a5e 100644 --- a/components/brave_today/browser/feed_building.cc +++ b/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; + } } } } diff --git a/components/brave_today/browser/feed_building_unittest.cc b/components/brave_today/browser/feed_building_unittest.cc index 283f6e672d66..193573de90e4 100644 --- a/components/brave_today/browser/feed_building_unittest.cc +++ b/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 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 history_hosts = {"www.espn.com"}; + + std::vector 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 history_hosts = {}; diff --git a/components/brave_today/browser/urls_unittest.cc b/components/brave_today/browser/urls_unittest.cc index e89b28109ac9..b59dc9164ed6 100644 --- a/components/brave_today/browser/urls_unittest.cc +++ b/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); diff --git a/components/brave_today/common/features.cc b/components/brave_today/common/features.cc index 354637a1415c..eecf46f019bc 100644 --- a/components/brave_today/common/features.cc +++ b/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{