This repository has been archived by the owner on Jun 20, 2023. It is now read-only.
Fix refresh check and improve test menu (EXPOSUREAPP-4049) #1772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s missleading data. While it was called "lastTimeDiagnosisKeysFromServerFetch" it was actually "last time we submitted keys to google". * To decide whether to refresh in "onResume", we now use "has there been any submission to the ENF?" * To display a timestamp on the risk card, we take the last successful submission to the ENF as the risk card displays the calculation results based on the latest submission. While we could use the last calculated risk level result timestamp, we currently also trigger risk calculations if there are no new submissions to the ENF, which would mean the timestamp is updated even though the result is not based on new data. I've also fixed the test fragment button behavior and added descriptions, the "Reset risk level" button surfaced the initial issue because it behaved like a "do a 75% data reset" button.
d4rken
added
bug
ralfgehrer
reviewed
Dec 1, 2020
| import kotlinx.coroutines.flow.first | ||
| import kotlinx.coroutines.flow.map | ||
|
|
||
| suspend fun ExposureDetectionTracker.lastSubmission( |
There was a problem hiding this comment.
I would not mind moving this into the same file as the interface itself. Was shortly confused about the location of lastSubmission when reviewing this PR
ralfgehrer
approved these changes
Dec 1, 2020
harambasicluka
approved these changes
Dec 1, 2020
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove "lastTimeDiagnosisKeysFromServerFetch" and replace it with less missleading data.
While it was called "lastTimeDiagnosisKeysFromServerFetch" it was actually "last time we submitted keys for exposure detection". Which was also missleading because it does not directly relate to what the risk card is displaying. It's neither the last download (because it only get's set on submission) and it get's set on start of submission so it likely does not represent the latest risk level until the second risk level calculation has been executed.
startedAt).finishedAt) to the ENF as the risk card displays the calculation results, which in turn are based on the latest submission. While we could use the last calculated risk level result timestamp, we currently also trigger risk calculations if there are no new submissions to the ENF, which would mean the timestamp is updated even though the result is not based on new data.onResumewe should consider displaying the last risk level result timestamp on the card. This is only viable though when the risk level task only get's launched as reaction to finished exposure detections.I've also fixed the test fragment button behavior and added descriptions, the "Reset risk level" button surfaced the initial issue because it behaved like a "do a 75% data reset" button.
@harambasicluka the bug you described here was likely caused by the incorrectly working test menu button "Reset risklevel", did more than just reset the risk level, it reset
LocalData. This lead to downloaded diagnosis keys existing such that no new download was deemed necessary. As no new downloaded happened the "lastTimeDiagnosisKeysFromServerFetch" used for detecting new installs was never set again despite being reset by "Reset risk level", this meant thatval wasNotYetFetched = LocalData.lastTimeDiagnosisKeysFromServerFetch() == nullalways returned true and let the risk level calculation be launched.