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

mcarare · Apr 21, 2022 · b643384 · b643384
1 parent 8cd7eda
commit b643384
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 44 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
@@ -8,21 +8,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
-    }
-
     sealed class Search
 
     internal open val extras: Map<*, String>?

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
@@ -6,9 +6,7 @@ package org.mozilla.fenix.components.metrics
 
 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.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.Pings
 import org.mozilla.fenix.ext.components
 
@@ -54,23 +52,7 @@ private class EventWrapper<T : Enum<T>>(
 @Suppress("DEPRECATION")
 // 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)
-            }
-        )
-    }
+    get() = null
 
 /**
  * Service responsible for sending the activation and installation pings.

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
@@ -39,6 +39,7 @@ 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.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
 import org.mozilla.fenix.GleanMetrics.CreditCards
 import org.mozilla.fenix.GleanMetrics.CustomTab
@@ -247,6 +248,16 @@ internal class ReleaseMetricController(
             ProgressiveWebApp.installTap.record(NoExtras())
         }
 
+        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)
@@ -363,15 +374,7 @@ internal class ReleaseMetricController(
             }
             null
         }
-        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
@@ -37,9 +39,10 @@ import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.fenix.GleanMetrics.AndroidAutofill
+import org.mozilla.fenix.GleanMetrics.Awesomebar
+import org.mozilla.fenix.GleanMetrics.BrowserSearch
 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
@@ -601,4 +604,66 @@ class MetricControllerTest {
 
         assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue())
     }
+
+    @Test
+    fun `GIVEN advertising 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,
+            "provider"
+        )
+
+        assertFalse(BrowserSearch.adClicks["provider"].testHasValue())
+        controller.run {
+            addClickedInSearchFact.process()
+        }
+        assertTrue(BrowserSearch.adClicks["provider"].testHasValue())
+        assertEquals(1, BrowserSearch.adClicks["provider"].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,
+            "provider"
+        )
+
+        assertFalse(BrowserSearch.withAds["provider"].testHasValue())
+
+        controller.run {
+            searchWithAdsOpenedFact.process()
+        }
+
+        assertTrue(BrowserSearch.withAds["provider"].testHasValue())
+        assertEquals(1, BrowserSearch.withAds["provider"].testGetValue())
+
+        // the user performed a search
+        val inContentSearchFact = Fact(
+            Component.FEATURE_SEARCH,
+            action,
+            InContentTelemetry.IN_CONTENT_SEARCH,
+            "provider"
+        )
+
+        assertFalse(BrowserSearch.inContent["provider"].testHasValue())
+
+        controller.run {
+            inContentSearchFact.process()
+        }
+
+        assertTrue(BrowserSearch.inContent["provider"].testHasValue())
+        assertEquals(1, BrowserSearch.inContent["provider"].testGetValue())
+
+        // the user performed another search
+        controller.run {
+            inContentSearchFact.process()
+        }
+
+        assertEquals(2, BrowserSearch.inContent["provider"].testGetValue())
+    }
 }