diff --git a/components/brave_new_tab_ui/components/default/braveToday/customize/Context.tsx b/components/brave_new_tab_ui/components/default/braveToday/customize/Context.tsx index d6c77c535709..7fe573994652 100644 --- a/components/brave_new_tab_ui/components/default/braveToday/customize/Context.tsx +++ b/components/brave_new_tab_ui/components/default/braveToday/customize/Context.tsx @@ -26,6 +26,7 @@ interface BraveNewsContext { subscribedPublisherIds: string[] // Publishers to suggest to the user. suggestedPublisherIds: string[] + updateSuggestedPublisherIds: () => void } export const BraveNewsContext = React.createContext({ @@ -36,7 +37,8 @@ export const BraveNewsContext = React.createContext({ filteredPublisherIds: [], subscribedPublisherIds: [], channels: {}, - suggestedPublisherIds: [] + suggestedPublisherIds: [], + updateSuggestedPublisherIds: () => {} }) export function BraveNewsContextProvider (props: { children: React.ReactNode }) { @@ -53,7 +55,7 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode }) return () => api.removeChannelsListener(handler) }, []) - const updateSuggestedPublishers = useCallback(async () => { + const updateSuggestedPublisherIds = useCallback(async () => { setSuggestedPublisherIds([]) const { suggestedPublisherIds } = await api.controller.getSuggestedPublisherIds() setSuggestedPublisherIds(suggestedPublisherIds) @@ -83,10 +85,6 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode }) sortedPublishers.filter(isPublisherEnabled).map(p => p.publisherId), [sortedPublishers]) - React.useEffect(() => { - updateSuggestedPublishers() - }, []) - const context = useMemo(() => ({ customizePage, setCustomizePage, @@ -95,8 +93,9 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode }) suggestedPublisherIds, sortedPublishers, filteredPublisherIds, - subscribedPublisherIds - }), [customizePage, channels, publishers]) + subscribedPublisherIds, + updateSuggestedPublisherIds + }), [customizePage, channels, publishers, suggestedPublisherIds, updateSuggestedPublisherIds]) return {props.children} diff --git a/components/brave_new_tab_ui/components/default/braveToday/customize/Discover.tsx b/components/brave_new_tab_ui/components/default/braveToday/customize/Discover.tsx index 1e0f236a1914..de0966517c57 100644 --- a/components/brave_new_tab_ui/components/default/braveToday/customize/Discover.tsx +++ b/components/brave_new_tab_ui/components/default/braveToday/customize/Discover.tsx @@ -56,7 +56,7 @@ export default function Discover () { function Home () { const [showingAllCategories, setShowingAllCategories] = React.useState(false) const channels = useChannels() - const { filteredPublisherIds } = useBraveNews() + const { filteredPublisherIds, updateSuggestedPublisherIds } = useBraveNews() const visibleChannelIds = React.useMemo(() => channels // If we're showing all channels, there's no end to the slice. @@ -67,6 +67,9 @@ function Home () { .map(c => c.channelName), [channels, showingAllCategories]) + // When we mount this component, update the suggested publisher ids. + React.useEffect(() => { updateSuggestedPublisherIds() }, []) + return ( <> diff --git a/components/brave_today/browser/brave_news_controller.cc b/components/brave_today/browser/brave_news_controller.cc index 1438bfa0939e..4f46d2e8a35a 100644 --- a/components/brave_today/browser/brave_news_controller.cc +++ b/components/brave_today/browser/brave_news_controller.cc @@ -101,7 +101,6 @@ BraveNewsController::BraveNewsController( &api_request_helper_, prefs_), suggestions_controller_(prefs_, - &channels_controller_, &publishers_controller_, &api_request_helper_, history_service), diff --git a/components/brave_today/browser/suggestions_controller.cc b/components/brave_today/browser/suggestions_controller.cc index 3dfe8ee89475..77c93b1a546a 100644 --- a/components/brave_today/browser/suggestions_controller.cc +++ b/components/brave_today/browser/suggestions_controller.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include "base/barrier_callback.h" #include "base/bind.h" @@ -22,7 +23,6 @@ #include "base/strings/string_util.h" #include "base/values.h" #include "brave/components/api_request_helper/api_request_helper.h" -#include "brave/components/brave_today/browser/channels_controller.h" #include "brave/components/brave_today/browser/locales_helper.h" #include "brave/components/brave_today/browser/publishers_controller.h" #include "brave/components/brave_today/browser/urls.h" @@ -122,9 +122,15 @@ SuggestionsController::PublisherSimilarities ParseSimilarityResponse( const auto& similarity_list = it.second.GetList(); for (const auto& similarity : similarity_list) { const auto& dict = similarity.GetDict(); + auto* source = dict.FindString("source"); + if (!source) { + VLOG(1) << "Found similarity with no publisher id: " + << dict.DebugString(); + continue; + } + auto score = dict.FindDouble("score").value_or(0); similarities[for_publisher].push_back( - {.publisher_id = *dict.FindString("source"), - .score = dict.FindDouble("score").value()}); + {.publisher_id = *source, .score = score}); } } @@ -134,12 +140,10 @@ SuggestionsController::PublisherSimilarities ParseSimilarityResponse( } // namespace SuggestionsController::SuggestionsController( PrefService* prefs, - ChannelsController* channels_controller, PublishersController* publishers_controller, api_request_helper::APIRequestHelper* api_request_helper, history::HistoryService* history_service) : prefs_(prefs), - channels_controller_(channels_controller), publishers_controller_(publishers_controller), api_request_helper_(api_request_helper), history_service_(history_service), @@ -160,10 +164,18 @@ void SuggestionsController::GetSuggestedPublisherIds( options.SetRecentDayRange(14); controller->history_service_->QueryHistory( std::u16string(), options, - base::BindOnce(&SuggestionsController:: - GetSuggestedPublisherIdsWithHistory, - base::Unretained(controller), - std::move(publishers), std::move(callback)), + base::BindOnce( + [](SuggestionsController* controller, + Publishers publishers, + GetSuggestedPublisherIdsCallback callback, + history::QueryResults results) { + auto result = + controller->GetSuggestedPublisherIdsWithHistory( + publishers, results); + std::move(callback).Run(std::move(result)); + }, + base::Unretained(controller), std::move(publishers), + std::move(callback)), &controller->task_tracker_); }, base::Unretained(controller), std::move(callback))); @@ -171,10 +183,10 @@ void SuggestionsController::GetSuggestedPublisherIds( base::Unretained(this), std::move(callback))); } -void SuggestionsController::GetSuggestedPublisherIdsWithHistory( - Publishers publishers, - GetSuggestedPublisherIdsCallback callback, - history::QueryResults history) { +std::vector +SuggestionsController::GetSuggestedPublisherIdsWithHistory( + const Publishers& publishers, + const history::QueryResults& history) { const auto visit_weightings = GetVisitWeightings(publishers, history); base::flat_map scores; @@ -192,15 +204,15 @@ void SuggestionsController::GetSuggestedPublisherIdsWithHistory( const bool explicitly_enabled = publisher->user_enabled_status == mojom::UserEnabled::ENABLED; const auto visited_score = GetVisitWeighting(publisher, visit_weightings); - const bool visited = visited_score != 0; - if (!explicitly_enabled) { + if (!explicitly_enabled && + publisher->user_enabled_status != mojom::UserEnabled::DISABLED) { scores[publisher_id] += visited_score; } // Only consider similar sources if we have visited this one or it has been // explicitly enabled. - if (!visited && !explicitly_enabled) { + if (visited_score == 0 && !explicitly_enabled) { continue; } @@ -219,17 +231,17 @@ void SuggestionsController::GetSuggestedPublisherIdsWithHistory( continue; } - // TODO(fallaciousreasoning): Should |visit_score| be multiplied by the - // visit weight for the original source? - auto visit_score = visited - ? ProjectToRange(info.score, kSimilarVisitedMin, - kSimilarVisitedMax) - : 0; - auto subscribed_score = + // Weight this visited score, based on the visited score of the source + // this one is similar to. + auto similar_visited_score = + visited_score * + ProjectToRange(info.score, kSimilarVisitedMin, kSimilarVisitedMax); + auto similar_subscribed_score = explicitly_enabled ? ProjectToRange(info.score, kSimilarSubscribedMin, kSimilarSubscribedMax) : 0; - scores[info.publisher_id] += visit_score + subscribed_score; + scores[info.publisher_id] += + similar_visited_score + similar_subscribed_score; } } @@ -247,7 +259,7 @@ void SuggestionsController::GetSuggestedPublisherIdsWithHistory( return scores.at(a_id) > scores.at(b_id); }); - std::move(callback).Run(std::move(result)); + return result; } void SuggestionsController::EnsureSimilarityMatrixIsUpdating() { diff --git a/components/brave_today/browser/suggestions_controller.h b/components/brave_today/browser/suggestions_controller.h index 15ea331f8f6d..ac41ff14cf37 100644 --- a/components/brave_today/browser/suggestions_controller.h +++ b/components/brave_today/browser/suggestions_controller.h @@ -17,7 +17,6 @@ #include "base/one_shot_event.h" #include "base/task/cancelable_task_tracker.h" #include "brave/components/api_request_helper/api_request_helper.h" -#include "brave/components/brave_today/browser/channels_controller.h" #include "brave/components/brave_today/browser/publishers_controller.h" #include "components/history/core/browser/history_service.h" #include "components/history/core/browser/history_types.h" @@ -37,7 +36,6 @@ class SuggestionsController { explicit SuggestionsController( PrefService* prefs, - ChannelsController* channels_controller, PublishersController* publishers_controller, api_request_helper::APIRequestHelper* api_request_helper, history::HistoryService* history_service); @@ -49,18 +47,17 @@ class SuggestionsController { void EnsureSimilarityMatrixIsUpdating(); private: + friend class SuggestionsControllerTest; void GetOrFetchSimilarityMatrix(base::OnceClosure callback); - void GetSuggestedPublisherIdsWithHistory( - Publishers publishers, - GetSuggestedPublisherIdsCallback callback, - history::QueryResults history); + std::vector GetSuggestedPublisherIdsWithHistory( + const Publishers& publishers, + const history::QueryResults& history); bool is_update_in_progress_ = false; raw_ptr prefs_; // Task tracker for HistoryService callbacks. base::CancelableTaskTracker task_tracker_; - raw_ptr channels_controller_; raw_ptr publishers_controller_; raw_ptr api_request_helper_; raw_ptr history_service_; diff --git a/components/brave_today/browser/suggestions_controller_unittest.cc b/components/brave_today/browser/suggestions_controller_unittest.cc new file mode 100644 index 000000000000..529f02582cc1 --- /dev/null +++ b/components/brave_today/browser/suggestions_controller_unittest.cc @@ -0,0 +1,278 @@ +// Copyright (c) 2022 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// you can obtain one at http://mozilla.org/MPL/2.0/. + +#include "brave/components/brave_today/browser/suggestions_controller.h" + +#include +#include + +#include "base/bind.h" +#include "base/containers/contains.h" +#include "base/run_loop.h" +#include "base/test/bind.h" +#include "brave/components/api_request_helper/api_request_helper.h" +#include "brave/components/brave_today/browser/direct_feed_controller.h" +#include "brave/components/brave_today/browser/publishers_controller.h" +#include "brave/components/brave_today/browser/unsupported_publisher_migrator.h" +#include "brave/components/brave_today/common/brave_news.mojom-shared.h" +#include "brave/components/brave_today/common/brave_news.mojom.h" +#include "chrome/test/base/testing_profile.h" +#include "components/history/core/browser/history_types.h" +#include "components/history/core/browser/url_row.h" +#include "content/public/test/browser_task_environment.h" +#include "net/traffic_annotation/network_traffic_annotation_test_helper.h" +#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h" +#include "services/network/public/cpp/shared_url_loader_factory.h" +#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h" +#include "services/network/test/test_url_loader_factory.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace brave_news { +namespace { +Publishers MakePublishers(const std::vector& publisher_urls) { + Publishers result; + size_t next_id = 1; + for (const auto& url : publisher_urls) { + auto publisher = mojom::Publisher::New(); + publisher->locales.push_back( + mojom::LocaleInfo::New("en_US", 0, std::vector{})); + publisher->site_url = GURL(url); + publisher->user_enabled_status = mojom::UserEnabled::NOT_MODIFIED; + result[std::to_string(next_id++)] = std::move(publisher); + } + return result; +} + +history::QueryResults MakeQueryResults(const std::vector& urls) { + history::QueryResults result; + std::vector url_results; + for (const auto& url : urls) { + url_results.emplace_back(history::URLResult(GURL(url), base::Time::Now())); + } + + result.SetURLResults(std::move(url_results)); + return result; +} +} // namespace + +class SuggestionsControllerTest : public testing::Test { + public: + SuggestionsControllerTest() + : api_request_helper_(TRAFFIC_ANNOTATION_FOR_TESTS, + test_url_loader_factory_.GetSafeWeakWrapper()), + direct_feed_controller_(profile_.GetPrefs(), nullptr), + unsupported_publisher_migrator_(profile_.GetPrefs(), + &direct_feed_controller_, + &api_request_helper_), + publishers_controller_(profile_.GetPrefs(), + &direct_feed_controller_, + &unsupported_publisher_migrator_, + &api_request_helper_), + suggestions_controller_(profile_.GetPrefs(), + &publishers_controller_, + &api_request_helper_, + nullptr) { + SetLocale("en_US"); + } + ~SuggestionsControllerTest() override = default; + + protected: + std::vector GetSuggestedPublisherIds( + const Publishers& publishers, + const history::QueryResults& history) { + return suggestions_controller_.GetSuggestedPublisherIdsWithHistory( + publishers, history); + } + + void SetSimilarityMatrix( + SuggestionsController::PublisherSimilarities similarities) { + suggestions_controller_.similarities_ = std::move(similarities); + } + + void SetLocale(const std::string& locale) { + suggestions_controller_.locale_ = locale; + } + + content::BrowserTaskEnvironment browser_task_environment_; + TestingProfile profile_; + network::TestURLLoaderFactory test_url_loader_factory_; + api_request_helper::APIRequestHelper api_request_helper_; + + DirectFeedController direct_feed_controller_; + UnsupportedPublisherMigrator unsupported_publisher_migrator_; + PublishersController publishers_controller_; + SuggestionsController suggestions_controller_; +}; + +TEST_F(SuggestionsControllerTest, VisitedSourcesAreSuggested) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + }); + const auto& history = MakeQueryResults( + {"https://example.com", "https://foo.com", "https://example.com"}); + auto suggestions = GetSuggestedPublisherIds(publishers, history); + + // Publisher 1 & publisher 3 have been visited. However, P1 was visited more + // times, so we should suggest it first. + EXPECT_EQ(2u, suggestions.size()); + EXPECT_EQ("1", suggestions[0]); + EXPECT_EQ("3", suggestions[1]); +} + +TEST_F(SuggestionsControllerTest, SubscribedVisitedSourcesAreNotSuggested) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + }); + publishers.at("1")->user_enabled_status = mojom::UserEnabled::ENABLED; + const auto& history = MakeQueryResults( + {"https://example.com", "https://foo.com", "https://example.com"}); + auto suggestions = GetSuggestedPublisherIds(publishers, history); + + // Publisher 1 is subscribed, so we shouldn't suggest it. However, we've + // visited publisher 3, so we should suggest that. + EXPECT_EQ(1u, suggestions.size()); + EXPECT_EQ("3", suggestions[0]); +} + +TEST_F(SuggestionsControllerTest, DisabledVisitedSourcesAreNotSuggested) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + }); + publishers.at("1")->user_enabled_status = mojom::UserEnabled::DISABLED; + const auto& history = MakeQueryResults( + {"https://example.com", "https://foo.com", "https://example.com"}); + auto suggestions = GetSuggestedPublisherIds(publishers, history); + + // P1 was disabled, so we shouldn't suggest it. P3 was visited so it should be + // suggested. + EXPECT_EQ(1u, suggestions.size()); + EXPECT_EQ("3", suggestions[0]); +} + +TEST_F(SuggestionsControllerTest, SimilarSourcesAreSuggested) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + "https://frob.com", + }); + + publishers.at("1")->user_enabled_status = mojom::UserEnabled::ENABLED; + SetSimilarityMatrix({{"1", + {{.publisher_id = "2", .score = 0.8}, + {.publisher_id = "4", .score = 0.9}}}}); + + auto suggestions = GetSuggestedPublisherIds(publishers, {}); + + // P1 is enabled so we should suggest sources similar to it. P4 is more + // similar to it than P2, so we should suggest it first. + EXPECT_EQ(2u, suggestions.size()); + EXPECT_EQ("4", suggestions[0]); + EXPECT_EQ("2", suggestions[1]); +} + +TEST_F(SuggestionsControllerTest, SimilarToVisitedSourcesAreSuggested) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + "https://frob.com", + }); + + auto history = MakeQueryResults({"https://example.com"}); + SetSimilarityMatrix({{"1", + {{.publisher_id = "2", .score = 0.8}, + {.publisher_id = "4", .score = 0.9}}}}); + + auto suggestions = GetSuggestedPublisherIds(publishers, history); + + // P1 has been visited and is not subscribed, so we should suggest it first. + // P4 and P2 are similar to P1 so they should be suggested too (P4 is more + // similar to P1, so suggest it first.) + EXPECT_EQ(3u, suggestions.size()); + EXPECT_EQ("1", suggestions[0]); + EXPECT_EQ("4", suggestions[1]); + EXPECT_EQ("2", suggestions[2]); +} + +TEST_F(SuggestionsControllerTest, VisitWeightingAltersSimilarToVisitWeighting) { + const auto& publishers = MakePublishers({ + "https://example.com", + "https://bar.com", + "https://foo.com", + "https://frob.com", + }); + + auto history = MakeQueryResults({"https://example.com", "https://example.com", + "https://example.com", "https://bar.com"}); + SetSimilarityMatrix({{"1", {{.publisher_id = "3", .score = 0.3}}}, + {"2", {{.publisher_id = "4", .score = 0.4}}}}); + + auto suggestions = GetSuggestedPublisherIds(publishers, history); + + // P1 has been visited many times, so sources similar to it should be ranked + // higher than sources similar to P2. + EXPECT_EQ(4u, suggestions.size()); + EXPECT_EQ("1", suggestions[0]); // Visited many times + EXPECT_EQ("2", suggestions[1]); // Visited, but just once + EXPECT_EQ("3", + suggestions[2]); // Similar to P1 (which was visited many times). + EXPECT_EQ("4", suggestions[3]); // Similar to P2 (which was visited once) +} + +TEST_F(SuggestionsControllerTest, + SuggestionsCanComeFromVistSimilarOrSimilarToVisit) { + const auto& publishers = MakePublishers({ + "https://visited.com", + "https://similar-to-visited.com", + "https://subscribed.com", + "https://similar-to-subscribed.com", + "https://unrelated.com", + }); + + publishers.at("3")->user_enabled_status = mojom::UserEnabled::ENABLED; + + auto history = MakeQueryResults({"https://visited.com"}); + SetSimilarityMatrix({{"1", {{.publisher_id = "2", .score = 0.8}}}, + {"3", {{.publisher_id = "4", .score = 0.8}}}}); + + auto suggestions = GetSuggestedPublisherIds(publishers, history); + EXPECT_EQ(3u, suggestions.size()); + // Note: Don't care about order here - we're going to be tweaking the weights + // and we don't want the test to fail all the time. + EXPECT_TRUE(base::Contains(suggestions, "1")); // Visited + EXPECT_TRUE( + base::Contains(suggestions, "2")); // Similar to P3 (which is subscribed) + EXPECT_TRUE( + base::Contains(suggestions, "4")); // Similar to P1 (which was visited) +} + +TEST_F(SuggestionsControllerTest, SourcesFromDifferentLocalesAreNotSuggested) { + const auto& publishers = MakePublishers({ + "https://visited.com", + "https://similar-to-visited.com", + "https://subscribed.com", + "https://similar-to-subscribed.com", + "https://unrelated.com", + }); + + publishers.at("3")->user_enabled_status = mojom::UserEnabled::ENABLED; + + auto history = MakeQueryResults({"https://visited.com"}); + SetSimilarityMatrix({{"1", {{.publisher_id = "2", .score = 0.8}}}, + {"3", {{.publisher_id = "4", .score = 0.8}}}}); + + SetLocale("en_NZ"); + auto suggestions = GetSuggestedPublisherIds(publishers, history); + EXPECT_EQ(0u, suggestions.size()); +} + +} // namespace brave_news diff --git a/components/brave_today/browser/test/BUILD.gn b/components/brave_today/browser/test/BUILD.gn index a7d616058c14..19caac4569ae 100644 --- a/components/brave_today/browser/test/BUILD.gn +++ b/components/brave_today/browser/test/BUILD.gn @@ -17,6 +17,7 @@ source_set("brave_news_unit_tests") { "//brave/components/brave_today/browser/locales_helper_unittest.cc", "//brave/components/brave_today/browser/publishers_controller_unittest.cc", "//brave/components/brave_today/browser/publishers_parsing_unittest.cc", + "//brave/components/brave_today/browser/suggestions_controller_unittest.cc", "//brave/components/brave_today/browser/unsupported_publisher_migrator_unittest.cc", "//brave/components/brave_today/browser/urls_unittest.cc", ]