For #24760 - Remove Event.wrapper for suggestions telemetry

2022-04-14 10:28:10 +03:00 · 2022-04-14 10:28:10 +03:00 · 5a21e75267
parent 84184e03d2
commit 5a21e75267
6 changed files with 92 additions and 164 deletions
--- a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt
+++ b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt
@ -16,14 +16,6 @@ sealed class Event {
    object HistorySearchGroupOpened : Event()
    object SearchWidgetInstalled : Event()

-    object SyncedTabSuggestionClicked : Event()
-    object BookmarkSuggestionClicked : Event()
-    object ClipboardSuggestionClicked : Event()
-    object HistorySuggestionClicked : Event()
-    object SearchActionClicked : Event()
-    object SearchSuggestionClicked : Event()
-    object OpenedTabSuggestionClicked : Event()
-
    // Interaction events with extras

    data class SearchWithAds(val providerName: String) : Event() {
--- a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt
+++ b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt
@ -8,11 +8,9 @@ import android.content.Context
 import mozilla.components.service.glean.Glean
 import mozilla.components.service.glean.private.NoExtraKeys
 import mozilla.components.support.base.log.logger.Logger
-import org.mozilla.fenix.GleanMetrics.Awesomebar
 import org.mozilla.fenix.GleanMetrics.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.Pings
 import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage
-import org.mozilla.fenix.GleanMetrics.SyncedTabs
 import org.mozilla.fenix.GleanMetrics.Messaging
 import org.mozilla.fenix.ext.components

@ -75,29 +73,6 @@ private val Event.wrapper: EventWrapper<*>?
            }
        )

-        is Event.SyncedTabSuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { SyncedTabs.syncedTabsSuggestionClicked.record(it) }
-        )
-
-        is Event.BookmarkSuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.bookmarkSuggestionClicked.record(it) }
-        )
-        is Event.ClipboardSuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.clipboardSuggestionClicked.record(it) }
-        )
-        is Event.HistorySuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.historySuggestionClicked.record(it) }
-        )
-        is Event.SearchActionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.searchActionClicked.record(it) }
-        )
-        is Event.SearchSuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.searchSuggestionClicked.record(it) }
-        )
-        is Event.OpenedTabSuggestionClicked -> EventWrapper<NoExtraKeys>(
-            { Awesomebar.openedTabSuggestionClicked.record(it) }
-        )
-
        is Event.Messaging.MessageShown -> EventWrapper<NoExtraKeys>(
            {
                Messaging.messageShown.record(
--- a/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt
+++ b/app/src/main/java/org/mozilla/fenix/components/metrics/MetricController.kt
@ -38,6 +38,7 @@ import org.mozilla.fenix.BuildConfig
 import org.mozilla.fenix.GleanMetrics.Addons
 import org.mozilla.fenix.GleanMetrics.ContextMenu
 import org.mozilla.fenix.GleanMetrics.AndroidAutofill
+import org.mozilla.fenix.GleanMetrics.Awesomebar
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
 import org.mozilla.fenix.GleanMetrics.CreditCards
 import org.mozilla.fenix.GleanMetrics.CustomTab
@ -47,6 +48,7 @@ import org.mozilla.fenix.GleanMetrics.MediaNotification
 import org.mozilla.fenix.GleanMetrics.MediaState
 import org.mozilla.fenix.GleanMetrics.PerfAwesomebar
 import org.mozilla.fenix.GleanMetrics.ProgressiveWebApp
+import org.mozilla.fenix.GleanMetrics.SyncedTabs
 import org.mozilla.fenix.search.awesomebar.ShortcutsSuggestionProvider
 import org.mozilla.fenix.utils.Settings

@ -206,6 +208,27 @@ internal class ReleaseMetricController(
                AndroidAutofill.unlockCancelled.record(NoExtras())
            }
        }
+        Component.FEATURE_SYNCEDTABS to SyncedTabsFacts.Items.SYNCED_TABS_SUGGESTION_CLICKED -> {
+            SyncedTabs.syncedTabsSuggestionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.BOOKMARK_SUGGESTION_CLICKED -> {
+            Awesomebar.bookmarkSuggestionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.CLIPBOARD_SUGGESTION_CLICKED -> {
+            Awesomebar.clipboardSuggestionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.HISTORY_SUGGESTION_CLICKED -> {
+            Awesomebar.historySuggestionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.SEARCH_ACTION_CLICKED -> {
+            Awesomebar.searchActionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.SEARCH_SUGGESTION_CLICKED -> {
+            Awesomebar.searchSuggestionClicked.record(NoExtras())
+        }
+        Component.FEATURE_AWESOMEBAR to AwesomeBarFacts.Items.OPENED_TAB_SUGGESTION_CLICKED -> {
+            Awesomebar.openedTabSuggestionClicked.record(NoExtras())
+        }
        Component.FEATURE_CONTEXTMENU to ContextMenuFacts.Items.TEXT_SELECTION_OPTION -> {
            when (metadata?.get("textSelectionOption")?.toString()) {
                CONTEXT_MENU_COPY -> ContextualMenu.copyTapped.record(NoExtras())
@ -340,27 +363,6 @@ internal class ReleaseMetricController(
            }
            null
        }
-        Component.FEATURE_SYNCEDTABS == component && SyncedTabsFacts.Items.SYNCED_TABS_SUGGESTION_CLICKED == item -> {
-            Event.SyncedTabSuggestionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.BOOKMARK_SUGGESTION_CLICKED == item -> {
-            Event.BookmarkSuggestionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.CLIPBOARD_SUGGESTION_CLICKED == item -> {
-            Event.ClipboardSuggestionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.HISTORY_SUGGESTION_CLICKED == item -> {
-            Event.HistorySuggestionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.SEARCH_ACTION_CLICKED == item -> {
-            Event.SearchActionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.SEARCH_SUGGESTION_CLICKED == item -> {
-            Event.SearchSuggestionClicked
-        }
-        Component.FEATURE_AWESOMEBAR == component && AwesomeBarFacts.Items.OPENED_TAB_SUGGESTION_CLICKED == item -> {
-            Event.OpenedTabSuggestionClicked
-        }
        Component.FEATURE_SEARCH == component && AdsTelemetry.SERP_ADD_CLICKED == item -> {
            Event.SearchAdClicked(value!!)
        }
--- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt
+++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogFragment.kt
@ -65,6 +65,7 @@ import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifAnyChanged
 import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged
 import mozilla.components.ui.autocomplete.InlineAutocompleteEditText
 import org.mozilla.fenix.BrowserDirection
+import org.mozilla.fenix.GleanMetrics.Awesomebar
 import org.mozilla.fenix.GleanMetrics.VoiceSearch
 import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.R
@ -319,7 +320,7 @@ class SearchDialogFragment : AppCompatDialogFragment(), UserInteractionHandler {
        }

        binding.fillLinkFromClipboard.setOnClickListener {
-            requireComponents.analytics.metrics.track(Event.ClipboardSuggestionClicked)
+            Awesomebar.clipboardSuggestionClicked.record(NoExtras())
            val clipboardUrl = requireContext().components.clipboardHandler.extractURL() ?: ""

            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
--- a/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt
@ -12,9 +12,7 @@ import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
-import org.mozilla.fenix.GleanMetrics.Awesomebar
 import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage
-import org.mozilla.fenix.GleanMetrics.SyncedTabs
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner

@RunWith(FenixRobolectricTestRunner::class)
@ -29,40 +27,6 @@ class GleanMetricsServiceTest {
        gleanService = GleanMetricsService(testContext)
    }

-    @Test
-    fun `synced tab event is correctly recorded`() {
-        assertFalse(SyncedTabs.syncedTabsSuggestionClicked.testHasValue())
-        gleanService.track(Event.SyncedTabSuggestionClicked)
-        assertTrue(SyncedTabs.syncedTabsSuggestionClicked.testHasValue())
-    }
-
-    @Test
-    fun `awesomebar events are correctly recorded`() {
-        assertFalse(Awesomebar.bookmarkSuggestionClicked.testHasValue())
-        gleanService.track(Event.BookmarkSuggestionClicked)
-        assertTrue(Awesomebar.bookmarkSuggestionClicked.testHasValue())
-
-        assertFalse(Awesomebar.clipboardSuggestionClicked.testHasValue())
-        gleanService.track(Event.ClipboardSuggestionClicked)
-        assertTrue(Awesomebar.clipboardSuggestionClicked.testHasValue())
-
-        assertFalse(Awesomebar.historySuggestionClicked.testHasValue())
-        gleanService.track(Event.HistorySuggestionClicked)
-        assertTrue(Awesomebar.historySuggestionClicked.testHasValue())
-
-        assertFalse(Awesomebar.searchActionClicked.testHasValue())
-        gleanService.track(Event.SearchActionClicked)
-        assertTrue(Awesomebar.searchActionClicked.testHasValue())
-
-        assertFalse(Awesomebar.searchSuggestionClicked.testHasValue())
-        gleanService.track(Event.SearchSuggestionClicked)
-        assertTrue(Awesomebar.searchSuggestionClicked.testHasValue())
-
-        assertFalse(Awesomebar.openedTabSuggestionClicked.testHasValue())
-        gleanService.track(Event.OpenedTabSuggestionClicked)
-        assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue())
-    }
-
    @Test
    fun `Home screen recently visited events are correctly recorded`() {
        assertFalse(RecentlyVisitedHomepage.historyHighlightOpened.testHasValue())
--- a/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt
@ -38,11 +38,13 @@ import org.junit.runner.RunWith
 import org.mozilla.fenix.GleanMetrics.AndroidAutofill
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
 import org.mozilla.fenix.GleanMetrics.CreditCards
+import org.mozilla.fenix.GleanMetrics.Awesomebar
 import org.mozilla.fenix.GleanMetrics.CustomTab
 import org.mozilla.fenix.GleanMetrics.LoginDialog
 import org.mozilla.fenix.GleanMetrics.MediaNotification
 import org.mozilla.fenix.GleanMetrics.ProgressiveWebApp
 import org.mozilla.fenix.components.metrics.ReleaseMetricController.Companion
+import org.mozilla.fenix.GleanMetrics.SyncedTabs
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
 import org.mozilla.fenix.utils.Settings

@ -198,56 +200,6 @@ class MetricControllerTest {
        verify(exactly = 1) { settings.topSitesSize = any() }
    }

-    @Test
-    fun `tracking synced tab event should be sent to enabled service`() {
-        val controller = ReleaseMetricController(
-            listOf(marketingService1),
-            isDataTelemetryEnabled = { true },
-            isMarketingDataTelemetryEnabled = { true },
-            mockk()
-        )
-        every { marketingService1.shouldTrack(Event.SyncedTabSuggestionClicked) } returns true
-        controller.start(MetricServiceType.Marketing)
-
-        controller.track(Event.SyncedTabSuggestionClicked)
-        verify { marketingService1.track(Event.SyncedTabSuggestionClicked) }
-    }
-
-    @Test
-    fun `tracking awesomebar events should be sent to enabled service`() {
-        val controller = ReleaseMetricController(
-            listOf(marketingService1),
-            isDataTelemetryEnabled = { true },
-            isMarketingDataTelemetryEnabled = { true },
-            mockk()
-        )
-        every { marketingService1.shouldTrack(Event.BookmarkSuggestionClicked) } returns true
-        every { marketingService1.shouldTrack(Event.ClipboardSuggestionClicked) } returns true
-        every { marketingService1.shouldTrack(Event.HistorySuggestionClicked) } returns true
-        every { marketingService1.shouldTrack(Event.SearchActionClicked) } returns true
-        every { marketingService1.shouldTrack(Event.SearchSuggestionClicked) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedTabSuggestionClicked) } returns true
-        controller.start(MetricServiceType.Marketing)
-
-        controller.track(Event.BookmarkSuggestionClicked)
-        verify { marketingService1.track(Event.BookmarkSuggestionClicked) }
-
-        controller.track(Event.ClipboardSuggestionClicked)
-        verify { marketingService1.track(Event.ClipboardSuggestionClicked) }
-
-        controller.track(Event.HistorySuggestionClicked)
-        verify { marketingService1.track(Event.HistorySuggestionClicked) }
-
-        controller.track(Event.SearchActionClicked)
-        verify { marketingService1.track(Event.SearchActionClicked) }
-
-        controller.track(Event.SearchSuggestionClicked)
-        verify { marketingService1.track(Event.SearchSuggestionClicked) }
-
-        controller.track(Event.OpenedTabSuggestionClicked)
-        verify { marketingService1.track(Event.OpenedTabSuggestionClicked) }
-    }
-
    @Test
    fun `web extension fact should set value in SharedPreference`() {
        val enabled = true
@ -279,29 +231,6 @@ class MetricControllerTest {
        assertEquals(settings.enabledAddonsList, "test2,test4")
    }

-    @Test
-    fun `WHEN changing Fact(component, item) without additional vals to events THEN it returns the right event`() {
-        // This naive test was added for a refactoring. It only covers the comparisons that were easy to add.
-        val controller = ReleaseMetricController(emptyList(), { true }, { true }, mockk())
-
-        val simpleMappings = listOf(
-            // CreditCardAutofillDialogFacts.Items is already tested.
-            Triple(Component.FEATURE_SYNCEDTABS, SyncedTabsFacts.Items.SYNCED_TABS_SUGGESTION_CLICKED, Event.SyncedTabSuggestionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.BOOKMARK_SUGGESTION_CLICKED, Event.BookmarkSuggestionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.CLIPBOARD_SUGGESTION_CLICKED, Event.ClipboardSuggestionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.HISTORY_SUGGESTION_CLICKED, Event.HistorySuggestionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.SEARCH_ACTION_CLICKED, Event.SearchActionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.SEARCH_SUGGESTION_CLICKED, Event.SearchSuggestionClicked),
-            Triple(Component.FEATURE_AWESOMEBAR, AwesomeBarFacts.Items.OPENED_TAB_SUGGESTION_CLICKED, Event.OpenedTabSuggestionClicked),
-        )
-
-        simpleMappings.forEach { (component, item, expectedEvent) ->
-            val fact = Fact(component, Action.CANCEL, item)
-            val message = "$expectedEvent $component $item"
-            assertEquals(message, expectedEvent, controller.factToEvent(fact))
-        }
-    }
-
    @Test
    fun `WHEN processing a fact with FEATURE_PROMPTS component THEN the right metric is recorded with no extras`() {
        val controller = ReleaseMetricController(emptyList(), { true }, { true }, mockk())
@ -533,4 +462,69 @@ class MetricControllerTest {

        assertTrue(ProgressiveWebApp.installTap.testHasValue())
    }
+
+    @Test
+    fun `WHEN processing a suggestion fact THEN the right metric is recorded`() {
+        val controller = ReleaseMetricController(emptyList(), { true }, { true }, mockk())
+
+        // Verify synced tabs suggestion clicked
+        assertFalse(SyncedTabs.syncedTabsSuggestionClicked.testHasValue())
+        var fact = Fact(Component.FEATURE_SYNCEDTABS, Action.CANCEL, SyncedTabsFacts.Items.SYNCED_TABS_SUGGESTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(SyncedTabs.syncedTabsSuggestionClicked.testHasValue())
+
+        // Verify bookmark suggestion clicked
+        assertFalse(Awesomebar.bookmarkSuggestionClicked.testHasValue())
+        fact = Fact(Component.FEATURE_AWESOMEBAR, Action.CANCEL, AwesomeBarFacts.Items.BOOKMARK_SUGGESTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(Awesomebar.bookmarkSuggestionClicked.testHasValue())
+
+        // Verify clipboard suggestion clicked
+        assertFalse(Awesomebar.clipboardSuggestionClicked.testHasValue())
+        fact = Fact(Component.FEATURE_AWESOMEBAR, Action.CANCEL, AwesomeBarFacts.Items.CLIPBOARD_SUGGESTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(Awesomebar.clipboardSuggestionClicked.testHasValue())
+
+        // Verify history suggestion clicked
+        assertFalse(Awesomebar.historySuggestionClicked.testHasValue())
+        fact = Fact(Component.FEATURE_AWESOMEBAR, Action.CANCEL, AwesomeBarFacts.Items.HISTORY_SUGGESTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(Awesomebar.historySuggestionClicked.testHasValue())
+
+        // Verify search action clicked
+        assertFalse(Awesomebar.searchActionClicked.testHasValue())
+        fact = Fact(Component.FEATURE_AWESOMEBAR, Action.CANCEL, AwesomeBarFacts.Items.SEARCH_ACTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(Awesomebar.searchActionClicked.testHasValue())
+
+        // Verify bookmark opened tab suggestion clicked
+        assertFalse(Awesomebar.openedTabSuggestionClicked.testHasValue())
+        fact = Fact(Component.FEATURE_AWESOMEBAR, Action.CANCEL, AwesomeBarFacts.Items.OPENED_TAB_SUGGESTION_CLICKED)
+
+        with(controller) {
+            fact.process()
+        }
+
+        assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue())
+    }
 }