[NTP::PhysicalWeb] Implement suggestion dismissal.

components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc

Index: components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
index 70d4cc97d19062ed4635db8450e245720d0d9ad7..7a7387d91c00b1fb870c32c87182479e347f3811 100644
--- a/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
@@ -17,6 +17,7 @@
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/content_suggestions_provider.h"
 #include "components/ntp_snippets/mock_content_suggestions_provider_observer.h"
+#include "components/ntp_snippets/offline_pages/offline_pages_test_utils.h"
 #include "components/ntp_snippets/status.h"
 #include "components/physical_web/data_source/fake_physical_web_data_source.h"
 #include "components/prefs/testing_pref_service.h"
@@ -25,12 +26,15 @@
 
 using base::DictionaryValue;
 using base::ListValue;
+using ntp_snippets::test::CaptureDismissedSuggestions;
 using physical_web::CreateDummyPhysicalWebPages;
 using physical_web::FakePhysicalWebDataSource;
 using testing::_;
 using testing::AnyNumber;
+using testing::AtMost;
 using testing::ElementsAre;
 using testing::ElementsAreArray;
+using testing::Mock;
 using testing::StrictMock;
 using testing::UnorderedElementsAre;
 
@@ -65,7 +69,10 @@ void CompareFetchMoreResult(
 class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
  public:
   PhysicalWebPageSuggestionsProviderTest()
-      : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) {}
+      : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) {
+    PhysicalWebPageSuggestionsProvider::RegisterProfilePrefs(
+        pref_service_->registry());
+  }
 
   void IgnoreOnCategoryStatusChangedToAvailable() {
     EXPECT_CALL(observer_, OnCategoryStatusChanged(_, provided_category(),
@@ -81,10 +88,16 @@ class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
     EXPECT_CALL(observer_, OnSuggestionInvalidated(_, _)).Times(AnyNumber());
   }
 
+  void IgnoreOnNewSuggestions() {
+    EXPECT_CALL(observer_, OnNewSuggestions(_, provided_category(), _))
+        .Times(AnyNumber());
+  }
+
   PhysicalWebPageSuggestionsProvider* CreateProvider() {
     DCHECK(!provider_);
     provider_ = base::MakeUnique<PhysicalWebPageSuggestionsProvider>(
-        &observer_, &category_factory_, &physical_web_data_source_);
+        &observer_, &category_factory_, &physical_web_data_source_,
+        pref_service_.get());
     return provider_.get();
   }
 
@@ -95,6 +108,12 @@ class PhysicalWebPageSuggestionsProviderTest : public testing::Test {
         KnownCategories::PHYSICAL_WEB_PAGES);
   }
 
+  ContentSuggestion::ID GetDummySuggestionId(int id) {
+    return ContentSuggestion::ID(
+        provided_category(),
+        "https://resolved_url.com/" + base::IntToString(id));
+  }
+
   void FireUrlFound(const std::string& url) {
     physical_web_data_source_.NotifyOnFound(url);
   }
@@ -206,4 +225,132 @@ TEST_F(PhysicalWebPageSuggestionsProviderTest,
   run_loop.Run();
 }
 
+TEST_F(PhysicalWebPageSuggestionsProviderTest, ShouldDismiss) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  physical_web_data_source()->SetMetadata(CreateDummyPhysicalWebPages({1}));
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, provided_category(), _))
+      .Times(AtMost(1));
+  CreateProvider();
+
+  provider()->DismissSuggestion(GetDummySuggestionId(1));
+
+  std::vector<ContentSuggestion> dismissed_suggestions;
+  provider()->GetDismissedSuggestionsForDebugging(
+      provided_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions,
+              ElementsAre(HasUrl("https://resolved_url.com/1")));
+}
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       ShouldInvalidateSuggestionOnUrlLost) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnNewSuggestions();
+  physical_web_data_source()->SetMetadata(CreateDummyPhysicalWebPages({1}));
+  CreateProvider();
+
+  EXPECT_CALL(*observer(), OnSuggestionInvalidated(_, GetDummySuggestionId(1)));
+  FireUrlLost("https://resolved_url.com/1");
+}
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       ShouldNotShowDismissedSuggestions) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  physical_web_data_source()->SetMetadata(CreateDummyPhysicalWebPages({1, 2}));
+  EXPECT_CALL(*observer(), OnNewSuggestions(_, provided_category(), _))
+      .Times(AtMost(1));
+  CreateProvider();
+  Mock::VerifyAndClearExpectations(observer());
+
+  provider()->DismissSuggestion(GetDummySuggestionId(1));
+
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({1, 2, 3}));
+  EXPECT_CALL(
+      *observer(),
+      OnNewSuggestions(_, provided_category(),
+                       ElementsAre(HasUrl("https://resolved_url.com/2"),
+                                   HasUrl("https://resolved_url.com/3"))));
+  FireUrlFound("https://resolved_url.com/3");
+}
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       ShouldPruneDismissedSuggestionsWhenFetching) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  IgnoreOnNewSuggestions();
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({1, 2, 3}));
+  CreateProvider();
+
+  provider()->DismissSuggestion(GetDummySuggestionId(1));
+  provider()->DismissSuggestion(GetDummySuggestionId(2));
+
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({2, 3, 4}));
+  FireUrlFound("https://resolved_url.com/4");
+
+  // |GetDismissedSuggestionsForDebugging| uses the data source to return
+  // suggestions, so the first page needs to be added back to the source, so
+  // that it can be returned.

[jkrcal, 2016/12/07 14:20:01]

nit: I find this comment and code misleading as it digs deep into implementation
of GetDismissedSuggestionsForDebugging. I think it suffices to say that
"Suggestion 1 should be pruned from dismissed suggestions now. To test it, we
add it back to the data source and it should be again returned in the
suggestions."

and code:

EXPECT_CALL(
    *observer(),
    OnNewSuggestions(_, provided_category(),
                     ElementsAre(HasUrl("https://resolved_url.com/1"),
                                 HasUrl("https://resolved_url.com/3"),
                                 HasUrl("https://resolved_url.com/4"))));
FireUrlFound("https://resolved_url.com/4");

(the same in the test below)

WDYT?

[vitaliii, 2016/12/08 07:13:41]

TBH, I am not sure. |OnNewSuggestions| is rather indirect way of getting
dismissed IDs and it seems strange to use it while having
|GetDismissedSuggestionsForDebugging|. Also in the test below |OnNewSuggestions|
is essential (behavior of fetching is tested), while here only dismissed IDs are
of interest.

The comment is to explain why page 1 is added back to the source after it has
been lost. I rewrote the comment.

[jkrcal, 2016/12/08 09:55:42]

Ok, not important enough to argue further ;)
Thanks!

[vitaliii, 2016/12/08 11:05:29]

Acknowledged.

+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({1, 2, 3, 4}));
+  std::vector<ContentSuggestion> dismissed_suggestions;
+  provider()->GetDismissedSuggestionsForDebugging(
+      provided_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions,
+              ElementsAre(HasUrl("https://resolved_url.com/2")));
+}
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       ShouldPruneDismissedSuggestionsWhenUrlLost) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  IgnoreOnNewSuggestions();
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({1, 2, 3}));
+  CreateProvider();
+
+  provider()->DismissSuggestion(GetDummySuggestionId(1));
+  provider()->DismissSuggestion(GetDummySuggestionId(2));
+
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({2, 3, 4}));
+  FireUrlLost("https://resolved_url.com/1");
+
+  // |GetDismissedSuggestionsForDebugging| uses the data source to return
+  // suggestions, so the first page needs to be added back to the source, so
+  // that it can be returned.
+  physical_web_data_source()->SetMetadata(
+      CreateDummyPhysicalWebPages({1, 2, 3, 4}));
+  std::vector<ContentSuggestion> dismissed_suggestions;
+  provider()->GetDismissedSuggestionsForDebugging(
+      provided_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions,
+              ElementsAre(HasUrl("https://resolved_url.com/2")));
+}
+
+TEST_F(PhysicalWebPageSuggestionsProviderTest,
+       ShouldStoreDismissedSuggestions) {
+  IgnoreOnCategoryStatusChangedToAvailable();
+  IgnoreOnSuggestionInvalidated();
+  IgnoreOnNewSuggestions();
+  physical_web_data_source()->SetMetadata(CreateDummyPhysicalWebPages({1, 2}));
+  CreateProvider();
+  provider()->DismissSuggestion(GetDummySuggestionId(1));
+  DestroyProvider();
+
+  CreateProvider();
+  std::vector<ContentSuggestion> dismissed_suggestions;
+  provider()->GetDismissedSuggestionsForDebugging(
+      provided_category(),
+      base::Bind(&CaptureDismissedSuggestions, &dismissed_suggestions));
+  EXPECT_THAT(dismissed_suggestions,
+              ElementsAre(HasUrl("https://resolved_url.com/1")));
+}

[jkrcal, 2016/12/07 14:20:01]

A general comment: could you please use minimal data sets for each given test?

E.g. in ShouldPruneDismissedSuggestionsWhenFetching, pages 3 and 4 are not
needed for the test.

I think more pages then necessary make it harder to understand what exactly are
you testing.

[vitaliii, 2016/12/08 07:13:42]

Done.

3 is needed, since I want to keep data source behavior realistic.

[jkrcal, 2016/12/08 09:55:42]

Acknowledged.

+
 }  // namespace ntp_snippets