From ce2c88512e6f91511dcacdc4c764be5b0235d1c0 Mon Sep 17 00:00:00 2001 From: Alexandru2909 Date: Wed, 13 Apr 2022 15:07:26 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/24710 - Remove Event.wrapper for RecentBookmarks telemetry --- .../mozilla/fenix/components/metrics/Event.kt | 6 ------ .../components/metrics/GleanMetricsService.kt | 16 --------------- .../controller/RecentBookmarksController.kt | 7 +++---- .../view/RecentBookmarksViewHolder.kt | 7 +++---- .../sessioncontrol/SessionControlAdapter.kt | 1 - .../SessionControlController.kt | 3 ++- .../metrics/GleanMetricsServiceTest.kt | 16 --------------- .../DefaultSessionControlControllerTest.kt | 17 ++++++++++------ .../DefaultRecentBookmarksControllerTest.kt | 20 +++++++++++++++---- 9 files changed, 35 insertions(+), 58 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 index 47829c5c87..7f6593679e 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 @@ -48,12 +48,6 @@ sealed class Event { object StartOnHomeEnterHomeScreen : Event() object StartOnHomeOpenTabsTray : Event() - // Recent bookmarks - object BookmarkClicked : Event() - object ShowAllBookmarks : Event() - object RecentBookmarksShown : Event() - data class RecentBookmarkCount(val count: Int) : Event() - // Recently visited/Recent searches object RecentSearchesGroupDeleted : 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 index db31fc0060..978dd4792c 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 @@ -16,7 +16,6 @@ import org.mozilla.fenix.GleanMetrics.HomeMenu import org.mozilla.fenix.GleanMetrics.HomeScreen import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.ProgressiveWebApp -import org.mozilla.fenix.GleanMetrics.RecentBookmarks import org.mozilla.fenix.GleanMetrics.RecentSearches import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage import org.mozilla.fenix.GleanMetrics.SearchTerms @@ -158,25 +157,10 @@ private val Event.wrapper: EventWrapper<*>? { StartOnHome.openTabsTray.record(it) } ) - is Event.BookmarkClicked -> EventWrapper( - { RecentBookmarks.bookmarkClicked.add() } - ) - - is Event.ShowAllBookmarks -> EventWrapper( - { RecentBookmarks.showAllBookmarks.add() } - ) - is Event.RecentSearchesGroupDeleted -> EventWrapper( { RecentSearches.groupDeleted.record(it) } ) - is Event.RecentBookmarksShown -> EventWrapper( - { RecentBookmarks.shown.record(it) } - ) - - is Event.RecentBookmarkCount -> EventWrapper( - { RecentBookmarks.recentBookmarksCount.set(this.count.toLong()) }, - ) is Event.SearchTermGroupCount -> EventWrapper( { SearchTerms.numberOfSearchTermGroup.record(it) }, { SearchTerms.numberOfSearchTermGroupKeys.valueOf(it) } diff --git a/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/controller/RecentBookmarksController.kt b/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/controller/RecentBookmarksController.kt index 643d1edb1f..25755a3676 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/controller/RecentBookmarksController.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/controller/RecentBookmarksController.kt @@ -11,12 +11,11 @@ import mozilla.appservices.places.BookmarkRoot import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.ALLOW_JAVASCRIPT_URL import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.RecentBookmarks import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.ext.components import org.mozilla.fenix.home.HomeFragmentDirections import org.mozilla.fenix.home.recentbookmarks.RecentBookmark import org.mozilla.fenix.home.recentbookmarks.interactor.RecentBookmarksInteractor @@ -60,11 +59,11 @@ class DefaultRecentBookmarksController( from = BrowserDirection.FromHome, flags = EngineSession.LoadUrlFlags.select(ALLOW_JAVASCRIPT_URL) ) - activity.components.core.metrics.track(Event.BookmarkClicked) + RecentBookmarks.bookmarkClicked.add() } override fun handleShowAllBookmarksClicked() { - activity.components.core.metrics.track(Event.ShowAllBookmarks) + RecentBookmarks.showAllBookmarks.add() dismissSearchDialogIfDisplayed() navController.navigate( HomeFragmentDirections.actionGlobalBookmarkFragment(BookmarkRoot.Mobile.id) diff --git a/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/view/RecentBookmarksViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/view/RecentBookmarksViewHolder.kt index 31a22b93b5..ab0395a34a 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/view/RecentBookmarksViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/view/RecentBookmarksViewHolder.kt @@ -10,22 +10,21 @@ import androidx.compose.ui.platform.ComposeView import androidx.compose.ui.res.stringResource import androidx.lifecycle.LifecycleOwner import mozilla.components.lib.state.ext.observeAsComposableState +import mozilla.components.service.glean.private.NoExtras import org.mozilla.fenix.R import org.mozilla.fenix.components.components -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.compose.ComposeViewHolder import org.mozilla.fenix.home.recentbookmarks.interactor.RecentBookmarksInteractor +import org.mozilla.fenix.GleanMetrics.RecentBookmarks as RecentBookmarksMetrics class RecentBookmarksViewHolder( composeView: ComposeView, viewLifecycleOwner: LifecycleOwner, val interactor: RecentBookmarksInteractor, - val metrics: MetricController ) : ComposeViewHolder(composeView, viewLifecycleOwner) { init { - metrics.track(Event.RecentBookmarksShown) + RecentBookmarksMetrics.shown.record(NoExtras()) } companion object { diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt index cb881458c4..c7280f1d8c 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlAdapter.kt @@ -241,7 +241,6 @@ class SessionControlAdapter( composeView = ComposeView(parent.context), viewLifecycleOwner = viewLifecycleOwner, interactor = interactor, - metrics = components.analytics.metrics ) RecentTabViewHolder.LAYOUT_ID -> return RecentTabViewHolder( composeView = ComposeView(parent.context), 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 a4306c2866..9820c21d07 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 @@ -35,6 +35,7 @@ import org.mozilla.fenix.GleanMetrics.Collections import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.Pocket +import org.mozilla.fenix.GleanMetrics.RecentBookmarks import org.mozilla.fenix.GleanMetrics.RecentTabs import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.HomeActivity @@ -654,6 +655,6 @@ class DefaultSessionControlController( RecentTabs.sectionVisible.set(true) } - metrics.track(Event.RecentBookmarkCount(state.recentBookmarks.size)) + RecentBookmarks.recentBookmarksCount.set(state.recentBookmarks.size.toLong()) } } diff --git a/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt b/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt index e2f6c8aa0d..8658a6c26e 100644 --- a/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt @@ -13,7 +13,6 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.GleanMetrics.Awesomebar -import org.mozilla.fenix.GleanMetrics.RecentBookmarks import org.mozilla.fenix.GleanMetrics.RecentlyVisitedHomepage import org.mozilla.fenix.GleanMetrics.SyncedTabs import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @@ -64,21 +63,6 @@ class GleanMetricsServiceTest { assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue()) } - @Test - fun `Home screen recent bookmarks events are correctly recorded`() { - assertFalse(RecentBookmarks.shown.testHasValue()) - gleanService.track(Event.RecentBookmarksShown) - assertTrue(RecentBookmarks.shown.testHasValue()) - - assertFalse(RecentBookmarks.bookmarkClicked.testHasValue()) - gleanService.track(Event.BookmarkClicked) - assertTrue(RecentBookmarks.bookmarkClicked.testHasValue()) - - assertFalse(RecentBookmarks.showAllBookmarks.testHasValue()) - gleanService.track(Event.ShowAllBookmarks) - assertTrue(RecentBookmarks.showAllBookmarks.testHasValue()) - } - @Test fun `Home screen recently visited events are correctly recorded`() { assertFalse(RecentlyVisitedHomepage.historyHighlightOpened.testHasValue()) 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 3078e76bef..0bb30c4ec3 100644 --- a/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/DefaultSessionControlControllerTest.kt @@ -52,6 +52,7 @@ import org.mozilla.fenix.GleanMetrics.Collections import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.Pings import org.mozilla.fenix.GleanMetrics.RecentTabs +import org.mozilla.fenix.GleanMetrics.RecentBookmarks import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R @@ -1127,10 +1128,12 @@ class DefaultSessionControlControllerTest { fun `WHEN handleReportSessionMetrics is called AND there are zero recent bookmarks THEN report Event#RecentBookmarkCount(0)`() { every { appState.recentBookmarks } returns emptyList() every { appState.recentTabs } returns emptyList() + assertFalse(RecentBookmarks.recentBookmarksCount.testHasValue()) + createController().handleReportSessionMetrics(appState) - verify { - metrics.track(Event.RecentBookmarkCount(0)) - } + + assertTrue(RecentBookmarks.recentBookmarksCount.testHasValue()) + assertEquals(0, RecentBookmarks.recentBookmarksCount.testGetValue()) } @Test @@ -1138,10 +1141,12 @@ class DefaultSessionControlControllerTest { val recentBookmark: RecentBookmark = mockk(relaxed = true) every { appState.recentBookmarks } returns listOf(recentBookmark) every { appState.recentTabs } returns emptyList() + assertFalse(RecentBookmarks.recentBookmarksCount.testHasValue()) + createController().handleReportSessionMetrics(appState) - verify { - metrics.track(Event.RecentBookmarkCount(1)) - } + + assertTrue(RecentBookmarks.recentBookmarksCount.testHasValue()) + assertEquals(1, RecentBookmarks.recentBookmarksCount.testGetValue()) } @Test diff --git a/app/src/test/java/org/mozilla/fenix/home/recentbookmarks/DefaultRecentBookmarksControllerTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentbookmarks/DefaultRecentBookmarksControllerTest.kt index d63c977d0b..8695f72515 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentbookmarks/DefaultRecentBookmarksControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentbookmarks/DefaultRecentBookmarksControllerTest.kt @@ -16,22 +16,31 @@ import kotlinx.coroutines.test.runBlockingTest import mozilla.appservices.places.BookmarkRoot import mozilla.components.concept.engine.EngineSession import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.ALLOW_JAVASCRIPT_URL +import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.rule.MainCoroutineRule +import mozilla.telemetry.glean.testing.GleanTestRule +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.BrowserDirection +import org.mozilla.fenix.GleanMetrics.RecentBookmarks 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.ext.components +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.home.HomeFragmentDirections import org.mozilla.fenix.home.recentbookmarks.controller.DefaultRecentBookmarksController @OptIn(ExperimentalCoroutinesApi::class) +@RunWith(FenixRobolectricTestRunner::class) class DefaultRecentBookmarksControllerTest { + @get:Rule + val gleanTestRule = GleanTestRule(testContext) @get:Rule val coroutinesTestRule = MainCoroutineRule() @@ -65,6 +74,7 @@ class DefaultRecentBookmarksControllerTest { every { navController.currentDestination } returns mockk { every { id } returns R.id.homeFragment } + assertFalse(RecentBookmarks.bookmarkClicked.testHasValue()) val bookmark = RecentBookmark( title = null, @@ -82,7 +92,7 @@ class DefaultRecentBookmarksControllerTest { from = BrowserDirection.FromHome ) } - verify { metrics.track(Event.BookmarkClicked) } + assertTrue(RecentBookmarks.bookmarkClicked.testHasValue()) verify(exactly = 0) { navController.navigateUp() } @@ -93,14 +103,15 @@ class DefaultRecentBookmarksControllerTest { every { navController.currentDestination } returns mockk { every { id } returns R.id.homeFragment } + assertFalse(RecentBookmarks.showAllBookmarks.testHasValue()) controller.handleShowAllBookmarksClicked() val directions = HomeFragmentDirections.actionGlobalBookmarkFragment(BookmarkRoot.Mobile.id) verify { navController.navigate(directions) - metrics.track(Event.ShowAllBookmarks) } + assertTrue(RecentBookmarks.showAllBookmarks.testHasValue()) verify(exactly = 0) { navController.navigateUp() } @@ -111,6 +122,7 @@ class DefaultRecentBookmarksControllerTest { every { navController.currentDestination } returns mockk { every { id } returns R.id.searchDialogFragment } + assertFalse(RecentBookmarks.showAllBookmarks.testHasValue()) controller.handleShowAllBookmarksClicked() @@ -120,7 +132,7 @@ class DefaultRecentBookmarksControllerTest { controller.dismissSearchDialogIfDisplayed() navController.navigateUp() navController.navigate(directions) - metrics.track(Event.ShowAllBookmarks) } + assertTrue(RecentBookmarks.showAllBookmarks.testHasValue()) } }