HaTS Next: Update preferences when survey is launched

To support not showing a user a survey too soon, the preferences checked
by the HatsService to determine survey eligibility are updated when
the HaTSNextWebDialog shows a survey.

Bug: 1110888
Change-Id: I54a977a1aebf7b2345abace84c2c66aa3c2cb600
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2326171Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Theodore Olsauskas-Warren <sauski@chromium.org>
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Cr-Commit-Position: refs/heads/master@{#799152}

chrome/browser/ui/hats/hats_service.cc

@@ -30,10 +30,14 @@
 #include "content/public/browser/web_contents.h"
 #include "net/base/network_change_notifier.h"
 
+constexpr char kHatsSurveyTriggerTesting[] = "testing";
 constexpr char kHatsSurveyTriggerSatisfaction[] = "satisfaction";
 constexpr char kHatsSurveyTriggerSettings[] = "settings";
 constexpr char kHatsSurveyTriggerSettingsPrivacy[] = "settings-privacy";
 
+constexpr char kHatsNextSurveyTriggerIDTesting[] =
+    "zishSVViB0kPN8UwQ150VGjBKuBP";
+
 namespace {
 
 const base::Feature* survey_features[] = {
@@ -51,8 +55,6 @@ constexpr char kHatsSurveyEnSiteID[] = "en_site_id";
 constexpr double kHatsSurveyProbabilityDefault = 0;
 
 constexpr char kHatsSurveyEnSiteIDDefault[] = "bhej2dndhpc33okm6xexsbyv4y";
-constexpr char kHatsNextSurveyTriggerIDDefault[] =
-    "zishSVViB0kPN8UwQ150VGjBKuBP";
 
 constexpr base::TimeDelta kMinimumTimeBetweenSurveyStarts =
     base::TimeDelta::FromDays(60);
@@ -152,18 +154,15 @@ HatsService::HatsService(Profile* profile) : profile_(profile) {
                                             kHatsSurveyEnSiteIDDefault)
                 .Get()));
   }
-  // Ensure a default survey exists (for demo purpose).
+  // Ensure a default survey exists (for testing and demo purpose).
   auto* default_survey_id =
       base::FeatureList::IsEnabled(
           features::kHappinessTrackingSurveysForDesktopMigration)
-          ? kHatsNextSurveyTriggerIDDefault
+          ? kHatsNextSurveyTriggerIDTesting
           : kHatsSurveyEnSiteIDDefault;
-  if (survey_configs_by_triggers_.find(kHatsSurveyTriggerSatisfaction) ==
-      survey_configs_by_triggers_.end()) {
-    survey_configs_by_triggers_.emplace(
-        kHatsSurveyTriggerSatisfaction,
-        SurveyConfig(kHatsSurveyProbabilityDefault, default_survey_id));
-  }
+  survey_configs_by_triggers_.emplace(
+      kHatsSurveyTriggerTesting,
+      SurveyConfig(kHatsSurveyProbabilityDefault, default_survey_id));
 }
 
 HatsService::~HatsService() = default;
@@ -211,6 +210,33 @@ bool HatsService::LaunchDelayedSurveyForWebContents(
   return success;
 }
 
+void HatsService::RecordSurveyAsShown(std::string survey_id) {
+  // Record the trigger associated with the survey_id. This is recorded instead
+  // of the survey ID itself, as the ID is specific to individual survey
+  // versions. There should be a cooldown before a user is prompted to take a
+  // survey from the same trigger, regardless of whether the survey was updated.
+  // TODO(crbug.com/1110888): When HaTS V1 is deprecated, improve nomenclature
+  // to remove confusion between trigger, trigger ID, survey ID and site ID.
+  auto trigger_survey_config = std::find_if(
+      survey_configs_by_triggers_.begin(), survey_configs_by_triggers_.end(),
+      [&](const std::pair<std::string, SurveyConfig>& pair) {
+        return pair.second.en_site_id_ == survey_id;
+      });
+
+  DCHECK(trigger_survey_config != survey_configs_by_triggers_.end());
+  std::string trigger = trigger_survey_config->first;
+
+  UMA_HISTOGRAM_ENUMERATION(kHatsShouldShowSurveyReasonHistogram,
+                            ShouldShowSurveyReasons::kYes);
+
+  DictionaryPrefUpdate update(profile_->GetPrefs(), prefs::kHatsSurveyMetadata);
+  base::DictionaryValue* pref_data = update.Get();
+  pref_data->SetIntPath(GetMajorVersionPath(trigger),
+                        version_info::GetVersion().components()[0]);
+  pref_data->SetPath(GetLastSurveyStartedTime(trigger),
+                     util::TimeToValue(base::Time::Now()));
+}
+
 void HatsService::SetSurveyMetadataForTesting(
     const HatsService::SurveyMetadata& metadata) {
   const std::string& trigger = kHatsSurveyTriggerSatisfaction;
@@ -447,18 +473,9 @@ void HatsService::CheckSurveyStatusAndMaybeShow(Browser* browser,
 }
 
 void HatsService::ShowSurvey(Browser* browser, const std::string& trigger) {
-  UMA_HISTOGRAM_ENUMERATION(kHatsShouldShowSurveyReasonHistogram,
-                            ShouldShowSurveyReasons::kYes);
-
-  browser->window()->ShowHatsBubble(
-      survey_configs_by_triggers_[trigger].en_site_id_);
-
-  DictionaryPrefUpdate update(profile_->GetPrefs(), prefs::kHatsSurveyMetadata);
-  base::DictionaryValue* pref_data = update.Get();
-  pref_data->SetIntPath(GetMajorVersionPath(trigger),
-                        version_info::GetVersion().components()[0]);
-  pref_data->SetPath(GetLastSurveyStartedTime(trigger),
-                     util::TimeToValue(base::Time::Now()));
+  auto survey_id = survey_configs_by_triggers_[trigger].en_site_id_;
+  RecordSurveyAsShown(survey_id);
+  browser->window()->ShowHatsBubble(survey_id);
   checker_.reset();
 }
 

chrome/browser/ui/hats/hats_service.h

@@ -27,10 +27,15 @@ class PrefRegistrySimple;
 class Profile;
 
 // Trigger identifiers currently used; duplicates not allowed.
+extern const char kHatsSurveyTriggerTesting[];
 extern const char kHatsSurveyTriggerSatisfaction[];
 extern const char kHatsSurveyTriggerSettings[];
 extern const char kHatsSurveyTriggerSettingsPrivacy[];
 
+// The Trigger ID for a test HaTS Next survey which is available for testing
+// and demo purposes when the migration feature flag is enabled.
+extern const char kHatsNextSurveyTriggerIDTesting[];
+
 // This class provides the client side logic for determining if a
 // survey should be shown for any trigger based on input from a finch
 // configuration. It is created on a per profile basis.
@@ -119,6 +124,12 @@ class HatsService : public KeyedService {
       content::WebContents* web_contents,
       int timeout_ms);
 
+  // Updates the user preferences to record that the survey associated with
+  // |survey_id| was shown to the user. |survey_id| is the unique_id provided
+  // to the HaTS Service to identify a survey. This is the trigger ID for HaTS
+  // Next, and the site ID for HaTS v1.
+  void RecordSurveyAsShown(std::string survey_id);
+
   void SetSurveyMetadataForTesting(const SurveyMetadata& metadata);
   void GetSurveyMetadataForTesting(HatsService::SurveyMetadata* metadata) const;
   void SetSurveyCheckerForTesting(

chrome/browser/ui/views/hats/hats_browsertest.cc

@@ -8,16 +8,23 @@
 #include "base/files/file_path.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
+#include "base/util/values/values_util.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/hats/hats_service.h"
+#include "chrome/browser/ui/hats/hats_service_factory.h"
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/hats/hats_bubble_view.h"
 #include "chrome/browser/ui/views/hats/hats_next_web_dialog.h"
 #include "chrome/browser/ui/views/hats/hats_web_dialog.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/version_info/version_info.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
@@ -193,18 +200,48 @@ class MockHatsNextWebDialog : public HatsNextWebDialog {
   MOCK_METHOD1(UpdateWidgetSize, void(gfx::Size));
 };
 
-typedef InProcessBrowserTest HatsNextWebDialogBrowserTest;
+class HatsNextWebDialogBrowserTest : public InProcessBrowserTest {
+ public:
+  HatsNextWebDialogBrowserTest() {
+    feature_list_.InitAndEnableFeature(
+        features::kHappinessTrackingSurveysForDesktopMigration);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
 
 // Test that the web dialog correctly receives change to history state that
 // indicates a survey is ready to be shown.
 IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyLoaded) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  // Use the preference path constants defined in hats_service.cc.
+  const std::string kLastSurveyStartedTime =
+      std::string(kHatsSurveyTriggerTesting) + ".last_survey_started_time";
+  const std::string kLastMajorVersion =
+      std::string(kHatsSurveyTriggerTesting) + ".last_major_version";
+
+  // Ensure the HatsService has been created, as the Web Dialog relies on it
+  // to record a survey has been shown.
+  HatsServiceFactory::GetForProfile(browser()->profile(), true);
+
   auto* dialog = new MockHatsNextWebDialog(
-      browser(), "load_for_testing",
+      browser(), kHatsNextSurveyTriggerIDTesting,
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
       base::TimeDelta::FromSeconds(100));
 
+  // Check that no record of a survey being shown is present.
+  const base::DictionaryValue* pref_data =
+      browser()->profile()->GetPrefs()->GetDictionary(
+          prefs::kHatsSurveyMetadata);
+  base::Optional<base::Time> last_survey_started_time =
+      util::ValueToTime(pref_data->FindPath(kLastSurveyStartedTime));
+  base::Optional<int> last_major_version =
+      pref_data->FindIntPath(kLastMajorVersion);
+  ASSERT_FALSE(last_survey_started_time.has_value());
+  ASSERT_FALSE(last_major_version.has_value());
+
   // The hats_next_mock.html will provide a state update to the dialog to
   // indicate that the survey has been loaded.
   base::RunLoop run_loop;
@@ -214,6 +251,17 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyLoaded) {
         run_loop.Quit();
       }));
   run_loop.Run();
+
+  // Check that a record of the survey being shown has been recorded.
+  pref_data = browser()->profile()->GetPrefs()->GetDictionary(
+      prefs::kHatsSurveyMetadata);
+  last_survey_started_time =
+      util::ValueToTime(pref_data->FindPath(kLastSurveyStartedTime));
+  last_major_version = pref_data->FindIntPath(kLastMajorVersion);
+  ASSERT_TRUE(last_survey_started_time.has_value());
+  ASSERT_TRUE(last_major_version.has_value());
+  ASSERT_EQ(static_cast<uint32_t>(*last_major_version),
+            version_info::GetVersion().components()[0]);
 }
 
 // Test that the web dialog correctly receives change to history state that
@@ -277,10 +325,10 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, NewWebContents) {
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
       base::TimeDelta::FromSeconds(100));
 
-  // The mock hats dialog will push a loaded state after it has attempted to
+  // The mock hats dialog will push a close state after it has attempted to
   // open another web contents.
   base::RunLoop run_loop;
-  EXPECT_CALL(*dialog, ShowWidget).WillOnce(testing::Invoke([&run_loop]() {
+  EXPECT_CALL(*dialog, CloseWidget).WillOnce(testing::Invoke([&run_loop]() {
     run_loop.Quit();
   }));
   run_loop.Run();

chrome/browser/ui/views/hats/hats_next_web_dialog.cc

@@ -6,8 +6,11 @@
 
 #include "chrome/browser/ui/browser_dialogs.h"
 
+#include "base/util/values/values_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_destroyer.h"
+#include "chrome/browser/ui/hats/hats_service.h"
+#include "chrome/browser/ui/hats/hats_service_factory.h"
 #include "chrome/browser/ui/views/frame/app_menu_button.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
@@ -15,7 +18,9 @@
 #include "chrome/browser/ui/webui/chrome_web_contents_handler.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_isolated_world_ids.h"
+#include "chrome/common/pref_names.h"
 #include "components/constrained_window/constrained_window_views.h"
+#include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -225,6 +230,11 @@ void HatsNextWebDialog::OnSurveyStateUpdateReceived(std::string state) {
   loading_timer_.AbandonAndStop();
 
   if (state == "loaded") {
+    // Record that the survey was shown, and display the widget.
+    auto* service =
+        HatsServiceFactory::GetForProfile(browser_->profile(), false);
+    DCHECK(service);
+    service->RecordSurveyAsShown(trigger_id_);
     ShowWidget();
   } else if (state == "close") {
     CloseWidget();

chrome/browser/ui/views/hats/hats_next_web_dialog.h

@@ -102,7 +102,7 @@ class HatsNextWebDialog : public ui::WebDialogDelegate,
   Browser* browser_;
 
   // The HaTS Next survey trigger ID that is provided to the HaTS webpage.
-  const std::string& trigger_id_;
+  const std::string trigger_id_;
 
   // The size of the dialog. Desired dimensions are provided by the site loaded
   // in the web contents. Initialised to arbitrary non-zero value as creation

chrome/test/data/hats/hats_next_mock.html

@@ -4,7 +4,9 @@
     <script>
       const params = new URLSearchParams(window.location.search);
 
-      if (params.get('trigger_id') == "load_for_testing") {
+      // Watch for the testing trigger_id for HaTS Next, defined on the browser
+      // side in hats_service.cc.
+      if (params.get('trigger_id') == 'zishSVViB0kPN8UwQ150VGjBKuBP') {
         history.pushState('', '', '#loaded');
       }
 
@@ -18,7 +20,7 @@
 
       if (params.get('trigger_id') == "open_new_web_contents_for_testing"){
         window.open('http://foo.com', '_blank');
-        history.pushState('', '', '#loaded');
+        history.pushState('', '', '#close');
       }
 
       if (params.get('trigger_id') == "resize_for_testing") {