HaTS Next: Record survey check on dialog creation

The "last_survey_check_time" preference value associated with a survey
trigger limits the rate at which contact is attempted to be made with
the HaTS service to once per 24hrs.

Previously the creation of a HaTS Next dialog, which immediately
attempts to contact the HaTS service, did not set this value. This
could result in an individual client making unlimited requests to the
HaTS service.

This CL adjusts logic such that this value is set on creation of a
HaTS Next dialog, and is consulted earlier in attempting to show a
survey. Specifically as part of the CanShowSurvey check, rather than
CheckSurveyStatusAndMaybeShow.

Bug: 1110888
Change-Id: I289245893ed0ff7c49afeb5b560b970df02a5416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640433Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Cr-Commit-Position: refs/heads/master@{#845563}

chrome/browser/ui/hats/hats_service.cc

@@ -500,6 +500,17 @@ bool HatsService::CanShowSurvey(const std::string& trigger) const {
     }
   }
 
+  // If an attempt to check with the HaTS servers whether a survey should be
+  // delivered was made too recently, another survey cannot be shown.
+  base::Optional<base::Time> last_survey_check_time =
+      util::ValueToTime(pref_data->FindPath(GetLastSurveyCheckTime(trigger)));
+  if (last_survey_check_time.has_value()) {
+    base::TimeDelta elapsed_time_since_last_check =
+        base::Time::Now() - *last_survey_check_time;
+    if (elapsed_time_since_last_check < kMinimumTimeBetweenSurveyChecks)
+      return false;
+  }
+
   return true;
 }
 
@@ -535,17 +546,6 @@ void HatsService::CheckSurveyStatusAndMaybeShow(
     return;
   }
 
-  base::Optional<base::Time> last_survey_check_time =
-      util::ValueToTime(pref_data->FindPath(GetLastSurveyCheckTime(trigger)));
-  if (last_survey_check_time.has_value()) {
-    base::TimeDelta elapsed_time_since_last_check =
-        base::Time::Now() - *last_survey_check_time;
-    if (elapsed_time_since_last_check < kMinimumTimeBetweenSurveyChecks) {
-      std::move(failure_callback).Run();
-      return;
-    }
-  }
-
   DCHECK(survey_configs_by_triggers_.find(trigger) !=
          survey_configs_by_triggers_.end());
 
@@ -559,6 +559,13 @@ void HatsService::CheckSurveyStatusAndMaybeShow(
         survey_configs_by_triggers_[trigger].en_site_id_,
         std::move(success_callback), std::move(failure_callback));
     hats_next_dialog_exists_ = true;
+
+    // As soon as the HaTS Next dialog is created it will attempt to contact
+    // the HaTS servers to check for a survey.
+    DictionaryPrefUpdate update(profile_->GetPrefs(),
+                                prefs::kHatsSurveyMetadata);
+    update->SetPath(GetLastSurveyCheckTime(trigger),
+                    util::TimeToValue(base::Time::Now()));
   } else {
     if (!checker_)
       checker_ = std::make_unique<HatsSurveyStatusChecker>(profile_);

chrome/browser/ui/hats/hats_service_browsertest.cc

@@ -583,8 +583,12 @@ class HatsServiceHatsNext : public HatsServiceProbabilityOne {
 // returns false until the service has been informed the dialog was closed.
 IN_PROC_BROWSER_TEST_F(HatsServiceHatsNext, SingleHatsNextDialog) {
   SetMetricsConsent(true);
-  EXPECT_TRUE(GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerTesting));
-  GetHatsService()->LaunchSurvey(kHatsSurveyTriggerTesting);
+  EXPECT_TRUE(
+      GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerSatisfaction));
+  GetHatsService()->LaunchSurvey(kHatsSurveyTriggerSatisfaction);
+
+  // Clear any metadata that would prevent another survey from being displayed.
+  GetHatsService()->SetSurveyMetadataForTesting({});
 
   // At this point a HaTS Next dialog is created and is attempting to contact
   // the wrapper website (which will fail as requests to non-localhost addresses
@@ -592,9 +596,25 @@ IN_PROC_BROWSER_TEST_F(HatsServiceHatsNext, SingleHatsNextDialog) {
   // request, the dialog waits for a timeout posted to the UI thread before
   // closing itself. Since this test is also on the UI thread, these checks,
   // which rely on the dialog still being open, will not race.
-  EXPECT_FALSE(GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerTesting));
+  EXPECT_FALSE(
+      GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerSatisfaction));
 
   // Inform the service directly that the dialog has been closed.
   GetHatsService()->HatsNextDialogClosed();
-  EXPECT_TRUE(GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerTesting));
+  EXPECT_TRUE(
+      GetHatsService()->ShouldShowSurvey(kHatsSurveyTriggerSatisfaction));
+}
+
+// Check that launching a HaTS Next survey records a survey check time
+IN_PROC_BROWSER_TEST_F(HatsServiceHatsNext, SurveyCheckTimeRecorded) {
+  SetMetricsConsent(true);
+
+  // Clear any existing survey metadata.
+  GetHatsService()->SetSurveyMetadataForTesting({});
+
+  GetHatsService()->LaunchSurvey(kHatsSurveyTriggerSatisfaction);
+
+  HatsService::SurveyMetadata metadata;
+  GetHatsService()->GetSurveyMetadataForTesting(&metadata);
+  EXPECT_TRUE(metadata.last_survey_check_time.has_value());
 }