From 4d0b79192def90a105795e5ecb96e15fddc2b088 Mon Sep 17 00:00:00 2001 From: Alexandru2909 Date: Wed, 6 Apr 2022 15:09:28 +0300 Subject: [PATCH] For #24638 - Remove Event.wrapper for Logins telemetry --- app/metrics.yaml | 1 + .../mozilla/fenix/components/metrics/Event.kt | 15 ----------- .../components/metrics/GleanMetricsService.kt | 26 ------------------- .../settings/logins/PasswordRevealHelper.kt | 6 ++--- .../logins/controller/LoginsListController.kt | 8 +++--- .../logins/fragment/EditLoginFragment.kt | 6 ++--- .../logins/fragment/LoginDetailFragment.kt | 10 +++---- .../fragment/SavedLoginsAuthFragment.kt | 5 ++-- .../logins/fragment/SavedLoginsFragment.kt | 1 - .../fragment/SavedLoginsSettingFragment.kt | 20 ++++++++------ .../logins/LoginsListControllerTest.kt | 21 +++++++++++---- 11 files changed, 46 insertions(+), 73 deletions(-) diff --git a/app/metrics.yaml b/app/metrics.yaml index 46efe73fd..e09866fe3 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -4726,6 +4726,7 @@ logins: A user changed their setting for asking to save logins extra_keys: setting: + type: string description: | The new setting for saving logins the user selected. Either `ask_to_save` or `never_save` 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 79b8664cc..c2e7f3e01 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 @@ -11,7 +11,6 @@ import org.mozilla.fenix.GleanMetrics.AppTheme import org.mozilla.fenix.GleanMetrics.Autoplay import org.mozilla.fenix.GleanMetrics.ContextMenu import org.mozilla.fenix.GleanMetrics.Events -import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.GleanMetrics.Pocket import org.mozilla.fenix.GleanMetrics.SearchTerms import org.mozilla.fenix.GleanMetrics.TopSites @@ -47,13 +46,6 @@ sealed class Event { object TopSiteContilePrivacy : Event() object GoogleTopSiteRemoved : Event() object BaiduTopSiteRemoved : Event() - object OpenLogins : Event() - object OpenOneLogin : Event() - object CopyLogin : Event() - object DeleteLogin : Event() - object EditLogin : Event() - object EditLoginSave : Event() - object ViewLoginPassword : Event() object PocketTopSiteClicked : Event() object PocketTopSiteRemoved : Event() object PocketHomeRecsShown : Event() @@ -190,13 +182,6 @@ sealed class Event { get() = hashMapOf(Addons.openAddonSettingKeys.addonId to addonId) } - data class SaveLoginsSettingChanged(val setting: Setting) : Event() { - enum class Setting { NEVER_SAVE, ASK_TO_SAVE } - - override val extras: Map? - get() = hashMapOf(Logins.saveLoginsSettingChangedKeys.setting to setting.name) - } - 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 9889281d3..dfefe8203 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 @@ -21,7 +21,6 @@ import org.mozilla.fenix.GleanMetrics.Events import org.mozilla.fenix.GleanMetrics.ExperimentsDefaultBrowser import org.mozilla.fenix.GleanMetrics.HomeMenu import org.mozilla.fenix.GleanMetrics.HomeScreen -import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.GleanMetrics.MediaState import org.mozilla.fenix.GleanMetrics.Metrics import org.mozilla.fenix.GleanMetrics.Pings @@ -128,31 +127,6 @@ private val Event.wrapper: EventWrapper<*>? is Event.MediaPictureInPictureState -> EventWrapper( { MediaState.pictureInPicture.record(it) } ) - is Event.OpenLogins -> EventWrapper( - { Logins.openLogins.record(it) } - ) - is Event.OpenOneLogin -> EventWrapper( - { Logins.openIndividualLogin.record(it) } - ) - is Event.CopyLogin -> EventWrapper( - { Logins.copyLogin.record(it) } - ) - is Event.ViewLoginPassword -> EventWrapper( - { Logins.viewPasswordLogin.record(it) } - ) - is Event.DeleteLogin -> EventWrapper( - { Logins.deleteSavedLogin.record(it) } - ) - is Event.EditLogin -> EventWrapper( - { Logins.openLoginEditor.record(it) } - ) - is Event.EditLoginSave -> EventWrapper( - { Logins.saveEditedLogin.record(it) } - ) - is Event.SaveLoginsSettingChanged -> EventWrapper( - { Logins.saveLoginsSettingChanged.record(it) }, - { Logins.saveLoginsSettingChangedKeys.valueOf(it) } - ) is Event.TopSiteOpenDefault -> EventWrapper( { TopSites.openDefault.record(it) } ) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/PasswordRevealHelper.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/PasswordRevealHelper.kt index b3797b455..f61edc3ad 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/PasswordRevealHelper.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/PasswordRevealHelper.kt @@ -8,9 +8,9 @@ import android.text.InputType import android.widget.ImageButton import android.widget.TextView import androidx.appcompat.content.res.AppCompatResources +import mozilla.components.service.glean.private.NoExtras +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.R -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.ext.components fun togglePasswordReveal(passwordText: TextView, revealPasswordButton: ImageButton) { val context = passwordText.context @@ -18,7 +18,7 @@ fun togglePasswordReveal(passwordText: TextView, revealPasswordButton: ImageButt if (passwordText.inputType == InputType.TYPE_TEXT_VARIATION_PASSWORD or InputType.TYPE_CLASS_TEXT ) { - context.components.analytics.metrics.track(Event.ViewLoginPassword) + Logins.viewPasswordLogin.record(NoExtras()) passwordText.inputType = InputType.TYPE_TEXT_VARIATION_VISIBLE_PASSWORD revealPasswordButton.setImageDrawable( AppCompatResources.getDrawable(context, R.drawable.mozac_ic_password_hide) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/LoginsListController.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/LoginsListController.kt index 564532104..189907070 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/LoginsListController.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/LoginsListController.kt @@ -5,9 +5,9 @@ package org.mozilla.fenix.settings.logins.controller import androidx.navigation.NavController +import mozilla.components.service.glean.private.NoExtras import org.mozilla.fenix.BrowserDirection -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.components.metrics.MetricController +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.logins.LoginsAction import org.mozilla.fenix.settings.logins.LoginsFragmentStore @@ -23,7 +23,6 @@ import org.mozilla.fenix.utils.Settings * @param navController NavController manages app navigation within a NavHost. * @param browserNavigator Controller allowing browser navigation to any Uri. * @param settings SharedPreferences wrapper for easier usage. - * @param metrics Controller that handles telemetry events. */ class LoginsListController( private val loginsFragmentStore: LoginsFragmentStore, @@ -34,12 +33,11 @@ class LoginsListController( from: BrowserDirection ) -> Unit, private val settings: Settings, - private val metrics: MetricController ) { fun handleItemClicked(item: SavedLogin) { loginsFragmentStore.dispatch(LoginsAction.LoginSelected(item)) - metrics.track(Event.OpenOneLogin) + Logins.openIndividualLogin.record(NoExtras()) navController.navigate( SavedLoginsFragmentDirections.actionSavedLoginsFragmentToLoginDetailFragment(item.guid) ) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt index 699ddbd8f..efde4dac8 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/EditLoginFragment.kt @@ -20,14 +20,14 @@ import androidx.lifecycle.lifecycleScope import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs import mozilla.components.lib.state.ext.consumeFrom +import mozilla.components.service.glean.private.NoExtras import mozilla.components.support.ktx.android.view.hideKeyboard +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.R import org.mozilla.fenix.components.StoreProvider -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.databinding.FragmentEditLoginBinding import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.redirectToReAuth -import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.settings import org.mozilla.fenix.ext.toEditable import org.mozilla.fenix.settings.logins.LoginsAction @@ -288,7 +288,7 @@ class EditLoginFragment : Fragment(R.layout.fragment_edit_login) { binding.usernameText.text.toString(), binding.passwordText.text.toString() ) - requireComponents.analytics.metrics.track(Event.EditLoginSave) + Logins.saveEditedLogin.record(NoExtras()) true } else -> false diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt index 628d58948..5e061b9f5 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt @@ -20,18 +20,18 @@ import androidx.navigation.fragment.findNavController import androidx.navigation.fragment.navArgs import com.google.android.material.snackbar.Snackbar import mozilla.components.lib.state.ext.consumeFrom +import mozilla.components.service.glean.private.NoExtras import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.SecureFragment import org.mozilla.fenix.components.FenixSnackbar import org.mozilla.fenix.components.StoreProvider -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.databinding.FragmentLoginDetailBinding import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.increaseTapArea import org.mozilla.fenix.ext.redirectToReAuth -import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.settings import org.mozilla.fenix.ext.showToolbar import org.mozilla.fenix.ext.simplifiedUrl @@ -183,7 +183,7 @@ class LoginDetailFragment : SecureFragment(R.layout.fragment_login_detail) { } private fun editLogin() { - requireComponents.analytics.metrics.track(Event.EditLogin) + Logins.openLoginEditor.record(NoExtras()) val directions = LoginDetailFragmentDirections.actionLoginDetailFragmentToEditLoginFragment( login!! @@ -199,7 +199,7 @@ class LoginDetailFragment : SecureFragment(R.layout.fragment_login_detail) { dialog.cancel() } setPositiveButton(R.string.dialog_delete_positive) { dialog: DialogInterface, _ -> - requireComponents.analytics.metrics.track(Event.DeleteLogin) + Logins.deleteSavedLogin.record(NoExtras()) interactor.onDeleteLogin(args.savedLoginId) dialog.dismiss() } @@ -221,7 +221,7 @@ class LoginDetailFragment : SecureFragment(R.layout.fragment_login_detail) { val clipboard = view.context.components.clipboardHandler clipboard.text = value showCopiedSnackbar(view.context.getString(snackbarText)) - view.context.components.analytics.metrics.track(Event.CopyLogin) + Logins.copyLogin.record(NoExtras()) } private fun showCopiedSnackbar(copiedItem: String) { diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsAuthFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsAuthFragment.kt index 3085b8377..7935461d2 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsAuthFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsAuthFragment.kt @@ -24,9 +24,10 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import mozilla.components.feature.autofill.preference.AutofillPreference import mozilla.components.service.fxa.SyncEngine +import mozilla.components.service.glean.private.NoExtras import mozilla.components.support.base.feature.ViewBoundFeatureWrapper +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.R -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.requireComponents import org.mozilla.fenix.ext.runIfFragmentIsAttached @@ -223,7 +224,7 @@ class SavedLoginsAuthFragment : PreferenceFragmentCompat() { * Called when authentication succeeds. */ private fun navigateToSavedLoginsFragment() { - context?.components?.analytics?.metrics?.track(Event.OpenLogins) + Logins.openLogins.record(NoExtras()) val directions = SavedLoginsAuthFragmentDirections.actionSavedLoginsAuthFragmentToLoginsListFragment() findNavController().navigate(directions) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsFragment.kt index 2a79d6690..0964f4537 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsFragment.kt @@ -78,7 +78,6 @@ class SavedLoginsFragment : SecureFragment() { navController = findNavController(), browserNavigator = ::openToBrowserAndLoad, settings = requireContext().settings(), - metrics = requireContext().components.analytics.metrics ) savedLoginsStorageController = SavedLoginsStorageController( diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsSettingFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsSettingFragment.kt index b781e0ee3..ea09f60da 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsSettingFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/fragment/SavedLoginsSettingFragment.kt @@ -7,10 +7,9 @@ package org.mozilla.fenix.settings.logins.fragment import android.os.Bundle import androidx.preference.Preference import androidx.preference.PreferenceFragmentCompat +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.R -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components -import org.mozilla.fenix.ext.metrics import org.mozilla.fenix.ext.showToolbar import org.mozilla.fenix.settings.RadioButtonPreference import org.mozilla.fenix.settings.SharedPreferenceUpdater @@ -35,9 +34,9 @@ class SavedLoginsSettingFragment : PreferenceFragmentCompat() { preferenceSave.onPreferenceChangeListener = object : SharedPreferenceUpdater() { override fun onPreferenceChange(preference: Preference, newValue: Any?): Boolean { if (newValue == true) { - context?.metrics?.track( - Event.SaveLoginsSettingChanged( - Event.SaveLoginsSettingChanged.Setting.ASK_TO_SAVE + Logins.saveLoginsSettingChanged.record( + Logins.SaveLoginsSettingChangedExtra( + Setting.ASK_TO_SAVE.name ) ) } @@ -54,9 +53,9 @@ class SavedLoginsSettingFragment : PreferenceFragmentCompat() { preferenceNeverSave.onPreferenceChangeListener = object : SharedPreferenceUpdater() { override fun onPreferenceChange(preference: Preference, newValue: Any?): Boolean { if (newValue == true) { - context?.metrics?.track( - Event.SaveLoginsSettingChanged( - Event.SaveLoginsSettingChanged.Setting.NEVER_SAVE + Logins.saveLoginsSettingChanged.record( + Logins.SaveLoginsSettingChangedExtra( + Setting.NEVER_SAVE.name ) ) } @@ -67,4 +66,9 @@ class SavedLoginsSettingFragment : PreferenceFragmentCompat() { } return preferenceNeverSave } + + companion object { + // Setting describing the approach of saving logins, used for telemetry + enum class Setting { NEVER_SAVE, ASK_TO_SAVE } + } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt index 2ca207d3c..3173b21f6 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt @@ -7,11 +7,17 @@ package org.mozilla.fenix.settings.logins import androidx.navigation.NavController import io.mockk.mockk import io.mockk.verifyAll +import mozilla.components.service.glean.testing.GleanTestRule +import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.BrowserDirection -import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.components.metrics.MetricController +import org.mozilla.fenix.GleanMetrics.Logins import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.settings.logins.controller.LoginsListController @@ -20,19 +26,20 @@ import org.mozilla.fenix.utils.Settings @RunWith(FenixRobolectricTestRunner::class) class LoginsListControllerTest { + @get:Rule + val gleanTestRule = GleanTestRule(testContext) + private val store: LoginsFragmentStore = mockk(relaxed = true) private val settings: Settings = mockk(relaxed = true) private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically private val navController: NavController = mockk(relaxed = true) private val browserNavigator: (String, Boolean, BrowserDirection) -> Unit = mockk(relaxed = true) - private val metrics: MetricController = mockk(relaxed = true) private val controller = LoginsListController( loginsFragmentStore = store, navController = navController, browserNavigator = browserNavigator, settings = settings, - metrics = metrics ) @Test @@ -48,16 +55,20 @@ class LoginsListControllerTest { @Test fun `handle login item clicked`() { val login: SavedLogin = mockk(relaxed = true) + assertFalse(Logins.openIndividualLogin.testHasValue()) controller.handleItemClicked(login) verifyAll { store.dispatch(LoginsAction.LoginSelected(login)) - metrics.track(Event.OpenOneLogin) navController.navigate( SavedLoginsFragmentDirections.actionSavedLoginsFragmentToLoginDetailFragment(login.guid) ) } + + assertTrue(Logins.openIndividualLogin.testHasValue()) + assertEquals(1, Logins.openIndividualLogin.testGetValue().size) + assertNull(Logins.openIndividualLogin.testGetValue().single().extra) } @Test