Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

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

Merged

Conversation

LukasLechnerDev
Copy link
Contributor

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 runs only once a day.

This PR adds another function to CheckInRepository, called checkInWithinRetention. This function only returns the check-ins which end date isn't older than 15 days.

This new function is now used:

  • CheckInViewModel: To not show outdated check-ins
  • MainActivityViewModel: To not include outdated check-ins in the badge-count in Bottom Nav
  • SubmissionTask: to not submit outdated check-ins
  • CheckInWarningMatcher: To not match outdated check-ins

I also refactored the retention logic of TraceLocations by also adding a traceLocationsWithinRetention property to DefaultTraceLocationRepository.

@LukasLechnerDev LukasLechnerDev requested a review from a team April 6, 2021 15:48
@LukasLechnerDev LukasLechnerDev added the maintainers Tag pull requests created by maintainers label Apr 6, 2021
@LukasLechnerDev LukasLechnerDev added this to the 2.0.0 milestone Apr 6, 2021
@vaubaehn
Copy link
Contributor

vaubaehn commented Apr 6, 2021

@LukasLechnerDev
Hi Lukas, some short questions to understand the idea behind this PR:
Why did you decide on 'hiding' the stale check-ins until the worker is run, instead of cleaning the database when the app is openend/going to foreground? Does database cleaning take much time or could cause any conflict in the opened app? Could there be any scenario, where a stale check-in could cause a problem (bug) but cannot easily be spotted by the users, because it's hidden?

@harambasicluka harambasicluka added the prio PRs to review first. label Apr 7, 2021
@LukasLechnerDev
Copy link
Contributor Author

Hi @vaubaehn ! Great questions!

Why did you decide on 'hiding' the stale check-ins until the worker is run, instead of cleaning the database when the app is openend/going to foreground?

We decided that doing it on every app start is not enough (apps can be started and running for a long time) and doing it every time the app is brought to the foreground is too much :). Either way, we still could have outdated check-ins as some time will pass from the point in time when the user opens the app or brings it to foreground and then sees the check-ins or submits the check-ins.

Therefore, I think it is best to just load the up-to date check-ins whenever we need them and also let a worker run daily to clean up all outdated checkins.

Does database cleaning take much time or could cause any conflict in the opened app?

I don't think it takes that much time. One issue we could have if we clean the db when the user is currently in the check-ins screen could be that suddenly some outdated check-ins disappear.

Could there be any scenario, where a stale check-in could cause a problem (bug) but cannot easily be spotted by the users, because it's hidden?

I can't think of such a scenario ...

@mtwalli mtwalli self-assigned this Apr 7, 2021
@LukasLechnerDev LukasLechnerDev force-pushed the feature/5910-Only-use-checkins-of-last-15-days branch from 3f60ea8 to 7de7a6f Compare April 7, 2021 12:57
@vaubaehn
Copy link
Contributor

vaubaehn commented Apr 7, 2021

@LukasLechnerDev
Hi Lukas, very convincing answers!
Thanks a lot for your thorough reply, hope it didn't steal much of your time.
I can follow you in all points, and I now understand why increasing complexity a bit here is a good idea.
The only critical point for possible issues then is the consistent usage of checkInWithinRetention function and traceLocationsWithinRetention property always and everywhere (but db cleaning), and not to mix up with allCheckIns, right? Could it then be helpful for future 'CWA newbies' to add a short comment on when to use allCheckIns and when to use checkInWithinRetention and traceLocationsWithinRetention to the basic repos (i.e., CheckInRepository.kt) to avoid confusion and mix up?

I'm very excitedly looking forward for release 2.0, pressing all thumbs and wishing it to become a great success!

@LukasLechnerDev
Copy link
Contributor Author

@vaubaehn Thanks for your feedback. You are right, it's definitely useful to document these functions better. I have added a commit (see above 👆)

Thank you for your wishes! We are also very excited for 2.0 🚀

mtwalli
mtwalli previously approved these changes Apr 7, 2021
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 😊

…ast-15-days

# Conflicts:
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/presencetracing/risk/CheckInWarningMatcher.kt
#	Corona-Warn-App/src/test/java/de/rki/coronawarnapp/presencetracing/risk/CheckInWarningMatcherTest.kt
…PresenceTracingWarningTask.kt and TraceWarningPackageSyncTool.kt
@jurajkusnier jurajkusnier self-assigned this Apr 8, 2021
LukasLechnerDev and others added 3 commits April 8, 2021 15:22
…ast-15-days

# Conflicts:
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/eventregistration/organizer/list/TraceLocationsViewModel.kt
#	Corona-Warn-App/src/test/java/de/rki/coronawarnapp/submission/task/SubmissionTaskTest.kt
@sonarcloud
Copy link

sonarcloud bot commented Apr 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

93.6% 93.6% Coverage
0.0% 0.0% Duplication

@LukasLechnerDev LukasLechnerDev merged commit 21f37b3 into release/2.0.x Apr 8, 2021
@LukasLechnerDev LukasLechnerDev deleted the feature/5910-Only-use-checkins-of-last-15-days branch April 8, 2021 15:03
SamuraiKek pushed a commit that referenced this pull request Apr 9, 2021
* 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>
fynngodau pushed a commit to fynngodau/cwa-app-android that referenced this pull request Apr 10, 2021
…p#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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers prio PRs to review first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants