Fix onboarding greeting when no given name is available.

Previously, the UI would show something like: "Good morning ,"
Now, the UI will show something like: "Good morning,"

Bug: b:165663569
Change-Id: Ie8ee966380723ce5bfc3cac3227a071046a7b611
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365293Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#800693}

ash/ash_strings.grd

@@ -2522,16 +2522,28 @@ This file contains the strings for ash.
         Hi, <ph name="USERNAME">$1</ph>
       </message>
       <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING" desc="Message shown to greet the user in Assistant UI during the morning.">
-        Good morning <ph name="GIVEN_NAME">$1<ex>David</ex>,</ph>
+        Good morning <ph name="GIVEN_NAME">$1<ex>David</ex></ph>,
+      </message>
+      <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING_WITHOUT_NAME" desc="Message shown to greet the user in Assistant UI during the morning.">
+        Good morning,
       </message>
       <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON" desc="Message shown to greet the user in Assistant UI during the afternoon.">
-        Good afternoon <ph name="GIVEN_NAME">$1<ex>David</ex>,</ph>
+        Good afternoon <ph name="GIVEN_NAME">$1<ex>David</ex></ph>,
+      </message>
+      <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON_WITHOUT_NAME" desc="Message shown to greet the user in Assistant UI during the afternoon.">
+        Good afternoon,
       </message>
       <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING" desc="Message shown to greet the user in Assistant UI during the evening.">
-        Good evening <ph name="GIVEN_NAME">$1<ex>David</ex>,</ph>
+        Good evening <ph name="GIVEN_NAME">$1<ex>David</ex></ph>,
+      </message>
+      <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING_WITHOUT_NAME" desc="Message shown to greet the user in Assistant UI during the evening.">
+        Good evening,
       </message>
       <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT" desc="Message shown to greet the user in Assistant UI during the night.">
-        Good night <ph name="GIVEN_NAME">$1<ex>David</ex>,</ph>
+        Good night <ph name="GIVEN_NAME">$1<ex>David</ex></ph>,
+      </message>
+      <message name="IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT_WITHOUT_NAME" desc="Message shown to greet the user in Assistant UI during the night.">
+        Good night,
       </message>
       <message name="IDS_ASSISTANT_BETTER_ONBOARDING_INTRO" desc="Message shown to introduce Assistant capabilities to the user.">
         I'm your Google Assistant, here to help you throughout your day!

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON.png.sha1

+83148c665e6b433d76894b2298b0b42a18b2e462
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON_WITHOUT_NAME.png.sha1

+3d8f95a9de3cc17db012de7687c733abf38bd6da
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING.png.sha1

+026de19971de5ee3b84640c7c06d1af1f1d6374e
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING_WITHOUT_NAME.png.sha1

+4c633c21cadb11932eb4f58c534dbdd75b572498
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING.png.sha1

+f69732519b4eabe4d934a135b9e9603b2cca9497
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING_WITHOUT_NAME.png.sha1

+c43df05f8725b86ff27b8aff30f9a72a56afb1b3
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT.png.sha1

+0345c299bd60edfcae483055b2336c3a3dd00c61
\ No newline at end of file

ash/ash_strings_grd/IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT_WITHOUT_NAME.png.sha1

+de972e794ef0ae88f428c8f273df15d6e02382fd
\ No newline at end of file

ash/assistant/test/assistant_ash_test_base.cc

@@ -19,6 +19,7 @@
 #include "ash/public/cpp/assistant/controller/assistant_ui_controller.h"
 #include "ash/public/cpp/test/assistant_test_api.h"
 #include "ash/public/cpp/test/test_image_downloader.h"
+#include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_helper.h"
 #include "base/run_loop.h"
@@ -115,27 +116,10 @@ void AssistantAshTestBase::SetUp() {
   // Make the display big enough to hold the app list.
   UpdateDisplay("1024x768");
 
-  // Enable Assistant in settings.
-  test_api_->SetAssistantEnabled(true);
-
-  // Enable screen context in settings.
-  test_api_->SetScreenContextEnabled(true);
-
-  // Set AssistantAllowedState to ALLOWED.
-  test_api_->GetAssistantState()->NotifyFeatureAllowed(
-      chromeos::assistant::AssistantAllowedState::ALLOWED);
-
-  // Set user consent so the suggestion chips are displayed.
-  SetConsentStatus(ConsentStatus::kActivityControlAccepted);
-
-  // At this point our Assistant service is ready for use.
-  // Indicate this by changing status from NOT_READY to READY.
-  test_api_->GetAssistantState()->NotifyStatusChanged(
-      chromeos::assistant::AssistantStatus::READY);
-
   test_api_->DisableAnimations();
-
   EnableKeyboard();
+
+  SetUpActiveUser();
 }
 
 void AssistantAshTestBase::TearDown() {
@@ -145,6 +129,30 @@ void AssistantAshTestBase::TearDown() {
   AshTestBase::TearDown();
 }
 
+void AssistantAshTestBase::CreateAndSwitchActiveUser(
+    const std::string& display_email,
+    const std::string& given_name) {
+  TestSessionControllerClient* session_controller_client =
+      ash_test_helper()->test_session_controller_client();
+
+  session_controller_client->Reset();
+
+  session_controller_client->AddUserSession(
+      display_email, user_manager::USER_TYPE_REGULAR,
+      /*enable_settings=*/true, /*provide_pref_service=*/true,
+      /*is_new_profile=*/false, given_name);
+
+  session_controller_client->SwitchActiveUser(Shell::Get()
+                                                  ->session_controller()
+                                                  ->GetUserSession(0)
+                                                  ->user_info.account_id);
+
+  session_controller_client->SetSessionState(
+      session_manager::SessionState::ACTIVE);
+
+  SetUpActiveUser();
+}
+
 void AssistantAshTestBase::ShowAssistantUi(AssistantEntryPoint entry_point) {
   if (entry_point == AssistantEntryPoint::kHotword) {
     // If the Assistant is triggered via Hotword, the interaction is triggered
@@ -343,4 +351,24 @@ TestAssistantService* AssistantAshTestBase::assistant_service() {
   return ash_test_helper()->test_assistant_service();
 }
 
+void AssistantAshTestBase::SetUpActiveUser() {
+  // Enable Assistant in settings.
+  test_api_->SetAssistantEnabled(true);
+
+  // Enable screen context in settings.
+  test_api_->SetScreenContextEnabled(true);
+
+  // Set AssistantAllowedState to ALLOWED.
+  test_api_->GetAssistantState()->NotifyFeatureAllowed(
+      chromeos::assistant::AssistantAllowedState::ALLOWED);
+
+  // Set user consent so the suggestion chips are displayed.
+  SetConsentStatus(ConsentStatus::kActivityControlAccepted);
+
+  // At this point our Assistant service is ready for use.
+  // Indicate this by changing status from NOT_READY to READY.
+  test_api_->GetAssistantState()->NotifyStatusChanged(
+      chromeos::assistant::AssistantStatus::READY);
+}
+
 }  // namespace ash

ash/assistant/test/assistant_ash_test_base.h

@@ -50,9 +50,14 @@ class AssistantAshTestBase : public AshTestBase {
   explicit AssistantAshTestBase(base::test::TaskEnvironment::TimeSource time);
   ~AssistantAshTestBase() override;
 
+  // AshTestBase:
   void SetUp() override;
   void TearDown() override;
 
+  // Creates and switches to a new active user.
+  void CreateAndSwitchActiveUser(const std::string& display_email,
+                                 const std::string& given_name);
+
   // Show the Assistant UI. The optional |entry_point| can be used to emulate
   // the different ways of launching the Assistant.
   void ShowAssistantUi(
@@ -199,6 +204,8 @@ class AssistantAshTestBase : public AshTestBase {
   TestAssistantService* assistant_service();
 
  private:
+  void SetUpActiveUser();
+
   std::unique_ptr<AssistantTestApi> test_api_;
   std::unique_ptr<TestAssistantSetup> test_setup_;
   std::unique_ptr<TestAssistantWebViewFactory> test_web_view_factory_;

ash/assistant/ui/main_stage/assistant_onboarding_view.cc

@@ -59,33 +59,50 @@ std::string GetGreetingMessage(AssistantViewDelegate* delegate) {
   base::Time::Exploded now;
   base::Time::Now().LocalExplode(&now);
 
+  const std::string given_name = delegate->GetPrimaryUserGivenName();
+
   if (now.hour < 5) {
-    return l10n_util::GetStringFUTF8(
-        IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT,
-        base::UTF8ToUTF16(delegate->GetPrimaryUserGivenName()));
+    return given_name.empty()
+               ? l10n_util::GetStringUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT_WITHOUT_NAME)
+               : l10n_util::GetStringFUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT,
+                     base::UTF8ToUTF16(given_name));
   }
 
   if (now.hour < 12) {
-    return l10n_util::GetStringFUTF8(
-        IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING,
-        base::UTF8ToUTF16(delegate->GetPrimaryUserGivenName()));
+    return given_name.empty()
+               ? l10n_util::GetStringUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING_WITHOUT_NAME)
+               : l10n_util::GetStringFUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_MORNING,
+                     base::UTF8ToUTF16(given_name));
   }
 
   if (now.hour < 17) {
-    return l10n_util::GetStringFUTF8(
-        IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON,
-        base::UTF8ToUTF16(delegate->GetPrimaryUserGivenName()));
+    return given_name.empty()
+               ? l10n_util::GetStringUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON_WITHOUT_NAME)
+               : l10n_util::GetStringFUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_AFTERNOON,
+                     base::UTF8ToUTF16(given_name));
   }
 
   if (now.hour < 23) {
-    return l10n_util::GetStringFUTF8(
-        IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING,
-        base::UTF8ToUTF16(delegate->GetPrimaryUserGivenName()));
+    return given_name.empty()
+               ? l10n_util::GetStringUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING_WITHOUT_NAME)
+               : l10n_util::GetStringFUTF8(
+                     IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_EVENING,
+                     base::UTF8ToUTF16(given_name));
   }
 
-  return l10n_util::GetStringFUTF8(
-      IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT,
-      base::UTF8ToUTF16(delegate->GetPrimaryUserGivenName()));
+  return given_name.empty()
+             ? l10n_util::GetStringUTF8(
+                   IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT_WITHOUT_NAME)
+             : l10n_util::GetStringFUTF8(
+                   IDS_ASSISTANT_BETTER_ONBOARDING_GREETING_NIGHT,
+                   base::UTF8ToUTF16(given_name));
 }
 
 }  // namespace

ash/assistant/ui/main_stage/assistant_onboarding_view_unittest.cc

@@ -168,90 +168,111 @@ class AssistantOnboardingViewTest : public AssistantAshTestBase {
 // Tests -----------------------------------------------------------------------
 
 TEST_F(AssistantOnboardingViewTest, ShouldHaveExpectedGreeting) {
-  // Cache the expected given name.
-  const std::string given_name = Shell::Get()
-                                     ->session_controller()
-                                     ->GetPrimaryUserSession()
-                                     ->user_info.given_name;
-
-  // Advance clock to midnight tomorrow.
-  AdvanceClock(base::Time::Now().LocalMidnight() +
-               base::TimeDelta::FromHours(24) - base::Time::Now());
-
-  {
-    // Verify 4:59 AM.
-    AdvanceClock(base::TimeDelta::FromHours(4) +
-                 base::TimeDelta::FromMinutes(59));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good night %s,", given_name.c_str())));
-  }
+  struct ExpectedGreeting {
+    std::string for_morning;
+    std::string for_afternoon;
+    std::string for_evening;
+    std::string for_night;
+  };
 
-  {
-    // Verify 5:00 AM.
-    AdvanceClock(base::TimeDelta::FromMinutes(1));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good morning %s,", given_name.c_str())));
-  }
+  struct TestCase {
+    std::string display_email;
+    std::string given_name;
+    ExpectedGreeting expected_greeting;
+  };
 
-  {
-    // Verify 11:59 AM.
-    AdvanceClock(base::TimeDelta::FromHours(6) +
-                 base::TimeDelta::FromMinutes(59));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good morning %s,", given_name.c_str())));
-  }
+  const std::vector<TestCase> test_cases = {
+      TestCase{/*display_email=*/"empty@test",
+               /*given_name=*/std::string(),
+               ExpectedGreeting{
+                   /*for_morning=*/"Good morning,",
+                   /*for_afternoon=*/"Good afternoon,",
+                   /*for_evening=*/"Good evening,",
+                   /*for_night=*/"Good night,",
+               }},
+      TestCase{/*display_email=*/"david@test",
+               /*given_name=*/"David",
+               ExpectedGreeting{
+                   /*for_morning=*/"Good morning David,",
+                   /*for_afternoon=*/"Good afternoon David,",
+                   /*for_evening=*/"Good evening David,",
+                   /*for_night=*/"Good night David,",
+               }}};
+
+  for (const auto& test_case : test_cases) {
+    CreateAndSwitchActiveUser(test_case.display_email, test_case.given_name);
+
+    // Advance clock to midnight tomorrow.
+    AdvanceClock(base::Time::Now().LocalMidnight() +
+                 base::TimeDelta::FromHours(24) - base::Time::Now());
+
+    {
+      // Verify 4:59 AM.
+      AdvanceClock(base::TimeDelta::FromHours(4) +
+                   base::TimeDelta::FromMinutes(59));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_night));
+    }
 
-  {
-    // Verify 12:00 PM.
-    AdvanceClock(base::TimeDelta::FromMinutes(1));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(base::StringPrintf("Good afternoon %s,",
-                                                   given_name.c_str())));
-  }
+    {
+      // Verify 5:00 AM.
+      AdvanceClock(base::TimeDelta::FromMinutes(1));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_morning));
+    }
 
-  {
-    // Verify 4:59 PM.
-    AdvanceClock(base::TimeDelta::FromHours(4) +
-                 base::TimeDelta::FromMinutes(59));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(base::StringPrintf("Good afternoon %s,",
-                                                   given_name.c_str())));
-  }
+    {
+      // Verify 11:59 AM.
+      AdvanceClock(base::TimeDelta::FromHours(6) +
+                   base::TimeDelta::FromMinutes(59));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_morning));
+    }
 
-  {
-    // Verify 5:00 PM.
-    AdvanceClock(base::TimeDelta::FromMinutes(1));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good evening %s,", given_name.c_str())));
-  }
+    {
+      // Verify 12:00 PM.
+      AdvanceClock(base::TimeDelta::FromMinutes(1));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_afternoon));
+    }
 
-  {
-    // Verify 10:59 PM.
-    AdvanceClock(base::TimeDelta::FromHours(5) +
-                 base::TimeDelta::FromMinutes(59));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good evening %s,", given_name.c_str())));
-  }
+    {
+      // Verify 4:59 PM.
+      AdvanceClock(base::TimeDelta::FromHours(4) +
+                   base::TimeDelta::FromMinutes(59));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_afternoon));
+    }
 
-  {
-    // Verify 11:00 PM.
-    AdvanceClock(base::TimeDelta::FromMinutes(1));
-    ScopedShowUi scoped_show_ui;
-    EXPECT_EQ(greeting_label()->GetText(),
-              base::UTF8ToUTF16(
-                  base::StringPrintf("Good night %s,", given_name.c_str())));
+    {
+      // Verify 5:00 PM.
+      AdvanceClock(base::TimeDelta::FromMinutes(1));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_evening));
+    }
+
+    {
+      // Verify 10:59 PM.
+      AdvanceClock(base::TimeDelta::FromHours(5) +
+                   base::TimeDelta::FromMinutes(59));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_evening));
+    }
+
+    {
+      // Verify 11:00 PM.
+      AdvanceClock(base::TimeDelta::FromMinutes(1));
+      ScopedShowUi scoped_show_ui;
+      EXPECT_EQ(greeting_label()->GetText(),
+                base::UTF8ToUTF16(test_case.expected_greeting.for_night));
+    }
   }
 }
 

ash/session/test_session_controller_client.cc

@@ -138,7 +138,8 @@ void TestSessionControllerClient::AddUserSession(
     user_manager::UserType user_type,
     bool enable_settings,
     bool provide_pref_service,
-    bool is_new_profile) {
+    bool is_new_profile,
+    const std::string& given_name) {
   auto account_id = AccountId::FromUserEmail(
       use_lower_case_user_id_ ? GetUserIdFromEmail(display_email)
                               : display_email);
@@ -150,6 +151,7 @@ void TestSessionControllerClient::AddUserSession(
   session.user_info.display_email = display_email;
   session.user_info.is_ephemeral = false;
   session.user_info.is_new_profile = is_new_profile;
+  session.user_info.given_name = given_name;
   session.should_enable_settings = enable_settings;
   session.should_show_notification_tray = true;
   controller_->UpdateUserSession(std::move(session));

ash/session/test_session_controller_client.h

@@ -82,7 +82,8 @@ class TestSessionControllerClient : public SessionControllerClient {
       user_manager::UserType user_type = user_manager::USER_TYPE_REGULAR,
       bool enable_settings = true,
       bool provide_pref_service = true,
-      bool is_new_profile = false);
+      bool is_new_profile = false,
+      const std::string& given_name = std::string());
 
   // Creates a test PrefService and associates it with the user.
   void ProvidePrefServiceForUser(const AccountId& account_id);