[EoS] Site blacklisting part 2

Make sure sites in the blacklist are not returned as part of a catalog get
operation. This builds upon Site blacklisting part 1.

Bug: 893845
Change-Id: I38686753167bbcb8e9f9d75d6c323bda813cab31
Reviewed-on: https://chromium-review.googlesource.com/c/1274058
Commit-Queue: Peter Williamson <petewil@chromium.org>
Reviewed-by: Cathy Li <chili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599325}

chrome/browser/android/explore_sites/get_catalog_task.cc

@@ -18,9 +18,10 @@ FROM categories
 WHERE version_token = ?
 ORDER BY category_id ASC;)";
 
-static const char kSelectSiteSql[] = R"(SELECT site_id, url, title
+static const char kSelectSiteSql[] = R"(SELECT site_id, sites.url, title
 FROM sites
-WHERE category_id = ?)";
+LEFT JOIN site_blacklist ON (sites.url = site_blacklist.url)
+WHERE category_id = ? AND site_blacklist.url IS NULL;)";
 
 const char kDeleteSiteSql[] = R"(DELETE FROM sites
 WHERE category_id NOT IN
@@ -126,6 +127,7 @@ GetCatalogSync(bool update_current, sql::Database* db) {
   if (!category_statement.Succeeded())
     return std::make_pair(GetCatalogStatus::kFailed, nullptr);
 
+  bool found_empty_category = false;
   for (auto& category : *result) {
     sql::Statement site_statement(
         db->GetCachedStatement(SQL_FROM_HERE, kSelectSiteSql));
@@ -139,6 +141,20 @@ GetCatalogSync(bool update_current, sql::Database* db) {
     }
     if (!site_statement.Succeeded())
       return std::make_pair(GetCatalogStatus::kFailed, nullptr);
+
+    if (category.sites.empty())
+      found_empty_category = true;
+  }
+
+  // Remove any categories with no sites.
+  if (found_empty_category) {
+    auto new_result = std::make_unique<GetCatalogTask::CategoryList>();
+    for (auto& category : *result) {
+      // Move all categories with sites into the new result
+      if (category.sites.size() > 0)
+        new_result->emplace_back(std::move(category));
+    }
+    result = std::move(new_result);
   }
 
   return std::make_pair(GetCatalogStatus::kSuccess, std::move(result));

chrome/browser/android/explore_sites/get_catalog_task_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/mock_callback.h"
+#include "chrome/browser/android/explore_sites/blacklist_site_task.h"
 #include "chrome/browser/android/explore_sites/explore_sites_schema.h"
 #include "components/offline_pages/task/task.h"
 #include "components/offline_pages/task/task_test_base.h"
@@ -52,6 +53,24 @@ void ValidateTestingCatalog(GetCatalogTask::CategoryList* catalog) {
   EXPECT_EQ("example_2", site->title);
 }
 
+// Same as above, categories with no sites left after blacklisting are removed.
+void ValidateBlacklistTestingCatalog(GetCatalogTask::CategoryList* catalog) {
+  EXPECT_FALSE(catalog == nullptr);
+
+  EXPECT_EQ(1U, catalog->size());
+  ExploreSitesCategory* cat = &catalog->at(0);
+  EXPECT_EQ(3, cat->category_id);
+  EXPECT_EQ("5678", cat->version_token);
+  EXPECT_EQ(1, cat->category_type);
+  EXPECT_EQ("label_1", cat->label);
+
+  EXPECT_EQ(1U, cat->sites.size());
+  ExploreSitesSite* site = &cat->sites[0];
+  EXPECT_EQ("https://www.example.com/1", site->url.spec());
+  EXPECT_EQ(3, site->category_id);
+  EXPECT_EQ("example_1", site->title);
+}
+
 void ExpectSuccessGetCatalogResult(
     GetCatalogStatus status,
     std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
@@ -59,6 +78,13 @@ void ExpectSuccessGetCatalogResult(
   ValidateTestingCatalog(catalog.get());
 }
 
+void ExpectBlacklistGetCatalogResult(
+    GetCatalogStatus status,
+    std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
+  EXPECT_EQ(GetCatalogStatus::kSuccess, status);
+  ValidateBlacklistTestingCatalog(catalog.get());
+}
+
 void ExpectEmptyGetCatalogResult(
     GetCatalogStatus status,
     std::unique_ptr<GetCatalogTask::CategoryList> catalog) {
@@ -100,6 +126,7 @@ class ExploreSitesGetCatalogTaskTest : public TaskTestBase {
   std::pair<std::string, std::string> GetCurrentAndDownloadingVersion();
   int GetNumberOfCategoriesInDB();
   int GetNumberOfSitesInDB();
+  void BlacklistSite(std::string url);
 
  private:
   std::unique_ptr<ExploreSitesStore> store_;
@@ -199,6 +226,13 @@ int ExploreSitesGetCatalogTaskTest::GetNumberOfSitesInDB() {
   return result;
 }
 
+void ExploreSitesGetCatalogTaskTest::BlacklistSite(std::string url) {
+  BlacklistSiteTask task(store(), url);
+  RunTask(&task);
+  // We don't actively wait for completion, so we rely on the blacklist request
+  // clearing the task queue before the task in the test proper runs.
+}
+
 TEST_F(ExploreSitesGetCatalogTaskTest, StoreFailure) {
   store()->SetInitializationStatusForTest(InitializationStatus::FAILURE);
 
@@ -227,6 +261,16 @@ TEST_F(ExploreSitesGetCatalogTaskTest, SimpleCatalog) {
   EXPECT_EQ(4, GetNumberOfSitesInDB());
 }
 
+// This tests that sites on the blacklist do not show up when we do a get
+// catalog task.
+TEST_F(ExploreSitesGetCatalogTaskTest, BlasklistedSitesDoNotAppear) {
+  BlacklistSite("https://www.example.com/2");
+  PopulateTestingCatalog();
+  GetCatalogTask task(store(), false,
+                      base::BindOnce(&ExpectBlacklistGetCatalogResult));
+  RunTask(&task);
+}
+
 TEST_F(ExploreSitesGetCatalogTaskTest, CatalogWithVersionUpdate) {
   PopulateTestingCatalog();
   // Update the testing catalog so that the older catalog is current and the