[fenix] For 1811851: Update message notification worker to handle when a user clicks or dismisses a Notification. (https://github.com/mozilla-mobile/fenix/pull/28655)

* For 1811851: Change MessageNotificationWorker.kt to handle when a user clicks or dismisses a Notification. * For 1811851: Re-work based on review --------- Co-authored-by: t-p-white <t-p-white> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 year ago · 9946146ecf
parent 733095355c
commit 9946146ecf
7 changed files with 254 additions and 56 deletions
--- a/app/src/main/AndroidManifest.xml
+++ b/app/src/main/AndroidManifest.xml
@ -275,6 +275,10 @@
            android:exported="false"
            android:theme="@style/DialogActivityTheme" />

+        <activity
+            android:name=".gleanplumb.NotificationClickedReceiverActivity"
+            android:exported="false" />
+
        <service
            android:name=".autofill.AutofillService"
            android:exported="true"
@ -320,6 +324,10 @@
        <service android:name=".session.PrivateNotificationService"
                android:exported="false" />

+        <service
+            android:name=".gleanplumb.NotificationDismissedService"
+            android:exported="false" />
+
        <service
            android:name=".push.FirebasePushService"
            android:exported="false">
--- a/app/src/main/java/org/mozilla/fenix/gleanplumb/MessageNotificationWorker.kt
+++ b/app/src/main/java/org/mozilla/fenix/gleanplumb/MessageNotificationWorker.kt
@ -4,10 +4,14 @@

 package org.mozilla.fenix.gleanplumb

+import android.app.Activity
 import android.app.Notification
 import android.app.PendingIntent
+import android.app.Service
 import android.content.Context
 import android.content.Intent
+import android.os.Bundle
+import android.os.IBinder
 import androidx.core.app.NotificationManagerCompat
 import androidx.work.ExistingPeriodicWorkPolicy
 import androidx.work.PeriodicWorkRequest
@ -27,6 +31,9 @@ import org.mozilla.fenix.utils.IntentUtils
 import org.mozilla.fenix.utils.createBaseNotification
 import java.util.concurrent.TimeUnit

+const val CLICKED_MESSAGE_ID = "clickedMessageId"
+const val DISMISSED_MESSAGE_ID = "dismissedMessageId"
+
 /**
 * Background [Worker] that polls Nimbus for available [Message]s at a given interval.
 * A [Notification] will be created using the configuration of the next highest priority [Message]
@ -37,7 +44,7 @@ class MessageNotificationWorker(
    workerParameters: WorkerParameters,
 ) : Worker(context, workerParameters) {

-    @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage
+    @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage.
    override fun doWork(): Result {
        GlobalScope.launch(Dispatchers.IO) {
            val context = applicationContext
@ -45,51 +52,76 @@ class MessageNotificationWorker(
            val messages = messagingStorage.getMessages()
            val nextMessage =
                messagingStorage.getNextMessage(MessageSurfaceId.NOTIFICATION, messages)
-
-            if (nextMessage == null || !nextMessage.shouldDisplayMessage()) {
-                return@launch
-            }
+                    ?: return@launch

            val nimbusMessagingController = NimbusMessagingController(messagingStorage)

            // Update message as displayed.
-            val messageAsDisplayed =
+            val updatedMessage =
                nimbusMessagingController.updateMessageAsDisplayed(nextMessage)
-            nimbusMessagingController.onMessageDisplayed(messageAsDisplayed)
-
-            // Generate the processed Message action
-            val processedAction = nimbusMessagingController.processMessageActionToUri(nextMessage)
-            val actionIntent = Intent(Intent.ACTION_VIEW, processedAction)
+            nimbusMessagingController.onMessageDisplayed(updatedMessage)

            NotificationManagerCompat.from(context).notify(
                MESSAGE_TAG,
-                SharedIdsHelper.getNextIdForTag(context, nextMessage.id),
-                buildNotification(nextMessage, actionIntent),
+                SharedIdsHelper.getNextIdForTag(context, updatedMessage.id),
+                buildNotification(
+                    context,
+                    updatedMessage,
+                ),
            )
        }

        return Result.success()
    }

-    private fun Message.shouldDisplayMessage() = metadata.displayCount == 0
+    private fun buildNotification(
+        context: Context,
+        message: Message,
+    ): Notification {
+        val onClickPendingIntent = createOnClickPendingIntent(context, message)
+        val onDismissPendingIntent = createOnDismissPendingIntent(context, message)
+
+        return createBaseNotification(
+            context,
+            MARKETING_CHANNEL_ID,
+            message.data.title,
+            message.data.text,
+            onClickPendingIntent,
+            onDismissPendingIntent,
+        )
+    }

-    private fun buildNotification(message: Message, intent: Intent): Notification {
-        with(applicationContext) {
-            val pendingIntent = PendingIntent.getActivity(
-                this,
-                SharedIdsHelper.getNextIdForTag(this, NOTIFICATION_PENDING_INTENT_TAG),
-                intent,
-                IntentUtils.defaultIntentPendingFlags,
-            )
+    private fun createOnClickPendingIntent(
+        context: Context,
+        message: Message,
+    ): PendingIntent {
+        val intent = Intent(context, NotificationClickedReceiverActivity::class.java)
+        intent.putExtra(CLICKED_MESSAGE_ID, message.id)
+        intent.addFlags(Intent.FLAG_ACTIVITY_EXCLUDE_FROM_RECENTS)
+
+        // Activity intent.
+        return PendingIntent.getActivity(
+            context,
+            SharedIdsHelper.getNextIdForTag(context, NOTIFICATION_PENDING_INTENT_TAG),
+            intent,
+            IntentUtils.defaultIntentPendingFlags,
+        )
+    }

-            return createBaseNotification(
-                this,
-                MARKETING_CHANNEL_ID,
-                message.data.title,
-                message.data.text,
-                pendingIntent,
-            )
-        }
+    private fun createOnDismissPendingIntent(
+        context: Context,
+        message: Message,
+    ): PendingIntent {
+        val intent = Intent(context, NotificationDismissedService::class.java)
+        intent.putExtra(DISMISSED_MESSAGE_ID, message.id)
+
+        // Service intent.
+        return PendingIntent.getService(
+            context,
+            SharedIdsHelper.getNextIdForTag(context, NOTIFICATION_PENDING_INTENT_TAG),
+            intent,
+            IntentUtils.defaultIntentPendingFlags,
+        )
    }

    companion object {
@ -109,17 +141,91 @@ class MessageNotificationWorker(
                MessageNotificationWorker::class.java,
                pollingInterval,
                TimeUnit.MINUTES,
-            ) // Only start polling after the given interval
+            ) // Only start polling after the given interval.
                .setInitialDelay(pollingInterval, TimeUnit.MINUTES)
                .build()

            val instanceWorkManager = WorkManager.getInstance(context)
            instanceWorkManager.enqueueUniquePeriodicWork(
                MESSAGE_WORK_NAME,
-                // We want to keep any existing scheduled work
+                // We want to keep any existing scheduled work.
                ExistingPeriodicWorkPolicy.KEEP,
                messageWorkRequest,
            )
        }
    }
 }
+
+/**
+ * When a [Message] [Notification] is dismissed by the user record telemetry data and update the
+ * [Message.metadata].
+ *
+ * This [Service] is only intended to be used by the [MessageNotificationWorker.createOnDismissPendingIntent] function.
+ */
+class NotificationDismissedService : Service() {
+
+    /**
+     * This service cannot be bound to.
+     */
+    override fun onBind(intent: Intent?): IBinder? = null
+
+    @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage.
+    override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
+        GlobalScope.launch {
+            if (intent != null) {
+                val nimbusMessagingController =
+                    NimbusMessagingController(applicationContext.components.analytics.messagingStorage)
+
+                // Get the relevant message.
+                val messageId = intent.getStringExtra(DISMISSED_MESSAGE_ID)!!
+                val message = nimbusMessagingController.getMessage(messageId)
+
+                if (message != null) {
+                    // Update message as 'dismissed'.
+                    nimbusMessagingController.onMessageDismissed(message.metadata)
+                }
+            }
+        }
+
+        return START_REDELIVER_INTENT
+    }
+}
+
+/**
+ * When a [Message] [Notification] is clicked by the user record telemetry data and update the
+ * [Message.metadata].
+ *
+ * This [Activity] is only intended to be used by the [MessageNotificationWorker.createOnClickPendingIntent] function.
+ */
+class NotificationClickedReceiverActivity : Activity() {
+
+    @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage.
+    override fun onCreate(savedInstanceState: Bundle?) {
+        super.onCreate(savedInstanceState)
+
+        GlobalScope.launch {
+            val nimbusMessagingController =
+                NimbusMessagingController(components.analytics.messagingStorage)
+
+            // Get the relevant message.
+            val messageId = intent.getStringExtra(CLICKED_MESSAGE_ID)!!
+            val message = nimbusMessagingController.getMessage(messageId)
+
+            if (message != null) {
+                // Update message as 'clicked'.
+                nimbusMessagingController.onMessageClicked(message.metadata)
+
+                // Create the intent.
+                val intent = nimbusMessagingController.getIntentForMessageAction(message.action)
+                intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
+                intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK)
+
+                // Start the message intent.
+                startActivity(intent)
+            }
+        }
+
+        // End this activity.
+        finish()
+    }
+}
--- a/app/src/main/java/org/mozilla/fenix/gleanplumb/NimbusMessagingController.kt
+++ b/app/src/main/java/org/mozilla/fenix/gleanplumb/NimbusMessagingController.kt
@ -4,6 +4,7 @@

 package org.mozilla.fenix.gleanplumb

+import android.content.Intent
 import android.net.Uri
 import androidx.core.net.toUri
 import org.mozilla.fenix.BuildConfig
@ -48,14 +49,45 @@ class NimbusMessagingController(
     * Records a messageDismissed event, and records that the message
     * has been dismissed.
     */
-    suspend fun onMessageDismissed(message: Message) {
-        sendDismissedMessageTelemetry(message.id)
-        val updatedMetadata = message.metadata.copy(dismissed = true)
+    suspend fun onMessageDismissed(messageMetadata: Message.Metadata) {
+        sendDismissedMessageTelemetry(messageMetadata.id)
+        val updatedMetadata = messageMetadata.copy(dismissed = true)
        messagingStorage.updateMetadata(updatedMetadata)
    }

    /**
-     * Once a message is clicked, the action needs to be examined for string substitutions
+     * Called once the user has clicked on a message.
+     *
+     * This records that the message has been clicked on, but does not record a
+     * glean event. That should be done via [processMessageActionToUri].
+     */
+    suspend fun onMessageClicked(messageMetadata: Message.Metadata) {
+        val updatedMetadata = messageMetadata.copy(pressed = true)
+        messagingStorage.updateMetadata(updatedMetadata)
+    }
+
+    /**
+     * Create and return the relevant [Intent] for the given [action].
+     *
+     * @param action the [Message.action] to create the [Intent] for.
+     * @return an [Intent] using the processed [Message.action].
+     */
+    fun getIntentForMessageAction(action: String): Intent {
+        return Intent(Intent.ACTION_VIEW, action.toDeepLinkSchemeUri())
+    }
+
+    /**
+     * Will attempt to get the [Message] for the given [id].
+     *
+     * @param id the [Message.id] of the [Message] to try to match.
+     * @return the [Message] with a matching [id], or null if no [Message] has a matching [id].
+     */
+    suspend fun getMessage(id: String): Message? {
+        return messagingStorage.getMessages().find { it.id == id }
+    }
+
+    /**
+     * The [message] action needs to be examined for string substitutions
     * and any `uuid` needs to be recorded in the Glean event.
     *
     * We call this `process` as it has a side effect of logging a Glean event while it
@ -68,17 +100,6 @@ class NimbusMessagingController(
        return action.toDeepLinkSchemeUri()
    }

-    /**
-     * Called once the user has clicked on a message.
-     *
-     * This records that the message has been clicked on, but does not record a
-     * glean event. That should be done via [processMessageActionToUri].
-     */
-    suspend fun onMessageClicked(message: Message) {
-        val updatedMetadata = message.metadata.copy(pressed = true)
-        messagingStorage.updateMetadata(updatedMetadata)
-    }
-
    private fun sendDismissedMessageTelemetry(messageId: String) {
        Messaging.messageDismissed.record(Messaging.MessageDismissedExtra(messageId))
    }
--- a/app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt
+++ b/app/src/main/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddleware.kt
@ -95,7 +95,7 @@ class MessagingMiddleware(
        context.dispatch(UpdateMessages(newMessages))
        consumeMessageToShowIfNeeded(context, message)
        coroutineScope.launch {
-            controller.onMessageDismissed(message)
+            controller.onMessageDismissed(message.metadata)
        }
    }

@ -106,7 +106,7 @@ class MessagingMiddleware(
    ) {
        // Update Nimbus storage.
        coroutineScope.launch {
-            controller.onMessageClicked(message)
+            controller.onMessageClicked(message.metadata)
        }
        // Update app state.
        val newMessages = removeMessage(context, message)
--- a/app/src/main/java/org/mozilla/fenix/utils/NotificationBase.kt
+++ b/app/src/main/java/org/mozilla/fenix/utils/NotificationBase.kt
@ -19,7 +19,8 @@ fun createBaseNotification(
    channelId: String,
    title: String?,
    text: String,
-    pendingIntent: PendingIntent,
+    onClick: PendingIntent? = null,
+    onDismiss: PendingIntent? = null,
 ): Notification {
    return NotificationCompat.Builder(context, channelId)
        .setSmallIcon(R.drawable.ic_status_logo)
@ -29,7 +30,8 @@ fun createBaseNotification(
        .setColor(ContextCompat.getColor(context, R.color.primary_text_light_theme))
        .setPriority(NotificationCompat.PRIORITY_DEFAULT)
        .setShowWhen(false)
-        .setContentIntent(pendingIntent)
+        .setContentIntent(onClick)
+        .setDeleteIntent(onDismiss)
        .setAutoCancel(true)
        .build()
 }
--- a/app/src/test/java/org/mozilla/fenix/gleanplumb/NimbusMessagingControllerTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/gleanplumb/NimbusMessagingControllerTest.kt
@ -4,8 +4,10 @@

 package org.mozilla.fenix.gleanplumb

+import android.content.Intent
 import android.net.Uri
 import androidx.core.net.toUri
+import io.mockk.coEvery
 import io.mockk.coVerify
 import io.mockk.every
 import io.mockk.mockk
@ -116,7 +118,7 @@ class NimbusMessagingControllerTest {
            val message = createMessage("id-1")
            assertNull(Messaging.messageDismissed.testGetValue())

-            controller.onMessageDismissed(message)
+            controller.onMessageDismissed(message.metadata)

            assertNotNull(Messaging.messageDismissed.testGetValue())
            val event = Messaging.messageDismissed.testGetValue()!!
@ -226,12 +228,71 @@ class NimbusMessagingControllerTest {
            val message = createMessage("id-1")
            assertFalse(message.metadata.pressed)

-            controller.onMessageClicked(message)
+            controller.onMessageClicked(message.metadata)

            val updatedMetadata = message.metadata.copy(pressed = true)
            coVerify { storage.updateMetadata(updatedMetadata) }
        }

+    @Test
+    fun `WHEN getIntentForMessageAction is called THEN return a generated Intent with the processed Message action`() {
+        val message = createMessage("id-1", action = "unknown")
+        every { storage.generateUuidAndFormatAction(message.action) } returns Pair(
+            null,
+            message.action,
+        )
+
+        val actualIntent = controller.getIntentForMessageAction(message.action)
+
+        // The processed Intent data
+        assertEquals(Intent.ACTION_VIEW, actualIntent.action)
+        val expectedUri = message.action.toUri()
+        assertEquals(expectedUri, actualIntent.data)
+    }
+
+    @Test
+    fun `GIVEN stored messages contains a matching message WHEN calling getMessage THEN return the matching message`() =
+        coroutineScope.runTest {
+            val message1 = createMessage("1")
+            val message2 = createMessage("2")
+            val message3 = createMessage("3")
+            val messages = listOf(message1, message2, message3)
+            coEvery { storage.getMessages() }.returns(messages)
+
+            val actualMessage = controller.getMessage(message2.id)
+
+            assertEquals(message2, actualMessage)
+        }
+
+    @Test
+    fun `GIVEN stored messages contains multiple matching messages WHEN calling getMessage THEN return the first matching message`() =
+        coroutineScope.runTest {
+            val id = "same id"
+            val message1 = createMessage(id)
+            val message2 = createMessage(id)
+            val message3 = createMessage(id)
+            val messages = listOf(message1, message2, message3)
+            coEvery { storage.getMessages() }.returns(messages)
+
+            val actualMessage = controller.getMessage(id)
+
+            assertEquals(message1, actualMessage)
+        }
+
+    @Test
+    fun `GIVEN stored messages doesn't contain a matching message WHEN calling getMessage THEN return null`() =
+        coroutineScope.runTest {
+            val message1 = createMessage("1")
+            val message2 = createMessage("2")
+            val message3 = createMessage("3")
+            val messages = listOf(message1, message2, message3)
+            coEvery { storage.getMessages() }.returns(messages)
+
+            val actualMessage = controller.getMessage("unknown id")
+
+            assertNull(actualMessage)
+        }
+
    private fun createMessage(
        id: String,
        messageData: MessageData = MessageData(),
--- a/app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/gleanplumb/state/MessagingMiddlewareTest.kt
@ -118,7 +118,7 @@ class MessagingMiddlewareTest {

        middleware.invoke(middlewareContext, {}, MessageClicked(message))

-        coVerify { messagingController.onMessageClicked(message) }
+        coVerify { messagingController.onMessageClicked(message.metadata) }
        coVerify { messagingStorage.updateMetadata(message.metadata.copy(pressed = true)) }
        verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
    }
@ -146,7 +146,7 @@ class MessagingMiddlewareTest {
            MessageDismissed(message),
        )

-        coVerify { messagingController.onMessageDismissed(message) }
+        coVerify { messagingController.onMessageDismissed(message.metadata) }
        coVerify { messagingStorage.updateMetadata(message.metadata.copy(dismissed = true)) }
        verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
    }
@ -195,7 +195,7 @@ class MessagingMiddlewareTest {

        spiedMiddleware.onMessageDismissed(middlewareContext, message)

-        coVerify { messagingController.onMessageDismissed(message) }
+        coVerify { messagingController.onMessageDismissed(message.metadata) }
        coVerify { messagingStorage.updateMetadata(message.metadata.copy(dismissed = true)) }
        verify { middlewareContext.dispatch(UpdateMessages(emptyList())) }
        verify { spiedMiddleware.removeMessage(middlewareContext, message) }