Bug 1819431 - Reimplement default browser notification with Nimbus Messaging equivalent (#1031)

* Bug 1819431 - Added default-browser-notification * Bug 1819431 - Remove DefaultBrowserNotificationWorker and friends * Bug 1819431 - Add tests for all messages * Bug 1819431 - Remove settings for default browser notification * Bug 1819431 - Remove metrics for default browser notification --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
2024-11-03 23:15:31 +00:00 · 2023-03-01 16:59:39 +00:00 · 2023-03-01 16:59:39 +00:00 · 110d24a4a8
commit 110d24a4a8
parent d6230dfa43
12 changed files with 137 additions and 212 deletions
--- a/app/metrics.yaml
+++ b/app/metrics.yaml
@ -184,33 +184,6 @@ events:
    metadata:
      tags:
        - Toolbar
-  default_browser_notif_tapped:
-    type: event
-    description: |
-      User tapped on the default browser notification
-    bugs:
-      - https://github.com/mozilla-mobile/fenix/issues/19847
-    data_reviews:
-      - https://github.com/mozilla-mobile/fenix/pull/20311
-    data_sensitivity:
-      - interaction
-    notification_emails:
-      - android-probes@mozilla.com
-      - kbrosnan@mozilla.com
-    expires: never
-  default_browser_notif_shown:
-    type: event
-    description: |
-      Default browser notification was shown to the user
-    bugs:
-      - https://github.com/mozilla-mobile/fenix/issues/27779
-    data_reviews:
-      - https://github.com/mozilla-mobile/fenix/pull/27780
-    data_sensitivity:
-      - interaction
-    notification_emails:
-      - android-probes@mozilla.com
-    expires: 122
  re_engagement_notif_tapped:
    type: event
    description: |
--- a/app/src/androidTest/java/org/mozilla/fenix/ui/NimbusMessagingMessageTest.kt
+++ b/app/src/androidTest/java/org/mozilla/fenix/ui/NimbusMessagingMessageTest.kt
@ -0,0 +1,119 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.fenix.ui
+
+import android.content.Context
+import androidx.test.platform.app.InstrumentationRegistry
+import androidx.test.uiautomator.UiDevice
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Assert.fail
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
+import org.mozilla.fenix.ext.components
+import org.mozilla.fenix.gleanplumb.CustomAttributeProvider
+import org.mozilla.fenix.helpers.HomeActivityIntentTestRule
+import org.mozilla.fenix.helpers.TestHelper
+import org.mozilla.fenix.nimbus.FxNimbus
+import org.mozilla.fenix.nimbus.Messaging
+
+/**
+ * This test is to test the integrity of messages hardcoded in the FML.
+ *
+ * It tests if the trigger expressions are valid, all the fields are complete
+ * and a simple check if they are localized (don't contain `_`).
+ */
+class NimbusMessagingMessageTest {
+    private lateinit var feature: Messaging
+    private lateinit var mDevice: UiDevice
+
+    private lateinit var context: Context
+
+    private val storage
+        get() = context.components.analytics.messagingStorage
+
+    @get:Rule
+    val activityTestRule =
+        HomeActivityIntentTestRule.withDefaultSettingsOverrides(skipOnboarding = true)
+
+    @Before
+    fun setUp() {
+        context = TestHelper.appContext
+        mDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
+        feature = FxNimbus.features.messaging.value()
+    }
+
+    /**
+     * Check if all messages in the FML are internally consistent with the
+     * rest of the FML. This check is done in the `NimbusMessagingStorage`
+     * class.
+     */
+    @Test
+    fun testAllMessageIntegrity() = runTest {
+        val messages = storage.getMessages()
+        val rawMessages = feature.messages
+        assertTrue(rawMessages.isNotEmpty())
+
+        if (messages.size != rawMessages.size) {
+            val expected = rawMessages.keys.toHashSet()
+            val observed = messages.map { it.id }.toHashSet()
+            val missing = expected - observed
+            fail("Problem with message(s) in FML: $missing")
+        }
+        assertEquals(messages.size, rawMessages.size)
+    }
+
+    /**
+     * Check if the messages' triggers are well formed JEXL.
+     */
+    @Test
+    fun testAllMessageTriggers() = runTest {
+        val nimbus = context.components.analytics.experiments
+        val helper = nimbus.createMessageHelper(
+            CustomAttributeProvider.getCustomAttributes(context),
+        )
+        val messages = storage.getMessages()
+        messages.forEach { message ->
+            storage.isMessageEligible(message, helper)
+            if (storage.malFormedMap.isNotEmpty()) {
+                fail("${message.id} has a problem with its JEXL trigger: ${storage.malFormedMap.keys}")
+            }
+        }
+    }
+
+    private fun checkIsLocalized(string: String) {
+        assertFalse(string.isBlank())
+        // The check will almost always succeed, since the generated code
+        // will not compile if this is true, and there is no resource available.
+        assertFalse(string.matches(Regex("[a-z][_a-z\\d]*")))
+    }
+
+    /**
+     * Check that the messages are localized.
+     */
+    @Test
+    fun testAllMessagesAreLocalized() {
+        feature.messages.values.forEach { message ->
+            message.buttonLabel?.let(::checkIsLocalized)
+            message.title?.let(::checkIsLocalized)
+            checkIsLocalized(message.text)
+        }
+    }
+
+    @Test
+    fun testIndividualMessagesAreValid() {
+        val expectedMessages = listOf(
+            "default-browser",
+            "default-browser-notification",
+        )
+        val rawMessages = feature.messages
+        for (id in expectedMessages) {
+            assertTrue(rawMessages.containsKey(id))
+        }
+    }
+}
--- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt
+++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt
@ -113,7 +113,6 @@ import org.mozilla.fenix.library.history.HistoryFragmentDirections
 import org.mozilla.fenix.library.historymetadata.HistoryMetadataGroupFragmentDirections
 import org.mozilla.fenix.library.recentlyclosed.RecentlyClosedFragmentDirections
 import org.mozilla.fenix.nimbus.FxNimbus
-import org.mozilla.fenix.onboarding.DefaultBrowserNotificationWorker
 import org.mozilla.fenix.onboarding.FenixOnboarding
 import org.mozilla.fenix.onboarding.ReEngagementNotificationWorker
 import org.mozilla.fenix.onboarding.ensureMarketingChannelExists
@ -433,7 +432,6 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
            // that we should not rely on the application being killed between user sessions.
            components.appStore.dispatch(AppAction.ResumedMetricsAction)

-            DefaultBrowserNotificationWorker.setDefaultBrowserNotificationIfNeeded(applicationContext)
            ReEngagementNotificationWorker.setReEngagementNotificationIfNeeded(applicationContext)
            MessageNotificationWorker.setMessageNotificationWorker(applicationContext)
        }
--- a/app/src/main/java/org/mozilla/fenix/home/intent/DefaultBrowserIntentProcessor.kt
+++ b/app/src/main/java/org/mozilla/fenix/home/intent/DefaultBrowserIntentProcessor.kt
@ -14,7 +14,6 @@ import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.browser.browsingmode.BrowsingMode
 import org.mozilla.fenix.ext.openSetDefaultBrowserOption
 import org.mozilla.fenix.ext.settings
-import org.mozilla.fenix.onboarding.DefaultBrowserNotificationWorker.Companion.isDefaultBrowserNotificationIntent
 import org.mozilla.fenix.onboarding.ReEngagementNotificationWorker
 import org.mozilla.fenix.onboarding.ReEngagementNotificationWorker.Companion.isReEngagementNotificationIntent

@ -30,12 +29,6 @@ class DefaultBrowserIntentProcessor(

    override fun process(intent: Intent, navController: NavController, out: Intent): Boolean {
        return when {
-            isDefaultBrowserNotificationIntent(intent) -> {
-                Events.defaultBrowserNotifTapped.record(NoExtras())
-
-                activity.openSetDefaultBrowserOption()
-                true
-            }
            isReEngagementNotificationIntent(intent) -> {
                Events.reEngagementNotifTapped.record(NoExtras())

--- a/app/src/main/java/org/mozilla/fenix/onboarding/DefaultBrowserNotificationWorker.kt
+++ b/app/src/main/java/org/mozilla/fenix/onboarding/DefaultBrowserNotificationWorker.kt
@ -1,107 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-package org.mozilla.fenix.onboarding
-
-import android.app.Notification
-import android.app.PendingIntent
-import android.content.Context
-import android.content.Intent
-import androidx.core.app.NotificationManagerCompat
-import androidx.work.ExistingWorkPolicy
-import androidx.work.OneTimeWorkRequest
-import androidx.work.WorkManager
-import androidx.work.Worker
-import androidx.work.WorkerParameters
-import mozilla.components.service.glean.private.NoExtras
-import mozilla.components.support.base.ids.SharedIdsHelper
-import org.mozilla.fenix.GleanMetrics.Events
-import org.mozilla.fenix.HomeActivity
-import org.mozilla.fenix.R
-import org.mozilla.fenix.ext.settings
-import org.mozilla.fenix.utils.IntentUtils
-import org.mozilla.fenix.utils.Settings
-import org.mozilla.fenix.utils.createBaseNotification
-import java.util.concurrent.TimeUnit
-
-class DefaultBrowserNotificationWorker(
-    context: Context,
-    workerParameters: WorkerParameters,
-) : Worker(context, workerParameters) {
-
-    override fun doWork(): Result {
-        val channelId = ensureMarketingChannelExists(applicationContext)
-
-        NotificationManagerCompat.from(applicationContext)
-            .notify(
-                NOTIFICATION_TAG,
-                DEFAULT_BROWSER_NOTIFICATION_ID,
-                buildNotification(channelId),
-            )
-
-        Events.defaultBrowserNotifShown.record(NoExtras())
-
-        // default browser notification should only happen once
-        applicationContext.settings().defaultBrowserNotificationDisplayed = true
-
-        return Result.success()
-    }
-
-    /**
-     * Build the default browser notification.
-     */
-    private fun buildNotification(channelId: String): Notification {
-        val intent = Intent(applicationContext, HomeActivity::class.java)
-        intent.putExtra(INTENT_DEFAULT_BROWSER_NOTIFICATION, true)
-
-        val pendingIntent = PendingIntent.getActivity(
-            applicationContext,
-            SharedIdsHelper.getNextIdForTag(applicationContext, NOTIFICATION_PENDING_INTENT_TAG),
-            intent,
-            IntentUtils.defaultIntentPendingFlags,
-        )
-
-        with(applicationContext) {
-            val appName = getString(R.string.app_name)
-            return createBaseNotification(
-                this,
-                channelId,
-                getString(R.string.notification_default_browser_title, appName),
-                getString(R.string.notification_default_browser_text, appName),
-                pendingIntent,
-            )
-        }
-    }
-
-    companion object {
-        private const val NOTIFICATION_PENDING_INTENT_TAG = "org.mozilla.fenix.default.browser"
-        private const val INTENT_DEFAULT_BROWSER_NOTIFICATION = "org.mozilla.fenix.default.browser.intent"
-        private const val NOTIFICATION_TAG = "org.mozilla.fenix.default.browser.tag"
-        private const val NOTIFICATION_WORK_NAME = "org.mozilla.fenix.default.browser.work"
-        private const val NOTIFICATION_DELAY = Settings.THREE_DAYS_MS
-
-        fun isDefaultBrowserNotificationIntent(intent: Intent) =
-            intent.extras?.containsKey(INTENT_DEFAULT_BROWSER_NOTIFICATION) ?: false
-
-        fun setDefaultBrowserNotificationIfNeeded(context: Context) {
-            val instanceWorkManager = WorkManager.getInstance(context)
-
-            if (!context.settings().shouldShowDefaultBrowserNotification()) {
-                // cancel notification work if already default browser
-                instanceWorkManager.cancelUniqueWork(NOTIFICATION_WORK_NAME)
-                return
-            }
-
-            val notificationWork = OneTimeWorkRequest.Builder(DefaultBrowserNotificationWorker::class.java)
-                .setInitialDelay(NOTIFICATION_DELAY, TimeUnit.MILLISECONDS)
-                .build()
-
-            instanceWorkManager.beginUniqueWork(
-                NOTIFICATION_WORK_NAME,
-                ExistingWorkPolicy.KEEP,
-                notificationWork,
-            ).enqueue()
-        }
-    }
-}
--- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt
+++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt
@ -621,15 +621,6 @@ class Settings(private val appContext: Context) : PreferencesHolder {
        return browsers.isDefaultBrowser
    }

-    var defaultBrowserNotificationDisplayed by booleanPreference(
-        appContext.getPreferenceKey(R.string.pref_key_should_show_default_browser_notification),
-        default = false,
-    )
-
-    fun shouldShowDefaultBrowserNotification(): Boolean {
-        return !defaultBrowserNotificationDisplayed && !isDefaultBrowserBlocking()
-    }
-
    var reEngagementNotificationShown by booleanPreference(
        appContext.getPreferenceKey(R.string.pref_key_re_engagement_notification_shown),
        default = false,
--- a/app/src/main/res/values/preference_keys.xml
+++ b/app/src/main/res/values/preference_keys.xml
@ -66,7 +66,6 @@
    <string name="pref_key_last_review_prompt_shown_time" translatable="false">pref_key_last_review_prompt_shown_time</string>
    <string name="pref_key_last_browse_activity_time" translatable="false">pref_key_last_browse_activity_time</string>
    <string name="pref_key_last_cfr_shown_time" translatable="false">pref_key_last_cfr_shown_time</string>
-    <string name="pref_key_should_show_default_browser_notification" translatable="false">pref_key_should_show_default_browser_notification</string>
    <string name="pref_key_re_engagement_notification_shown" translatable="false">pref_key_re_engagement_notification_shown</string>
    <string name="pref_key_re_engagement_notification_enabled" translatable="false">pref_key_re_engagement_notification_enabled</string>
    <string name="pref_key_is_first_run" translatable="false">pref_key_is_first_run</string>
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@ -1159,11 +1159,11 @@
    <!-- Name of the marketing notification channel. Displayed in the "App notifications" system settings for the app -->
    <string name="notification_marketing_channel_name">Marketing</string>
    <!-- Title shown in the notification that pops up to remind the user to set fenix as default browser.
-    %1$s is a placeholder that will be replaced by the app name (Fenix). -->
-    <string name="notification_default_browser_title">%1$s is fast and private</string>
+    The app name is in the text, due to limitations with localizing Nimbus experiments -->
+    <string name="nimbus_notification_default_browser_title" tools:ignore="UnusedResources">Firefox is fast and private</string>
    <!-- Text shown in the notification that pops up to remind the user to set fenix as default browser.
-    %1$s is a placeholder that will be replaced by the app name (Fenix). -->
-    <string name="notification_default_browser_text">Make %1$s your default browser</string>
+    The app name is in the text, due to limitations with localizing Nimbus experiments -->
+    <string name="nimbus_notification_default_browser_text" tools:ignore="UnusedResources">Make Firefox your default browser</string>
    <!-- Title shown in the notification that pops up to re-engage the user -->
    <string name="notification_re_engagement_title">Try private browsing</string>
    <!-- Text shown in the notification that pops up to re-engage the user.
--- a/app/src/test/java/org/mozilla/fenix/home/intent/DefaultBrowserIntentProcessorTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/home/intent/DefaultBrowserIntentProcessorTest.kt
@ -44,30 +44,6 @@ class DefaultBrowserIntentProcessorTest {
        verify { out wasNot Called }
    }

-    @Test
-    fun `process default browser notification intents`() {
-        val navController: NavController = mockk(relaxed = true)
-        val out: Intent = mockk()
-        val activity: HomeActivity = mockk()
-
-        val intent = Intent().apply {
-            putExtra("org.mozilla.fenix.default.browser.intent", true)
-        }
-        every { activity.startActivity(any()) } returns Unit
-        every { activity.applicationContext } returns testContext
-
-        assertNull(Events.defaultBrowserNotifTapped.testGetValue())
-
-        val result = DefaultBrowserIntentProcessor(activity)
-            .process(intent, navController, out)
-
-        assert(result)
-
-        assertNotNull(Events.defaultBrowserNotifTapped.testGetValue())
-        verify { navController wasNot Called }
-        verify { out wasNot Called }
-    }
-
    @Test
    fun `process re-engagement notification intents`() {
        val navController: NavController = mockk(relaxed = true)
--- a/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt
@ -748,26 +748,6 @@ class SettingsTest {
        assertFalse(localSetting.shouldStartOnHome())
    }

-    @Test
-    fun `GIVEN shownDefaultBrowserNotification and isDefaultBrowser WHEN calling shouldShowDefaultBrowserNotification THEN return correct value`() {
-        val localSetting = spyk(settings)
-        every { localSetting.isDefaultBrowserBlocking() } returns false
-
-        localSetting.defaultBrowserNotificationDisplayed = false
-        assert(localSetting.shouldShowDefaultBrowserNotification())
-
-        localSetting.defaultBrowserNotificationDisplayed = true
-        assertFalse(localSetting.shouldShowDefaultBrowserNotification())
-
-        every { localSetting.isDefaultBrowserBlocking() } returns true
-
-        localSetting.defaultBrowserNotificationDisplayed = false
-        assertFalse(localSetting.shouldShowDefaultBrowserNotification())
-
-        localSetting.defaultBrowserNotificationDisplayed = true
-        assertFalse(localSetting.shouldShowDefaultBrowserNotification())
-    }
-
    @Test
    fun `GIVEN re-engagement notification shown and number of app launch THEN should set re-engagement notification returns correct value`() {
        val localSetting = spyk(settings)
--- a/detekt-baseline.xml
+++ b/detekt-baseline.xml
@ -858,7 +858,6 @@
    <ID>UndocumentedPublicFunction:Settings.kt$Settings$fun setDeleteDataOnQuit(type: DeleteBrowsingDataOnQuitType, value: Boolean)</ID>
    <ID>UndocumentedPublicFunction:Settings.kt$Settings$fun setSitePermissionSettingListener(lifecycleOwner: LifecycleOwner, listener: () -&gt; Unit)</ID>
    <ID>UndocumentedPublicFunction:Settings.kt$Settings$fun shouldDeleteAnyDataOnQuit()</ID>
-    <ID>UndocumentedPublicFunction:Settings.kt$Settings$fun shouldShowDefaultBrowserNotification(): Boolean</ID>
    <ID>UndocumentedPublicFunction:ShareCloseView.kt$ShareCloseInteractor$fun onShareClosed()</ID>
    <ID>UndocumentedPublicFunction:ShareCloseView.kt$ShareCloseView$fun setTabs(tabs: List&lt;ShareData&gt;)</ID>
    <ID>UndocumentedPublicFunction:ShareController.kt$DefaultShareController$@VisibleForTesting fun getShareText()</ID>
--- a/messaging.fml.yaml
+++ b/messaging.fml.yaml
@ -70,6 +70,11 @@ features:
            DEVICE_IOS: os == 'iOS'
            ALWAYS: "true"
            NEVER: "false"
+            DAY_1_AFTER_INSTALL: days_since_install == 1
+            DAY_2_AFTER_INSTALL: days_since_install == 2
+            DAY_3_AFTER_INSTALL: days_since_install == 3
+            DAY_4_AFTER_INSTALL: days_since_install == 4
+            DAY_5_AFTER_INSTALL: days_since_install == 5

            # Using custom attributes for the browser
            I_AM_DEFAULT_BROWSER: "is_default_browser"
@ -144,6 +149,15 @@ features:
              trigger: [ "I_AM_NOT_DEFAULT_BROWSER","USER_ESTABLISHED_INSTALL" ]
              style: "PERSISTENT"
              button-label: preferences_set_as_default_browser
+            default-browser-notification:
+              title: nimbus_notification_default_browser_title
+              text: nimbus_notification_default_browser_text
+              surface: notification
+              style: NOTIFICATION
+              trigger:
+                - I_AM_NOT_DEFAULT_BROWSER
+                - DAY_3_AFTER_INSTALL
+              action: MAKE_DEFAULT_BROWSER

      - channel: developer
        value:
@ -156,16 +170,6 @@ features:
              max-display-count: 1
          notification-config:
            refresh-interval: 120 # minutes (2 hours)
-          messages:
-            default-browser-notification:
-              title: preferences_set_as_default_browser
-              text: default_browser_experiment_card_text
-              surface: notification
-              style: NOTIFICATION
-              action: MAKE_DEFAULT_BROWSER
-              trigger:
-                - I_AM_NOT_DEFAULT_BROWSER
-                - INACTIVE_2_DAYS

 objects:
  MessageData: