From 51df82acb1c7d69a16baa5a42b477645e0a4ed78 Mon Sep 17 00:00:00 2001 From: mcarare Date: Tue, 29 Mar 2022 15:28:40 +0300 Subject: [PATCH] For #24210: Remove wrapper from "search bar tapped" event. --- app/metrics.yaml | 1 + .../mozilla/fenix/components/metrics/Event.kt | 7 ----- .../components/metrics/GleanMetricsService.kt | 4 --- .../toolbar/BrowserToolbarController.kt | 3 ++- .../org/mozilla/fenix/home/HomeFragment.kt | 7 +++-- .../DefaultBrowserToolbarControllerTest.kt | 27 ++++++++++++++----- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/app/metrics.yaml b/app/metrics.yaml index d0119f1582..3c8d2e538b 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -47,6 +47,7 @@ events: description: | The view the user was on when they initiated the search (For example: `Home` or `Browser`) + type: string 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 f1c4edc4f4..f666cbb618 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 @@ -284,13 +284,6 @@ sealed class Event { get() = hashMapOf(Logins.saveLoginsSettingChangedKeys.setting to setting.name) } - data class SearchBarTapped(val source: Source) : Event() { - enum class Source { HOME, BROWSER } - - override val extras: Map? - get() = mapOf(Events.searchBarTappedKeys.source to source.name) - } - data class EnteredUrl(val autoCompleted: Boolean) : Event() { override val extras: Map? get() = mapOf(Events.enteredUrlKeys.autocomplete to autoCompleted.toString()) 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 1f708e7542..d187facb79 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 @@ -89,10 +89,6 @@ private class EventWrapper>( // FIXME(#19967): Migrate to non-deprecated API. private val Event.wrapper: EventWrapper<*>? get() = when (this) { - is Event.SearchBarTapped -> EventWrapper( - { Events.searchBarTapped.record(it) }, - { Events.searchBarTappedKeys.valueOf(it) } - ) is Event.EnteredUrl -> EventWrapper( { Events.enteredUrl.record(it) }, { Events.enteredUrlKeys.valueOf(it) } diff --git a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt index 40499ef261..e8cc890da5 100644 --- a/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt +++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt @@ -15,6 +15,7 @@ import mozilla.components.concept.engine.EngineView import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.support.ktx.kotlin.isUrl import mozilla.components.ui.tabcounter.TabCounterMenu +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.BrowserAnimator @@ -92,7 +93,7 @@ class DefaultBrowserToolbarController( } override fun handleToolbarClick() { - metrics.track(Event.SearchBarTapped(Event.SearchBarTapped.Source.BROWSER)) + Events.searchBarTapped.record(Events.SearchBarTappedExtra("BROWSER")) // If we're displaying awesomebar search results, Home screen will not be visible (it's // covered up with the search results). So, skip the navigation event in that case. // If we don't, there's a visual flickr as we navigate to Home and then display search diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index fc3bb1856e..8b9b77ff45 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -76,6 +76,7 @@ import mozilla.components.ui.tabcounter.TabCounterMenu import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.Config import org.mozilla.fenix.FeatureFlags +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.BrowserAnimator.Companion.getToolbarNavOptions @@ -491,7 +492,6 @@ class HomeFragment : Fragment() { view.resources.getDimensionPixelSize(R.dimen.search_bar_search_engine_icon_padding) binding.toolbarWrapper.setOnClickListener { navigateToSearch() - requireComponents.analytics.metrics.track(Event.SearchBarTapped(Event.SearchBarTapped.Source.HOME)) } binding.toolbarWrapper.setOnLongClickListener { @@ -865,13 +865,16 @@ class HomeFragment : Fragment() { navigateToSearch() } - private fun navigateToSearch() { + @VisibleForTesting + internal fun navigateToSearch() { val directions = HomeFragmentDirections.actionGlobalSearchDialog( sessionId = null ) nav(R.id.homeFragment, directions, getToolbarNavOptions(requireContext())) + + Events.searchBarTapped.record(Events.SearchBarTappedExtra("HOME")) } @SuppressWarnings("ComplexMethod", "LongMethod") diff --git a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt index e1557a674e..1ea7986654 100644 --- a/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/toolbar/DefaultBrowserToolbarControllerTest.kt @@ -25,17 +25,21 @@ import mozilla.components.feature.search.SearchUseCases import mozilla.components.feature.session.SessionUseCases import mozilla.components.feature.tabs.TabsUseCases import mozilla.components.feature.top.sites.TopSitesUseCases +import mozilla.components.service.glean.testing.GleanTestRule import mozilla.components.support.test.ext.joinBlocking import mozilla.components.support.test.libstate.ext.waitUntilIdle import mozilla.components.support.test.middleware.CaptureActionsMiddleware +import mozilla.components.support.test.robolectric.testContext import mozilla.components.ui.tabcounter.TabCounterMenu 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.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.BrowserAnimator @@ -91,6 +95,9 @@ class DefaultBrowserToolbarControllerTest { private lateinit var store: BrowserStore private val captureMiddleware = CaptureActionsMiddleware() + @get:Rule + val gleanTestRule = GleanTestRule(testContext) + @Before fun setUp() { MockKAnnotations.init(this) @@ -221,6 +228,8 @@ class DefaultBrowserToolbarControllerTest { @Test fun handleToolbarClick() { val controller = createController() + assertFalse(Events.searchBarTapped.testHasValue()) + controller.handleToolbarClick() val homeDirections = BrowserFragmentDirections.actionGlobalHome() @@ -228,9 +237,11 @@ class DefaultBrowserToolbarControllerTest { sessionId = "1" ) - verify { - metrics.track(Event.SearchBarTapped(Event.SearchBarTapped.Source.BROWSER)) - } + assertTrue(Events.searchBarTapped.testHasValue()) + val snapshot = Events.searchBarTapped.testGetValue() + assertEquals(1, snapshot.size) + assertEquals("BROWSER", snapshot.single().extra?.getValue("source")) + verify { // shows the home screen "behind" the search dialog navController.navigate(homeDirections) @@ -243,6 +254,8 @@ class DefaultBrowserToolbarControllerTest { val searchResultsTab = createTab("https://google.com?q=mozilla+website", searchTerms = "mozilla website") store.dispatch(TabListAction.AddTabAction(searchResultsTab, select = true)).joinBlocking() + assertFalse(Events.searchBarTapped.testHasValue()) + val controller = createController() controller.handleToolbarClick() @@ -251,9 +264,11 @@ class DefaultBrowserToolbarControllerTest { sessionId = searchResultsTab.id ) - verify { - metrics.track(Event.SearchBarTapped(Event.SearchBarTapped.Source.BROWSER)) - } + assertTrue(Events.searchBarTapped.testHasValue()) + val snapshot = Events.searchBarTapped.testGetValue() + assertEquals(1, snapshot.size) + assertEquals("BROWSER", snapshot.single().extra?.getValue("source")) + // Does not show the home screen "behind" the search dialog if the current session has search terms. verify(exactly = 0) { navController.navigate(homeDirections)