From 32cdbfa9331b76e835303a8c530cdaed1044344e Mon Sep 17 00:00:00 2001 From: Abhay Sood Date: Thu, 16 Nov 2023 16:38:45 +0530 Subject: [PATCH] fix(android): bugfixes for different SDK versions --- .../measure/android/fakes/FakeEventTracker.kt | 6 +++ .../sh/measure/android/events/EventTracker.kt | 2 +- .../network_change/NetworkChangesCollector.kt | 30 +++++++++---- .../sh/measure/android/session/Resource.kt | 2 +- .../android/session/ResourceFactory.kt | 16 ++++--- .../android/fakes/FakeResourceFactory.kt | 2 +- .../NetworkChangesCollectorTest.kt | 43 ++++++++++++++++--- 7 files changed, 78 insertions(+), 23 deletions(-) diff --git a/measure-android/measure/src/androidTest/java/sh/measure/android/fakes/FakeEventTracker.kt b/measure-android/measure/src/androidTest/java/sh/measure/android/fakes/FakeEventTracker.kt index 78a7e8eca..da362aaf8 100644 --- a/measure-android/measure/src/androidTest/java/sh/measure/android/fakes/FakeEventTracker.kt +++ b/measure-android/measure/src/androidTest/java/sh/measure/android/fakes/FakeEventTracker.kt @@ -12,6 +12,7 @@ import sh.measure.android.gestures.ScrollEvent import sh.measure.android.lifecycle.ActivityLifecycleEvent import sh.measure.android.lifecycle.ApplicationLifecycleEvent import sh.measure.android.lifecycle.FragmentLifecycleEvent +import sh.measure.android.network_change.NetworkChangeEvent @Suppress("MemberVisibilityCanBePrivate") internal class FakeEventTracker: EventTracker { @@ -25,6 +26,7 @@ internal class FakeEventTracker: EventTracker { val trackedApplicationLifecycleEvents = mutableListOf() val trackedColdLaunchEvents = mutableListOf() val trackedWarmLaunchEvents = mutableListOf() + val trackedNetworkChangeEvents = mutableListOf() val trackedHotLaunchEvents = mutableListOf() val trackedAttachments = mutableListOf() @@ -72,6 +74,10 @@ internal class FakeEventTracker: EventTracker { trackedHotLaunchEvents.add(event) } + override fun trackNetworkChange(event: NetworkChangeEvent) { + trackedNetworkChangeEvents.add(event) + } + override fun storeAttachment(attachmentInfo: AttachmentInfo) { trackedAttachments.add(attachmentInfo) } diff --git a/measure-android/measure/src/main/java/sh/measure/android/events/EventTracker.kt b/measure-android/measure/src/main/java/sh/measure/android/events/EventTracker.kt index 63be68e68..5a5030a38 100644 --- a/measure-android/measure/src/main/java/sh/measure/android/events/EventTracker.kt +++ b/measure-android/measure/src/main/java/sh/measure/android/events/EventTracker.kt @@ -101,7 +101,7 @@ internal class MeasureEventTracker( } override fun trackNetworkChange(event: NetworkChangeEvent) { - logger.log(LogLevel.Debug, "Tracking network change $event") + logger.log(LogLevel.Error, "Tracking network change ${event.network_generation}, ${event.network_type}, ${event.carrier_name}") sessionController.storeEvent(event.toEvent()) } diff --git a/measure-android/measure/src/main/java/sh/measure/android/network_change/NetworkChangesCollector.kt b/measure-android/measure/src/main/java/sh/measure/android/network_change/NetworkChangesCollector.kt index 74e6cdd66..33adbdcef 100644 --- a/measure-android/measure/src/main/java/sh/measure/android/network_change/NetworkChangesCollector.kt +++ b/measure-android/measure/src/main/java/sh/measure/android/network_change/NetworkChangesCollector.kt @@ -105,7 +105,21 @@ internal class NetworkChangesCollector( val previousNetworkType = currentNetworkType val previousNetworkGeneration = currentNetworkGeneration val newNetworkGeneration = getNetworkGenerationIfAvailable(newNetworkType) + val carrierName = getNetworkOperatorName(newNetworkType) + // for Android O+, the callback is called as soon as it's registered. However, we + // only want to track changes. + // This also means, the first change event will contain non-null previous states + // for Android O+. + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + if (currentNetworkType == null) { + currentNetworkType = newNetworkType + currentNetworkGeneration = newNetworkGeneration + return + } + } + + // only track significant changes if (!shouldTrackNetworkChange( newNetworkType, previousNetworkType, @@ -115,7 +129,7 @@ internal class NetworkChangesCollector( ) { return } - val carrierName = getNetworkOperatorName(newNetworkType) + eventTracker.trackNetworkChange( NetworkChangeEvent( previous_network_type = previousNetworkType, @@ -170,21 +184,19 @@ internal class NetworkChangesCollector( newNetworkGeneration: String?, previousNetworkGeneration: String? ): Boolean { - when { - previousNetworkType == null || previousNetworkType == NetworkType.NO_NETWORK -> { - return true - } - + return when { + // track if network type has changed previousNetworkType != newNetworkType -> { - return true + true } + // track if network type is cellular, but network generation has changed newNetworkType == NetworkType.CELLULAR && newNetworkGeneration != null && newNetworkGeneration != previousNetworkGeneration -> { - return true + true } else -> { - return false + false } } } diff --git a/measure-android/measure/src/main/java/sh/measure/android/session/Resource.kt b/measure-android/measure/src/main/java/sh/measure/android/session/Resource.kt index 91a20257f..7e65ad5e8 100644 --- a/measure-android/measure/src/main/java/sh/measure/android/session/Resource.kt +++ b/measure-android/measure/src/main/java/sh/measure/android/session/Resource.kt @@ -29,6 +29,6 @@ data class Resource( val app_unique_id: String? = null, // package name, val network_type: String? = null, val network_generation: String? = null, - val carrier_name: String? = null, + val network_carrier_name: String? = null, val measure_sdk_version: String? = null, ) diff --git a/measure-android/measure/src/main/java/sh/measure/android/session/ResourceFactory.kt b/measure-android/measure/src/main/java/sh/measure/android/session/ResourceFactory.kt index 8318e2517..2c54c7dce 100644 --- a/measure-android/measure/src/main/java/sh/measure/android/session/ResourceFactory.kt +++ b/measure-android/measure/src/main/java/sh/measure/android/session/ResourceFactory.kt @@ -62,7 +62,7 @@ internal class ResourceFactoryImpl( app_unique_id = context.packageName, network_type = networkType, network_generation = getNetworkGeneration(networkType), - carrier_name = getCarrierName(networkType), + network_carrier_name = getCarrierName(networkType), measure_sdk_version = getMeasureVersion(), ) } @@ -71,6 +71,7 @@ internal class ResourceFactoryImpl( if (networkType != NetworkType.CELLULAR) return null systemServiceProvider.telephonyManager?.networkOperatorName.let { if (it.isNullOrBlank()) return null + logger.log(LogLevel.Error, "Carrier name: $it") return it } } @@ -128,9 +129,10 @@ internal class ResourceFactoryImpl( private fun getNetworkGeneration(networkType: String?): String? { if (networkType != NetworkType.CELLULAR) return null return if (hasPhoneStatePermission(context)) { - systemServiceProvider.telephonyManager?.getNetworkGeneration() + val networkGeneration = systemServiceProvider.telephonyManager?.getNetworkGeneration() + logger.log(LogLevel.Error, "Network generation: $networkGeneration") + networkGeneration } else { - logger.log(LogLevel.Debug, "No permission to access phone state") // TODO improve msg null } } @@ -142,9 +144,13 @@ internal class ResourceFactoryImpl( return null } return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - getNetworkTypeAboveApi23(connectivityManager) + val networkTypeAboveApi23 = getNetworkTypeAboveApi23(connectivityManager) + logger.log(LogLevel.Error, "Network type: $networkTypeAboveApi23") + networkTypeAboveApi23 } else { - getNetworkTypeBelowApi23(connectivityManager) + val networkTypeBelowApi23 = getNetworkTypeBelowApi23(connectivityManager) + logger.log(LogLevel.Error, "Network type: $networkTypeBelowApi23") + networkTypeBelowApi23 } } diff --git a/measure-android/measure/src/test/java/sh/measure/android/fakes/FakeResourceFactory.kt b/measure-android/measure/src/test/java/sh/measure/android/fakes/FakeResourceFactory.kt index 46f5f7ab2..cff18a232 100644 --- a/measure-android/measure/src/test/java/sh/measure/android/fakes/FakeResourceFactory.kt +++ b/measure-android/measure/src/test/java/sh/measure/android/fakes/FakeResourceFactory.kt @@ -32,6 +32,6 @@ private fun fakeResource() = Resource( app_unique_id = "app_unique_id", network_type = NetworkType.WIFI, network_generation = NetworkGeneration.FIFTH_GEN, - carrier_name = "Android", + network_carrier_name = "Android", measure_sdk_version = "measure_sdk_version" ) \ No newline at end of file diff --git a/measure-android/measure/src/test/java/sh/measure/android/network_change/NetworkChangesCollectorTest.kt b/measure-android/measure/src/test/java/sh/measure/android/network_change/NetworkChangesCollectorTest.kt index b46bb542b..0fbc04cd3 100644 --- a/measure-android/measure/src/test/java/sh/measure/android/network_change/NetworkChangesCollectorTest.kt +++ b/measure-android/measure/src/test/java/sh/measure/android/network_change/NetworkChangesCollectorTest.kt @@ -5,6 +5,7 @@ import android.app.Application import android.content.Context import android.net.ConnectivityManager import android.net.NetworkCapabilities +import android.os.Build import android.telephony.TelephonyManager import androidx.test.ext.junit.runners.AndroidJUnit4 import org.junit.Assert @@ -12,6 +13,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.Mockito import org.mockito.Mockito.mock import org.mockito.kotlin.verify import org.robolectric.RuntimeEnvironment @@ -97,12 +99,13 @@ class NetworkChangesCollectorTest { } @Test - @Config(sdk = [23]) + @Config(sdk = [23, 33]) fun `NetworkChangesCollector tracks change to cellular network with carrier_name and network_generation`() { shadowOf(context as Application).grantPermissions(Manifest.permission.ACCESS_NETWORK_STATE) shadowOf(context as Application).grantPermissions(Manifest.permission.READ_PHONE_STATE) shadowOf(telephonyManager).setNetworkOperatorName("Test Carrier") setNetworkTypeInTelephonyManager(networkType = TelephonyManager.NETWORK_TYPE_NR) + var previousNetworkType: String? = null NetworkChangesCollector( context = context, @@ -113,11 +116,16 @@ class NetworkChangesCollectorTest { systemServiceProvider = systemServiceProvider, ).register() + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + // first change is discarded for Android O and above + triggerNetworkCapabilitiesChange(addTransportType = NetworkCapabilities.TRANSPORT_WIFI) + previousNetworkType = NetworkType.WIFI + } triggerNetworkCapabilitiesChange(addTransportType = NetworkCapabilities.TRANSPORT_CELLULAR) verify(eventTracker).trackNetworkChange( NetworkChangeEvent( - previous_network_type = null, + previous_network_type = previousNetworkType, network_type = NetworkType.CELLULAR, previous_network_generation = null, network_generation = NetworkGeneration.FIFTH_GEN, @@ -159,6 +167,28 @@ class NetworkChangesCollectorTest { ) } + @Test + @Config(sdk = [26, 33]) + fun `NetworkChangesCollector discards first change for SDK 26 and above`() { + shadowOf(context as Application).grantPermissions(Manifest.permission.ACCESS_NETWORK_STATE) + shadowOf(context as Application).grantPermissions(Manifest.permission.READ_BASIC_PHONE_STATE) + shadowOf(telephonyManager).setNetworkOperatorName("Test Carrier") + setNetworkTypeInTelephonyManager(networkType = TelephonyManager.NETWORK_TYPE_NR) + + NetworkChangesCollector( + context = context, + logger = logger, + eventTracker = eventTracker, + timeProvider = timeProvider, + currentThread = currentThread, + systemServiceProvider = systemServiceProvider, + ).register() + + triggerNetworkCapabilitiesChange(addTransportType = NetworkCapabilities.TRANSPORT_CELLULAR) + + Mockito.verifyNoInteractions(eventTracker) + } + @Test @Config(sdk = [33]) fun `NetworkChangesCollector tracks change to cellular network with carrier_name & network_generation if READ_BASIC_PHONE_STATE permission is available`() { @@ -176,11 +206,14 @@ class NetworkChangesCollectorTest { systemServiceProvider = systemServiceProvider, ).register() + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + triggerNetworkCapabilitiesChange(addTransportType = NetworkCapabilities.TRANSPORT_WIFI) + } triggerNetworkCapabilitiesChange(addTransportType = NetworkCapabilities.TRANSPORT_CELLULAR) verify(eventTracker).trackNetworkChange( NetworkChangeEvent( - previous_network_type = null, + previous_network_type = NetworkType.WIFI, network_type = NetworkType.CELLULAR, previous_network_generation = null, network_generation = NetworkGeneration.FIFTH_GEN, @@ -281,7 +314,6 @@ class NetworkChangesCollectorTest { assertTrue(shouldTrackChange) } - @Test fun `NetworkChangesCollector should track network change for cellular network when previous gen is not equal to new gen`() { // Simulate a previous cellular network type with a different previous and new generation @@ -312,7 +344,6 @@ class NetworkChangesCollectorTest { assertTrue(shouldTrackChange) } - @Test fun `NetworkChangesCollector should not track network change for same network type and generation`() { // Simulate a cellular network type with the same previous and new generation @@ -405,7 +436,7 @@ class NetworkChangesCollectorTest { @Test @Config(sdk = [23]) - fun `NetworkChangesCollector tracks network change event with previous network when network is lost`() { + fun `NetworkChangesCollector discards first change with previous network when network is lost`() { shadowOf(context as Application).grantPermissions(Manifest.permission.ACCESS_NETWORK_STATE) shadowOf(context as Application).grantPermissions(Manifest.permission.READ_PHONE_STATE) shadowOf(telephonyManager).setNetworkOperatorName("Test Carrier")