For #24214 - Remove Event.wrapper for BookmarksManagement metrics

app/src/main/java/org/mozilla/fenix/components/metrics/Event.kt

@@ -30,16 +30,6 @@ sealed class Event {
     object ClearedPrivateData : Event()
     object AddBookmark : Event()
     object RemoveBookmark : Event()
-    object OpenedBookmark : Event()
-    object OpenedBookmarkInNewTab : Event()
-    object OpenedBookmarksInNewTabs : Event()
-    object OpenedBookmarkInPrivateTab : Event()
-    object OpenedBookmarksInPrivateTabs : Event()
-    object EditedBookmark : Event()
-    object MovedBookmark : Event()
-    object ShareBookmark : Event()
-    object CopyBookmark : Event()
-    object AddBookmarkFolder : Event()
     object RemoveBookmarkFolder : Event()
     object RemoveBookmarks : Event()
     object CustomTabsClosed : Event()

app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt

@@ -13,7 +13,6 @@ import org.mozilla.fenix.GleanMetrics.AndroidAutofill
 import org.mozilla.fenix.GleanMetrics.AppTheme
 import org.mozilla.fenix.GleanMetrics.Autoplay
 import org.mozilla.fenix.GleanMetrics.Awesomebar
-import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.GleanMetrics.BrowserSearch
 import org.mozilla.fenix.GleanMetrics.ContextMenu
 import org.mozilla.fenix.GleanMetrics.ContextualMenu
@@ -147,45 +146,6 @@ private val Event.wrapper: EventWrapper<*>?
         is Event.DefaultBrowserNotifTapped -> EventWrapper<NoExtraKeys>(
             { Events.defaultBrowserNotifTapped.record(it) }
         )
-        is Event.OpenedBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.open.record(it) }
-        )
-        is Event.OpenedBookmarkInNewTab -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.openInNewTab.record(it) }
-        )
-        is Event.OpenedBookmarksInNewTabs -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.openInNewTabs.record(it) }
-        )
-        is Event.OpenedBookmarkInPrivateTab -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.openInPrivateTab.record(it) }
-        )
-        is Event.OpenedBookmarksInPrivateTabs -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.openInPrivateTabs.record(it) }
-        )
-        is Event.EditedBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.edited.record(it) }
-        )
-        is Event.MovedBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.moved.record(it) }
-        )
-        is Event.RemoveBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.removed.record(it) }
-        )
-        is Event.RemoveBookmarks -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.multiRemoved.record(it) }
-        )
-        is Event.ShareBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.shared.record(it) }
-        )
-        is Event.CopyBookmark -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.copied.record(it) }
-        )
-        is Event.AddBookmarkFolder -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.folderAdd.record(it) }
-        )
-        is Event.RemoveBookmarkFolder -> EventWrapper<NoExtraKeys>(
-            { BookmarksManagement.folderRemove.record(it) }
-        )
         is Event.CustomTabsMenuOpened -> EventWrapper<NoExtraKeys>(
             { CustomTab.menu.record(it) }
         )
@@ -818,6 +778,7 @@ private val Event.wrapper: EventWrapper<*>?
         is Event.AddonInstalled -> null
         is Event.SearchWidgetInstalled -> null
         is Event.SyncAuthFromSharedReuse, Event.SyncAuthFromSharedCopy -> null
+        else -> null
     }
 
 /**

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt

@@ -35,7 +35,9 @@ import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
 import mozilla.components.lib.state.ext.consumeFrom
 import mozilla.components.support.base.feature.UserInteractionHandler
+import mozilla.telemetry.glean.private.NoExtras
 import org.mozilla.fenix.FeatureFlags
+import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.NavHostActivity
 import org.mozilla.fenix.R
@@ -221,14 +223,14 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
                 openItemsInNewTab { node -> node.url }
 
                 showTabTray()
-                metrics?.track(Event.OpenedBookmarksInNewTabs)
+                BookmarksManagement.openInNewTabs.record(NoExtras())
                 true
             }
             R.id.open_bookmarks_in_private_tabs_multi_select -> {
                 openItemsInNewTab(private = true) { node -> node.url }
 
                 showTabTray()
-                metrics?.track(Event.OpenedBookmarksInPrivateTabs)
+                BookmarksManagement.openInPrivateTabs.record(NoExtras())
                 true
             }
             R.id.share_bookmark_multi_select -> {
@@ -420,8 +422,17 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
         return {
             deleteSelectedBookmarks(pendingBookmarksToDelete)
             pendingBookmarkDeletionJob = null
-            // Since this runs in a coroutine, we can't depend upon the fragment still being attached
+            when (event) {
-            metrics?.track(event)
+                is Event.RemoveBookmarkFolder ->
+                    BookmarksManagement.folderRemove.record(NoExtras())
+                is Event.RemoveBookmarks ->
+                    BookmarksManagement.multiRemoved.record(NoExtras())
+                is Event.RemoveBookmark ->
+                    BookmarksManagement.removed.record(NoExtras())
+                else -> {
+                    throw IllegalArgumentException("Illegal event type in getDeleteOperation")
+                }
+            }
             refreshBookmarks()
         }
     }

app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractor.kt

@@ -6,6 +6,8 @@ package org.mozilla.fenix.library.bookmarks
 
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
+import mozilla.telemetry.glean.private.NoExtras
+import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.browser.browsingmode.BrowsingMode
 import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.components.metrics.MetricController
@@ -53,7 +55,7 @@ class BookmarkFragmentInteractor(
         require(item.type == BookmarkNodeType.ITEM)
         item.url?.let {
             bookmarksController.handleCopyUrl(item)
-            metrics.track(Event.CopyBookmark)
+            BookmarksManagement.copied.record(NoExtras())
         }
     }
 
@@ -61,7 +63,7 @@ class BookmarkFragmentInteractor(
         require(item.type == BookmarkNodeType.ITEM)
         item.url?.let {
             bookmarksController.handleBookmarkSharing(item)
-            metrics.track(Event.ShareBookmark)
+            BookmarksManagement.shared.record(NoExtras())
         }
     }
 
@@ -69,7 +71,7 @@ class BookmarkFragmentInteractor(
         require(item.type == BookmarkNodeType.ITEM)
         item.url?.let {
             bookmarksController.handleOpeningBookmark(item, BrowsingMode.Normal)
-            metrics.track(Event.OpenedBookmarkInNewTab)
+            BookmarksManagement.openInNewTab.record(NoExtras())
         }
     }
 
@@ -77,7 +79,7 @@ class BookmarkFragmentInteractor(
         require(item.type == BookmarkNodeType.ITEM)
         item.url?.let {
             bookmarksController.handleOpeningBookmark(item, BrowsingMode.Private)
-            metrics.track(Event.OpenedBookmarkInPrivateTab)
+            BookmarksManagement.openInPrivateTab.record(NoExtras())
         }
     }
 
@@ -106,7 +108,7 @@ class BookmarkFragmentInteractor(
         Do exhaustive when (item.type) {
             BookmarkNodeType.ITEM -> {
                 bookmarksController.handleBookmarkTapped(item)
-                metrics.track(Event.OpenedBookmark)
+                BookmarksManagement.open.record(NoExtras())
             }
             BookmarkNodeType.FOLDER -> bookmarksController.handleBookmarkExpand(item)
             BookmarkNodeType.SEPARATOR -> throw IllegalStateException("Cannot open separators")

app/src/main/java/org/mozilla/fenix/library/bookmarks/addfolder/AddBookmarkFolderFragment.kt

@@ -21,8 +21,9 @@ import kotlinx.coroutines.withContext
 import mozilla.appservices.places.BookmarkRoot
 import mozilla.components.support.ktx.android.view.hideKeyboard
 import mozilla.components.support.ktx.android.view.showKeyboard
+import mozilla.telemetry.glean.private.NoExtras
+import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.R
-import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.databinding.FragmentEditBookmarkBinding
 import org.mozilla.fenix.ext.nav
 import org.mozilla.fenix.ext.requireComponents
@@ -107,7 +108,7 @@ class AddBookmarkFolderFragment : Fragment(R.layout.fragment_edit_bookmark) {
                     )
                     sharedViewModel.selectedFolder =
                         requireComponents.core.bookmarksStorage.getTree(newGuid)
-                    requireComponents.analytics.metrics.track(Event.AddBookmarkFolder)
+                    BookmarksManagement.folderAdd.record(NoExtras())
                     withContext(Main) {
                         Navigation.findNavController(requireActivity(), R.id.container)
                             .popBackStack()

app/src/main/java/org/mozilla/fenix/library/bookmarks/edit/EditBookmarkFragment.kt

@@ -34,10 +34,11 @@ import mozilla.components.concept.storage.BookmarkNodeType
 import mozilla.components.support.ktx.android.content.getColorFromAttr
 import mozilla.components.support.ktx.android.view.hideKeyboard
 import mozilla.components.support.ktx.android.view.showKeyboard
+import mozilla.telemetry.glean.private.NoExtras
+import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.NavHostActivity
 import org.mozilla.fenix.R
 import org.mozilla.fenix.components.FenixSnackbar
-import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.databinding.FragmentEditBookmarkBinding
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.getRootView
@@ -208,7 +209,7 @@ class EditBookmarkFragment : Fragment(R.layout.fragment_edit_bookmark) {
                     // Use fragment's lifecycle; the view may be gone by the time dialog is interacted with.
                     lifecycleScope.launch(IO) {
                         requireComponents.core.bookmarksStorage.deleteNode(args.guidToEdit)
-                        requireComponents.analytics.metrics.track(Event.RemoveBookmark)
+                        BookmarksManagement.removed.record(NoExtras())
 
                         launch(Main) {
                             Navigation.findNavController(requireActivity(), R.id.container)
@@ -249,13 +250,13 @@ class EditBookmarkFragment : Fragment(R.layout.fragment_edit_bookmark) {
             try {
                 requireComponents.let { components ->
                     if (title != bookmarkNode?.title || url != bookmarkNode?.url) {
-                        components.analytics.metrics.track(Event.EditedBookmark)
+                        BookmarksManagement.edited.record(NoExtras())
                     }
                     val parentGuid = sharedViewModel.selectedFolder?.guid ?: bookmarkNode!!.parentGuid
                     val parentChanged = initialParentGuid != parentGuid
                     // Only track the 'moved' event if new parent was selected.
                     if (parentChanged) {
-                        components.analytics.metrics.track(Event.MovedBookmark)
+                        BookmarksManagement.moved.record(NoExtras())
                     }
                     components.core.bookmarksStorage.updateNode(
                         args.guidToEdit,

app/src/test/java/org/mozilla/fenix/components/metrics/GleanMetricsServiceTest.kt

@@ -15,7 +15,6 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.fenix.GleanMetrics.Addons
 import org.mozilla.fenix.GleanMetrics.Awesomebar
-import org.mozilla.fenix.GleanMetrics.BookmarksManagement
 import org.mozilla.fenix.GleanMetrics.CreditCards
 import org.mozilla.fenix.GleanMetrics.Events
 import org.mozilla.fenix.GleanMetrics.History
@@ -95,61 +94,6 @@ class GleanMetricsServiceTest {
         assertTrue(Awesomebar.openedTabSuggestionClicked.testHasValue())
     }
 
-    @Test
-    fun `bookmark events are correctly recorded`() {
-        assertFalse(BookmarksManagement.open.testHasValue())
-        gleanService.track(Event.OpenedBookmark)
-        assertTrue(BookmarksManagement.open.testHasValue())
-
-        assertFalse(BookmarksManagement.openInNewTab.testHasValue())
-        gleanService.track(Event.OpenedBookmarkInNewTab)
-        assertTrue(BookmarksManagement.openInNewTab.testHasValue())
-
-        assertFalse(BookmarksManagement.openInNewTabs.testHasValue())
-        gleanService.track(Event.OpenedBookmarksInNewTabs)
-        assertTrue(BookmarksManagement.openInNewTabs.testHasValue())
-
-        assertFalse(BookmarksManagement.openInPrivateTab.testHasValue())
-        gleanService.track(Event.OpenedBookmarkInPrivateTab)
-        assertTrue(BookmarksManagement.openInPrivateTab.testHasValue())
-
-        assertFalse(BookmarksManagement.openInPrivateTabs.testHasValue())
-        gleanService.track(Event.OpenedBookmarksInPrivateTabs)
-        assertTrue(BookmarksManagement.openInPrivateTabs.testHasValue())
-
-        assertFalse(BookmarksManagement.edited.testHasValue())
-        gleanService.track(Event.EditedBookmark)
-        assertTrue(BookmarksManagement.edited.testHasValue())
-
-        assertFalse(BookmarksManagement.moved.testHasValue())
-        gleanService.track(Event.MovedBookmark)
-        assertTrue(BookmarksManagement.moved.testHasValue())
-
-        assertFalse(BookmarksManagement.removed.testHasValue())
-        gleanService.track(Event.RemoveBookmark)
-        assertTrue(BookmarksManagement.removed.testHasValue())
-
-        assertFalse(BookmarksManagement.multiRemoved.testHasValue())
-        gleanService.track(Event.RemoveBookmarks)
-        assertTrue(BookmarksManagement.multiRemoved.testHasValue())
-
-        assertFalse(BookmarksManagement.shared.testHasValue())
-        gleanService.track(Event.ShareBookmark)
-        assertTrue(BookmarksManagement.shared.testHasValue())
-
-        assertFalse(BookmarksManagement.copied.testHasValue())
-        gleanService.track(Event.CopyBookmark)
-        assertTrue(BookmarksManagement.copied.testHasValue())
-
-        assertFalse(BookmarksManagement.folderAdd.testHasValue())
-        gleanService.track(Event.AddBookmarkFolder)
-        assertTrue(BookmarksManagement.folderAdd.testHasValue())
-
-        assertFalse(BookmarksManagement.folderRemove.testHasValue())
-        gleanService.track(Event.RemoveBookmarkFolder)
-        assertTrue(BookmarksManagement.folderRemove.testHasValue())
-    }
-
     @Test
     fun `History events are correctly recorded`() {
         assertFalse(History.openedItemInNewTab.testHasValue())

app/src/test/java/org/mozilla/fenix/components/metrics/MetricControllerTest.kt

@@ -270,62 +270,6 @@ class MetricControllerTest {
         verify { marketingService1.track(Event.OpenedTabSuggestionClicked) }
     }
 
-    @Test
-    fun `tracking bookmark events should be sent to enabled service`() {
-        val controller = ReleaseMetricController(
-            listOf(marketingService1),
-            isDataTelemetryEnabled = { true },
-            isMarketingDataTelemetryEnabled = { true },
-            mockk()
-        )
-        every { marketingService1.shouldTrack(Event.AddBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.RemoveBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedBookmarkInNewTab) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedBookmarksInNewTabs) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedBookmarkInPrivateTab) } returns true
-        every { marketingService1.shouldTrack(Event.OpenedBookmarksInPrivateTabs) } returns true
-        every { marketingService1.shouldTrack(Event.EditedBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.MovedBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.ShareBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.CopyBookmark) } returns true
-        every { marketingService1.shouldTrack(Event.AddBookmarkFolder) } returns true
-        every { marketingService1.shouldTrack(Event.RemoveBookmarkFolder) } returns true
-        every { marketingService1.shouldTrack(Event.RemoveBookmarks) } returns true
-
-        controller.start(MetricServiceType.Marketing)
-
-        controller.track(Event.AddBookmark)
-        controller.track(Event.RemoveBookmark)
-        controller.track(Event.OpenedBookmark)
-        controller.track(Event.OpenedBookmarkInNewTab)
-        controller.track(Event.OpenedBookmarksInNewTabs)
-        controller.track(Event.OpenedBookmarkInPrivateTab)
-        controller.track(Event.OpenedBookmarksInPrivateTabs)
-        controller.track(Event.EditedBookmark)
-        controller.track(Event.MovedBookmark)
-        controller.track(Event.ShareBookmark)
-        controller.track(Event.CopyBookmark)
-        controller.track(Event.AddBookmarkFolder)
-        controller.track(Event.RemoveBookmarkFolder)
-        controller.track(Event.RemoveBookmarks)
-
-        verify { marketingService1.track(Event.AddBookmark) }
-        verify { marketingService1.track(Event.RemoveBookmark) }
-        verify { marketingService1.track(Event.OpenedBookmark) }
-        verify { marketingService1.track(Event.OpenedBookmarkInNewTab) }
-        verify { marketingService1.track(Event.OpenedBookmarksInNewTabs) }
-        verify { marketingService1.track(Event.OpenedBookmarkInPrivateTab) }
-        verify { marketingService1.track(Event.OpenedBookmarksInPrivateTabs) }
-        verify { marketingService1.track(Event.EditedBookmark) }
-        verify { marketingService1.track(Event.MovedBookmark) }
-        verify { marketingService1.track(Event.ShareBookmark) }
-        verify { marketingService1.track(Event.CopyBookmark) }
-        verify { marketingService1.track(Event.AddBookmarkFolder) }
-        verify { marketingService1.track(Event.RemoveBookmarkFolder) }
-        verify { marketingService1.track(Event.RemoveBookmarks) }
-    }
-
     @Test
     fun `history events should be sent to enabled service`() {
         val controller = ReleaseMetricController(

app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkFragmentInteractorTest.kt

@@ -6,18 +6,30 @@ package org.mozilla.fenix.library.bookmarks
 
 import io.mockk.mockk
 import io.mockk.verify
-import io.mockk.verifyOrder
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
+import mozilla.components.support.test.robolectric.testContext
+import mozilla.telemetry.glean.testing.GleanTestRule
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+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.BookmarksManagement
 import org.mozilla.fenix.browser.browsingmode.BrowsingMode
 import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.components.metrics.MetricController
+import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
 
 @SuppressWarnings("TooManyFunctions", "LargeClass")
+@RunWith(FenixRobolectricTestRunner::class) // For GleanTestRule
 class BookmarkFragmentInteractorTest {
 
+    @get:Rule
+    val gleanTestRule = GleanTestRule(testContext)
+
     private lateinit var interactor: BookmarkFragmentInteractor
 
     private val bookmarkController: DefaultBookmarkController = mockk(relaxed = true)
@@ -52,10 +64,10 @@ class BookmarkFragmentInteractorTest {
     fun `open a bookmark item`() {
         interactor.open(item)
 
-        verifyOrder {
+        verify { bookmarkController.handleBookmarkTapped(item) }
-            bookmarkController.handleBookmarkTapped(item)
+        assertTrue(BookmarksManagement.open.testHasValue())
-            metrics.track(Event.OpenedBookmark)
+        assertEquals(1, BookmarksManagement.open.testGetValue().size)
-        }
+        assertNull(BookmarksManagement.open.testGetValue().single().extra)
     }
 
     @Test(expected = IllegalStateException::class)
@@ -121,40 +133,40 @@ class BookmarkFragmentInteractorTest {
     fun `copy a bookmark item`() {
         interactor.onCopyPressed(item)
 
-        verifyOrder {
+        verify { bookmarkController.handleCopyUrl(item) }
-            bookmarkController.handleCopyUrl(item)
+        assertTrue(BookmarksManagement.copied.testHasValue())
-            metrics.track(Event.CopyBookmark)
+        assertEquals(1, BookmarksManagement.copied.testGetValue().size)
-        }
+        assertNull(BookmarksManagement.copied.testGetValue().single().extra)
     }
 
     @Test
     fun `share a bookmark item`() {
         interactor.onSharePressed(item)
 
-        verifyOrder {
+        verify { bookmarkController.handleBookmarkSharing(item) }
-            bookmarkController.handleBookmarkSharing(item)
+        assertTrue(BookmarksManagement.shared.testHasValue())
-            metrics.track(Event.ShareBookmark)
+        assertEquals(1, BookmarksManagement.shared.testGetValue().size)
-        }
+        assertNull(BookmarksManagement.shared.testGetValue().single().extra)
     }
 
     @Test
     fun `open a bookmark item in a new tab`() {
         interactor.onOpenInNormalTab(item)
 
-        verifyOrder {
+        verify { bookmarkController.handleOpeningBookmark(item, BrowsingMode.Normal) }
-            bookmarkController.handleOpeningBookmark(item, BrowsingMode.Normal)
+        assertTrue(BookmarksManagement.openInNewTab.testHasValue())
-            metrics.track(Event.OpenedBookmarkInNewTab)
+        assertEquals(1, BookmarksManagement.openInNewTab.testGetValue().size)
-        }
+        assertNull(BookmarksManagement.openInNewTab.testGetValue().single().extra)
     }
 
     @Test
     fun `open a bookmark item in a private tab`() {
         interactor.onOpenInPrivateTab(item)
 
-        verifyOrder {
+        verify { bookmarkController.handleOpeningBookmark(item, BrowsingMode.Private) }
-            bookmarkController.handleOpeningBookmark(item, BrowsingMode.Private)
+        assertTrue(BookmarksManagement.openInPrivateTab.testHasValue())
-            metrics.track(Event.OpenedBookmarkInPrivateTab)
+        assertEquals(1, BookmarksManagement.openInPrivateTab.testGetValue().size)
-        }
+        assertNull(BookmarksManagement.openInPrivateTab.testGetValue().single().extra)
     }
 
     @Test