Only use check-ins of last 15 days (EXPOSUREAPP-5910) (#2752)

* Only use check-ins of last 15 days * Refactor TraceLocation Retention * Fix SubmissionTaskTest.kt * Fix MainActivityViewModelTest.kt * Address PR feedback * Add CheckInRetentionTest.kt and TraceLocationRetentionTest.kt * Add some comments * Add more comments * Fix tests * Load checkInsWithinRetention instead of allCheckIns in ContactDiary, PresenceTracingWarningTask.kt and TraceWarningPackageSyncTool.kt Co-authored-by: harambasicluka <64483219+harambasicluka@users.noreply.github.com> Co-authored-by: Mohamed <mohamed.metwalli@sap.com>
corona-warn-app · Apr 9, 2021 · 20db850 · 20db850
1 parent 3d8901d
commit 20db850
Show file tree

Hide file tree

Showing 23 changed files with 353 additions and 55 deletions.
diff --git a/.../main/java/de/rki/coronawarnapp/contactdiary/ui/overview/ContactDiaryOverviewViewModel.kt b/.../main/java/de/rki/coronawarnapp/contactdiary/ui/overview/ContactDiaryOverviewViewModel.kt
@@ -60,15 +60,15 @@ class ContactDiaryOverviewViewModel @AssistedInject constructor(
 
     private val riskLevelPerDateFlow = riskLevelStorage.ewDayRiskStates
     private val traceLocationCheckInRiskFlow = riskLevelStorage.traceLocationCheckInRiskStates
-    private val allCheckInsFlow = checkInRepository.allCheckIns
+    private val checkInsWithinRetentionFlow = checkInRepository.checkInsWithinRetention
 
     val listItems = combine(
         flowOf(dates),
         locationVisitsFlow,
         personEncountersFlow,
         riskLevelPerDateFlow,
         traceLocationCheckInRiskFlow,
-        allCheckInsFlow
+        checkInsWithinRetentionFlow
     ) { dateList, locationVisists, personEncounters, riskLevelPerDateList, traceLocationCheckInRiskList, checkInList ->
         mutableListOf<DiaryOverviewItem>().apply {
             add(OverviewSubHeaderItem)

diff --git a/...rn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckInRepository.kt b/...rn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckInRepository.kt
@@ -3,6 +3,7 @@ package de.rki.coronawarnapp.eventregistration.checkins
 import de.rki.coronawarnapp.eventregistration.storage.TraceLocationDatabase
 import de.rki.coronawarnapp.eventregistration.storage.dao.CheckInDao
 import de.rki.coronawarnapp.eventregistration.storage.entity.toCheckIn
+import de.rki.coronawarnapp.util.TimeStamper
 import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.map
@@ -13,7 +14,8 @@ import javax.inject.Singleton
 
 @Singleton
 class CheckInRepository @Inject constructor(
-    traceLocationDatabaseFactory: TraceLocationDatabase.Factory
+    traceLocationDatabaseFactory: TraceLocationDatabase.Factory,
+    private val timeStamper: TimeStamper
 ) {
 
     private val traceLocationDatabase: TraceLocationDatabase by lazy {
@@ -24,10 +26,27 @@ class CheckInRepository @Inject constructor(
         traceLocationDatabase.eventCheckInDao()
     }
 
+    /**
+     * Returns all stored check-ins
+     *
+     * Attention: this could also include check-ins that are older than
+     * the retention period. Therefore, you should probably use [checkInsWithinRetention]
+     */
     val allCheckIns: Flow<List<CheckIn>> = checkInDao
         .allEntries()
         .map { list -> list.map { it.toCheckIn() } }
 
+    /**
+     * Returns check-ins that are within the retention period. Even though we have a worker that deletes all stale
+     * check-ins it's still possible to have stale check-ins in the database because the worker only runs once a day.
+     */
+    val checkInsWithinRetention: Flow<List<CheckIn>> = allCheckIns.map { checkInList ->
+        val now = timeStamper.nowUTC
+        checkInList.filter { checkIn ->
+            checkIn.isWithinRetention(now)
+        }
+    }
+
     suspend fun getCheckInById(checkInId: Long): CheckIn? {
         Timber.d("getCheckInById(checkInId=$checkInId)")
         return checkInDao.entryForId(checkInId)?.toCheckIn()

diff --git a/...arn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckInRetention.kt b/...arn-App/src/main/java/de/rki/coronawarnapp/eventregistration/checkins/CheckInRetention.kt
@@ -0,0 +1,21 @@
+package de.rki.coronawarnapp.eventregistration.checkins
+
+import de.rki.coronawarnapp.util.TimeAndDateExtensions.seconds
+import org.joda.time.Instant
+import java.util.concurrent.TimeUnit
+
+private const val CHECK_IN_RETENTION_DAYS = 15
+private val CHECK_IN_RETENTION_SECONDS = TimeUnit.DAYS.toSeconds(CHECK_IN_RETENTION_DAYS.toLong())
+
+/**
+ * returns true if the end date of the check-in isn't older than [CHECK_IN_RETENTION_DAYS], otherwise false
+ */
+fun CheckIn.isWithinRetention(now: Instant): Boolean {
+    val retentionThreshold = (now.seconds - CHECK_IN_RETENTION_SECONDS)
+    return checkInEnd.seconds >= retentionThreshold
+}
+
+/**
+ * Returns true if a check-in is stale and therefore can be deleted, otherwise false
+ */
+fun CheckIn.isOutOfRetention(now: Instant) = !isWithinRetention(now)
diff --git a/...ava/de/rki/coronawarnapp/eventregistration/storage/repo/DefaultTraceLocationRepository.kt b/...ava/de/rki/coronawarnapp/eventregistration/storage/repo/DefaultTraceLocationRepository.kt
@@ -6,6 +6,8 @@ import de.rki.coronawarnapp.eventregistration.checkins.qrcode.toTraceLocations
 import de.rki.coronawarnapp.eventregistration.storage.TraceLocationDatabase
 import de.rki.coronawarnapp.eventregistration.storage.dao.TraceLocationDao
 import de.rki.coronawarnapp.eventregistration.storage.entity.toTraceLocationEntity
+import de.rki.coronawarnapp.eventregistration.storage.retention.isWithinRetention
+import de.rki.coronawarnapp.util.TimeStamper
 import de.rki.coronawarnapp.util.coroutine.AppScope
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.flow.Flow
@@ -18,7 +20,8 @@ import javax.inject.Singleton
 @Singleton
 class DefaultTraceLocationRepository @Inject constructor(
     traceLocationDatabaseFactory: TraceLocationDatabase.Factory,
-    @AppScope private val appScope: CoroutineScope
+    @AppScope private val appScope: CoroutineScope,
+    private val timeStamper: TimeStamper
 ) : TraceLocationRepository {
 
     private val traceLocationDatabase: TraceLocationDatabase by lazy {
@@ -36,9 +39,28 @@ class DefaultTraceLocationRepository @Inject constructor(
         return checkIn.toTraceLocation()
     }
 
+    /**
+     * Returns all stored trace locations
+     *
+     * Attention: this could also include trace locations that are older than
+     * the retention period. Therefore, you should probably use [traceLocationsWithinRetention]
+     */
     override val allTraceLocations: Flow<List<TraceLocation>>
         get() = traceLocationDao.allEntries().map { it.toTraceLocations() }
 
+    /**
+     * Returns trace locations that are within the retention period. Even though we have a worker that deletes all stale
+     * trace locations it's still possible to have stale trace-locations in the database because the worker only runs
+     * once a day.
+     */
+    override val traceLocationsWithinRetention: Flow<List<TraceLocation>>
+        get() = allTraceLocations.map { traceLocationList ->
+            val now = timeStamper.nowUTC
+            traceLocationList.filter { traceLocation ->
+                traceLocation.isWithinRetention(now)
+            }
+        }
+
     override suspend fun addTraceLocation(traceLocation: TraceLocation): TraceLocation {
         Timber.d("Add trace location: %s", traceLocation)
         val traceLocationEntity = traceLocation.toTraceLocationEntity()

diff --git a/.../main/java/de/rki/coronawarnapp/eventregistration/storage/repo/TraceLocationRepository.kt b/.../main/java/de/rki/coronawarnapp/eventregistration/storage/repo/TraceLocationRepository.kt
@@ -5,8 +5,21 @@ import kotlinx.coroutines.flow.Flow
 
 interface TraceLocationRepository {
 
+    /**
+     * Returns all stored trace locations
+     *
+     * Attention: this could also include trace locations that are older than
+     * the retention period. Therefore, you should probably use [traceLocationsWithinRetention]
+     */
     val allTraceLocations: Flow<List<TraceLocation>>
 
+    /**
+     * Returns trace locations that are within the retention period. Even though we have a worker that deletes all stale
+     * trace locations it's still possible to have stale trace-locations in the database because the worker only runs
+     * once a day.
+     */
+    val traceLocationsWithinRetention: Flow<List<TraceLocation>>
+
     suspend fun traceLocationForId(id: Long): TraceLocation
 
     suspend fun addTraceLocation(traceLocation: TraceLocation): TraceLocation

diff --git a/.../src/main/java/de/rki/coronawarnapp/eventregistration/storage/retention/CheckInCleaner.kt b/.../src/main/java/de/rki/coronawarnapp/eventregistration/storage/retention/CheckInCleaner.kt
@@ -2,11 +2,10 @@ package de.rki.coronawarnapp.eventregistration.storage.retention
 
 import dagger.Reusable
 import de.rki.coronawarnapp.eventregistration.checkins.CheckInRepository
-import de.rki.coronawarnapp.util.TimeAndDateExtensions.seconds
+import de.rki.coronawarnapp.eventregistration.checkins.isOutOfRetention
 import de.rki.coronawarnapp.util.TimeStamper
 import kotlinx.coroutines.flow.first
 import timber.log.Timber
-import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 
 @Reusable
@@ -17,18 +16,15 @@ class CheckInCleaner @Inject constructor(
 
     suspend fun cleanUp() {
         Timber.d("Starting to clean up stale check-ins.")
-        val retentionThreshold = (timeStamper.nowUTC.seconds - RETENTION_SECONDS)
+
+        val now = timeStamper.nowUTC
         val checkInsToDelete = checkInRepository.allCheckIns.first()
-            .filter {
-                it.checkInEnd.seconds < retentionThreshold
-            }
+            .filter { checkIn -> checkIn.isOutOfRetention(now) }
+
         Timber.d("Cleaning up ${checkInsToDelete.size} stale check-ins.")
+
         checkInRepository.deleteCheckIns(checkInsToDelete)
-        Timber.d("Clean up of stale check-ins completed.")
-    }
 
-    companion object {
-        private const val RETENTION_DAYS = 15
-        private val RETENTION_SECONDS = TimeUnit.DAYS.toSeconds(RETENTION_DAYS.toLong())
+        Timber.d("Clean up of stale check-ins completed.")
     }
 }
diff --git a/...ain/java/de/rki/coronawarnapp/eventregistration/storage/retention/TraceLocationCleaner.kt b/...ain/java/de/rki/coronawarnapp/eventregistration/storage/retention/TraceLocationCleaner.kt
@@ -2,11 +2,9 @@ package de.rki.coronawarnapp.eventregistration.storage.retention
 
 import dagger.Reusable
 import de.rki.coronawarnapp.eventregistration.storage.repo.TraceLocationRepository
-import de.rki.coronawarnapp.util.TimeAndDateExtensions.seconds
 import de.rki.coronawarnapp.util.TimeStamper
 import kotlinx.coroutines.flow.first
 import timber.log.Timber
-import java.util.concurrent.TimeUnit
 import javax.inject.Inject
 
 @Reusable
@@ -17,19 +15,16 @@ class TraceLocationCleaner @Inject constructor(
 
     suspend fun cleanUp() {
         Timber.d("Starting to clean up stale trace locations.")
-        val retentionThreshold = (timeStamper.nowUTC.seconds - RETENTION_SECONDS)
+
+        val now = timeStamper.nowUTC
         traceLocationRepository.allTraceLocations.first()
-            .filter {
-                it.endDate != null && it.endDate.seconds < retentionThreshold
+            .filter { traceLocation ->
+                traceLocation.isOutOfRetention(now)
             }.forEach {
                 Timber.d("Cleaning up stale trace location: %s", it)
                 traceLocationRepository.deleteTraceLocation(it)
             }
-        Timber.d("Clean up of stale trace locations completed.")
-    }
 
-    companion object {
-        private const val RETENTION_DAYS = 15
-        private val RETENTION_SECONDS = TimeUnit.DAYS.toSeconds(RETENTION_DAYS.toLong())
+        Timber.d("Clean up of stale trace locations completed.")
     }
 }
diff --git a/...n/java/de/rki/coronawarnapp/eventregistration/storage/retention/TraceLocationRetention.kt b/...n/java/de/rki/coronawarnapp/eventregistration/storage/retention/TraceLocationRetention.kt
@@ -0,0 +1,27 @@
+package de.rki.coronawarnapp.eventregistration.storage.retention
+
+import de.rki.coronawarnapp.eventregistration.checkins.qrcode.TraceLocation
+import de.rki.coronawarnapp.util.TimeAndDateExtensions.seconds
+import org.joda.time.Instant
+import java.util.concurrent.TimeUnit
+
+private const val TRACE_LOCATION_RETENTION_DAYS = 15
+private val TRACE_LOCATION_RETENTION_SECONDS = TimeUnit.DAYS.toSeconds(TRACE_LOCATION_RETENTION_DAYS.toLong())
+
+/**
+ * returns true if a trace location either has no end date or has an end date that isn't older than
+ * [TRACE_LOCATION_RETENTION_DAYS], otherwise false
+ */
+fun TraceLocation.isWithinRetention(now: Instant): Boolean {
+    val retentionThreshold = (now.seconds - TRACE_LOCATION_RETENTION_SECONDS)
+    return if (endDate == null) {
+        true
+    } else {
+        endDate.seconds >= retentionThreshold
+    }
+}
+
+/**
+ * Returns true if a trace location is stale and therefore can be deleted, otherwise false
+ */
+fun TraceLocation.isOutOfRetention(now: Instant) = !isWithinRetention(now)
diff --git a/...in/java/de/rki/coronawarnapp/presencetracing/risk/execution/PresenceTracingWarningTask.kt b/...in/java/de/rki/coronawarnapp/presencetracing/risk/execution/PresenceTracingWarningTask.kt
@@ -67,7 +67,7 @@ class PresenceTracingWarningTask @Inject constructor(
 
         presenceTracingRiskRepository.deleteStaleData()
 
-        val checkIns = checkInsRepository.allCheckIns.firstOrNull() ?: emptyList()
+        val checkIns = checkInsRepository.checkInsWithinRetention.firstOrNull() ?: emptyList()
         Timber.tag(TAG).d("There are %d check-ins to match against.", checkIns.size)
 
         if (checkIns.isEmpty()) {

diff --git a/...java/de/rki/coronawarnapp/presencetracing/warning/download/TraceWarningPackageSyncTool.kt b/...java/de/rki/coronawarnapp/presencetracing/warning/download/TraceWarningPackageSyncTool.kt
@@ -41,7 +41,7 @@ class TraceWarningPackageSyncTool @Inject constructor(
     internal suspend fun syncPackagesForLocation(location: LocationCode): SyncResult {
         Timber.tag(TAG).d("syncTraceWarningPackages(location=%s)", location)
 
-        val oldestCheckIn = checkInRepository.allCheckIns.first().minByOrNull { it.checkInStart }.also {
+        val oldestCheckIn = checkInRepository.checkInsWithinRetention.first().minByOrNull { it.checkInStart }.also {
             Timber.tag(TAG).d("Our oldest check-in is %s", it)
         }
 

diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/task/SubmissionTask.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/task/SubmissionTask.kt
@@ -142,7 +142,7 @@ class SubmissionTask @Inject constructor(
         )
         Timber.tag(TAG).d("Transformed keys with symptoms %s from %s to %s", symptoms, keys, transformedKeys)
 
-        val checkIns = checkInsRepository.allCheckIns.first()
+        val checkIns = checkInsRepository.checkInsWithinRetention.first()
         val transformedCheckIns = checkInsTransformer.transform(checkIns, symptoms)
 
         Timber.tag(TAG).d("Transformed CheckIns from: %s to: %s", checkIns, transformedCheckIns)

diff --git a/...ain/java/de/rki/coronawarnapp/ui/eventregistration/attendee/checkins/CheckInsViewModel.kt b/...ain/java/de/rki/coronawarnapp/ui/eventregistration/attendee/checkins/CheckInsViewModel.kt
@@ -56,7 +56,7 @@ class CheckInsViewModel @AssistedInject constructor(
 
     val checkins: LiveData<List<CheckInsItem>> = combine(
         intervalFlow(1000),
-        checkInsRepository.allCheckIns,
+        checkInsRepository.checkInsWithinRetention,
         cameraPermissionProvider.deniedPermanently
     ) { _, checkIns, denied ->
         mutableListOf<CheckInsItem>().apply {

diff --git a/.../java/de/rki/coronawarnapp/ui/eventregistration/organizer/list/TraceLocationsViewModel.kt b/.../java/de/rki/coronawarnapp/ui/eventregistration/organizer/list/TraceLocationsViewModel.kt
@@ -9,7 +9,6 @@ import de.rki.coronawarnapp.eventregistration.checkins.qrcode.TraceLocation
 import de.rki.coronawarnapp.eventregistration.storage.repo.TraceLocationRepository
 import de.rki.coronawarnapp.ui.eventregistration.organizer.list.items.TraceLocationItem
 import de.rki.coronawarnapp.ui.eventregistration.organizer.list.items.TraceLocationVH
-import de.rki.coronawarnapp.util.TimeStamper
 import de.rki.coronawarnapp.util.coroutine.DispatcherProvider
 import de.rki.coronawarnapp.util.ui.SingleLiveEvent
 import de.rki.coronawarnapp.util.viewmodel.CWAViewModel
@@ -21,16 +20,13 @@ class TraceLocationsViewModel @AssistedInject constructor(
     dispatcherProvider: DispatcherProvider,
     checkInsRepository: CheckInRepository,
     private val traceLocationRepository: TraceLocationRepository,
-    private val timeStamper: TimeStamper
 ) : CWAViewModel(dispatcherProvider = dispatcherProvider) {
 
     val events = SingleLiveEvent<TraceLocationEvent>()
 
-    val traceLocations: LiveData<List<TraceLocationItem>> = traceLocationRepository.allTraceLocations
+    val traceLocations: LiveData<List<TraceLocationItem>> = traceLocationRepository.traceLocationsWithinRetention
         .map { traceLocations ->
-            traceLocations
-                .filter { it.endDate == null || it.endDate.isAfter(timeStamper.nowUTC.toDateTime().minusDays(15)) }
-                .sortedBy { it.description }
+            traceLocations.sortedBy { it.description }
         }
         .combine(checkInsRepository.allCheckIns) { traceLocations, checkIns ->
             traceLocations.map { traceLocation ->

diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainActivityViewModel.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainActivityViewModel.kt
@@ -42,7 +42,7 @@ class MainActivityViewModel @AssistedInject constructor(
     private val mutableIsTraceLocationOnboardingDone = MutableLiveData<Boolean>()
     val isTraceLocationOnboardingDone: LiveData<Boolean> = mutableIsTraceLocationOnboardingDone
 
-    val activeCheckIns = checkInRepository.allCheckIns
+    val activeCheckIns = checkInRepository.checkInsWithinRetention
         .map { checkins -> checkins.filter { !it.completed }.size }
         .asLiveData(context = dispatcherProvider.Default)
 

diff --git a/...t/java/de/rki/coronawarnapp/contactdiary/ui/overview/ContactDiaryOverviewViewModelTest.kt b/...t/java/de/rki/coronawarnapp/contactdiary/ui/overview/ContactDiaryOverviewViewModelTest.kt
@@ -69,7 +69,7 @@ open class ContactDiaryOverviewViewModelTest {
         every { contactDiaryRepository.personEncounters } returns flowOf(emptyList())
         every { riskLevelStorage.ewDayRiskStates } returns flowOf(emptyList())
         every { riskLevelStorage.traceLocationCheckInRiskStates } returns flowOf(emptyList())
-        every { checkInRepository.allCheckIns } returns flowOf(emptyList())
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(emptyList())
 
         mockStringsForContactDiaryExporterTests(context)
         every { timeStamper.nowUTC } returns Instant.ofEpochMilli(dateMillis)
@@ -361,7 +361,7 @@ open class ContactDiaryOverviewViewModelTest {
         every { riskLevelStorage.ewDayRiskStates } returns flowOf(listOf(aggregatedRiskPerDateResultLowRisk))
         every { riskLevelStorage.traceLocationCheckInRiskStates } returns flowOf(listOf(traceLocationCheckInRiskHigh))
         every { contactDiaryRepository.locationVisits } returns flowOf(listOf(locationEventHighRiskVisit))
-        every { checkInRepository.allCheckIns } returns flowOf(listOf(checkInHigh))
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(listOf(checkInHigh))
 
         var item = createInstance().listItems.getOrAwaitValue().first {
             it is DayOverviewItem && it.date == date
@@ -384,7 +384,7 @@ open class ContactDiaryOverviewViewModelTest {
         )
         every { riskLevelStorage.traceLocationCheckInRiskStates } returns flowOf(listOf(traceLocationCheckInRiskLow))
         every { contactDiaryRepository.locationVisits } returns flowOf(listOf(locationEventLowRiskVisit))
-        every { checkInRepository.allCheckIns } returns flowOf(listOf(checkInLow))
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(listOf(checkInLow))
 
         item = createInstance().listItems.getOrAwaitValue().first {
             it is DayOverviewItem && it.date == date
@@ -414,7 +414,7 @@ open class ContactDiaryOverviewViewModelTest {
     fun `low risk event by attending event with low risk`() {
         every { contactDiaryRepository.locationVisits } returns flowOf(listOf(locationEventLowRiskVisit))
         every { riskLevelStorage.traceLocationCheckInRiskStates } returns flowOf(listOf(traceLocationCheckInRiskLow))
-        every { checkInRepository.allCheckIns } returns flowOf(listOf(checkInLow))
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(listOf(checkInLow))
 
         val item = createInstance().listItems.getOrAwaitValue().first {
             it is DayOverviewItem && it.date == date
@@ -436,7 +436,7 @@ open class ContactDiaryOverviewViewModelTest {
     fun `high risk event by attending event with high risk`() {
         every { contactDiaryRepository.locationVisits } returns flowOf(listOf(locationEventHighRiskVisit))
         every { riskLevelStorage.traceLocationCheckInRiskStates } returns flowOf(listOf(traceLocationCheckInRiskHigh))
-        every { checkInRepository.allCheckIns } returns flowOf(listOf(checkInHigh))
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(listOf(checkInHigh))
 
         val item = createInstance().listItems.getOrAwaitValue().first {
             it is DayOverviewItem && it.date == date
@@ -469,7 +469,7 @@ open class ContactDiaryOverviewViewModelTest {
             )
         )
 
-        every { checkInRepository.allCheckIns } returns flowOf(listOf(checkInHigh, checkInLow))
+        every { checkInRepository.checkInsWithinRetention } returns flowOf(listOf(checkInHigh, checkInLow))
 
         val item = createInstance().listItems.getOrAwaitValue().first {
             it is DayOverviewItem && it.date == date