diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt index 7e1b48d36..5bbec5f2e 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppAction.kt +++ b/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. */ diff --git a/app/src/main/java/org/mozilla/fenix/gleanplumb/DefaultMessageController.kt b/app/src/main/java/org/mozilla/fenix/gleanplumb/DefaultMessageController.kt index ad1851b1a..5d8b17408 100644 --- a/app/src/main/java/org/mozilla/fenix/gleanplumb/DefaultMessageController.kt +++ b/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)) { diff --git a/app/src/main/java/org/mozilla/fenix/gleanplumb/MessageController.kt b/app/src/main/java/org/mozilla/fenix/gleanplumb/MessageController.kt index 63a03aaa9..5eb03b746 100644 --- a/app/src/main/java/org/mozilla/fenix/gleanplumb/MessageController.kt +++ b/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) } diff --git a/app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt b/app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt index 1cf51b5f0..d49a1cf98 100644 --- a/app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt +++ b/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, diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt index f7038bb14..ff5536c42 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt +++ b/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 diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt index fb39bd17b..9abce1d95 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlInteractor.kt +++ b/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) - } } diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/onboarding/MessageCardViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/onboarding/MessageCardViewHolder.kt index 8af3c3b24..080b12ad3 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/onboarding/MessageCardViewHolder.kt +++ b/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 { diff --git a/app/src/test/java/org/mozilla/fenix/gleanplumb/DefaultMessageControllerTest.kt b/app/src/test/java/org/mozilla/fenix/gleanplumb/DefaultMessageControllerTest.kt index a34ed5f91..8991e8eee 100644 --- a/app/src/test/java/org/mozilla/fenix/gleanplumb/DefaultMessageControllerTest.kt +++ b/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, diff --git a/app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt index 45fae99c6..d8596f6c6 100644 --- a/app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt +++ b/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( - middlewareContext, {}, - MessageDisplayed(message) - ) + spiedMiddleware.onMessagedDisplayed(message, middlewareContext) 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) } } } diff --git a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt index 8e8e851e1..85d28139d 100644 --- a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt +++ b/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() 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(