Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28605

t-p-white · 2023-01-20T14:25:07Z

Added a MessageNotificationWorker to poll Nimbus for new messages and create a notification configured using the highest priority new message (if available).

Updated the messaging.fml.yaml notification polling duration values as discussed with @jhugman and added test-notification value for messages.

Refactored DefaultMessageController handleAction fun - the intent creation logic is now in Message - tests updated accordingly.

Moved the bulk of the Notification builder logic into separate file as commonality is shared by all Workers.

Demo:

Screen.Recording.2023-01-10.at.14.26.25.mov

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-debug task.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Used by GitHub Actions.

t-p-white · 2023-01-20T14:30:08Z

app/src/main/java/org/mozilla/fenix/HomeActivity.kt

@@ -1143,8 +1153,6 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
        const val OPEN_TO_BROWSER_AND_LOAD = "open_to_browser_and_load"
        const val OPEN_TO_SEARCH = "open_to_search"
        const val PRIVATE_BROWSING_MODE = "private_browsing_mode"
-        const val EXTRA_DELETE_PRIVATE_TABS = "notification_delete_and_open"


these are unused

t-p-white · 2023-01-20T14:49:15Z

Original PR

… messages and create a notification configured using the highest priority new message (if available).

jhugman

This is looking very nice now.

I have left a few nits and questions, but not enough to slow things down with another review cycle. Please address these, then land.

Good job, Tom!

jhugman · 2023-01-23T11:41:37Z

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

+import org.mozilla.fenix.onboarding.MARKETING_CHANNEL_ID
+import org.mozilla.fenix.utils.IntentUtils
+import org.mozilla.fenix.utils.createBaseNotification


Nit: might MARKETING_CHANNEL_ID move to the same place as createBaseNotification?

I think it's more closely aligned with the helper class at the moment tbh, but we can look at it in the next issue 👍

jhugman · 2023-01-23T11:45:40Z

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

+            nextMessage?.apply {
+                if (shouldDisplayMessage()) {
+                    val nimbusMessagingController = NimbusMessagingController(messagingStorage)


Could we remove some of the nesting here? e.g.

val nextMessage = messagingStorage.getNextMessage( MessageSurfaceId.NOTIFICATION, messages ) ?: return@launch if (!nextMessage.shouldDisplay()) { return@launch } val nimbusMessagingController = NimbusMessagingController(messagingStorage) // … etc

jhugman · 2023-01-23T11:56:34Z

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

-    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)


Is this test missing?

yes, good spot!

jhugman · 2023-01-23T11:57:00Z

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

-        coVerify { storage.updateMetadata(message.metadata.copy(dismissed = true)) }
-    }
+            assertEquals(expectedMessage, controller.updateMessageAsDisplayed(message))
+        }


jhugman · 2023-01-23T11:59:33Z

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

-        val action = messagingController.processMessageAction(message)
-        handleAction(action)
-        appStore.dispatch(MessageClicked(message))
+        with(message) {


Nit: is this with necessary here?

Agree it's probably overkill now

jhugman · 2023-01-23T12:01:39Z

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

+        return Result.success()
+    }
+
+    private fun Message.shouldDisplayMessage() = metadata.displayCount == 0


Note to future self: this is temporary while awaiting EXP-3069.

jhugman · 2023-01-23T12:02:08Z

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

+
+                    // Generate the processed Message action
+                    val processedAction = nimbusMessagingController.processMessageActionToUri(this)
+                    val actionIntent = Intent(Intent.ACTION_VIEW, processedAction)


Note to future self: this is temporary while awaiting EXP-3068.

jhugman · 2023-01-23T12:20:21Z

app/src/main/java/org/mozilla/fenix/HomeActivity.kt

+                val notificationManagerCompat = NotificationManagerCompat.from(this)
+                if (notificationManagerCompat.isNotificationChannelEnabled(MARKETING_CHANNEL_ID)) {


Triple checking here: has onboarding already run by now? Has the user been asked for permission?

Also: should we be using areNotificationsEnabledSafe?

Yep 👍 line 336 showNotificationPermissionPromptIfRequired() will ensure the dialog is displayed if needed.

On a fresh install:
Application is loaded, onResume is called before showNotificationPermissionPromptIfRequired dialog is displayed - will fail at isNotificationChannelEnabled check. This will be followed by the permission dialog which once closed/dismissed will trigger onResume again where we will check for persmission.

areNotificationsEnabledSafe check is used as part of isNotificationChannelEnabled 🙂

…s messaging (mozilla-mobile/fenix#28605) * For 1809444: Added a MessageNotificationWorker to poll Nimbus for new messages and create a notification configured using the highest priority new message (if available). * For 1809444: Changes from PR review Co-authored-by: t-p-white <t-p-white> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

t-p-white added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Jan 20, 2023

t-p-white requested a review from jhugman January 20, 2023 14:25

t-p-white requested review from a team as code owners January 20, 2023 14:25

t-p-white commented Jan 20, 2023

View reviewed changes

t-p-white added needs:review PRs that need to be reviewed and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Jan 20, 2023

t-p-white force-pushed the 1809444 branch 4 times, most recently from cc0d9f3 to 6a945a8 Compare January 23, 2023 11:14

For 1809444: Added a MessageNotificationWorker to poll Nimbus for new…

5d4f362

… messages and create a notification configured using the highest priority new message (if available).

t-p-white force-pushed the 1809444 branch from dfe5a3e to 5d4f362 Compare January 23, 2023 11:35

jhugman approved these changes Jan 23, 2023

View reviewed changes

For 1809444: Changes from PR review

402ecfe

t-p-white added pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] and removed needs:review PRs that need to be reviewed labels Jan 23, 2023

Merge branch 'main' into 1809444

4775b9e

mergify bot merged commit 37411c0 into mozilla-mobile:main Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28605

Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28605

t-p-white commented Jan 20, 2023

t-p-white Jan 20, 2023

t-p-white commented Jan 20, 2023 •

edited

jhugman left a comment

jhugman Jan 23, 2023

t-p-white Jan 23, 2023

jhugman Jan 23, 2023

jhugman Jan 23, 2023

t-p-white Jan 23, 2023

jhugman Jan 23, 2023

jhugman Jan 23, 2023

t-p-white Jan 23, 2023

jhugman Jan 23, 2023

jhugman Jan 23, 2023

jhugman Jan 23, 2023

t-p-white Jan 23, 2023 •

edited

		val notificationManagerCompat = NotificationManagerCompat.from(this)
		if (notificationManagerCompat.isNotificationChannelEnabled(MARKETING_CHANNEL_ID)) {

Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28605

Bug 1809444: Add Worker to generate Notifications using Nimbus messaging #28605

Conversation

t-p-white commented Jan 20, 2023

Pull Request checklist

QA

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

Choose a reason for hiding this comment

t-p-white commented Jan 20, 2023 • edited

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-p-white Jan 23, 2023 • edited

Choose a reason for hiding this comment

t-p-white commented Jan 20, 2023 •

edited

t-p-white Jan 23, 2023 •

edited