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

Adjust EW/PT riskresult combination (EXPOSUREAPP-6169) #2788

Merged
merged 13 commits into from
Apr 9, 2021
Merged

Adjust EW/PT riskresult combination (EXPOSUREAPP-6169) #2788

merged 13 commits into from
Apr 9, 2021

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Apr 9, 2021 •
edited
Loading

  • The home screen uses a "CombinedRiskResult" consisting of the PT and EW risk result, the combination behavior was adjusted in this PR. A failed calculation from either PT or EW, will cause the combined result to also have the state "failed", so either risk calculation failing should show the error card with the retry button on the home screen.
  • The EW risk result has a value matchedKeyCount to detect "low risk, despite encounters", which is used in the risk details screen to display an additional info box, and show a link to the FAQ. There is now a new matchedRiskCount which is the combination of matchedKeyCount (EW) and checkInOverlapCount (PT).
  • Some refactoring as I incorrectly renamed two methods previously.
  • Tracing progress status now also reflects download/riskcalculation of the PresenceTracingWarningTask.

d4rken added 2 commits April 9, 2021 11:14
@d4rken
Refactoring, move extensions closer to their use-case.
fe9eecf
@d4rken
Adjust riskState combination, if either calculation fails, the combin…
462aca8 
…ed riskstate is also CALCULATION_FAILED and should show the white failure card on the home screen.
@d4rken d4rken added the maintainers Tag pull requests created by maintainers label Apr 9, 2021
@d4rken d4rken added this to the 2.0.0 milestone Apr 9, 2021
@d4rken d4rken marked this pull request as ready for review April 9, 2021 13:30
@d4rken d4rken requested a review from a team April 9, 2021 13:30
@harambasicluka harambasicluka added the prio PRs to review first. label Apr 9, 2021
@LukasLechnerDev LukasLechnerDev self-assigned this Apr 9, 2021
@chris-cwa chris-cwa self-assigned this Apr 9, 2021
@chris-cwa chris-cwa self-requested a review April 9, 2021 14:08
@LukasLechnerDev
Copy link
Contributor

I think it not good to show the user a failed risk calculation if one of the two fails. He could have a high risk because of EW, but since the PT fails, he sees the failed Risk card instead of the high-risk card and so isn't aware that he has a high risk.

@harambasicluka
Copy link
Contributor

I think it not good to show the user a failed risk calculation if one of the two fails. He could have a high risk because of EW, but since the PT fails, he sees the failed Risk card instead of the high-risk card and so isn't aware that he has a high risk.

@mlenkeit what do you think?

@d4rken
Copy link
Member Author

d4rken commented Apr 9, 2021

I think it not good to show the user a failed risk calculation if one of the two fails. He could have a high risk because of EW, but since the PT fails, he sees the failed Risk card instead of the high-risk card and so isn't aware that he has a high risk.

I discussed this with Max ( /cc @mlenkeit ), I think both variants are valid, if desired we can change this back in a follow up PR on monday.

@mlenkeit
Copy link
Member

mlenkeit commented Apr 9, 2021

I think it not good to show the user a failed risk calculation if one of the two fails. He could have a high risk because of EW, but since the PT fails, he sees the failed Risk card instead of the high-risk card and so isn't aware that he has a high risk.

I discussed this with Max ( /cc @mlenkeit ), I think both variants are valid, if desired we can change this back in a follow up PR on monday.

Was originally discussed with POs. Matthias is right.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 9, 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

32.2% 32.2% Coverage
0.0% 0.0% Duplication

@harambasicluka harambasicluka merged commit ac00163 into release/2.0.x Apr 9, 2021
@harambasicluka harambasicluka deleted the fix/6169-risk-calc-polishing branch April 9, 2021 14:59
fynngodau pushed a commit to fynngodau/cwa-app-android that referenced this pull request Apr 10, 2021
@d4rken @LukasLechnerDev
@harambasicluka @fynngodau
Adjust EW/PT riskresult combination (EXPOSUREAPP-6169) (corona-warn-a…
7f25cff 
…pp#2788)

* Refactoring, move extensions closer to their use-case.

* Adjust riskState combination, if either calculation fails, the combined riskstate is also CALCULATION_FAILED and should show the white failure card on the home screen.

* Introduce ptRiskLevelResult.checkInOverlapCount as analog to ewRiskLevelResult.matchedKeyCount for the CombinedEwPtRiskLevelResult to allow the UI show information for "low risk with encounters" situations.

* LINTs

* Adjust unit tests to reflect the risk status combination priority (FAILED>HIGH>LOW).

* Home screen download/calculation progress should include PresenceTracingWarningTask

* LINTs

Co-authored-by: Lukas Lechner <lukas.lechner@sap.com>
Co-authored-by: harambasicluka <64483219+harambasicluka@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@LukasLechnerDev LukasLechnerDev LukasLechnerDev approved these changes

@chris-cwa chris-cwa chris-cwa approved these changes

Assignees

@LukasLechnerDev LukasLechnerDev

@chris-cwa chris-cwa

Labels
maintainers Tag pull requests created by maintainers prio PRs to review first.
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@d4rken @LukasLechnerDev @harambasicluka @mlenkeit @chris-cwa