From cf254451cfceada742452f0dbf7a765ffac46ab7 Mon Sep 17 00:00:00 2001 From: mcarare Date: Tue, 29 Mar 2022 18:55:29 +0300 Subject: [PATCH] For #24210: Remove wrapper from "url entered" event. --- app/metrics.yaml | 1 + .../mozilla/fenix/components/metrics/Event.kt | 5 --- .../components/metrics/GleanMetricsService.kt | 4 --- .../SessionControlController.kt | 10 +++--- .../fenix/search/SearchDialogController.kt | 14 ++++---- .../DefaultSessionControlControllerTest.kt | 14 +++++++- .../search/SearchDialogControllerTest.kt | 33 ++++++++++++++----- 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/app/metrics.yaml b/app/metrics.yaml index 386228706a6f..beb01fb05042 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -75,6 +75,7 @@ events: description: | A boolean that tells us whether the URL was autofilled by an Autocomplete suggestion + type: boolean bugs: - https://github.com/mozilla-mobile/fenix/issues/959 - https://github.com/mozilla-mobile/fenix/issues/19923 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 index edfbb3b9611f..dc6aa5430d94 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt @@ -386,11 +386,6 @@ sealed class Event { ) } - data class EnteredUrl(val autoCompleted: Boolean) : Event() { - override val extras: Map? - get() = mapOf(Events.enteredUrlKeys.autocomplete to autoCompleted.toString()) - } - data class PerformedSearch(val eventSource: EventSource) : Event() { sealed class EngineSource { abstract val engine: SearchEngine 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 index 3be0073fb146..e12139630deb 100644 --- a/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt +++ b/app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt @@ -96,10 +96,6 @@ private class EventWrapper>( // FIXME(#19967): Migrate to non-deprecated API. private val Event.wrapper: EventWrapper<*>? get() = when (this) { - is Event.EnteredUrl -> EventWrapper( - { Events.enteredUrl.record(it) }, - { Events.enteredUrlKeys.valueOf(it) } - ) is Event.PerformedSearch -> EventWrapper( { Metrics.searchCount[this.eventSource.countLabel].add(1) diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt index 6d078d81aa2c..13d91b264706 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt @@ -29,6 +29,7 @@ import mozilla.components.support.ktx.android.view.showKeyboard import mozilla.components.support.ktx.kotlin.isUrl import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.FeatureFlags +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.HomeActivity @@ -589,20 +590,19 @@ class DefaultSessionControlController( engine = searchEngine ) - val event = if (clipboardText.isUrl() || searchEngine == null) { - Event.EnteredUrl(false) + if (clipboardText.isUrl() || searchEngine == null) { + Events.enteredUrl.record(Events.EnteredUrlExtra(autocomplete = false)) } else { val searchAccessPoint = Event.PerformedSearch.SearchAccessPoint.ACTION - searchAccessPoint.let { sap -> + val event = searchAccessPoint.let { sap -> MetricsUtils.createSearchEvent( searchEngine, store, sap ) } + event?.let { activity.metrics.track(it) } } - - event?.let { activity.metrics.track(it) } } override fun handlePaste(clipboardText: String) { diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt index 5a761e795092..38ab54c01401 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt @@ -18,6 +18,7 @@ import mozilla.components.concept.engine.EngineSession.LoadUrlFlags import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.support.ktx.kotlin.isUrl import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.SearchShortcuts import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R @@ -101,24 +102,25 @@ class SearchDialogController( requestDesktopMode = fromHomeScreen && activity.settings().openNextTabInDesktopMode ) - val event = if (url.isUrl() || searchEngine == null) { - Event.EnteredUrl(false) + if (url.isUrl() || searchEngine == null) { + Events.enteredUrl.record(Events.EnteredUrlExtra(autocomplete = false)) } else { val searchAccessPoint = when (fragmentStore.state.searchAccessPoint) { Event.PerformedSearch.SearchAccessPoint.NONE -> Event.PerformedSearch.SearchAccessPoint.ACTION else -> fragmentStore.state.searchAccessPoint } - searchAccessPoint?.let { sap -> + val event = searchAccessPoint?.let { sap -> MetricsUtils.createSearchEvent( searchEngine, store, sap ) } + event?.let { + metrics.track(it) + } } - - event?.let { metrics.track(it) } } override fun handleEditingCancelled() { @@ -157,7 +159,7 @@ class SearchDialogController( flags = flags ) - metrics.track(Event.EnteredUrl(false)) + Events.enteredUrl.record(Events.EnteredUrlExtra(autocomplete = false)) } override fun handleSearchTermsTapped(searchTerms: String) { diff --git a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt index 1e61dd1a4fde..6e54c25012fd 100644 --- a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt @@ -33,16 +33,21 @@ import mozilla.components.feature.session.SessionUseCases import mozilla.components.feature.tab.collections.TabCollection import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.feature.top.sites.TopSite +import mozilla.components.service.glean.testing.GleanTestRule import mozilla.components.support.test.ext.joinBlocking +import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.rule.MainCoroutineRule import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Ignore import org.junit.Rule import org.junit.Test +import org.junit.runner.RunWith import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.HomeActivity @@ -61,6 +66,7 @@ import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings import org.mozilla.fenix.gleanplumb.Message import org.mozilla.fenix.gleanplumb.MessageController +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.recentbookmarks.RecentBookmark import org.mozilla.fenix.home.recenttabs.RecentTab import org.mozilla.fenix.home.sessioncontrol.DefaultSessionControlController @@ -69,11 +75,15 @@ import org.mozilla.fenix.utils.Settings import mozilla.components.feature.tab.collections.Tab as ComponentTab @OptIn(ExperimentalCoroutinesApi::class) +@RunWith(FenixRobolectricTestRunner::class) // for gleanTestRule class DefaultSessionControlControllerTest { @get:Rule val coroutinesTestRule = MainCoroutineRule() + @get:Rule + val gleanTestRule = GleanTestRule(testContext) + private val activity: HomeActivity = mockk(relaxed = true) private val appStore: AppStore = mockk(relaxed = true) private val navController: NavController = mockk(relaxed = true) @@ -793,6 +803,8 @@ class DefaultSessionControlControllerTest { @Test fun handlePasteAndGo() { + assertFalse(Events.enteredUrl.testHasValue()) + createController().handlePasteAndGo("text") verify { @@ -813,8 +825,8 @@ class DefaultSessionControlControllerTest { from = BrowserDirection.FromHome, engine = searchEngine ) - metrics.track(any()) } + assertTrue(Events.enteredUrl.testHasValue()) } @Test diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt index c3c7a27ddb75..fcbf4a1c1f9a 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt @@ -29,6 +29,7 @@ import mozilla.components.support.test.middleware.CaptureActionsMiddleware import mozilla.components.support.test.robolectric.testContext import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue import org.junit.Before @@ -36,10 +37,10 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.SearchShortcuts import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -48,12 +49,9 @@ import org.mozilla.fenix.search.SearchDialogFragmentDirections.Companion.actionG import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Settings -@RunWith(FenixRobolectricTestRunner::class) // For gleanTestRule +@RunWith(FenixRobolectricTestRunner::class) // for gleanTestRule class SearchDialogControllerTest { - @get:Rule - val gleanTestRule = GleanTestRule(testContext) - @MockK(relaxed = true) private lateinit var activity: HomeActivity @MockK(relaxed = true) private lateinit var store: SearchDialogFragmentStore @MockK(relaxed = true) private lateinit var navController: NavController @@ -64,6 +62,9 @@ class SearchDialogControllerTest { private lateinit var middleware: CaptureActionsMiddleware private lateinit var browserStore: BrowserStore + @get:Rule + val gleanTestRule = GleanTestRule(testContext) + @Before fun setUp() { MockKAnnotations.init(this) @@ -88,6 +89,7 @@ class SearchDialogControllerTest { @Test fun handleUrlCommitted() { val url = "https://www.google.com/" + assertFalse(Events.enteredUrl.testHasValue()) createController().handleUrlCommitted(url) @@ -99,7 +101,11 @@ class SearchDialogControllerTest { engine = searchEngine ) } - verify { metrics.track(Event.EnteredUrl(false)) } + + assertTrue(Events.enteredUrl.testHasValue()) + val snapshot = Events.enteredUrl.testGetValue() + assertEquals(1, snapshot.size) + assertEquals("false", snapshot.single().extra?.getValue("autocomplete")) } @Test @@ -157,6 +163,7 @@ class SearchDialogControllerTest { @Test fun handleMozillaUrlCommitted() { val url = "moz://a" + assertFalse(Events.enteredUrl.testHasValue()) createController().handleUrlCommitted(url) @@ -168,7 +175,11 @@ class SearchDialogControllerTest { engine = searchEngine ) } - verify { metrics.track(Event.EnteredUrl(false)) } + + assertTrue(Events.enteredUrl.testHasValue()) + val snapshot = Events.enteredUrl.testGetValue() + assertEquals(1, snapshot.size) + assertEquals("false", snapshot.single().extra?.getValue("autocomplete")) } @Test @@ -257,6 +268,7 @@ class SearchDialogControllerTest { fun handleUrlTapped() { val url = "https://www.google.com/" val flags = EngineSession.LoadUrlFlags.all() + assertFalse(Events.enteredUrl.testHasValue()) createController().handleUrlTapped(url, flags) createController().handleUrlTapped(url) @@ -269,7 +281,12 @@ class SearchDialogControllerTest { flags = flags ) } - verify { metrics.track(Event.EnteredUrl(false)) } + + assertTrue(Events.enteredUrl.testHasValue()) + val snapshot = Events.enteredUrl.testGetValue() + assertEquals(2, snapshot.size) + assertEquals("false", snapshot.first().extra?.getValue("autocomplete")) + assertEquals("false", snapshot[1].extra?.getValue("autocomplete")) } @Test