[Remote suggestions] Do not fetch before EULA accepted

components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 
 #include <memory>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/status.h"
 #include "components/ntp_snippets/user_classifier.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/variations/variations_params_manager.h"
+#include "components/web_resource/web_resource_pref_names.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::ElementsAre;
 using testing::Eq;
 using testing::Field;
 using testing::InSequence;
 using testing::Invoke;
 using testing::IsEmpty;
 using testing::Mock;
@@ skipping 80 matching lines @@
                                    "browser_foregrounded,browser_cold_start"}},
         params_manager_(ntp_snippets::kStudyName,
                         default_variation_params_,
                         {kArticleSuggestionsFeature.name}),
         underlying_provider_(nullptr),
         scheduling_provider_(nullptr),
         user_classifier_(/*pref_service=*/nullptr) {
     SchedulingRemoteSuggestionsProvider::RegisterProfilePrefs(
         utils_.pref_service()->registry());
     RequestThrottler::RegisterProfilePrefs(utils_.pref_service()->registry());
+    // TODO(jkrcal) Create a static function in EulaAcceptedNotifier that
+    // registers this pref and replace the call in browser_process_impl.cc & in
+    // eula_accepted_notifier_unittest.cc with the new static function.
+    local_state_.registry()->RegisterBooleanPref(::prefs::kEulaAccepted, false);
     ResetProvider();
   }
 
   void ResetProvider() {
     auto underlying_provider =
         base::MakeUnique<StrictMock<MockRemoteSuggestionsProvider>>(
             /*observer=*/nullptr);
     underlying_provider_ = underlying_provider.get();
 
     auto test_clock = base::MakeUnique<base::SimpleTestClock>();
     test_clock_ = test_clock.get();
     test_clock_->SetNow(base::Time::Now());
 
     scheduling_provider_ =
         base::MakeUnique<SchedulingRemoteSuggestionsProvider>(
             /*observer=*/nullptr, std::move(underlying_provider),
             &persistent_scheduler_, &user_classifier_, utils_.pref_service(),
-            std::move(test_clock));
+            &local_state_, std::move(test_clock));
   }
 
   void SetVariationParameter(const std::string& param_name,
                              const std::string& param_value) {
     std::map<std::string, std::string> params = default_variation_params_;
     params[param_name] = param_value;
 
     params_manager_.ClearAllVariationParams();
     params_manager_.SetVariationParamsWithFeatureAssociations(
         ntp_snippets::kStudyName, params,
         {ntp_snippets::kArticleSuggestionsFeature.name});
   }
 
+  bool IsEulaNotifierAvailable() {
+    // Just check that the constructer does not return null unique_ptr. The
+    // notifier will be again desctructed right away.
+    return web_resource::EulaAcceptedNotifier::Create(&local_state_);

[tschumann, 2017/03/22 17:56:41]

let's make this more specific and add != nullptr
you can then reduce the comment to:
// Create() returns a unique_ptr, so this is no leak. 

[jkrcal, 2017/03/23 09:29:42]

Done.

+  }
+
+  void SetEulaAcceptedPref() {
+    local_state_.SetBoolean(::prefs::kEulaAccepted, true);
+  }
+
   // GMock cannot deal with move-only types. We need to pass the vector to the
   // mock function as const ref using this wrapper callback.
   void FetchDoneWrapper(
       MockFunction<void(Status status_code,
                         const std::vector<ContentSuggestion>& suggestions)>*
           fetch_done,
       Status status_code,
       std::vector<ContentSuggestion> suggestions) {
     fetch_done->Call(status_code, suggestions);
   }
 
  protected:
   std::map<std::string, std::string> default_variation_params_;
   variations::testing::VariationParamsManager params_manager_;
   StrictMock<MockPersistentScheduler> persistent_scheduler_;
   StrictMock<MockRemoteSuggestionsProvider>* underlying_provider_;
   std::unique_ptr<SchedulingRemoteSuggestionsProvider> scheduling_provider_;
   base::SimpleTestClock* test_clock_;
 
   void ActivateUnderlyingProvider() {
+    SetEulaAcceptedPref();
     scheduling_provider_->OnProviderActivated();
   }
 
+  void ActivateUnderlyingProviderWithoutEula() {

[tschumann, 2017/03/22 17:56:41]

I'd get rid of this function as it's misleading. This function has no control
over whether Eula was accepted or not.

[jkrcal, 2017/03/23 09:29:43]

Done.

+    scheduling_provider_->OnProviderActivated();
+  }
+
   void InactivateUnderlyingProvider() {
     scheduling_provider_->OnProviderDeactivated();
   }
 
  private:
   test::RemoteSuggestionsTestUtils utils_;
   UserClassifier user_classifier_;
+  TestingPrefServiceSimple local_state_;
 
   DISALLOW_COPY_AND_ASSIGN(SchedulingRemoteSuggestionsProviderTest);
 };
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldIgnoreSignalsWhenNotEnabled) {
   scheduling_provider_->OnPersistentSchedulerWakeUp();
   scheduling_provider_->OnNTPOpened();
   scheduling_provider_->OnBrowserForegrounded();
   scheduling_provider_->OnBrowserColdStart();
 }
 
 TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreEulaStateOnPlatformsWhereNotAvaiable) {
+  // Do not run the test if Eula notifier is available

[tschumann, 2017/03/22 17:56:41]

// Only run this tests on platforms that don't support Eula.
?

[jkrcal, 2017/03/23 09:29:43]

Done.

+  if (IsEulaNotifierAvailable()) {
+    return;
+  }
+
+  // First enable the scheduler.

[tschumann, 2017/03/22 17:56:41]

together with the comment about inlining
ActivateUnderlyingProviderWithoutEula().
// Activating the provider should schedule the background fetch.
EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
scheduling_provider_->OnProviderActivated();
// Verify fetches get triggered.
EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
scheduling_provider_->OnPersistentSchedulerWakeUp();

[jkrcal, 2017/03/23 09:29:42]

Done.

+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // A trigger results in a fetch even though Eula is not accepted (because it
+  // is not needed on this platform).
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldIgnoreSignalsWhenEulaNotAccepted) {
+  // Do not run the test if Eula notifier is not available.

[tschumann, 2017/03/22 17:56:41]

// Only run this tests on platforms supporting Eula.

[jkrcal, 2017/03/23 09:29:42]

Done.

+  if (!IsEulaNotifierAvailable()) {
+    return;
+  }
+  // First enable the scheduler.

[tschumann, 2017/03/22 17:56:41]

// Activating the provider should schedule the background fetch.
?

[jkrcal, 2017/03/23 09:29:43]

Done.

+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // All signals are ignored because of Eula not being accepted.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+  scheduling_provider_->OnNTPOpened();
+  scheduling_provider_->OnBrowserForegrounded();
+  scheduling_provider_->OnBrowserColdStart();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
+       ShouldFetchWhenEulaGetsAccepted) {
+  // Do not run the test if Eula notifier is not available.

[tschumann, 2017/03/22 17:56:41]

Only run this tests on platforms supporting Eula.

[jkrcal, 2017/03/23 09:29:42]

Done.

+  if (!IsEulaNotifierAvailable()) {
+    return;
+  }
+  // First enable the scheduler.
+  EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
+  ActivateUnderlyingProviderWithoutEula();
+
+  // Make one (ignored) call to make sure we are interested in eula state.
+  scheduling_provider_->OnPersistentSchedulerWakeUp();
+
+  // Accepting Eula afterwards results in a background fetch.
+  EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
+  SetEulaAcceptedPref();
+}
+
+TEST_F(SchedulingRemoteSuggestionsProviderTest,
        ShouldIgnoreSignalsWhenDisabledByParam) {
   // First set an empty list of allowed trigger types.
   SetVariationParameter("scheduler_trigger_types", "-");
   ResetProvider();
 
   // Then enable the scheduler.
   EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
   ActivateUnderlyingProvider();
 
   scheduling_provider_->OnPersistentSchedulerWakeUp();
@@ skipping 543 matching lines @@
   for (int x = 0; x < 5; ++x) {
     scheduling_provider_->OnPersistentSchedulerWakeUp();
     signal_fetch_done.Run(Status::Success());
   }
 
   // For the 6th time, it is blocked by the scheduling provider.
   scheduling_provider_->OnPersistentSchedulerWakeUp();
 }
 
 }  // namespace ntp_snippets