From 4d1ecb54fa8a08b853feb2980f8fff8b33fc49f9 Mon Sep 17 00:00:00 2001 From: chris-cwa <69595386+chris-cwa@users.noreply.github.com> Date: Fri, 13 Nov 2020 11:01:33 +0100 Subject: [PATCH] Detect changes to app config (EXPOSUREAPP-2718) (#1583) * + config changed detector * tests * ConfigChangeDetector.kt improvements. * Fix config collecting scoping * Move the identifier update into the RiskLevelTask * Add test coverage * Logs NO-OPs too. * Filter emissions early if the value didn't change. * extracted constant Co-authored-by: Matthias Urhahn <matthias.urhahn@sap.com> --- .../coronawarnapp/CoronaWarnApplication.kt | 4 + .../appconfig/ConfigChangeDetector.kt | 63 +++++++++++ .../coronawarnapp/risk/DefaultRiskLevels.kt | 12 +- .../rki/coronawarnapp/risk/RiskLevelData.kt | 31 ++++++ .../rki/coronawarnapp/risk/RiskLevelTask.kt | 11 +- .../de/rki/coronawarnapp/risk/RiskLevels.kt | 6 +- .../appconfig/ConfigChangeDetectorTest.kt | 104 ++++++++++++++++++ .../coronawarnapp/risk/RiskLevelDataTest.kt | 40 +++++++ .../coronawarnapp/risk/RiskLevelTaskTest.kt | 80 ++++++++++++++ .../rki/coronawarnapp/risk/RiskLevelsTest.kt | 5 +- 10 files changed, 343 insertions(+), 13 deletions(-) create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetector.kt create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelData.kt create mode 100644 Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetectorTest.kt create mode 100644 Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelDataTest.kt create mode 100644 Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelTaskTest.kt diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/CoronaWarnApplication.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/CoronaWarnApplication.kt index ebe7c473f..4f6ca66e8 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/CoronaWarnApplication.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/CoronaWarnApplication.kt @@ -11,6 +11,7 @@ import androidx.work.WorkManager import dagger.android.AndroidInjector import dagger.android.DispatchingAndroidInjector import dagger.android.HasAndroidInjector +import de.rki.coronawarnapp.appconfig.ConfigChangeDetector import de.rki.coronawarnapp.bugreporting.loghistory.LogHistoryTree import de.rki.coronawarnapp.deadman.DeadmanNotificationScheduler import de.rki.coronawarnapp.exception.reporting.ErrorReportReceiver @@ -44,6 +45,7 @@ class CoronaWarnApplication : Application(), HasAndroidInjector { @Inject lateinit var taskController: TaskController @Inject lateinit var foregroundState: ForegroundState @Inject lateinit var workManager: WorkManager + @Inject lateinit var configChangeDetector: ConfigChangeDetector @Inject lateinit var deadmanNotificationScheduler: DeadmanNotificationScheduler @LogHistoryTree @Inject lateinit var rollingLogHistory: Timber.Tree @@ -79,6 +81,8 @@ class CoronaWarnApplication : Application(), HasAndroidInjector { if (LocalData.onboardingCompletedTimestamp() != null) { deadmanNotificationScheduler.schedulePeriodic() } + + configChangeDetector.launch() } private val activityLifecycleCallback = object : ActivityLifecycleCallbacks { diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetector.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetector.kt new file mode 100644 index 000000000..cf17b9f3e --- /dev/null +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetector.kt @@ -0,0 +1,63 @@ +package de.rki.coronawarnapp.appconfig + +import androidx.annotation.VisibleForTesting +import de.rki.coronawarnapp.risk.RiskLevel +import de.rki.coronawarnapp.risk.RiskLevelData +import de.rki.coronawarnapp.risk.RiskLevelTask +import de.rki.coronawarnapp.storage.RiskLevelRepository +import de.rki.coronawarnapp.task.TaskController +import de.rki.coronawarnapp.task.common.DefaultTaskRequest +import de.rki.coronawarnapp.util.coroutine.AppScope +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.catch +import kotlinx.coroutines.flow.distinctUntilChangedBy +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach +import timber.log.Timber +import javax.inject.Inject + +class ConfigChangeDetector @Inject constructor( + private val appConfigProvider: AppConfigProvider, + private val taskController: TaskController, + @AppScope private val appScope: CoroutineScope, + private val riskLevelData: RiskLevelData +) { + + fun launch() { + Timber.v("Monitoring config changes.") + appConfigProvider.currentConfig + .distinctUntilChangedBy { it.identifier } + .onEach { + Timber.v("Running app config change checks.") + check(it.identifier) + } + .catch { Timber.e(it, "App config change checks failed.") } + .launchIn(appScope) + } + + @VisibleForTesting + internal fun check(newIdentifier: String) { + if (riskLevelData.lastUsedConfigIdentifier == null) { + // No need to reset anything if we didn't calculate a risklevel yet. + Timber.d("Config changed, but no previous identifier is available.") + return + } + + val oldConfigId = riskLevelData.lastUsedConfigIdentifier + if (newIdentifier != oldConfigId) { + Timber.i("New config id ($newIdentifier) differs from last one ($oldConfigId), resetting.") + RiskLevelRepositoryDeferrer.resetRiskLevel() + taskController.submit(DefaultTaskRequest(RiskLevelTask::class)) + } else { + Timber.v("Config identifier ($oldConfigId) didn't change, NOOP.") + } + } + + @VisibleForTesting + internal object RiskLevelRepositoryDeferrer { + + fun resetRiskLevel() { + RiskLevelRepository.setRiskLevelScore(RiskLevel.UNDETERMINED) + } + } +} diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/DefaultRiskLevels.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/DefaultRiskLevels.kt index 68d9b8b15..d0ca47e3b 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/DefaultRiskLevels.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/DefaultRiskLevels.kt @@ -5,7 +5,7 @@ import androidx.core.app.NotificationManagerCompat import com.google.android.gms.nearby.exposurenotification.ExposureSummary import de.rki.coronawarnapp.CoronaWarnApplication import de.rki.coronawarnapp.R -import de.rki.coronawarnapp.appconfig.AppConfigProvider +import de.rki.coronawarnapp.appconfig.RiskCalculationConfig import de.rki.coronawarnapp.exception.RiskLevelCalculationException import de.rki.coronawarnapp.notification.NotificationHelper import de.rki.coronawarnapp.risk.RiskLevel.UNKNOWN_RISK_INITIAL @@ -20,9 +20,7 @@ import javax.inject.Singleton import kotlin.math.round @Singleton -class DefaultRiskLevels @Inject constructor( - private val appConfigProvider: AppConfigProvider -) : RiskLevels { +class DefaultRiskLevels @Inject constructor() : RiskLevels { override fun updateRepository(riskLevel: RiskLevel, time: Long) { val rollbackItems = mutableListOf<RollbackItem>() @@ -78,8 +76,10 @@ class DefaultRiskLevels @Inject constructor( } } - override suspend fun isIncreasedRisk(lastExposureSummary: ExposureSummary): Boolean { - val appConfiguration = appConfigProvider.getAppConfig() + override suspend fun isIncreasedRisk( + lastExposureSummary: ExposureSummary, + appConfiguration: RiskCalculationConfig + ): Boolean { Timber.tag(TAG).v("Retrieved configuration from backend") // custom attenuation parameters to weigh the attenuation // values provided by the Google API diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelData.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelData.kt new file mode 100644 index 000000000..83372c3f5 --- /dev/null +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelData.kt @@ -0,0 +1,31 @@ +package de.rki.coronawarnapp.risk + +import android.content.Context +import androidx.core.content.edit +import de.rki.coronawarnapp.util.di.AppContext +import javax.inject.Inject +import javax.inject.Singleton + +@Singleton +class RiskLevelData @Inject constructor( + @AppContext private val context: Context +) { + + private val prefs by lazy { + context.getSharedPreferences(NAME_SHARED_PREFS, Context.MODE_PRIVATE) + } + + /** + * The identifier of the config used during the last risklevel calculation + */ + var lastUsedConfigIdentifier: String? + get() = prefs.getString(PKEY_RISKLEVEL_CALC_LAST_CONFIG_ID, null) + set(value) = prefs.edit(true) { + putString(PKEY_RISKLEVEL_CALC_LAST_CONFIG_ID, value) + } + + companion object { + private const val NAME_SHARED_PREFS = "risklevel_localdata" + private const val PKEY_RISKLEVEL_CALC_LAST_CONFIG_ID = "risklevel.config.identifier.last" + } +} diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelTask.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelTask.kt index 81959b959..556afb09b 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelTask.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelTask.kt @@ -2,6 +2,8 @@ package de.rki.coronawarnapp.risk import android.content.Context import com.google.android.gms.nearby.exposurenotification.ExposureSummary +import de.rki.coronawarnapp.appconfig.AppConfigProvider +import de.rki.coronawarnapp.appconfig.ConfigData import de.rki.coronawarnapp.exception.ExceptionCategory import de.rki.coronawarnapp.exception.RiskLevelCalculationException import de.rki.coronawarnapp.exception.reporting.report @@ -38,7 +40,9 @@ class RiskLevelTask @Inject constructor( @AppContext private val context: Context, private val enfClient: ENFClient, private val timeStamper: TimeStamper, - private val backgroundModeStatus: BackgroundModeStatus + private val backgroundModeStatus: BackgroundModeStatus, + private val riskLevelData: RiskLevelData, + private val appConfigProvider: AppConfigProvider ) : Task<DefaultProgress, RiskLevelTask.Result> { private val internalProgress = ConflatedBroadcastChannel<DefaultProgress>() @@ -60,6 +64,8 @@ class RiskLevelTask @Inject constructor( return Result(NO_CALCULATION_POSSIBLE_TRACING_OFF) } + val configData: ConfigData = appConfigProvider.getAppConfig() + with(riskLevels) { return Result( when { @@ -75,7 +81,7 @@ class RiskLevelTask @Inject constructor( UNKNOWN_RISK_OUTDATED_RESULTS_MANUAL } - isIncreasedRisk(getNewExposureSummary()).also { + isIncreasedRisk(getNewExposureSummary(), configData).also { checkCancel() } -> INCREASED_RISK @@ -87,6 +93,7 @@ class RiskLevelTask @Inject constructor( }.also { checkCancel() updateRepository(it, timeStamper.nowUTC.millis) + riskLevelData.lastUsedConfigIdentifier = configData.identifier } ) } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevels.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevels.kt index b8cd2f00c..7389beeaf 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevels.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevels.kt @@ -1,6 +1,7 @@ package de.rki.coronawarnapp.risk import com.google.android.gms.nearby.exposurenotification.ExposureSummary +import de.rki.coronawarnapp.appconfig.RiskCalculationConfig import de.rki.coronawarnapp.server.protocols.internal.AttenuationDurationOuterClass interface RiskLevels { @@ -15,7 +16,10 @@ interface RiskLevels { */ fun isActiveTracingTimeAboveThreshold(): Boolean - suspend fun isIncreasedRisk(lastExposureSummary: ExposureSummary): Boolean + suspend fun isIncreasedRisk( + lastExposureSummary: ExposureSummary, + appConfiguration: RiskCalculationConfig + ): Boolean fun updateRepository( riskLevel: RiskLevel, diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetectorTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetectorTest.kt new file mode 100644 index 000000000..e06b40b3e --- /dev/null +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/appconfig/ConfigChangeDetectorTest.kt @@ -0,0 +1,104 @@ +package de.rki.coronawarnapp.appconfig + +import de.rki.coronawarnapp.risk.RiskLevelData +import de.rki.coronawarnapp.task.TaskController +import io.mockk.MockKAnnotations +import io.mockk.Runs +import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.just +import io.mockk.mockk +import io.mockk.mockkObject +import io.mockk.verify +import io.mockk.verifySequence +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.TestCoroutineScope +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import testhelpers.BaseTest + +class ConfigChangeDetectorTest : BaseTest() { + + @MockK lateinit var appConfigProvider: AppConfigProvider + @MockK lateinit var taskController: TaskController + @MockK lateinit var riskLevelData: RiskLevelData + + private val currentConfigFake = MutableStateFlow(mockConfigId("initial")) + + @BeforeEach + fun setup() { + MockKAnnotations.init(this) + + mockkObject(ConfigChangeDetector.RiskLevelRepositoryDeferrer) + every { ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() } just Runs + + every { taskController.submit(any()) } just Runs + every { appConfigProvider.currentConfig } returns currentConfigFake + } + + private fun mockConfigId(id: String): ConfigData { + return mockk<ConfigData>().apply { + every { identifier } returns id + } + } + + private fun createInstance() = ConfigChangeDetector( + appConfigProvider = appConfigProvider, + taskController = taskController, + appScope = TestCoroutineScope(), + riskLevelData = riskLevelData + ) + + @Test + fun `new identifier without previous one is ignored`() { + + every { riskLevelData.lastUsedConfigIdentifier } returns null + + createInstance().launch() + + verify(exactly = 0) { + taskController.submit(any()) + ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() + } + } + + @Test + fun `new identifier results in new risk level calculation`() { + every { riskLevelData.lastUsedConfigIdentifier } returns "I'm a new identifier" + + createInstance().launch() + + verifySequence { + ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() + taskController.submit(any()) + } + } + + @Test + fun `same idetifier results in no op`() { + every { riskLevelData.lastUsedConfigIdentifier } returns "initial" + + createInstance().launch() + + verify(exactly = 0) { + taskController.submit(any()) + ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() + } + } + + @Test + fun `new emissions keep triggering the check`() { + every { riskLevelData.lastUsedConfigIdentifier } returns "initial" + + createInstance().launch() + currentConfigFake.value = mockConfigId("Straw") + currentConfigFake.value = mockConfigId("berry") + + verifySequence { + ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() + taskController.submit(any()) + ConfigChangeDetector.RiskLevelRepositoryDeferrer.resetRiskLevel() + taskController.submit(any()) + } + } +} diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelDataTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelDataTest.kt new file mode 100644 index 000000000..41a2b3517 --- /dev/null +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelDataTest.kt @@ -0,0 +1,40 @@ +package de.rki.coronawarnapp.risk + +import android.content.Context +import io.kotest.matchers.shouldBe +import io.mockk.MockKAnnotations +import io.mockk.every +import io.mockk.impl.annotations.MockK +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import testhelpers.BaseTest +import testhelpers.preferences.MockSharedPreferences + +class RiskLevelDataTest : BaseTest() { + + @MockK lateinit var context: Context + lateinit var preferences: MockSharedPreferences + + @BeforeEach + fun setup() { + MockKAnnotations.init(this) + preferences = MockSharedPreferences() + every { context.getSharedPreferences("risklevel_localdata", Context.MODE_PRIVATE) } returns preferences + } + + fun createInstance() = RiskLevelData(context = context) + + @Test + fun `update last used config identifier`() { + createInstance().apply { + lastUsedConfigIdentifier shouldBe null + lastUsedConfigIdentifier = "Banana" + lastUsedConfigIdentifier shouldBe "Banana" + preferences.dataMapPeek.containsValue("Banana") shouldBe true + + lastUsedConfigIdentifier = null + lastUsedConfigIdentifier shouldBe null + preferences.dataMapPeek.isEmpty() shouldBe true + } + } +} diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelTaskTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelTaskTest.kt new file mode 100644 index 000000000..889dc3a37 --- /dev/null +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelTaskTest.kt @@ -0,0 +1,80 @@ +package de.rki.coronawarnapp.risk + +import android.content.Context +import android.net.ConnectivityManager +import android.net.Network +import android.net.NetworkCapabilities +import de.rki.coronawarnapp.appconfig.AppConfigProvider +import de.rki.coronawarnapp.appconfig.ConfigData +import de.rki.coronawarnapp.nearby.ENFClient +import de.rki.coronawarnapp.task.Task +import de.rki.coronawarnapp.util.BackgroundModeStatus +import de.rki.coronawarnapp.util.TimeStamper +import io.mockk.MockKAnnotations +import io.mockk.Runs +import io.mockk.coEvery +import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.just +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.runBlockingTest +import org.joda.time.Instant +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import testhelpers.BaseTest + +class RiskLevelTaskTest : BaseTest() { + + @MockK lateinit var riskLevels: RiskLevels + @MockK lateinit var context: Context + @MockK lateinit var enfClient: ENFClient + @MockK lateinit var timeStamper: TimeStamper + @MockK lateinit var backgroundModeStatus: BackgroundModeStatus + @MockK lateinit var riskLevelData: RiskLevelData + @MockK lateinit var configData: ConfigData + @MockK lateinit var appConfigProvider: AppConfigProvider + + private val arguments: Task.Arguments = object : Task.Arguments {} + + private fun createTask() = RiskLevelTask( + riskLevels = riskLevels, + context = context, + enfClient = enfClient, + timeStamper = timeStamper, + backgroundModeStatus = backgroundModeStatus, + riskLevelData = riskLevelData, + appConfigProvider = appConfigProvider + ) + + @BeforeEach + fun setup() { + MockKAnnotations.init(this) + coEvery { appConfigProvider.getAppConfig() } returns configData + every { configData.identifier } returns "config-identifier" + + every { context.getSystemService(Context.CONNECTIVITY_SERVICE) } returns mockk<ConnectivityManager>().apply { + every { activeNetwork } returns mockk<Network>().apply { + every { getNetworkCapabilities(any()) } returns mockk<NetworkCapabilities>().apply { + every { hasCapability(any()) } returns true + } + } + } + + every { enfClient.isTracingEnabled } returns flowOf(true) + every { timeStamper.nowUTC } returns Instant.EPOCH + every { riskLevels.updateRepository(any(), any()) } just Runs + + every { riskLevelData.lastUsedConfigIdentifier = any() } just Runs + } + + @Test + fun `last used config ID is set after calculation`() = runBlockingTest { + every { riskLevels.calculationNotPossibleBecauseOfNoKeys() } returns true + val task = createTask() + task.run(arguments) + + verify { riskLevelData.lastUsedConfigIdentifier = "config-identifier" } + } +} diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelsTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelsTest.kt index 845722a2c..fee774fd3 100644 --- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelsTest.kt +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelsTest.kt @@ -1,11 +1,9 @@ package de.rki.coronawarnapp.risk import com.google.android.gms.nearby.exposurenotification.ExposureSummary -import de.rki.coronawarnapp.appconfig.AppConfigProvider import de.rki.coronawarnapp.server.protocols.internal.AttenuationDurationOuterClass import io.kotest.matchers.shouldBe import io.mockk.MockKAnnotations -import io.mockk.impl.annotations.MockK import junit.framework.TestCase.assertEquals import org.junit.Before import org.junit.Test @@ -13,13 +11,12 @@ import testhelpers.BaseTest class RiskLevelsTest : BaseTest() { - @MockK lateinit var appConfigProvider: AppConfigProvider private lateinit var riskLevels: DefaultRiskLevels @Before fun setUp() { MockKAnnotations.init(this) - riskLevels = DefaultRiskLevels(appConfigProvider) + riskLevels = DefaultRiskLevels() } @Test -- GitLab