[fenix] Address reviewer comments

app/src/main/java/org/mozilla/fenix/gleanplumb/NimbusMessagingController.kt

@@ -12,9 +12,9 @@ import org.mozilla.fenix.GleanMetrics.Messaging
  * Class extracted from [MessagingMiddleware] to do the bookkeeping for message actions, in terms
  * of Glean messages and the messaging store.
  */
-open class NimbusMessagingController(
-    protected val messagingStorage: NimbusMessagingStorage,
-    private val clock: () -> Long = { System.currentTimeMillis() },
+class NimbusMessagingController(
+    private val messagingStorage: NimbusMessagingStorage,
+    private val now: () -> Long = { System.currentTimeMillis() },
 ) {
     /**
      * Called when a message is just about to be shown to the user.
@@ -23,11 +23,11 @@ open class NimbusMessagingController(
      *
      * Records glean events for messageShown and messageExpired.
      */
-    suspend fun onMessageDisplayed(oldMessage: Message): Message {
+    suspend fun processDisplayedMessage(oldMessage: Message): Message {
         sendShownMessageTelemetry(oldMessage.id)
         val newMetadata = oldMessage.metadata.copy(
             displayCount = oldMessage.metadata.displayCount + 1,
-            lastTimeShown = clock(),
+            lastTimeShown = now(),
         )
         val newMessage = oldMessage.copy(
             metadata = newMetadata,

app/src/main/java/org/mozilla/fenix/gleanplumb/NimbusMessagingStorage.kt

@@ -126,9 +126,12 @@ class NimbusMessagingStorage(
      * e.g.
      * `https://example.com/{locale}/whatsnew.html?version={app_version}`
      *
-     * A special variable, `{uuid}` is also detected, and a random UUID is
-     * put in its place. If `{uuid}` is detected, then it is returned as the first
-     * value of the returned [Pair].
+     * If the string `{uuid}` is detected in the [action] string, then it is
+     * replaced with a random UUID. This is returned as the first value of the returned
+     * [Pair].
+     *
+     * The fully resolved (with all substitutions) action is returned as the second value
+     * of the [Pair].
      */
     fun getMessageAction(action: String): Pair<String?, String> {
         val helper = gleanPlumb.createMessageHelper(customAttributes)

app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt

@@ -74,7 +74,7 @@ class MessagingMiddleware(
         context: AppStoreMiddlewareContext,
     ) {
         coroutineScope.launch {
-            val newMessage = controller.onMessageDisplayed(oldMessage)
+            val newMessage = controller.processDisplayedMessage(oldMessage)
             val newMessages = if (!newMessage.isExpired) {
                 updateMessage(context, oldMessage, newMessage)
             } else {

app/src/test/java/org/mozilla/fenix/gleanplumb/NimbusMessagingControllerTest.kt

@@ -45,7 +45,7 @@ class NimbusMessagingControllerTest {
     }
 
     @Test
-    fun `WHEN calling onMessageDismissed THEN record an event and updates metadata`() = coroutineScope.runTest {
+    fun `WHEN calling onMessageDismissed THEN record a messageDismissed event and updates metadata`() = coroutineScope.runTest {
         val message = createMessage("id-1")
         assertNull(Messaging.messageDismissed.testGetValue())
 
@@ -60,12 +60,12 @@ class NimbusMessagingControllerTest {
     }
 
     @Test
-    fun `WHEN calling onMessageDisplayed THEN record an event and updates metadata`() = coroutineScope.runTest {
+    fun `WHEN calling processDisplayedMessage THEN record a messageDisplayed event and updates metadata`() = coroutineScope.runTest {
         val message = createMessage("id-1")
         assertNull(Messaging.messageShown.testGetValue())
         assertEquals(0, message.metadata.displayCount)
 
-        val updated = controller.onMessageDisplayed(message)
+        val updated = controller.processDisplayedMessage(message)
 
         assertNotNull(Messaging.messageShown.testGetValue())
         val event = Messaging.messageShown.testGetValue()!!
@@ -77,12 +77,12 @@ class NimbusMessagingControllerTest {
     }
 
     @Test
-    fun `WHEN calling onMessageDisplayed on an expiring message THEN record an event`() = coroutineScope.runTest {
+    fun `WHEN calling processDisplayedMessage on an expiring message THEN record a messageExpired event`() = coroutineScope.runTest {
         val message = createMessage("id-1", style = StyleData(maxDisplayCount = 1))
         assertNull(Messaging.messageShown.testGetValue())
         assertEquals(0, message.metadata.displayCount)
 
-        val updated = controller.onMessageDisplayed(message)
+        val updated = controller.processDisplayedMessage(message)
 
         assertNotNull(Messaging.messageShown.testGetValue())
         val shownEvent = Messaging.messageShown.testGetValue()!!
@@ -126,7 +126,7 @@ class NimbusMessagingControllerTest {
     }
 
     @Test
-    fun `GIVEN a URL WHEN calling createMessageAction THEN record an event`() {
+    fun `GIVEN a URL WHEN calling createMessageAction THEN record a messageClicked event`() {
         val message = createMessage("id-1", action = "http://mozilla.org")
         every { storage.getMessageAction(any()) } returns Pair(null, message.action)
 
@@ -141,7 +141,7 @@ class NimbusMessagingControllerTest {
     }
 
     @Test
-    fun `GIVEN a URL with a {uuid} WHEN calling createMessageAction THEN record an event`() {
+    fun `GIVEN a URL with a {uuid} WHEN calling createMessageAction THEN record a messageClicked event with a uuid`() {
         val message = createMessage("id-1", action = "http://mozilla.org?uuid={uuid}")
         val uuid = UUID.randomUUID().toString()
         every { storage.getMessageAction(any()) } returns Pair(uuid, message.action)

app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt

@@ -59,7 +59,7 @@ class MessagingMiddlewareTest {
     fun setUp() {
         appStore = mockk(relaxed = true)
         messagingStorage = mockk(relaxed = true)
-        messagingController = NimbusMessagingController(messagingStorage, clock = { 0L })
+        messagingController = NimbusMessagingController(messagingStorage) { 0L }
         middlewareContext = mockk(relaxed = true)
         every { middlewareContext.store } returns appStore
 
@@ -169,7 +169,7 @@ class MessagingMiddlewareTest {
 
         spiedMiddleware.onMessagedDisplayed(message, middlewareContext)
 
-        coVerify { messagingController.onMessageDisplayed(message) }
+        coVerify { messagingController.processDisplayedMessage(message) }
         coVerify { messagingStorage.updateMetadata(message.metadata.copy(displayCount = 1)) }
         verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
     }
@@ -310,7 +310,7 @@ class MessagingMiddlewareTest {
 
         verify { spiedMiddleware.updateMessage(middlewareContext, oldMessage, updatedMessage) }
         verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
-        coVerify { messagingController.onMessageDisplayed(oldMessage) }
+        coVerify { messagingController.processDisplayedMessage(oldMessage) }
         coVerify { messagingStorage.updateMetadata(updatedMessage.metadata) }
     }
 
@@ -343,7 +343,7 @@ class MessagingMiddlewareTest {
         verify { spiedMiddleware.consumeMessageToShowIfNeeded(middlewareContext, oldMessage) }
         verify { spiedMiddleware.removeMessage(middlewareContext, oldMessage) }
         verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
-        coVerify { messagingController.onMessageDisplayed(oldMessage) }
+        coVerify { messagingController.processDisplayedMessage(oldMessage) }
         coVerify { messagingStorage.updateMetadata(updatedMessage.metadata) }
     }
 }

messaging.fml.yaml

@@ -114,7 +114,7 @@ features:
               priority: 100
               max-display-count: 1
           notification-config:
-            polling-interval: 180 # 3 minutes
+            polling-interval: 15 # minutes
 
 objects:
   MessageData: