From 1c446e4721db98d98a94322a086ffd384daee199 Mon Sep 17 00:00:00 2001 From: Ana Capatina Date: Tue, 1 Apr 2025 14:34:11 +0100 Subject: [PATCH 1/4] Added more params to the m_webview_received_http_error_5xx_daily pixel. --- .../app/browser/BrowserTabViewModel.kt | 29 ++-- .../HttpErrorDailyReportingWorker.kt | 2 +- .../app/browser/httperrors/HttpErrorPixels.kt | 115 ++++++++++++++ .../HttpErrorDailyReportingWorkerTest.kt | 2 +- .../httperrors/RealHttpErrorPixelsTest.kt | 144 +++++++++++++++++- 5 files changed, 276 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index 48bc3924e3dc..c65ee77a0b0a 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -3438,19 +3438,22 @@ class BrowserTabViewModel @Inject constructor( } private fun updateHttpErrorCount(statusCode: Int) { - when { - // 400 errors - statusCode == HTTP_STATUS_CODE_BAD_REQUEST_ERROR -> httpErrorPixels.get().updateCountPixel( - HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_400_DAILY, - ) - // all 4xx errors apart from 400 - statusCode / 100 == HTTP_STATUS_CODE_CLIENT_ERROR_PREFIX -> httpErrorPixels.get().updateCountPixel( - HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_4XX_DAILY, - ) - // all 5xx errors - statusCode / 100 == HTTP_STATUS_CODE_SERVER_ERROR_PREFIX -> httpErrorPixels.get().updateCountPixel( - HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, - ) + viewModelScope.launch(dispatchers.io()) { + when { + // 400 errors + statusCode == HTTP_STATUS_CODE_BAD_REQUEST_ERROR -> httpErrorPixels.get().updateCountPixel( + HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_400_DAILY, + ) + // all 4xx errors apart from 400 + statusCode / 100 == HTTP_STATUS_CODE_CLIENT_ERROR_PREFIX -> httpErrorPixels.get().updateCountPixel( + HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_4XX_DAILY, + ) + // all 5xx errors + statusCode / 100 == HTTP_STATUS_CODE_SERVER_ERROR_PREFIX -> httpErrorPixels.get().update5xxCountPixel( + HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, + statusCode, + ) + } } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorker.kt b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorker.kt index 427e67427cc0..d57168e42ba7 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorker.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorker.kt @@ -48,7 +48,7 @@ class HttpErrorDailyReportingWorker(context: Context, workerParameters: WorkerPa return withContext(dispatchers.io()) { httpErrorPixels.fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_400_DAILY) httpErrorPixels.fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_4XX_DAILY) - httpErrorPixels.fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY) + httpErrorPixels.fire5xxCountPixels() return@withContext Result.success() } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt index 3421970481ce..e964ec72a1ed 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt @@ -18,32 +18,79 @@ package com.duckduckgo.app.browser.httperrors import android.content.Context import android.content.SharedPreferences +import android.net.ConnectivityManager +import android.net.NetworkCapabilities import androidx.core.content.edit import com.duckduckgo.app.statistics.pixels.Pixel +import com.duckduckgo.browser.api.WebViewVersionProvider import com.duckduckgo.di.scopes.AppScope +import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry +import com.duckduckgo.networkprotection.api.NetworkProtectionState +import com.duckduckgo.networkprotection.api.NetworkProtectionState.ConnectionState +import com.duckduckgo.subscriptions.api.Product.NetP +import com.duckduckgo.subscriptions.api.SubscriptionStatus +import com.duckduckgo.subscriptions.api.Subscriptions import com.squareup.anvil.annotations.ContributesBinding import java.time.Instant import java.util.concurrent.TimeUnit import javax.inject.Inject +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.map interface HttpErrorPixels { fun updateCountPixel(httpErrorPixelName: HttpErrorPixelName) + suspend fun update5xxCountPixel(httpErrorPixelName: HttpErrorPixelName, statusCode: Int) fun fireCountPixel(httpErrorPixelName: HttpErrorPixelName) + fun fire5xxCountPixels() } @ContributesBinding(AppScope::class) class RealHttpErrorPixels @Inject constructor( private val pixel: Pixel, private val context: Context, + private val webViewVersionProvider: WebViewVersionProvider, + private val networkProtectionState: NetworkProtectionState, + private val subscriptions: Subscriptions, + private val vpnFeaturesRegistry: VpnFeaturesRegistry, ) : HttpErrorPixels { private val preferences: SharedPreferences by lazy { context.getSharedPreferences(FILENAME, Context.MODE_PRIVATE) } + private val pixel5xxKeys: MutableSet by lazy { + preferences.getStringSet(PIXEL_5XX_KEYS_SET, mutableSetOf()) ?: mutableSetOf() + } override fun updateCountPixel(httpErrorPixelName: HttpErrorPixelName) { val count = preferences.getInt(httpErrorPixelName.appendCountSuffix(), 0) preferences.edit { putInt(httpErrorPixelName.appendCountSuffix(), count + 1) } } + override suspend fun update5xxCountPixel( + httpErrorPixelName: HttpErrorPixelName, + statusCode: Int, + ) { + combine( + subscriptions.getEntitlementStatus().map { entitledProducts -> entitledProducts.contains(NetP) }, + networkProtectionState.getConnectionStateFlow(), + flowOf(webViewVersionProvider.getFullVersion()), + ) { netpEntitlementStatus, connectionState, webViewFullVersion -> + val subscriptionStatus = subscriptions.getSubscriptionStatus() + val pProVpnConnected = isVpnConnected(netpEntitlementStatus, connectionState, subscriptionStatus) + val externalVpnConnected = isExternalVpnDetected(pProVpnConnected) + + "${httpErrorPixelName.pixelName}|$statusCode|$pProVpnConnected|$externalVpnConnected|$webViewFullVersion|_count" + } + .collect { pixelPrefKey -> + val updatedSet = pixel5xxKeys + updatedSet.add(pixelPrefKey) + val count = preferences.getInt(pixelPrefKey, 0) + preferences.edit { + putInt(pixelPrefKey, count + 1) + putStringSet(PIXEL_5XX_KEYS_SET, updatedSet) + } + } + } + override fun fireCountPixel(httpErrorPixelName: HttpErrorPixelName) { val now = Instant.now().toEpochMilli() @@ -64,6 +111,44 @@ class RealHttpErrorPixels @Inject constructor( } } + override fun fire5xxCountPixels() { + val now = Instant.now().toEpochMilli() + val updatedSet = pixel5xxKeys + updatedSet.forEach { pixelKey -> + val count = preferences.getInt(pixelKey, 0) + if (count != 0) { + val timestamp = preferences.getLong("${pixelKey}_timestamp", 0L) + if (timestamp == 0L || now >= timestamp) { + pixelKey.split("|").let { split -> + if (split.size == 6) { + val httpErrorPixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY + val statusCode = split[1].toInt() + val pProVpnConnected = split[2].toBoolean() + val externalVpnConnected = split[3].toBoolean() + val webViewFullVersion = split[4] + pixel.fire( + httpErrorPixelName, + mapOf( + HttpErrorPixelParameters.HTTP_ERROR_CODE_COUNT to count.toString(), + "error_code" to statusCode.toString(), + "ppro_user" to pProVpnConnected.toString(), + "vpn_user" to externalVpnConnected.toString(), + "webview_version" to webViewFullVersion, + ), + ) + } + } + .also { + preferences.edit { + putLong("${pixelKey}_timestamp", now.plus(TimeUnit.HOURS.toMillis(WINDOW_INTERVAL_HOURS))) + putInt(pixelKey, 0) + } + } + } + } + } + } + private fun HttpErrorPixelName.appendTimestampSuffix(): String { return "${this.pixelName}_timestamp" } @@ -72,8 +157,38 @@ class RealHttpErrorPixels @Inject constructor( return "${this.pixelName}_count" } + private fun isVpnConnected( + netpEntitlementStatus: Boolean, + connectionState: ConnectionState, + subscriptionStatus: SubscriptionStatus, + ): Boolean { + if (subscriptionStatus in setOf( + SubscriptionStatus.AUTO_RENEWABLE, + SubscriptionStatus.NOT_AUTO_RENEWABLE, + SubscriptionStatus.GRACE_PERIOD, + ) + ) { + if (netpEntitlementStatus) { + return connectionState.isConnected() + } + } + return false + } + + private suspend fun isExternalVpnDetected(pProVpnConnected: Boolean): Boolean { + if (pProVpnConnected) return false + if (vpnFeaturesRegistry.isAnyFeatureRunning()) return false + + return runCatching { + val connectivityManager = context.applicationContext.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager + val activeNetwork = connectivityManager.activeNetwork + connectivityManager.getNetworkCapabilities(activeNetwork)?.hasTransport(NetworkCapabilities.TRANSPORT_VPN) ?: false + }.getOrDefault(false) + } + companion object { private const val FILENAME = "com.duckduckgo.app.browser.httperrors" private const val WINDOW_INTERVAL_HOURS = 24L + internal const val PIXEL_5XX_KEYS_SET = "pixel_5xx_keys_set" } } diff --git a/app/src/test/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorkerTest.kt b/app/src/test/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorkerTest.kt index 5aecfaae81de..a2210f969acb 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorkerTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/httperrors/HttpErrorDailyReportingWorkerTest.kt @@ -53,7 +53,7 @@ internal class HttpErrorDailyReportingWorkerTest { verify(mockHttpErrorPixels).fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_400_DAILY) verify(mockHttpErrorPixels).fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_4XX_DAILY) - verify(mockHttpErrorPixels).fireCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY) + verify(mockHttpErrorPixels).fire5xxCountPixels() assertEquals(result, ListenableWorker.Result.success()) } } diff --git a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt index e7eb76b45f34..3ca5b8d1a203 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt @@ -20,13 +20,26 @@ import android.content.Context import android.content.SharedPreferences import androidx.core.content.edit import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.duckduckgo.app.browser.httperrors.RealHttpErrorPixels.Companion.PIXEL_5XX_KEYS_SET import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.app.statistics.pixels.Pixel.PixelType.Count +import com.duckduckgo.browser.api.WebViewVersionProvider +import com.duckduckgo.common.test.CoroutineTestRule import com.duckduckgo.common.test.api.InMemorySharedPreferences +import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry +import com.duckduckgo.networkprotection.api.NetworkProtectionState +import com.duckduckgo.networkprotection.api.NetworkProtectionState.ConnectionState +import com.duckduckgo.subscriptions.api.Product.NetP +import com.duckduckgo.subscriptions.api.SubscriptionStatus +import com.duckduckgo.subscriptions.api.Subscriptions import java.time.Instant import java.util.concurrent.TimeUnit +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Before +import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.mockito.kotlin.any @@ -39,17 +52,31 @@ import org.mockito.kotlin.whenever @RunWith(AndroidJUnit4::class) class RealHttpErrorPixelsTest { + @get:Rule + val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + private lateinit var testee: HttpErrorPixels private lateinit var prefs: SharedPreferences private val mockPixel: Pixel = mock() private val mockContext: Context = mock() + private val mockWebViewVersionProvider: WebViewVersionProvider = mock() + private val mockNetworkProtectionState: NetworkProtectionState = mock() + private val mockSubscriptions: Subscriptions = mock() + private val mockVpnFeaturesRegistry: VpnFeaturesRegistry = mock() @Before fun setup() { prefs = InMemorySharedPreferences() whenever(mockContext.getSharedPreferences("com.duckduckgo.app.browser.httperrors", 0)).thenReturn(prefs) - testee = RealHttpErrorPixels(mockPixel, mockContext) + testee = RealHttpErrorPixels( + mockPixel, + mockContext, + mockWebViewVersionProvider, + mockNetworkProtectionState, + mockSubscriptions, + mockVpnFeaturesRegistry, + ) } @Test @@ -131,4 +158,119 @@ class RealHttpErrorPixelsTest { type = eq(Count), ) } + + @Test + fun whenUpdate5xxCountPixelCalledThenSharedPrefUpdated() = runTest { + val netpFlow = flowOf(listOf(NetP)) + val connectionStateFlow = flowOf(ConnectionState.CONNECTED) + + whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) + whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) + whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) + whenever(mockWebViewVersionProvider.getFullVersion()).thenReturn("123.45.67.89") + + // The pixelKey format is: pixelName|statusCode|pProVpnConnected|externalVpnConnected|webViewVersion|_count + val expectedKey = "${HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY.pixelName}|503|true|false|123.45.67.89|_count" + + testee.update5xxCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, 503) + + assertEquals(1, prefs.getInt(expectedKey, 0)) + assertTrue(prefs.getStringSet(PIXEL_5XX_KEYS_SET, emptySet())!!.contains(expectedKey)) + } + + @Test + fun whenUpdate5xxCountPixelCalledMultipleTimesThenCounterIncremented() = runTest { + val netpFlow = flowOf(listOf(NetP)) + val connectionStateFlow = flowOf(ConnectionState.CONNECTED) + + whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) + whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) + whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) + whenever(mockWebViewVersionProvider.getFullVersion()).thenReturn("123.45.67.89") + + // The pixelKey format is: pixelName|statusCode|pProVpnConnected|externalVpnConnected|webViewVersion|_count + val expectedKey = "${HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY.pixelName}|503|true|false|123.45.67.89|_count" + + testee.update5xxCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, 503) + testee.update5xxCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, 503) + + assertEquals(2, prefs.getInt(expectedKey, 0)) + } + + @Test + fun whenFire5xxCountPixelCalledWithNonZeroCountPixelSent() = runTest { + val pixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY + val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" + val pixelKeys = mutableSetOf(pixelKey) + + prefs.edit { + putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) + putInt(pixelKey, 5) + } + + testee.fire5xxCountPixels() + + verify(mockPixel).fire( + pixel = eq(pixelName), + parameters = eq( + mapOf( + HttpErrorPixelParameters.HTTP_ERROR_CODE_COUNT to "5", + "error_code" to "503", + "ppro_user" to "true", + "vpn_user" to "false", + "webview_version" to "123.45.67.89", + ), + ), + encodedParameters = any(), + type = any(), + ) + + assertEquals(0, prefs.getInt(pixelKey, -1)) + } + + @Test + fun whenFire5xxCountPixelCalledWithZeroCountPixelNotSent() { + val pixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY + val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" + val pixelKeys = mutableSetOf(pixelKey) + + prefs.edit { + putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) + putInt(pixelKey, 0) + } + + testee.fire5xxCountPixels() + + verify(mockPixel, never()).fire( + pixel = eq(pixelName), + parameters = any(), + encodedParameters = any(), + type = any(), + ) + } + + @Test + fun whenFire5xxCountPixelsCalledBeforeTimeWindowThenPixelNotSent() { + val pixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY + val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" + val pixelKeys = mutableSetOf(pixelKey) + val now = Instant.now().toEpochMilli() + + prefs.edit { + putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) + putInt(pixelKey, 5) + putLong("${pixelKey}_timestamp", now + TimeUnit.HOURS.toMillis(1)) // 1 hour in future + } + + testee.fire5xxCountPixels() + + verify(mockPixel, never()).fire( + pixel = eq(pixelName), + parameters = any(), + encodedParameters = any(), + type = any(), + ) + + assertEquals(5, prefs.getInt(pixelKey, -1)) + } } From e46787816c80a552ffbd80072fb9b4f49a682251 Mon Sep 17 00:00:00 2001 From: Ana Capatina Date: Tue, 1 Apr 2025 18:40:07 +0100 Subject: [PATCH 2/4] Added feature flag. --- .../app/browser/httperrors/HttpErrorPixels.kt | 12 ++++++++++++ .../remoteconfig/AndroidBrowserConfigFeature.kt | 8 ++++++++ .../browser/httperrors/RealHttpErrorPixelsTest.kt | 15 +++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt index e964ec72a1ed..2c2adcbd761d 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt @@ -21,6 +21,7 @@ import android.content.SharedPreferences import android.net.ConnectivityManager import android.net.NetworkCapabilities import androidx.core.content.edit +import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.browser.api.WebViewVersionProvider import com.duckduckgo.di.scopes.AppScope @@ -53,6 +54,7 @@ class RealHttpErrorPixels @Inject constructor( private val networkProtectionState: NetworkProtectionState, private val subscriptions: Subscriptions, private val vpnFeaturesRegistry: VpnFeaturesRegistry, + private val androidBrowserConfig: AndroidBrowserConfigFeature, ) : HttpErrorPixels { private val preferences: SharedPreferences by lazy { context.getSharedPreferences(FILENAME, Context.MODE_PRIVATE) } @@ -69,6 +71,11 @@ class RealHttpErrorPixels @Inject constructor( httpErrorPixelName: HttpErrorPixelName, statusCode: Int, ) { + // Kill switch + if (!androidBrowserConfig.self().isEnabled() || !androidBrowserConfig.httpError5xxPixel().isEnabled()) { + return + } + combine( subscriptions.getEntitlementStatus().map { entitledProducts -> entitledProducts.contains(NetP) }, networkProtectionState.getConnectionStateFlow(), @@ -112,6 +119,11 @@ class RealHttpErrorPixels @Inject constructor( } override fun fire5xxCountPixels() { + // Kill switch + if (!androidBrowserConfig.self().isEnabled() || !androidBrowserConfig.httpError5xxPixel().isEnabled()) { + return + } + val now = Instant.now().toEpochMilli() val updatedSet = pixel5xxKeys updatedSet.forEach { pixelKey -> diff --git a/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt b/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt index 99a49055ad64..89b1eb56293f 100644 --- a/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt +++ b/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt @@ -99,4 +99,12 @@ interface AndroidBrowserConfigFeature { */ @Toggle.DefaultValue(false) fun fireproofedWebLocalStorage(): Toggle + + /** + * @return `true` when the remote config has the global "httpError5xxPixel" androidBrowserConfig + * sub-feature flag enabled + * If the remote feature is not present defaults to `false` + */ + @Toggle.DefaultValue(false) + fun httpError5xxPixel(): Toggle } diff --git a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt index 3ca5b8d1a203..6691eb56498c 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt @@ -21,11 +21,14 @@ import android.content.SharedPreferences import androidx.core.content.edit import androidx.test.ext.junit.runners.AndroidJUnit4 import com.duckduckgo.app.browser.httperrors.RealHttpErrorPixels.Companion.PIXEL_5XX_KEYS_SET +import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.app.statistics.pixels.Pixel.PixelType.Count import com.duckduckgo.browser.api.WebViewVersionProvider import com.duckduckgo.common.test.CoroutineTestRule import com.duckduckgo.common.test.api.InMemorySharedPreferences +import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory +import com.duckduckgo.feature.toggles.api.Toggle.State import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry import com.duckduckgo.networkprotection.api.NetworkProtectionState import com.duckduckgo.networkprotection.api.NetworkProtectionState.ConnectionState @@ -64,6 +67,7 @@ class RealHttpErrorPixelsTest { private val mockNetworkProtectionState: NetworkProtectionState = mock() private val mockSubscriptions: Subscriptions = mock() private val mockVpnFeaturesRegistry: VpnFeaturesRegistry = mock() + private var fakeAndroidConfigBrowserFeature = FakeFeatureToggleFactory.create(AndroidBrowserConfigFeature::class.java) @Before fun setup() { @@ -76,6 +80,7 @@ class RealHttpErrorPixelsTest { mockNetworkProtectionState, mockSubscriptions, mockVpnFeaturesRegistry, + fakeAndroidConfigBrowserFeature, ) } @@ -164,6 +169,8 @@ class RealHttpErrorPixelsTest { val netpFlow = flowOf(listOf(NetP)) val connectionStateFlow = flowOf(ConnectionState.CONNECTED) + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) @@ -183,6 +190,8 @@ class RealHttpErrorPixelsTest { val netpFlow = flowOf(listOf(NetP)) val connectionStateFlow = flowOf(ConnectionState.CONNECTED) + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) @@ -203,6 +212,8 @@ class RealHttpErrorPixelsTest { val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" val pixelKeys = mutableSetOf(pixelKey) + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) prefs.edit { putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) putInt(pixelKey, 5) @@ -234,6 +245,8 @@ class RealHttpErrorPixelsTest { val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" val pixelKeys = mutableSetOf(pixelKey) + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) prefs.edit { putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) putInt(pixelKey, 0) @@ -256,6 +269,8 @@ class RealHttpErrorPixelsTest { val pixelKeys = mutableSetOf(pixelKey) val now = Instant.now().toEpochMilli() + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) prefs.edit { putStringSet(PIXEL_5XX_KEYS_SET, pixelKeys) putInt(pixelKey, 5) From 263bd24f7a86b725ee43898ca2f9adb4ffe41370 Mon Sep 17 00:00:00 2001 From: Ana Capatina Date: Wed, 2 Apr 2025 10:50:13 +0100 Subject: [PATCH 3/4] Addressed PR comments. --- .../app/browser/httperrors/HttpErrorPixels.kt | 78 +++++-------------- .../httperrors/RealHttpErrorPixelsTest.kt | 67 ++++++++++------ 2 files changed, 64 insertions(+), 81 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt index 2c2adcbd761d..24bb86f701f2 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/httperrors/HttpErrorPixels.kt @@ -18,26 +18,17 @@ package com.duckduckgo.app.browser.httperrors import android.content.Context import android.content.SharedPreferences -import android.net.ConnectivityManager -import android.net.NetworkCapabilities import androidx.core.content.edit import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.browser.api.WebViewVersionProvider import com.duckduckgo.di.scopes.AppScope -import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry +import com.duckduckgo.mobile.android.vpn.network.ExternalVpnDetector import com.duckduckgo.networkprotection.api.NetworkProtectionState -import com.duckduckgo.networkprotection.api.NetworkProtectionState.ConnectionState -import com.duckduckgo.subscriptions.api.Product.NetP -import com.duckduckgo.subscriptions.api.SubscriptionStatus -import com.duckduckgo.subscriptions.api.Subscriptions import com.squareup.anvil.annotations.ContributesBinding import java.time.Instant import java.util.concurrent.TimeUnit import javax.inject.Inject -import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.flowOf -import kotlinx.coroutines.flow.map interface HttpErrorPixels { fun updateCountPixel(httpErrorPixelName: HttpErrorPixelName) @@ -52,8 +43,7 @@ class RealHttpErrorPixels @Inject constructor( private val context: Context, private val webViewVersionProvider: WebViewVersionProvider, private val networkProtectionState: NetworkProtectionState, - private val subscriptions: Subscriptions, - private val vpnFeaturesRegistry: VpnFeaturesRegistry, + private val externalVpnDetector: ExternalVpnDetector, private val androidBrowserConfig: AndroidBrowserConfigFeature, ) : HttpErrorPixels { @@ -76,26 +66,25 @@ class RealHttpErrorPixels @Inject constructor( return } - combine( - subscriptions.getEntitlementStatus().map { entitledProducts -> entitledProducts.contains(NetP) }, - networkProtectionState.getConnectionStateFlow(), - flowOf(webViewVersionProvider.getFullVersion()), - ) { netpEntitlementStatus, connectionState, webViewFullVersion -> - val subscriptionStatus = subscriptions.getSubscriptionStatus() - val pProVpnConnected = isVpnConnected(netpEntitlementStatus, connectionState, subscriptionStatus) - val externalVpnConnected = isExternalVpnDetected(pProVpnConnected) + val pProVpnConnected = runCatching { + networkProtectionState.isRunning() + }.getOrDefault(false) + + val externalVpnConnected = runCatching { + externalVpnDetector.isExternalVpnDetected() + }.getOrDefault(false) + + val webViewFullVersion = webViewVersionProvider.getFullVersion() - "${httpErrorPixelName.pixelName}|$statusCode|$pProVpnConnected|$externalVpnConnected|$webViewFullVersion|_count" + val pixelPrefKey = "${httpErrorPixelName.pixelName}|$statusCode|$pProVpnConnected|$externalVpnConnected|$webViewFullVersion|_count" + + val updatedSet = pixel5xxKeys + updatedSet.add(pixelPrefKey) + val count = preferences.getInt(pixelPrefKey, 0) + preferences.edit { + putInt(pixelPrefKey, count + 1) + putStringSet(PIXEL_5XX_KEYS_SET, updatedSet) } - .collect { pixelPrefKey -> - val updatedSet = pixel5xxKeys - updatedSet.add(pixelPrefKey) - val count = preferences.getInt(pixelPrefKey, 0) - preferences.edit { - putInt(pixelPrefKey, count + 1) - putStringSet(PIXEL_5XX_KEYS_SET, updatedSet) - } - } } override fun fireCountPixel(httpErrorPixelName: HttpErrorPixelName) { @@ -169,35 +158,6 @@ class RealHttpErrorPixels @Inject constructor( return "${this.pixelName}_count" } - private fun isVpnConnected( - netpEntitlementStatus: Boolean, - connectionState: ConnectionState, - subscriptionStatus: SubscriptionStatus, - ): Boolean { - if (subscriptionStatus in setOf( - SubscriptionStatus.AUTO_RENEWABLE, - SubscriptionStatus.NOT_AUTO_RENEWABLE, - SubscriptionStatus.GRACE_PERIOD, - ) - ) { - if (netpEntitlementStatus) { - return connectionState.isConnected() - } - } - return false - } - - private suspend fun isExternalVpnDetected(pProVpnConnected: Boolean): Boolean { - if (pProVpnConnected) return false - if (vpnFeaturesRegistry.isAnyFeatureRunning()) return false - - return runCatching { - val connectivityManager = context.applicationContext.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager - val activeNetwork = connectivityManager.activeNetwork - connectivityManager.getNetworkCapabilities(activeNetwork)?.hasTransport(NetworkCapabilities.TRANSPORT_VPN) ?: false - }.getOrDefault(false) - } - companion object { private const val FILENAME = "com.duckduckgo.app.browser.httperrors" private const val WINDOW_INTERVAL_HOURS = 24L diff --git a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt index 6691eb56498c..fce6daac52ef 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/httperrors/RealHttpErrorPixelsTest.kt @@ -29,17 +29,13 @@ import com.duckduckgo.common.test.CoroutineTestRule import com.duckduckgo.common.test.api.InMemorySharedPreferences import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory import com.duckduckgo.feature.toggles.api.Toggle.State -import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry +import com.duckduckgo.mobile.android.vpn.network.ExternalVpnDetector import com.duckduckgo.networkprotection.api.NetworkProtectionState -import com.duckduckgo.networkprotection.api.NetworkProtectionState.ConnectionState -import com.duckduckgo.subscriptions.api.Product.NetP -import com.duckduckgo.subscriptions.api.SubscriptionStatus -import com.duckduckgo.subscriptions.api.Subscriptions import java.time.Instant import java.util.concurrent.TimeUnit -import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule @@ -65,8 +61,7 @@ class RealHttpErrorPixelsTest { private val mockContext: Context = mock() private val mockWebViewVersionProvider: WebViewVersionProvider = mock() private val mockNetworkProtectionState: NetworkProtectionState = mock() - private val mockSubscriptions: Subscriptions = mock() - private val mockVpnFeaturesRegistry: VpnFeaturesRegistry = mock() + private val mockExternalVpnDetector: ExternalVpnDetector = mock() private var fakeAndroidConfigBrowserFeature = FakeFeatureToggleFactory.create(AndroidBrowserConfigFeature::class.java) @Before @@ -78,8 +73,7 @@ class RealHttpErrorPixelsTest { mockContext, mockWebViewVersionProvider, mockNetworkProtectionState, - mockSubscriptions, - mockVpnFeaturesRegistry, + mockExternalVpnDetector, fakeAndroidConfigBrowserFeature, ) } @@ -166,14 +160,10 @@ class RealHttpErrorPixelsTest { @Test fun whenUpdate5xxCountPixelCalledThenSharedPrefUpdated() = runTest { - val netpFlow = flowOf(listOf(NetP)) - val connectionStateFlow = flowOf(ConnectionState.CONNECTED) - fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) - whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) - whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) - whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) + whenever(mockNetworkProtectionState.isRunning()).thenReturn(true) + whenever(mockExternalVpnDetector.isExternalVpnDetected()).thenReturn(false) whenever(mockWebViewVersionProvider.getFullVersion()).thenReturn("123.45.67.89") // The pixelKey format is: pixelName|statusCode|pProVpnConnected|externalVpnConnected|webViewVersion|_count @@ -187,14 +177,10 @@ class RealHttpErrorPixelsTest { @Test fun whenUpdate5xxCountPixelCalledMultipleTimesThenCounterIncremented() = runTest { - val netpFlow = flowOf(listOf(NetP)) - val connectionStateFlow = flowOf(ConnectionState.CONNECTED) - fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = true)) - whenever(mockSubscriptions.getEntitlementStatus()).thenReturn(netpFlow) - whenever(mockNetworkProtectionState.getConnectionStateFlow()).thenReturn(connectionStateFlow) - whenever(mockSubscriptions.getSubscriptionStatus()).thenReturn(SubscriptionStatus.AUTO_RENEWABLE) + whenever(mockNetworkProtectionState.isRunning()).thenReturn(true) + whenever(mockExternalVpnDetector.isExternalVpnDetected()).thenReturn(false) whenever(mockWebViewVersionProvider.getFullVersion()).thenReturn("123.45.67.89") // The pixelKey format is: pixelName|statusCode|pProVpnConnected|externalVpnConnected|webViewVersion|_count @@ -206,6 +192,23 @@ class RealHttpErrorPixelsTest { assertEquals(2, prefs.getInt(expectedKey, 0)) } + @Test + fun whenUpdate5xxCountPixelCalledWirhFeatureFlagDisabledThenSharedPrefNotUpdated() = runTest { + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = false)) + whenever(mockNetworkProtectionState.isRunning()).thenReturn(true) + whenever(mockExternalVpnDetector.isExternalVpnDetected()).thenReturn(false) + whenever(mockWebViewVersionProvider.getFullVersion()).thenReturn("123.45.67.89") + + // The pixelKey format is: pixelName|statusCode|pProVpnConnected|externalVpnConnected|webViewVersion|_count + val expectedKey = "${HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY.pixelName}|503|true|false|123.45.67.89|_count" + + testee.update5xxCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, 503) + + assertEquals(0, prefs.getInt(expectedKey, 0)) + assertFalse(prefs.getStringSet(PIXEL_5XX_KEYS_SET, emptySet())!!.contains(expectedKey)) + } + @Test fun whenFire5xxCountPixelCalledWithNonZeroCountPixelSent() = runTest { val pixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY @@ -288,4 +291,24 @@ class RealHttpErrorPixelsTest { assertEquals(5, prefs.getInt(pixelKey, -1)) } + + @Test + fun whenFire5xxCountPixelsCalledWithTheFeatureFlagDisabledThenPixelNotSent() { + val pixelName = HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY + val pixelKey = "${pixelName.pixelName}|503|true|false|123.45.67.89|_count" + + fakeAndroidConfigBrowserFeature.self().setRawStoredState(State(enable = true)) + fakeAndroidConfigBrowserFeature.httpError5xxPixel().setRawStoredState(State(enable = false)) + + testee.fire5xxCountPixels() + + verify(mockPixel, never()).fire( + pixel = eq(pixelName), + parameters = any(), + encodedParameters = any(), + type = any(), + ) + + assertEquals(-1, prefs.getInt(pixelKey, -1)) + } } From f6ffc0653529db37eaa95c71f97129379908611d Mon Sep 17 00:00:00 2001 From: Ana Capatina Date: Wed, 2 Apr 2025 11:44:48 +0100 Subject: [PATCH 4/4] Updated test. --- .../com/duckduckgo/app/browser/BrowserTabViewModelTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index 8e6b6ad27938..abdca8709cc2 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -4945,10 +4945,10 @@ class BrowserTabViewModelTest { } @Test - fun whenPageIsChangedWithHttpError5XXThenUpdateCountPixelCalledForWebViewReceivedHttpError5XXDaily() = runTest { + fun whenPageIsChangedWithHttpError5XXThenUpdate5xxCountPixelCalledForWebViewReceivedHttpError5XXDaily() = runTest { testee.recordHttpErrorCode(statusCode = 504, url = "example2.com") - verify(mockHttpErrorPixels).updateCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY) + verify(mockHttpErrorPixels).update5xxCountPixel(HttpErrorPixelName.WEBVIEW_RECEIVED_HTTP_ERROR_5XX_DAILY, 504) } @Test