Skip to content

Commit

Permalink
For mozilla-mobile#24210: Remove wrapper from "url entered" event.
Browse files Browse the repository at this point in the history
  • Loading branch information
mcarare committed Mar 30, 2022
1 parent 7ada2ef commit cf25445
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 29 deletions.
1 change: 1 addition & 0 deletions app/metrics.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,11 +386,6 @@ sealed class Event {
)
}

data class EnteredUrl(val autoCompleted: Boolean) : Event() {
override val extras: Map<Events.enteredUrlKeys, String>?
get() = mapOf(Events.enteredUrlKeys.autocomplete to autoCompleted.toString())
}

data class PerformedSearch(val eventSource: EventSource) : Event() {
sealed class EngineSource {
abstract val engine: SearchEngine
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,6 @@ private class EventWrapper<T : Enum<T>>(
// 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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -793,6 +803,8 @@ class DefaultSessionControlControllerTest {

@Test
fun handlePasteAndGo() {
assertFalse(Events.enteredUrl.testHasValue())

createController().handlePasteAndGo("text")

verify {
Expand All @@ -813,8 +825,8 @@ class DefaultSessionControlControllerTest {
from = BrowserDirection.FromHome,
engine = searchEngine
)
metrics.track(any<Event.EnteredUrl>())
}
assertTrue(Events.enteredUrl.testHasValue())
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,18 @@ 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
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
Expand All @@ -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
Expand All @@ -64,6 +62,9 @@ class SearchDialogControllerTest {
private lateinit var middleware: CaptureActionsMiddleware<BrowserState, BrowserAction>
private lateinit var browserStore: BrowserStore

@get:Rule
val gleanTestRule = GleanTestRule(testContext)

@Before
fun setUp() {
MockKAnnotations.init(this)
Expand All @@ -88,6 +89,7 @@ class SearchDialogControllerTest {
@Test
fun handleUrlCommitted() {
val url = "https://www.google.com/"
assertFalse(Events.enteredUrl.testHasValue())

createController().handleUrlCommitted(url)

Expand All @@ -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
Expand Down Expand Up @@ -157,6 +163,7 @@ class SearchDialogControllerTest {
@Test
fun handleMozillaUrlCommitted() {
val url = "moz://a"
assertFalse(Events.enteredUrl.testHasValue())

createController().handleUrlCommitted(url)

Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down

0 comments on commit cf25445

Please sign in to comment.