Improve snooze handling

duckduckgo · Nov 22, 2023 · efb9415 · efb9415
1 parent e88dfac
commit efb9415
Show file tree

Hide file tree

Showing 16 changed files with 106 additions and 190 deletions.
diff --git a/...otection/vpn-api/src/main/java/com/duckduckgo/mobile/android/vpn/state/VpnStateMonitor.kt b/...otection/vpn-api/src/main/java/com/duckduckgo/mobile/android/vpn/state/VpnStateMonitor.kt
@@ -15,6 +15,7 @@
  */
 
 package com.duckduckgo.mobile.android.vpn.state
+
 import com.duckduckgo.mobile.android.vpn.VpnFeature
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.AlwaysOnState.Companion.DEFAULT
 import kotlinx.coroutines.flow.Flow
@@ -45,7 +46,10 @@ interface VpnStateMonitor {
         val alwaysOnState: AlwaysOnState = DEFAULT,
     )
 
-    data class AlwaysOnState(val enabled: Boolean, val lockedDown: Boolean) {
+    data class AlwaysOnState(
+        val enabled: Boolean,
+        val lockedDown: Boolean
+    ) {
         companion object {
             val DEFAULT = AlwaysOnState(enabled = false, lockedDown = false)
             val ALWAYS_ON_ENABLED = AlwaysOnState(enabled = true, lockedDown = false)
@@ -56,17 +60,18 @@ interface VpnStateMonitor {
     }
 
     sealed class VpnRunningState {
-        object ENABLING : VpnRunningState()
-        object ENABLED : VpnRunningState()
-        data class DISABLED(val snoozedTriggerAtMillis: Long? = null) : VpnRunningState()
-        object INVALID : VpnRunningState()
+        data object ENABLING : VpnRunningState()
+        data object ENABLED : VpnRunningState()
+        data object DISABLED : VpnRunningState()
+        data object INVALID : VpnRunningState()
     }
 
-    enum class VpnStopReason {
-        SELF_STOP,
-        ERROR,
-        REVOKED,
-        UNKNOWN,
-        RESTART,
+    sealed class VpnStopReason {
+        data object SELF_STOP : VpnStopReason()
+        data object ERROR : VpnStopReason()
+        data object REVOKED : VpnStopReason()
+        data object UNKNOWN : VpnStopReason()
+        data object RESTART : VpnStopReason()
+        data class SNOOZED(val snoozedTriggerAtMillis: Long? = null) : VpnStopReason()
     }
 }
diff --git a/...vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/heartbeat/VpnServiceHeartbeat.kt b/...vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/heartbeat/VpnServiceHeartbeat.kt
@@ -27,6 +27,7 @@ import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.ERR
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.RESTART
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.REVOKED
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.SELF_STOP
+import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.SNOOZED
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.UNKNOWN
 import com.duckduckgo.mobile.android.vpn.store.VpnDatabase
 import com.squareup.anvil.annotations.ContributesMultibinding
@@ -75,7 +76,7 @@ class VpnServiceHeartbeat @Inject constructor(
         logcat { "onVpnStopped called" }
         when (vpnStopReason) {
             ERROR -> logcat { "HB monitor: sudden vpn stopped $vpnStopReason" }
-            SELF_STOP, REVOKED, RESTART, UNKNOWN -> {
+            SELF_STOP, REVOKED, RESTART, UNKNOWN, is SNOOZED -> {
                 logcat { "HB monitor: self stopped or revoked or restart: $vpnStopReason" }
                 // we absolutely want this to finish before VPN is stopped to avoid race conditions reading out the state
                 runBlocking { storeHeartbeat(VpnServiceHeartbeatMonitor.DATA_HEART_BEAT_TYPE_STOPPED) }

diff --git a/...impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/TrackerBlockingVpnService.kt b/...impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/TrackerBlockingVpnService.kt
@@ -230,15 +230,21 @@ class TrackerBlockingVpnService : VpnService(), CoroutineScope by MainScope(), V
                     stopSelf()
                 }
             }
-            ACTION_STOP_VPN, ACTION_SNOOZE_VPN -> {
+            ACTION_STOP_VPN -> {
+                synchronized(this) {
+                    launch(serviceDispatcher) {
+                        async {
+                            stopVpn(VpnStopReason.SELF_STOP)
+                        }.await()
+                    }
+                }
+            }
+            ACTION_SNOOZE_VPN -> {
                 synchronized(this) {
                     launch(serviceDispatcher) {
                         async {
                             val snoozeTriggerAtMillisExtra = intent.getLongExtra(ACTION_SNOOZE_VPN_EXTRA, 0L)
-                            stopVpn(
-                                VpnStopReason.SELF_STOP,
-                                snoozed = snoozeTriggerAtMillisExtra != 0L,
-                            )
+                            stopVpn(VpnStopReason.SNOOZED(snoozeTriggerAtMillisExtra))
                         }.await()
                     }
                 }
@@ -526,7 +532,6 @@ class TrackerBlockingVpnService : VpnService(), CoroutineScope by MainScope(), V
     private suspend fun stopVpn(
         reason: VpnStopReason,
         hasVpnAlreadyStarted: Boolean = true,
-        snoozed: Boolean = false,
     ) = withContext(serviceDispatcher) {
         logcat { "VPN log: Stopping VPN. $reason" }
 
@@ -552,12 +557,7 @@ class TrackerBlockingVpnService : VpnService(), CoroutineScope by MainScope(), V
         }
 
         // Set the state to DISABLED here, then call the on stop/failure callbacks
-        vpnServiceStateStatsDao.insert(
-            createVpnState(
-                state = if (snoozed) VpnServiceState.SNOOZED else VpnServiceState.DISABLED,
-                stopReason = reason,
-            ),
-        )
+        vpnServiceStateStatsDao.insert(createVpnState(state = VpnServiceState.DISABLED, stopReason = reason))
 
         vpnStateServiceReference?.let {
             runCatching { unbindService(vpnStateServiceConnection).also { vpnStateServiceReference = null } }
@@ -569,7 +569,7 @@ class TrackerBlockingVpnService : VpnService(), CoroutineScope by MainScope(), V
 
     private fun sendStopPixels(reason: VpnStopReason) {
         when (reason) {
-            VpnStopReason.SELF_STOP, VpnStopReason.RESTART, VpnStopReason.UNKNOWN -> {} // no-op
+            VpnStopReason.SELF_STOP, VpnStopReason.RESTART, VpnStopReason.UNKNOWN, is VpnStopReason.SNOOZED -> {} // no-op
             VpnStopReason.ERROR -> deviceShieldPixels.startError()
             VpnStopReason.REVOKED -> deviceShieldPixels.suddenKillByVpnRevoked()
         }
@@ -688,6 +688,7 @@ class TrackerBlockingVpnService : VpnService(), CoroutineScope by MainScope(), V
                 VpnStopReason.REVOKED -> VpnStoppingReason.REVOKED
                 VpnStopReason.ERROR -> VpnStoppingReason.ERROR
                 VpnStopReason.UNKNOWN -> VpnStoppingReason.UNKNOWN
+                is VpnStopReason.SNOOZED -> VpnStoppingReason.SNOOZED
             }
         }
 

diff --git a/...impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/state/RealVpnStateMonitor.kt b/...impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/state/RealVpnStateMonitor.kt
@@ -30,12 +30,12 @@ import com.duckduckgo.mobile.android.vpn.model.VpnStoppingReason.ERROR
 import com.duckduckgo.mobile.android.vpn.model.VpnStoppingReason.RESTART
 import com.duckduckgo.mobile.android.vpn.model.VpnStoppingReason.REVOKED
 import com.duckduckgo.mobile.android.vpn.model.VpnStoppingReason.SELF_STOP
+import com.duckduckgo.mobile.android.vpn.model.VpnStoppingReason.SNOOZED
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnRunningState
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnState
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason
 import com.squareup.anvil.annotations.ContributesBinding
-import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 import kotlinx.coroutines.flow.*
 import logcat.logcat
@@ -52,22 +52,22 @@ class RealVpnStateMonitor @Inject constructor(
         return vpnServiceStateStatsDao.getStateStats().map { mapState(it) }
             .filter {
                 // we only care about the following states
-                (it.state == VpnRunningState.ENABLED) || (it.state == VpnRunningState.ENABLING) || (it.state is VpnRunningState.DISABLED)
+                (it.state == VpnRunningState.ENABLED) || (it.state == VpnRunningState.ENABLING) || (it.state == VpnRunningState.DISABLED)
             }
             .onEach { logcat { "service state value $it" } }
             .map { vpnState ->
                 val isFeatureEnabled = vpnFeaturesRegistry.isFeatureRunning(vpnFeature)
 
                 if (!isFeatureEnabled && vpnState.state !is VpnRunningState.DISABLED) {
-                    vpnState.copy(state = VpnRunningState.DISABLED())
+                    vpnState.copy(state = VpnRunningState.DISABLED)
                 } else {
                     vpnState
                 }
             }
             .onStart {
                 val vpnState = mapState(vpnServiceStateStatsDao.getLastStateStats())
                 VpnState(
-                    state = if (vpnFeaturesRegistry.isFeatureRunning(vpnFeature)) VpnRunningState.ENABLED else VpnRunningState.DISABLED(),
+                    state = if (vpnFeaturesRegistry.isFeatureRunning(vpnFeature)) VpnRunningState.ENABLED else VpnRunningState.DISABLED,
                     alwaysOnState = vpnState.alwaysOnState,
                     stopReason = vpnState.stopReason,
                 ).also { emit(it) }
@@ -105,13 +105,13 @@ class RealVpnStateMonitor @Inject constructor(
             SELF_STOP -> VpnStopReason.SELF_STOP
             REVOKED -> VpnStopReason.REVOKED
             ERROR -> VpnStopReason.ERROR
+            SNOOZED -> VpnStopReason.SNOOZED()
             else -> VpnStopReason.UNKNOWN
         }
         val runningState = when (lastState?.state) {
             ENABLING -> VpnRunningState.ENABLING
             ENABLED -> VpnRunningState.ENABLED
-            DISABLED -> VpnRunningState.DISABLED()
-            SNOOZED -> VpnRunningState.DISABLED(TimeUnit.MINUTES.toMillis(20)) // TODO - remove hardcoded time
+            DISABLED -> VpnRunningState.DISABLED
             null, INVALID -> VpnRunningState.INVALID
         }
         val alwaysOnState = when (lastState?.alwaysOnState) {

diff --git a/...l/src/main/java/com/duckduckgo/mobile/android/vpn/service/state/VpnStateMonitorService.kt b/...l/src/main/java/com/duckduckgo/mobile/android/vpn/service/state/VpnStateMonitorService.kt
@@ -28,7 +28,6 @@ import com.duckduckgo.di.scopes.VpnScope
 import com.duckduckgo.mobile.android.vpn.dao.HeartBeatEntity
 import com.duckduckgo.mobile.android.vpn.heartbeat.VpnServiceHeartbeatMonitor
 import com.duckduckgo.mobile.android.vpn.model.VpnServiceState.DISABLED
-import com.duckduckgo.mobile.android.vpn.model.VpnServiceState.SNOOZED
 import com.duckduckgo.mobile.android.vpn.model.VpnServiceStateStats
 import com.duckduckgo.mobile.android.vpn.service.TrackerBlockingVpnService
 import com.duckduckgo.mobile.android.vpn.store.VpnDatabase
@@ -78,7 +77,7 @@ class VpnStateMonitorService : Service() {
     // check last state, if it was enabled then we store disabled state reason unknown
     private fun maybeUpdateVPNState() {
         val lastStateStats = vpnDatabase.vpnServiceStateDao().getLastStateStats()
-        if (lastStateStats?.state != DISABLED && lastStateStats?.state != SNOOZED) {
+        if (lastStateStats?.state != DISABLED) {
             logcat { "VpnStateMonitorService destroyed but VPN state stored as ${lastStateStats?.state}, inserting DISABLED" }
             vpnDatabase.vpnServiceStateDao().insert(VpnServiceStateStats(state = DISABLED))
         }

diff --git a/...a/com/duckduckgo/mobile/android/vpn/ui/notification/AppTPReminderNotificationScheduler.kt b/...a/com/duckduckgo/mobile/android/vpn/ui/notification/AppTPReminderNotificationScheduler.kt
@@ -29,7 +29,6 @@ import com.duckduckgo.common.utils.DispatcherProvider
 import com.duckduckgo.common.utils.plugins.PluginPoint
 import com.duckduckgo.di.scopes.VpnScope
 import com.duckduckgo.mobile.android.app.tracking.AppTrackingProtection
-import com.duckduckgo.mobile.android.vpn.AppTpVpnFeature
 import com.duckduckgo.mobile.android.vpn.feature.removal.VpnFeatureRemover
 import com.duckduckgo.mobile.android.vpn.service.TrackerBlockingVpnService
 import com.duckduckgo.mobile.android.vpn.service.VpnReminderNotificationContentPlugin
@@ -39,15 +38,12 @@ import com.duckduckgo.mobile.android.vpn.service.VpnReminderNotificationWorker
 import com.duckduckgo.mobile.android.vpn.service.VpnReminderReceiver
 import com.duckduckgo.mobile.android.vpn.service.VpnServiceCallbacks
 import com.duckduckgo.mobile.android.vpn.service.notification.getHighestPriorityPluginForType
-import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason
 import com.squareup.anvil.annotations.ContributesMultibinding
 import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicReference
 import javax.inject.Inject
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.flow.drop
-import kotlinx.coroutines.flow.firstOrNull
 import kotlinx.coroutines.launch
 import logcat.logcat
 
@@ -61,7 +57,6 @@ class AppTPReminderNotificationScheduler @Inject constructor(
     private val vpnReminderNotificationBuilder: VpnReminderNotificationBuilder,
     private val vpnReminderNotificationContentPluginPoint: PluginPoint<VpnReminderNotificationContentPlugin>,
     private val appTrackingProtection: AppTrackingProtection,
-    private val vpnStateMonitor: VpnStateMonitor,
 ) : VpnServiceCallbacks {
     private var isAppTPEnabled: AtomicReference<Boolean> = AtomicReference(false)
 
@@ -87,6 +82,7 @@ class AppTPReminderNotificationScheduler @Inject constructor(
                 VpnStopReason.RESTART -> {} // no-op
                 VpnStopReason.SELF_STOP -> onVPNManuallyStopped(coroutineScope)
                 VpnStopReason.REVOKED -> onVPNRevoked()
+                is VpnStopReason.SNOOZED -> onVPNSnooze(coroutineScope, vpnStopReason.snoozedTriggerAtMillis)
                 else -> onVPNUndesiredStop()
             }
         }
@@ -97,15 +93,18 @@ class AppTPReminderNotificationScheduler @Inject constructor(
             if (vpnFeatureRemover.isFeatureRemoved()) {
                 logcat { "VPN Manually stopped because user disabled the feature, nothing to do" }
             } else {
-                val snoozeTrigger = vpnStateMonitor.getStateFlow(AppTpVpnFeature.APPTP_VPN).drop(1).firstOrNull()
-                snoozeTrigger?.let { vpnState ->
-                    if (vpnState.state is VpnStateMonitor.VpnRunningState.DISABLED) {
-                        val state = vpnState.state as VpnStateMonitor.VpnRunningState.DISABLED
-                        if (state.snoozedTriggerAtMillis == null) {
-                            handleNotifForDisabledAppTP()
-                        }
-                    }
-                }
+                handleNotifForDisabledAppTP()
+            }
+        }
+    }
+
+    private fun onVPNSnooze(
+        coroutineScope: CoroutineScope,
+        triggerAtMillis: Long?,
+    ) {
+        coroutineScope.launch(dispatchers.io()) {
+            if (triggerAtMillis == null) {
+                handleNotifForDisabledAppTP()
             }
         }
     }

diff --git a/...om/duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldActivityFeedViewModel.kt b/...om/duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldActivityFeedViewModel.kt
@@ -123,7 +123,7 @@ class DeviceShieldActivityFeedViewModel @Inject constructor(
             .flowOn(dispatcherProvider.io())
             .onStart {
                 startTickerRefresher()
-                emit(TrackerFeedViewState(listOf(TrackerLoadingSkeleton), VpnState(DISABLED(), ERROR)))
+                emit(TrackerFeedViewState(listOf(TrackerLoadingSkeleton), VpnState(DISABLED, ERROR)))
                 delay(300)
             }
     }

diff --git a/...java/com/duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldTrackerActivity.kt b/...java/com/duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldTrackerActivity.kt
@@ -63,6 +63,7 @@ import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnRunningState
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnState
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.REVOKED
 import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.SELF_STOP
+import com.duckduckgo.mobile.android.vpn.state.VpnStateMonitor.VpnStopReason.SNOOZED
 import com.duckduckgo.mobile.android.vpn.ui.AppBreakageCategory
 import com.duckduckgo.mobile.android.vpn.ui.alwayson.AlwaysOnAlertDialogFragment
 import com.duckduckgo.mobile.android.vpn.ui.report.DeviceShieldAppTrackersInfo
@@ -569,25 +570,27 @@ class DeviceShieldTrackerActivity :
             binding.deviceShieldTrackerShieldImage.setImageResource(R.drawable.apptp_shield_disabled)
             binding.deviceShieldTrackerLabelEnabled.gone()
 
-            val (disabledLabel, annotation) = if (runningState.stopReason == REVOKED) {
-                R.string.atp_ActivityRevokedLabel to REPORT_ISSUES_ANNOTATION
-            } else if (runningState.stopReason == SELF_STOP) {
-                R.string.atp_ActivityDisabledLabel to REPORT_ISSUES_ANNOTATION
-            } else {
-                R.string.atp_ActivityDisabledBySystemLabel to RE_ENABLE_ANNOTATION
-            }
-            binding.deviceShieldTrackerLabelDisabled.apply {
-                setClickableLink(
-                    annotation,
-                    getText(disabledLabel),
-                ) {
-                    if (annotation == REPORT_ISSUES_ANNOTATION) {
-                        launchFeedback()
-                    } else if (annotation == RE_ENABLE_ANNOTATION) {
-                        reEnableAppTrackingProtection()
+            if (runningState.stopReason !is SNOOZED) {
+                val (disabledLabel, annotation) = if (runningState.stopReason == REVOKED) {
+                    R.string.atp_ActivityRevokedLabel to REPORT_ISSUES_ANNOTATION
+                } else if (runningState.stopReason == SELF_STOP) {
+                    R.string.atp_ActivityDisabledLabel to REPORT_ISSUES_ANNOTATION
+                } else {
+                    R.string.atp_ActivityDisabledBySystemLabel to RE_ENABLE_ANNOTATION
+                }
+                binding.deviceShieldTrackerLabelDisabled.apply {
+                    setClickableLink(
+                        annotation,
+                        getText(disabledLabel),
+                    ) {
+                        if (annotation == REPORT_ISSUES_ANNOTATION) {
+                            launchFeedback()
+                        } else if (annotation == RE_ENABLE_ANNOTATION) {
+                            reEnableAppTrackingProtection()
+                        }
                     }
+                    show()
                 }
-                show()
             }
         }
     }

diff --git a/...duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldTrackerActivityViewModel.kt b/...duckduckgo/mobile/android/vpn/ui/tracker_activity/DeviceShieldTrackerActivityViewModel.kt
@@ -63,7 +63,7 @@ class DeviceShieldTrackerActivityViewModel @Inject constructor(
         return@withContext vpnStateMonitor
             .getStateFlow(AppTpVpnFeature.APPTP_VPN)
             // we only cared about enabled and disabled states for AppTP
-            .filter { (it.state == VpnStateMonitor.VpnRunningState.ENABLED) || (it.state is VpnStateMonitor.VpnRunningState.DISABLED) }
+            .filter { (it.state == VpnStateMonitor.VpnRunningState.ENABLED) || (it.state == VpnStateMonitor.VpnRunningState.DISABLED) }
             .combine(refreshVpnRunningState.asStateFlow()) { state, _ -> state }
     }