For mozilla-mobile#24758: Remove wrapper from browser search metrics.

mcarare · Apr 14, 2022 · 2bfceca · 2bfceca
1 parent dea0145
commit 2bfceca
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 41 deletions.
diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt
@@ -48,21 +48,6 @@ sealed class Event {
 
     // Interaction events with extras
 
-    data class SearchWithAds(val providerName: String) : Event() {
-        val label: String
-            get() = providerName
-    }
-
-    data class SearchAdClicked(val keyName: String) : Event() {
-        val label: String
-            get() = keyName
-    }
-
-    data class SearchInContent(val keyName: String) : Event() {
-        val label: String
-            get() = keyName
-    }
-
     object AutoPlaySettingVisited : Event()
 
     data class AutoPlaySettingChanged(val setting: AutoplaySetting) : Event() {

diff --git a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt
@@ -10,7 +10,6 @@ import mozilla.components.service.glean.private.NoExtraKeys
 import mozilla.components.support.base.log.logger.Logger
 import org.mozilla.fenix.GleanMetrics.Autoplay
 import org.mozilla.fenix.GleanMetrics.Awesomebar
-import org.mozilla.fenix.GleanMetrics.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.HomeMenu
 import org.mozilla.fenix.GleanMetrics.HomeScreen
 import org.mozilla.fenix.GleanMetrics.Pings
@@ -67,22 +66,6 @@ private class EventWrapper<T : Enum<T>>(
 // FIXME(#19967): Migrate to non-deprecated API.
 private val Event.wrapper: EventWrapper<*>?
     get() = when (this) {
-        is Event.SearchWithAds -> EventWrapper<NoExtraKeys>(
-            {
-                BrowserSearch.withAds[label].add(1)
-            }
-        )
-        is Event.SearchAdClicked -> EventWrapper<NoExtraKeys>(
-            {
-                BrowserSearch.adClicks[label].add(1)
-            }
-        )
-        is Event.SearchInContent -> EventWrapper<NoExtraKeys>(
-            {
-                BrowserSearch.inContent[label].add(1)
-            }
-        )
-
         is Event.AutoPlaySettingVisited -> EventWrapper<NoExtraKeys>(
             { Autoplay.visitedSetting.record(it) }
         )

diff --git 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.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
 import org.mozilla.fenix.GleanMetrics.CreditCards
 import org.mozilla.fenix.GleanMetrics.CustomTab
@@ -216,6 +217,16 @@ internal class ReleaseMetricController(
             }
         }
 
+        Component.FEATURE_SEARCH to AdsTelemetry.SERP_ADD_CLICKED -> {
+            BrowserSearch.adClicks[value!!].add()
+        }
+        Component.FEATURE_SEARCH to AdsTelemetry.SERP_SHOWN_WITH_ADDS -> {
+            BrowserSearch.withAds[value!!].add()
+        }
+        Component.FEATURE_SEARCH to InContentTelemetry.IN_CONTENT_SEARCH -> {
+            BrowserSearch.inContent[value!!].add()
+        }
+
         else -> {
             this.toEvent()?.also {
                 track(it)
@@ -358,15 +369,7 @@ internal class ReleaseMetricController(
         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!!)
-        }
-        Component.FEATURE_SEARCH == component && AdsTelemetry.SERP_SHOWN_WITH_ADDS == item -> {
-            Event.SearchWithAds(value!!)
-        }
-        Component.FEATURE_SEARCH == component && InContentTelemetry.IN_CONTENT_SEARCH == item -> {
-            Event.SearchInContent(value!!)
-        }
+
         else -> null
     }
 

diff --git a/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt
@@ -19,6 +19,8 @@ import mozilla.components.feature.media.facts.MediaFacts
 import mozilla.components.feature.prompts.dialog.LoginDialogFacts
 import mozilla.components.feature.prompts.facts.CreditCardAutofillDialogFacts
 import mozilla.components.feature.pwa.ProgressiveWebAppFacts
+import mozilla.components.feature.search.telemetry.ads.AdsTelemetry
+import mozilla.components.feature.search.telemetry.incontent.InContentTelemetry
 import mozilla.components.feature.syncedtabs.facts.SyncedTabsFacts
 import mozilla.components.feature.top.sites.facts.TopSitesFacts
 import mozilla.components.support.base.Component
@@ -36,6 +38,7 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.fenix.GleanMetrics.AndroidAutofill
+import org.mozilla.fenix.GleanMetrics.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
 import org.mozilla.fenix.GleanMetrics.CreditCards
 import org.mozilla.fenix.GleanMetrics.CustomTab
@@ -523,4 +526,66 @@ class MetricControllerTest {
             assertEquals(null, event.testGetValue().single().extra)
         }
     }
+
+    @Test
+    fun `GIVEN add search facts WHEN the list is processed THEN the right metric is recorded`() {
+        val controller = ReleaseMetricController(emptyList(), { true }, { false }, mockk())
+        val action = mockk<Action>()
+
+        // an ad was clicked in a Search Engine Result Page
+        val addClickedInSearchFact = Fact(
+            Component.FEATURE_SEARCH,
+            action,
+            AdsTelemetry.SERP_ADD_CLICKED,
+            "addProvider"
+        )
+
+        assertFalse(BrowserSearch.adClicks["addProvider"].testHasValue())
+        controller.run {
+            addClickedInSearchFact.process()
+        }
+        assertTrue(BrowserSearch.adClicks["addProvider"].testHasValue())
+        assertEquals(1, BrowserSearch.adClicks["addProvider"].testGetValue())
+
+        // the user opened a Search Engine Result Page of one of our search providers which contains ads
+        val searchWithAdsOpenedFact = Fact(
+            Component.FEATURE_SEARCH,
+            action,
+            AdsTelemetry.SERP_SHOWN_WITH_ADDS,
+            "addProvider"
+        )
+
+        assertFalse(BrowserSearch.withAds["addProvider"].testHasValue())
+
+        controller.run {
+            searchWithAdsOpenedFact.process()
+        }
+
+        assertTrue(BrowserSearch.withAds["addProvider"].testHasValue())
+        assertEquals(1, BrowserSearch.withAds["addProvider"].testGetValue())
+
+        // the user performed a search
+        val inContentSearchFact = Fact(
+            Component.FEATURE_SEARCH,
+            action,
+            InContentTelemetry.IN_CONTENT_SEARCH,
+            "addProvider"
+        )
+
+        assertFalse(BrowserSearch.inContent["addProvider"].testHasValue())
+
+        controller.run {
+            inContentSearchFact.process()
+        }
+
+        assertTrue(BrowserSearch.inContent["addProvider"].testHasValue())
+        assertEquals(1, BrowserSearch.inContent["addProvider"].testGetValue())
+
+        // the user performed another search
+        controller.run {
+            inContentSearchFact.process()
+        }
+
+        assertEquals(2, BrowserSearch.inContent["addProvider"].testGetValue())
+    }
 }