[Brave News] Suggestions Followup for #15447 (#15522)

2022-10-26 23:33:36 -04:00
parent 8c68838899
commit de51e0c2fb
7 changed files with 329 additions and 42 deletions
@@ -26,6 +26,7 @@ interface BraveNewsContext {
  subscribedPublisherIds: string[]
  // Publishers to suggest to the user.
  suggestedPublisherIds: string[]
+  updateSuggestedPublisherIds: () => void
 }

 export const BraveNewsContext = React.createContext<BraveNewsContext>({
@@ -36,7 +37,8 @@ export const BraveNewsContext = React.createContext<BraveNewsContext>({
  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)
@@ -82,10 +84,6 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode })
    sortedPublishers.filter(isPublisherEnabled).map(p => p.publisherId),
    [sortedPublishers])

-  React.useEffect(() => {
-    updateSuggestedPublishers()
-  }, [])
-
  const context = useMemo<BraveNewsContext>(() => ({
    customizePage,
    setCustomizePage,
@@ -94,8 +92,9 @@ export function BraveNewsContextProvider (props: { children: React.ReactNode })
    suggestedPublisherIds,
    sortedPublishers,
    filteredPublisherIds,
-    subscribedPublisherIds
-  }), [customizePage, channels, publishers])
+    subscribedPublisherIds,
+    updateSuggestedPublisherIds
+  }), [customizePage, channels, publishers, suggestedPublisherIds, updateSuggestedPublisherIds])

  return <BraveNewsContext.Provider value={context}>
    {props.children}
@@ -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 (
    <>
      <Suggestions/>
@@ -101,7 +101,6 @@ BraveNewsController::BraveNewsController(
                       &api_request_helper_,
                       prefs_),
      suggestions_controller_(prefs_,
-                              &channels_controller_,
                              &publishers_controller_,
                              &api_request_helper_,
                              history_service),
@@ -9,6 +9,7 @@
 #include <string>
 #include <unordered_set>
 #include <utility>
+#include <vector>

 #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<std::string>
+SuggestionsController::GetSuggestedPublisherIdsWithHistory(
+    const Publishers& publishers,
+    const history::QueryResults& history) {
  const auto visit_weightings = GetVisitWeightings(publishers, history);
  base::flat_map<std::string, double> scores;

@@ -188,15 +200,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;
    }

@@ -215,17 +227,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;
    }
  }

@@ -243,7 +255,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() {
@@ -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<std::string> GetSuggestedPublisherIdsWithHistory(
+      const Publishers& publishers,
+      const history::QueryResults& history);

  bool is_update_in_progress_ = false;
  raw_ptr<PrefService> prefs_;
  // Task tracker for HistoryService callbacks.
  base::CancelableTaskTracker task_tracker_;

-  raw_ptr<ChannelsController> channels_controller_;
  raw_ptr<PublishersController> publishers_controller_;
  raw_ptr<api_request_helper::APIRequestHelper> api_request_helper_;
  raw_ptr<history::HistoryService> history_service_;
@@ -0,0 +1,276 @@
+// 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 <utility>
+#include <vector>
+
+#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 "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<std::string>& publisher_urls) {
+  Publishers result;
+  size_t next_id = 1;
+  for (const auto& url : publisher_urls) {
+    auto publisher = mojom::Publisher::New();
+    publisher->locales = {"en_US"};
+    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<std::string>& urls) {
+  history::QueryResults result;
+  std::vector<history::URLResult> 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<std::string> 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
@@ -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",
  ]