For #25446: Ensure for every message shown we have recordExposure

app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt

@@ -136,11 +136,6 @@ sealed class AppAction : Action {
          */
         data class MessageClicked(val message: Message) : MessagingAction()
 
-        /**
-         * Indicates the given [message] was shown.
-         */
-        data class MessageDisplayed(val message: Message) : MessagingAction()
-
         /**
          * Indicates the given [message] was dismissed.
          */

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

@@ -14,7 +14,6 @@ import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.components.AppStore
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageClicked
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDismissed
-import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDisplayed
 
 /**
  * Handles default interactions with the ui of GleanPlumb messages.
@@ -44,14 +43,6 @@ class DefaultMessageController(
         appStore.dispatch(MessageDismissed(message))
     }
 
-    override fun onMessageDisplayed(message: Message) {
-        if (message.maxDisplayCount <= message.metadata.displayCount + 1) {
-            Messaging.messageExpired.record(Messaging.MessageExpiredExtra(message.id))
-        }
-        Messaging.messageShown.record(Messaging.MessageShownExtra(message.id))
-        appStore.dispatch(MessageDisplayed(message))
-    }
-
     @VisibleForTesting
     internal fun handleAction(action: String): Intent {
         val partialAction = if (action.startsWith("http", ignoreCase = true)) {

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

@@ -17,9 +17,4 @@ interface MessageController {
      * Indicates the provided [message] was dismissed by a user.
      */
     fun onMessageDismissed(message: Message)
-
-    /**
-     * Indicates the provided [message] was displayed to a user.
-     */
-    fun onMessageDisplayed(message: Message)
 }

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

@@ -10,12 +10,12 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import mozilla.components.lib.state.Middleware
 import mozilla.components.lib.state.MiddlewareContext
+import org.mozilla.fenix.GleanMetrics.Messaging
 import org.mozilla.fenix.components.appstate.AppAction
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.ConsumeMessageToShow
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.Evaluate
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageClicked
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDismissed
-import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDisplayed
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.Restore
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.UpdateMessageToShow
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.UpdateMessages
@@ -47,6 +47,7 @@ class MessagingMiddleware(
                 val message = messagingStorage.getNextMessage(context.state.messaging.messages)
                 if (message != null) {
                     context.dispatch(UpdateMessageToShow(message))
+                    onMessagedDisplayed(message, context)
                 } else {
                     context.dispatch(ConsumeMessageToShow)
                 }
@@ -56,8 +57,6 @@ class MessagingMiddleware(
 
             is MessageDismissed -> onMessageDismissed(context, action.message)
 
-            is MessageDisplayed -> onMessagedDisplayed(action.message, context)
-
             else -> {
                 // no-op
             }
@@ -70,6 +69,7 @@ class MessagingMiddleware(
         oldMessage: Message,
         context: AppStoreMiddlewareContext
     ) {
+        sendShownMessageTelemetry(oldMessage.id)
         val newMetadata = oldMessage.metadata.copy(
             displayCount = oldMessage.metadata.displayCount + 1,
             lastTimeShown = now()
@@ -80,6 +80,7 @@ class MessagingMiddleware(
         val newMessages = if (newMetadata.displayCount < oldMessage.maxDisplayCount) {
             updateMessage(context, oldMessage, newMessage)
         } else {
+            sendExpiredMessageTelemetry(newMessage.id)
             consumeMessageToShowIfNeeded(context, oldMessage)
             removeMessage(context, oldMessage)
         }
@@ -89,6 +90,16 @@ class MessagingMiddleware(
         }
     }
 
+    @VisibleForTesting
+    internal fun sendShownMessageTelemetry(messageId: String) {
+        Messaging.messageShown.record(Messaging.MessageShownExtra(messageId))
+    }
+
+    @VisibleForTesting
+    internal fun sendExpiredMessageTelemetry(messageId: String) {
+        Messaging.messageExpired.record(Messaging.MessageExpiredExtra(messageId))
+    }
+
     @VisibleForTesting
     internal fun onMessageDismissed(
         context: AppStoreMiddlewareContext,

app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt

@@ -188,11 +188,6 @@ interface SessionControlController {
      */
     fun handleMessageClosed(message: Message)
 
-    /**
-     * @see [MessageCardInteractor.onMessageDisplayed]
-     */
-    fun handleMessageDisplayed(message: Message)
-
     /**
      * @see [TabSessionInteractor.onPrivateModeButtonClicked]
      */
@@ -619,10 +614,6 @@ class DefaultSessionControlController(
         messageController.onMessageDismissed(message)
     }
 
-    override fun handleMessageDisplayed(message: Message) {
-        messageController.onMessageDisplayed(message)
-    }
-
     override fun handlePrivateModeButtonClicked(
         newMode: BrowsingMode,
         userHasBeenOnboarded: Boolean

app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt

@@ -239,11 +239,6 @@ interface MessageCardInteractor {
      * Called when close button on a [Message] card.
      */
     fun onMessageClosedClicked(message: Message)
-
-    /**
-     * Called when close button on a [Message] card.
-     */
-    fun onMessageDisplayed(message: Message)
 }
 
 /**
@@ -470,8 +465,4 @@ class SessionControlInteractor(
     override fun onMessageClosedClicked(message: Message) {
         controller.handleMessageClosed(message)
     }
-
-    override fun onMessageDisplayed(message: Message) {
-        controller.handleMessageDisplayed(message)
-    }
 }

app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/onboarding/MessageCardViewHolder.kt

@@ -47,7 +47,6 @@ class MessageCardViewHolder(
                 interactor.onMessageClosedClicked(message)
             }
         }
-        interactor.onMessageDisplayed(message)
     }
 
     companion object {

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

@@ -24,7 +24,6 @@ import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.components.AppStore
 import org.mozilla.fenix.components.appstate.AppAction
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageClicked
-import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDisplayed
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
 import org.mozilla.fenix.nimbus.MessageData
 
@@ -103,28 +102,6 @@ class DefaultMessageControllerTest {
         verify { store.dispatch(AppAction.MessagingAction.MessageDismissed(message)) }
     }
 
-    @Test
-    fun `WHEN calling onMessageDisplayed THEN report to the messageManager`() {
-        val data = MessageData()
-        val message = mockMessage(data)
-        assertNull(Messaging.messageExpired.testGetValue())
-        assertNull(Messaging.messageShown.testGetValue())
-
-        controller.onMessageDisplayed(message)
-
-        assertNotNull(Messaging.messageExpired.testGetValue())
-        val messageExpiredEvent = Messaging.messageExpired.testGetValue()!!
-        assertEquals(1, messageExpiredEvent.size)
-        assertEquals(message.id, messageExpiredEvent.single().extra!!["message_key"])
-
-        assertNotNull(Messaging.messageShown.testGetValue())
-        val event = Messaging.messageShown.testGetValue()!!
-        assertEquals(1, event.size)
-        assertEquals(message.id, event.single().extra!!["message_key"])
-
-        verify { store.dispatch(MessageDisplayed(message)) }
-    }
-
     private fun mockMessage(data: MessageData = MessageData()) = Message(
         id = "id",
         data = data,

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

@@ -28,7 +28,6 @@ import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.ConsumeMe
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.Evaluate
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageClicked
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDismissed
-import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.MessageDisplayed
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.Restore
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.UpdateMessageToShow
 import org.mozilla.fenix.components.appstate.AppAction.MessagingAction.UpdateMessages
@@ -143,7 +142,7 @@ class MessagingMiddlewareTest {
     }
 
     @Test
-    fun `WHEN MessageDisplayed THEN update storage`() = runTestOnMain {
+    fun `GIEN a expiring message WHEN MessageDisplayed THEN update storage`() = runTestOnMain {
         val message = Message(
             "control-id",
             mockk(relaxed = true),
@@ -161,13 +160,11 @@ class MessagingMiddlewareTest {
         every { appState.messaging } returns messagingState
         every { middlewareContext.state } returns appState
 
-        spiedMiddleware.invoke(
+        spiedMiddleware.onMessagedDisplayed(message, middlewareContext)
-            middlewareContext, {},
-            MessageDisplayed(message)
-        )
 
         coVerify { messagingStorage.updateMetadata(message.metadata.copy(displayCount = 1)) }
         verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
+        verify { spiedMiddleware.sendExpiredMessageTelemetry(message.id) }
     }
 
     @Test
@@ -340,5 +337,6 @@ class MessagingMiddlewareTest {
         verify { spiedMiddleware.removeMessage(middlewareContext, oldMessage) }
         verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
         coVerify { messagingStorage.updateMetadata(updatedMessage.metadata) }
+        verify { spiedMiddleware.sendShownMessageTelemetry(oldMessage.id) }
     }
 }

app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt

@@ -1228,13 +1228,12 @@ class DefaultSessionControlControllerTest {
     }
 
     @Test
-    fun `WHEN handleMessageClicked,handleMessageClosed and handleMessageDisplayed are called THEN delegate to messageController`() {
+    fun `WHEN handleMessageClicked and handleMessageClosed are called THEN delegate to messageController`() {
         val controller = createController()
         val message = mockk<Message>()
 
         controller.handleMessageClicked(message)
         controller.handleMessageClosed(message)
-        controller.handleMessageDisplayed(message)
 
         verify {
             messageController.onMessagePressed(message)
@@ -1242,9 +1241,6 @@ class DefaultSessionControlControllerTest {
         verify {
             messageController.onMessageDismissed(message)
         }
-        verify {
-            messageController.onMessageDisplayed(message)
-        }
     }
 
     private fun createController(