Update Assistant timers v2 buttons and renotify behavior.

Previously, a notification for a firing timer in v2 would have a
"CANCEL" button. This is being relabeled to "STOP".

Previously, a notification for a firing timer in v2 would only popup to
the user if it hadn't already been marked as read. It would be marked
as read if the user interacted with it. Now, we will cause the
notification to be "renotified" to the user when the timer is firing.

Bug: b:165306385, b:165306366
Change-Id: Ieaace3594fc5294abbd76a892f4df35d85c618ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363449
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799374}

ash/assistant/assistant_alarm_timer_controller_impl.cc

@@ -220,15 +220,15 @@ std::vector<AssistantNotificationButton> CreateTimerNotificationButtons(
     }
   }
 
-  // "CANCEL" button.
-  buttons.push_back(
-      {l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_CANCEL_BUTTON),
-       assistant::util::CreateAlarmTimerDeepLink(
-           AlarmTimerAction::kRemoveAlarmOrTimer, timer.id)
-           .value(),
-       /*remove_notification_on_click=*/true});
-
   if (timer.state == AssistantTimerState::kFired) {
+    // "STOP" button.
+    buttons.push_back(
+        {l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_STOP_BUTTON),
+         assistant::util::CreateAlarmTimerDeepLink(
+             AlarmTimerAction::kRemoveAlarmOrTimer, timer.id)
+             .value(),
+         /*remove_notification_on_click=*/true});
+
     // "ADD 1 MIN" button.
     buttons.push_back({l10n_util::GetStringUTF8(
                            IDS_ASSISTANT_TIMER_NOTIFICATION_ADD_1_MIN_BUTTON),
@@ -237,6 +237,14 @@ std::vector<AssistantNotificationButton> CreateTimerNotificationButtons(
                            base::TimeDelta::FromMinutes(1))
                            .value(),
                        /*remove_notification_on_click=*/false});
+  } else {
+    // "CANCEL" button.
+    buttons.push_back({l10n_util::GetStringUTF8(
+                           IDS_ASSISTANT_TIMER_NOTIFICATION_CANCEL_BUTTON),
+                       assistant::util::CreateAlarmTimerDeepLink(
+                           AlarmTimerAction::kRemoveAlarmOrTimer, timer.id)
+                           .value(),
+                       /*remove_notification_on_click=*/true});
   }
 
   return buttons;
@@ -269,7 +277,9 @@ AssistantNotificationPriority CreateTimerNotificationPriority(
 }
 
 // Creates a notification for the given |timer|.
-AssistantNotification CreateTimerNotification(const AssistantTimer& timer) {
+AssistantNotification CreateTimerNotification(
+    const AssistantTimer& timer,
+    const AssistantNotification* existing_notification = nullptr) {
   AssistantNotification notification;
   notification.title = CreateTimerNotificationTitle(timer);
   notification.message = CreateTimerNotificationMessage(timer);
@@ -280,6 +290,16 @@ AssistantNotification CreateTimerNotification(const AssistantTimer& timer) {
   notification.priority = CreateTimerNotificationPriority(timer);
   notification.remove_on_click = !IsTimersV2Enabled();
   notification.is_pinned = IsTimersV2Enabled();
+
+  // If we are creating a notification to replace an |existing_notification| and
+  // our new notification has higher priority, we want the system to "renotify"
+  // the user of the notification change. This will cause the new notification
+  // to popup to the user even if it was previously marked as read.
+  if (existing_notification &&
+      notification.priority > existing_notification->priority) {
+    notification.renotify = true;
+  }
+
   return notification;
 }
 
@@ -411,14 +431,17 @@ void AssistantAlarmTimerControllerImpl::OnTimerUpdated(
   // Schedule the next tick of |timer|.
   ScheduleNextTick(timer);
 
-  // When a |timer| is updated we need to update the corresponding notification
-  // unless it has already been dismissed by the user.
   auto* notification_controller =
       assistant_controller_->notification_controller();
-  if (notification_controller->model()->HasNotificationForId(
-          CreateTimerNotificationId(timer))) {
+  const auto* existing_notification =
+      notification_controller->model()->GetNotificationById(
+          CreateTimerNotificationId(timer));
+
+  // When a |timer| is updated we need to update the corresponding notification
+  // unless it has already been dismissed by the user.
+  if (existing_notification) {
     notification_controller->AddOrUpdateNotification(
-        CreateTimerNotification(timer));
+        CreateTimerNotification(timer, existing_notification));
   }
 }
 

ash/assistant/assistant_alarm_timer_controller_unittest.cc

@@ -152,6 +152,8 @@ class ExpectedNotification {
       os << "\npriority: '" << static_cast<int>(notif.priority_.value()) << "'";
     if (notif.remove_on_click_.has_value())
       os << "\nremove_on_click: '" << notif.remove_on_click_.value() << "'";
+    if (notif.renotify_.has_value())
+      os << "\nrenotify: '" << notif.renotify_.value() << "'";
     if (notif.title_.has_value())
       os << "\ntitle: '" << notif.title_.value() << "'";
     return os;
@@ -168,6 +170,7 @@ class ExpectedNotification {
            notification.priority == priority_.value_or(notification.priority) &&
            notification.remove_on_click ==
                remove_on_click_.value_or(notification.remove_on_click) &&
+           notification.renotify == renotify_.value_or(notification.renotify) &&
            notification.title == title_.value_or(notification.title);
   }
 
@@ -201,6 +204,11 @@ class ExpectedNotification {
     return *this;
   }
 
+  ExpectedNotification& WithRenotify(bool renotify) {
+    renotify_ = renotify;
+    return *this;
+  }
+
   ExpectedNotification& WithTitle(const std::string& title) {
     title_ = title;
     return *this;
@@ -217,6 +225,7 @@ class ExpectedNotification {
   base::Optional<std::string> message_;
   base::Optional<AssistantNotificationPriority> priority_;
   base::Optional<bool> remove_on_click_;
+  base::Optional<bool> renotify_;
   base::Optional<std::string> title_;
 };
 
@@ -818,9 +827,9 @@ TEST_F(AssistantAlarmTimerControllerTest,
   // We expect the timer notification to have two buttons.
   ASSERT_EQ(2u, observer.last_notification().buttons.size());
 
-  // We expect a "CANCEL" button which will remove the timer.
+  // We expect a "STOP" button which will remove the timer.
   EXPECT_EQ(ExpectedButton()
-                .WithLabel(IDS_ASSISTANT_TIMER_NOTIFICATION_CANCEL_BUTTON)
+                .WithLabel(IDS_ASSISTANT_TIMER_NOTIFICATION_STOP_BUTTON)
                 .WithActionUrl(
                     assistant::util::CreateAlarmTimerDeepLink(
                         assistant::util::AlarmTimerAction::kRemoveAlarmOrTimer,
@@ -883,6 +892,53 @@ TEST_F(AssistantAlarmTimerControllerTest,
       observer.last_notification());
 }
 
+// Tests that a notification is added for a timer and has the expected value to
+// *not* cause renotification.
+// NOTE: This test is only applicable to timers v1.
+TEST_F(AssistantAlarmTimerControllerTest,
+       TimerNotificationHasExpectedRenotify) {
+  ASSERT_FALSE(chromeos::assistant::features::IsTimersV2Enabled());
+
+  // Observe notifications.
+  ScopedNotificationModelObserver observer;
+
+  // Fire a timer.
+  FireTimer{kTimerId};
+
+  // Make assertions about the notification.
+  EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithRenotify(false),
+            observer.last_notification());
+}
+
+// Tests that a notification is added for a timer and has the expected value to
+// cause renotification.
+// Note: This test is only applicable to timers v2.
+TEST_F(AssistantAlarmTimerControllerTest,
+       TimerNotificationHasExpectedRenotifyV2) {
+  // Enable timers v2.
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      chromeos::assistant::features::kAssistantTimersV2);
+  ASSERT_TRUE(chromeos::assistant::features::IsTimersV2Enabled());
+
+  // Observe notifications.
+  ScopedNotificationModelObserver observer;
+
+  // Schedule a timer.
+  ScheduleTimer{kTimerId};
+
+  // Make assertions about the notification.
+  EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithRenotify(false),
+            observer.last_notification());
+
+  // Fire the timer.
+  FireTimer{kTimerId};
+
+  // Make assertions about the notification.
+  EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithRenotify(true),
+            observer.last_notification());
+}
+
 // Tests that a notification is added for a timer and has the expected priority.
 // NOTE: This test is only applicable to timers v1.
 TEST_F(AssistantAlarmTimerControllerTest,

ash/assistant/assistant_notification_controller_impl.cc

@@ -51,6 +51,7 @@ std::unique_ptr<message_center::Notification> CreateSystemNotification(
           /*delegate=*/nullptr, kNotificationAssistantIcon,
           message_center::SystemNotificationWarningLevel::NORMAL);
 
+  system_notification->set_renotify(notification.renotify);
   system_notification->set_pinned(notification.is_pinned);
 
   switch (notification.priority) {

ash/assistant/assistant_notification_controller_impl_unittest.cc

@@ -106,6 +106,11 @@ class AssistantNotificationBuilder {
     return *this;
   }
 
+  AssistantNotificationBuilder& WithRenotify(bool renotify) {
+    notification_.renotify = renotify;
+    return *this;
+  }
+
   AssistantNotificationBuilder& WithTimeout(
       base::Optional<base::TimeDelta> timeout) {
     notification_.expiry_time =
@@ -554,6 +559,39 @@ TEST_F(AssistantNotificationControllerTest, ShouldPropagateIsPinned) {
   EXPECT_TRUE(system_notification->pinned());
 }
 
+TEST_F(AssistantNotificationControllerTest, ShouldPropagateRenotify) {
+  constexpr char kId[] = "id";
+
+  // Create an Assistant notification w/ default renotify behavior.
+  AddOrUpdateNotification(AssistantNotificationBuilder().WithId(kId).Build());
+
+  // Verify expected system notification.
+  auto* system_notification =
+      message_center::MessageCenter::Get()->FindVisibleNotificationById(kId);
+  ASSERT_NE(nullptr, system_notification);
+  EXPECT_FALSE(system_notification->renotify());
+
+  // Create an Assistant notification w/ explicitly disabled renotify behavior.
+  AddOrUpdateNotification(
+      AssistantNotificationBuilder().WithId(kId).WithRenotify(false).Build());
+
+  // Verify expected system notification.
+  system_notification =
+      message_center::MessageCenter::Get()->FindVisibleNotificationById(kId);
+  ASSERT_NE(nullptr, system_notification);
+  EXPECT_FALSE(system_notification->renotify());
+
+  // Create an Assistant notification w/ explicitly enabled renotify behavior.
+  AddOrUpdateNotification(
+      AssistantNotificationBuilder().WithId(kId).WithRenotify(true).Build());
+
+  // Verify expected system notification.
+  system_notification =
+      message_center::MessageCenter::Get()->FindVisibleNotificationById(kId);
+  ASSERT_NE(nullptr, system_notification);
+  EXPECT_TRUE(system_notification->renotify());
+}
+
 TEST_F(AssistantNotificationControllerTest,
        ShouldMaybeRetrieveNotificationPayload) {
   constexpr char kId[] = "id";

chromeos/services/assistant/public/cpp/assistant_notification.h

@@ -85,6 +85,11 @@ struct COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) AssistantNotification {
   // If |true|, the notification will be removed on click.
   bool remove_on_click = true;
 
+  // If |true|, the notification state (e.g. its popup and read status) will be
+  // reset so as to renotify the user of this notification.
+  // See `message_center::Notification::renotify()`.
+  bool renotify = false;
+
   // If |true|, the user can't remove the notification.
   bool is_pinned = false;