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

Enable news v2 by default for Desktop #15779

Merged
merged 1 commit into from
Nov 8, 2022
Merged

Enable news v2 by default for Desktop #15779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions components/brave_today/browser/channels_controller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions components/brave_today/browser/channels_controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
};
Expand Down
18 changes: 10 additions & 8 deletions components/brave_today/browser/feed_building.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@fallaciousreasoning I needed to add this conditional since I was experiencing a crash when I modified the tests to include multiple channels. Somehow that was giving me a CHECK crash to do with. the base::Contains concerning FlatMap. I'm not sure why we haven't been experiencing that crash in actual builds though.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I don't think it should be possible to hit this in production - channels is a set of all the Channels from all the LocaleInfos from all the Publishers. So if we get a channel id from a LocaleInfo it must exist in the set.

Obviously, that assumption isn't necessarily true in tests, as we can make up whatever data/channels we like. Another solution would be to make sure the list of publishers that the controller has include all possible channels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, I'm inclined to leave that check in since, in that function specifically, channels doesn't necessarily have to be the full list of all channels. It could just be the channels which have a subscription. And it's comparing IDs from one function parameter with another. Since it causes a crash might be safer to defend against a scenario where we have a new Publisher with a new channel and for some reason we didn't update the in memory channels list.

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;
}
}
}
}
Expand Down
62 changes: 55 additions & 7 deletions components/brave_today/browser/feed_building_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
}
]
)";
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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/");
}

Expand Down Expand Up @@ -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 = {};
Expand Down
14 changes: 11 additions & 3 deletions components/brave_today/browser/urls_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);

Expand Down
12 changes: 10 additions & 2 deletions components/brave_today/common/features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
fallaciousreasoning marked this conversation as resolved.
Show resolved Hide resolved
#else
base::FEATURE_ENABLED_BY_DEFAULT
#endif
};

const base::Feature kBraveNewsCardPeekFeature{"BraveNewsCardPeek",
base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kBraveNewsSubscribeButtonFeature{
Expand Down