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

More accurate timestamps on risk card (EXPOSUREAPP-1991) #1487

Closed
wants to merge 3 commits into from
Closed

More accurate timestamps on risk card (EXPOSUREAPP-1991) #1487

wants to merge 3 commits into from

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Oct 27, 2020

Via #1473 we now have better infos about when the ENF calculation is finished. Let's display that timestamp instead of when we finished downloading the key pkgs (because download+provision is asynchronous from calculation).

This would fix #948 where the timestamp between the riskcard and the ENF log is off by 1-2 minutes due us using the download timestamp.

For now if there is not yet a timestamp available via CalculationTracker we'll fallback to the download timestamp otherwise everyone would see the wrong info after upgrading. We can remove that fallback in a few weeks after almost everyone has upgraded and CalculationTracker has a high chance of holding data to display.

How to test

  • Trigger a view key pkg submissions and compare the timestamp on the risk card with the timestamps from the ENF log.

@d4rken
Show the timestamp of the last finished calculation on the risk card,
5c57c2e 
instead of the timestamp of the last time we fetched the key pkgs.
@d4rken d4rken added enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers labels Oct 27, 2020
@d4rken d4rken added this to the 1.6.0 milestone Oct 27, 2020
@d4rken d4rken requested a review from a team October 27, 2020 08:39
@harambasicluka
Copy link
Contributor

Hint for testing: Change the landscape to WRU-XD, it looks like that there aren't packages on INT.

harambasicluka
harambasicluka suggested changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@harambasicluka harambasicluka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks fine.

Testing:

Passed UI Settings
[x] 12:19 12:19
[ ] 12:22 12:23
[ ] 12:25 12:24
[x] 12:30 12:30
[ ] 12:33 12:31

Tested on Samsung Galaxy S8 with Android 9 by time traveling from day to day.

2020-10-31 12:47:47.999 2157-2157/de.rki.coronawarnapp.test V/CalculationTrackerStorage: init()
2020-10-31 12:47:47.999 2157-2157/de.rki.coronawarnapp.test V/DefaultCalculationTracker: init()
2020-10-31 12:47:48.005 2157-2157/de.rki.coronawarnapp.test V/DefaultCalculationTracker:HD: init()
2020-10-31 12:47:48.011 2157-2216/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupAutoSave: Observing calculation changes.
2020-10-31 12:47:48.017 2157-2218/de.rki.coronawarnapp.test V/DefaultCalculationTracker:HD: internal onStart
2020-10-31 12:47:48.039 2157-2216/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Loaded calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z)}
2020-10-31 12:47:48.040 2157-2216/de.rki.coronawarnapp.test V/DefaultCalculationTracker:HD: startValue={a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z)}
2020-10-31 12:47:48.042 2157-2219/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Data didn't change, skipping save.
2020-10-31 12:47:48.042 2157-2216/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Running timeout check on: [Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z)]
2020-10-31 12:47:48.043 2157-2216/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Time now: 2020-10-31T11:47:48.042Z
2020-10-31 12:47:48.044 2157-2216/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Data didn't change, skipping save.
2020-11-02 12:48:24.381 2157-2219/de.rki.coronawarnapp.test I/DefaultCalculationTracker: trackNewCalaculation(token=e1c204cd-892d-46e6-92e7-aa2e9b626555)
2020-11-02 12:48:24.385 2157-2222/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=null, finishedAt=null)}
2020-11-02 12:48:43.512 2157-2223/de.rki.coronawarnapp.test I/DefaultCalculationTracker: finishCalculation(token=e1c204cd-892d-46e6-92e7-aa2e9b626555, result=NO_MATCHES)
2020-11-02 12:48:43.520 2157-2222/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z)}
2020-11-02 12:50:48.020 2157-2219/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Running timeout check on: [Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z)]
2020-11-02 12:50:48.025 2157-2219/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Time now: 2020-11-02T11:50:48.022Z
2020-11-02 12:50:48.030 2157-2219/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z)}
2020-11-04 12:51:08.258 2157-2219/de.rki.coronawarnapp.test I/DefaultCalculationTracker: trackNewCalaculation(token=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62)
2020-11-04 12:51:08.262 2157-2222/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z), b20454bf-1211-4a57-8cd7-cd1fdcb5bc62=Calculation(identifier=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62, startedAt=2020-11-04T11:51:08.259Z, result=null, finishedAt=null)}
2020-11-04 12:51:27.408 2157-2222/de.rki.coronawarnapp.test I/DefaultCalculationTracker: finishCalculation(token=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62, result=NO_MATCHES)
2020-11-04 12:51:27.412 2157-2215/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z), b20454bf-1211-4a57-8cd7-cd1fdcb5bc62=Calculation(identifier=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62, startedAt=2020-11-04T11:51:08.259Z, result=NO_MATCHES, finishedAt=2020-11-04T11:51:27.409Z)}
2020-11-04 12:53:48.018 2157-2218/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Running timeout check on: [Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z), Calculation(identifier=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62, startedAt=2020-11-04T11:51:08.259Z, result=NO_MATCHES, finishedAt=2020-11-04T11:51:27.409Z)]
2020-11-04 12:53:48.020 2157-2218/de.rki.coronawarnapp.test V/DefaultCalculationTracker$calculationStates$2$setupTimeoutEnforcer: Time now: 2020-11-04T11:53:48.019Z
2020-11-04 12:53:48.027 2157-2218/de.rki.coronawarnapp.test V/CalculationTrackerStorage: Storing calculation data: {a36074a8-a2b3-42e2-b47b-f9a334647d19=Calculation(identifier=a36074a8-a2b3-42e2-b47b-f9a334647d19, startedAt=2020-10-31T11:43:13.290Z, result=NO_MATCHES, finishedAt=2020-10-31T11:43:30.186Z), e1c204cd-892d-46e6-92e7-aa2e9b626555=Calculation(identifier=e1c204cd-892d-46e6-92e7-aa2e9b626555, startedAt=2020-11-02T11:48:24.382Z, result=NO_MATCHES, finishedAt=2020-11-02T11:48:43.514Z), b20454bf-1211-4a57-8cd7-cd1fdcb5bc62=Calculation(identifier=b20454bf-1211-4a57-8cd7-cd1fdcb5bc62, startedAt=2020-11-04T11:51:08.259Z, result=NO_MATCHES, finishedAt=2020-11-04T11:51:27.409Z)}

@d4rken d4rken marked this pull request as draft October 27, 2020 12:41
@vaubaehn
Copy link
Contributor

vaubaehn commented Oct 27, 2020 •
edited
Loading

For now if there is not yet a timestamp available via CalculationTracker we'll fallback to the download timestamp otherwise everyone would see the wrong info after upgrading. We can remove that fallback in a few weeks after almost everyone has upgraded and CalculationTracker has a high chance of holding data to display.

Wouldn't we need the fallback also later for new/first time installations? Or after a data reset?

@d4rken
Copy link
Member Author

d4rken commented Oct 27, 2020

Wouldn't we need the fallback also later for new/first time installations? Or after a data reset?

At that point the user wouldn't expect a time stamp though, so displaying the text for "no value being available" is okay.

I think it's wouldn't even come to that, as it would display the grey card for "not enough data" which doesn't show a timestamp. Right? 🤔

@d4rken
Copy link
Member Author

d4rken commented Oct 27, 2020

Marked as draft as it turns out that the timestamp from Google does not fit the expected pattern. Going to clarify with Google what their timestamp actually represents, from Luka's tests, it's not just rounding up/down either.

@vaubaehn
Copy link
Contributor

At that point the user wouldn't expect a time stamp though, so displaying the text for "no value being available" is okay.

I think it's wouldn't even come to that, as it would display the grey card for "not enough data" which doesn't show a timestamp. Right? 🤔

I agree, if the risk card refresh is triggered by receiving the two 'exposure check fnished' broadcasts from ENF only ('unknown risk' until the first exposure check result is retrieved), not by closing ProvideDiagnosisKeysTransaction anymore... In my age, the brain needs more effort to re-connect changed flows consistently ;)

@vaubaehn
Copy link
Contributor

from Luka's tests, it's not just rounding up/down either.

@harambasicluka

Hi Luka, if you find some time, could you help me understand your tests:
Table: UI is CWA and Settings is ENF?
Log: the log doesn't correspond to the table, just shows that logging works in general, right?

What I also don't understand:
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardStateTest.kt - line 58
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/common/BaseTracingStateTest.kt - line 65
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsStateTest.kt - line 60
here lastENFCalculation is always referring to lastTimeDiagnosisKeysFetched - shouldn't it refer to lastUpdateDate also for the tests?

Thanks in advance!

@d4rken
Copy link
Member Author

d4rken commented Oct 27, 2020

What I also don't understand:
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardStateTest.kt - line 58
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/common/BaseTracingStateTest.kt - line 65
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsStateTest.kt - line 60
here lastENFCalculation is always referring to lastTimeDiagnosisKeysFetched - shouldn't it refer to lastUpdateDate also for the tests?

It should. Doesn't change the results though, just local variable names. Will fix before this gets merged, (if it get's merged), after we get a response from Google. Good catch 👍

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

60.0% 60.0% Coverage
0.0% 0.0% Duplication

@harambasicluka
Copy link
Contributor

from Luka's tests, it's not just rounding up/down either.

@harambasicluka

Hi Luka, if you find some time, could you help me understand your tests:
Table: UI is CWA and Settings is ENF?
Log: the log doesn't correspond to the table, just shows that logging works in general, right?

@vaubaehn I just checked the timestamp displayed on the risk card and than went in to the os settings (Settings > Google > COVID-19 exposure notifications > Exposure checks) and compared the time displayed here.

@harambasicluka harambasicluka modified the milestones: 1.6.0, 1.7.0 Oct 28, 2020
chris-cwa
chris-cwa reviewed Oct 28, 2020
View reviewed changes
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardStateProvider.kt
tracingRepository.lastTimeDiagnosisKeysFetched.onEach {
Timber.v("lastTimeDiagnosisKeysFetched: $it")
enfClient.latestFinishedCalculation().onEach {
Timber.v("latestFinishedCalculation: $it")
Copy link
Contributor

Choose a reason for hiding this comment

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

#1412 (comment)

@d4rken d4rken changed the base branch from release/1.6.x to release/1.7.x October 28, 2020 20:05
@d4rken d4rken removed this from the 1.7.0 milestone Nov 11, 2020
@d4rken
Copy link
Member Author

d4rken commented Nov 29, 2020

This PR is now too stale, still waiting on clarification from Google.

@d4rken d4rken closed this Nov 29, 2020
@d4rken d4rken deleted the feature/1991-more-accurate-timestamps branch November 29, 2020 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chris-cwa chris-cwa chris-cwa left review comments

@harambasicluka harambasicluka harambasicluka requested changes

Assignees
No one assigned
Labels
enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@d4rken @harambasicluka @vaubaehn @chris-cwa @BMItr @ralfgehrer