Removed traces of grey risk card (EXPOSUREAPP-3051) #1699

SamuraiKek · 2020-11-23T15:05:31Z

Removes the initial "unknown risk" grey card
Adds a new card for "Failed because no internet"
Refactored the RiskLevel to be determined by two other values. Our "RiskLevel" is now based on the whether "does an aggregated result exist" and "a failure reason which must exist if there is no aggregated result". This makes us very flexible to add new failure states the the risk level calculation process. The FailureReasons are enums stored in a database. More robust than the previous plain numbers.
Simplified the RiskCardState by dropping manual key retrieval time an using "isBackgroundJobEnabled" directly.

Testing

Try to produce all different card states 😉

mlenkeit · 2020-11-24T10:01:36Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevel.kt

@@ -58,7 +56,6 @@ enum class RiskLevel(val raw: Int) {
            )
        private val HIGH_RISK_LEVELS = arrayOf(INCREASED_RISK)
        private val LOW_RISK_LEVELS = arrayOf(
-            UNKNOWN_RISK_INITIAL,
            NO_CALCULATION_POSSIBLE_TRACING_OFF,
            LOW_LEVEL_RISK,
            UNKNOWN_RISK_OUTDATED_RESULTS,


What do we need this for?

We were using it for displaying the grey risk cards. Basically before the risk calculation could take place after the onboarding, the initial risk score was attributed this constant value. By removing the grey card, I don't see a need for this constant anymore as the initial risk value is attributed to the LOW_RISK constant now.

mlenkeit · 2020-11-24T10:02:27Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardState.kt

+                RiskLevelConstants.UNKNOWN_RISK_OUTDATED_RESULTS,
+                RiskLevelConstants.UNKNOWN_RISK_OUTDATED_RESULTS_MANUAL ->


What do we need this for?

While these two are also used in relation to the grey risk card, they are also needed for displaying the "Risiko-Ermittlung
nicht möglich" card which is a white card from what I can see in the Android Figma file. AFAIK there are instances where the risk calculation can fail after it's been calculated at least once, so these constants should be relevant for the cards with the white background (at least for now).

mlenkeit · 2020-11-24T10:02:47Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/common/BaseTracingState.kt

                RiskLevelConstants.LOW_LEVEL_RISK -> R.color.colorSemanticLowRisk
-                else -> R.color.colorSemanticNeutralRisk
+                else -> R.color.colorSemanticUnknownRisk


What do we need this for?

Opposite to what the names suggest, the colorSemanticNeutralRisk is the grey color used for the grey card, and colorSemanticUnknownRisk is white. The fallback color was grey before and I've now changed it to white since there won't be a grey card anymore.
Maybe these color names should be interchanged but I didn't want to introduce more unnecessary modifications for now.

...Warn-App/src/test/java/de/rki/coronawarnapp/ui/riskdetails/DefaultRiskDetailPresenterTest.kt

harambasicluka · 2020-11-24T15:24:55Z

@mlenkeit should all grey cards be removed or only the initial one?

harambasicluka · 2020-11-24T16:17:51Z

Agreed with @mlenkeit that this is fine, but @SamuraiKek could you verify when UNKNOWN_RISK_OUTDATED_RESULTS would be visible? Not sure when i have seen it the last time? I think it's in but technically overruled by the white card.

SamuraiKek · 2020-11-25T08:56:29Z

Agreed with @mlenkeit that this is fine, but @SamuraiKek could you verify when UNKNOWN_RISK_OUTDATED_RESULTS would be visible? Not sure when i have seen it the last time? I think it's in but technically overruled by the white card.

I'm not sure when it would be visible. I think the values are assigned in RiskLevelTask, when the calculationNotPossibleBecauseOfOutdatedResults() method is called. Now whether this method is ever called, or if the values are overwritten someplace else, I'm not really sure right now. I'll try and look more into it when working on #1615 but for now, I don't see why keeping them in would cause any issue.

# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevel.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelTask.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/storage/RiskLevelRepository.kt # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardState.kt # Corona-Warn-App/src/main/res/values-bg/strings.xml # Corona-Warn-App/src/main/res/values-de/strings.xml # Corona-Warn-App/src/main/res/values-en/strings.xml # Corona-Warn-App/src/main/res/values-pl/strings.xml # Corona-Warn-App/src/main/res/values-ro/strings.xml # Corona-Warn-App/src/main/res/values-tr/strings.xml # Corona-Warn-App/src/main/res/values/strings.xml # Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelTest.kt # Corona-Warn-App/src/test/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardStateTest.kt

harambasicluka

To resolve the merge conflicts it should be fine if you just add the removed strings again and only remove them in german.

…states.

Refactored "RiskLevel" to be calculated based on either result or failure reasons. Added the new "no internet" error card.

# Conflicts: # Corona-Warn-App/src/main/res/values-bg/strings.xml # Corona-Warn-App/src/main/res/values-en/strings.xml # Corona-Warn-App/src/main/res/values-pl/strings.xml # Corona-Warn-App/src/main/res/values-ro/strings.xml # Corona-Warn-App/src/main/res/values-tr/strings.xml # Corona-Warn-App/src/main/res/values/strings.xml

…into feature/remove_grey_risk_cards

Oliver-Zimmerman · 2020-11-25T15:37:42Z

If we remove the UNKNOWN state then this PR becomes obsolete:
#1686

harambasicluka

Looks really good! I just got one question, will go to device testing now.

...a-Warn-App/src/main/java/de/rki/coronawarnapp/risk/storage/legacy/RiskLevelResultMigrator.kt

harambasicluka

I tested the following states, please correct me if I missed one:

INCREASED_RISK -> via test menu
LOW_LEVEL_RISK -> initially at the start
UNKNOWN_RISK_OUTDATED_RESULTS -> disabled tracing
UNKNOWN_RISK_OUTDATED_RESULTS_MANUAL -> not able to get into this state, but I'm also not sure when I've seen it the last time, should be investigated in a follow up ticket
UNKNOWN_RISK_NO_INTERNET -> turned wifi off, time traveled, forced risk calculation via tester menu

…orona-warn-app/cwa-app-android into feature/remove_grey_risk_cards

…into feature/remove_grey_risk_cards

sonarqubecloud · 2020-11-25T17:37:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

52.1% Coverage
0.0% Duplication

vaubaehn · 2020-11-26T19:45:39Z

Agreed with @mlenkeit that this is fine, but @SamuraiKek could you verify when UNKNOWN_RISK_OUTDATED_RESULTS would be visible? Not sure when i have seen it the last time? I think it's in but technically overruled by the white card.

I'm not sure when it would be visible. I think the values are assigned in RiskLevelTask, when the calculationNotPossibleBecauseOfOutdatedResults() method is called. Now whether this method is ever called, or if the values are overwritten someplace else, I'm not really sure right now. I'll try and look more into it when working on #1615 but for now, I don't see why keeping them in would cause any issue.

Iirc it's also visible in the current version when there is a failure with exposure checking for more than 24 hrs (API 39508, API 10 no public key, BackgroundWorker does not start...), but tracing is on - see also #1081 (EXPOSUREAPP-2796). For now, failure and tracing off will result in the same risk card. But I understood

We move to a new risklevel/failure data state [...]

like this will be addressed elsewhere?

Removed traces of grey risk card.

c2c6ebc

SamuraiKek requested review from janetback and SabineLoss as code owners November 23, 2020 15:05

SamuraiKek requested a review from a team November 23, 2020 15:05

SamuraiKek added the enhancement Improvement of an existing feature label Nov 23, 2020

SamuraiKek added this to the 1.8.0 milestone Nov 23, 2020

Removed UNKNOWN_RISK_INITIAL constant.

57e5fa9

harambasicluka changed the title ~~Removed traces of grey risk card.~~ Removed traces of grey risk card (EXPOSUREAPP-3051) Nov 24, 2020

harambasicluka mentioned this pull request Nov 24, 2020

Grey card undetermine state without wifi (DEV, EXPOSUREAPP-3892) #1686

Closed

mlenkeit reviewed Nov 24, 2020

View reviewed changes

mlenkeit requested a review from harambasicluka November 24, 2020 10:09

Removed unused method.

8c11921

harambasicluka mentioned this pull request Nov 24, 2020

Persist ExposureWindow based risk level calculation results and update the UI (EXPOSUREAPP-3910,EXPOSUREAPP-3855) #1705

Merged

4 tasks

SamuraiKek and others added 2 commits November 24, 2020 16:17

Removed old comments.

8bede9b

Merge branch 'release/1.8.x' into feature/remove_grey_risk_cards

8e97544

d4rken self-assigned this Nov 24, 2020

d4rken self-requested a review November 24, 2020 16:20

harambasicluka previously approved these changes Nov 25, 2020

View reviewed changes

harambasicluka self-assigned this Nov 25, 2020

d4rken added 2 commits November 25, 2020 15:29

Introduce risklevel calculation failure state to get consistent card …

b24b913

…states.

Cleaned up the risk card states.

0467cc4

Refactored "RiskLevel" to be calculated based on either result or failure reasons. Added the new "no internet" error card.

d4rken dismissed harambasicluka’s stale review via 0467cc4 November 25, 2020 15:23

d4rken added 3 commits November 25, 2020 16:23

Merge branch 'release/1.8.x' into feature/remove_grey_risk_cards

8154577

Merge remote-tracking branch 'origin/feature/remove_grey_risk_cards' …

7798aa4

…into feature/remove_grey_risk_cards

harambasicluka unassigned d4rken and harambasicluka Nov 25, 2020

harambasicluka reviewed Nov 25, 2020

View reviewed changes

...a-Warn-App/src/main/java/de/rki/coronawarnapp/risk/storage/legacy/RiskLevelResultMigrator.kt Show resolved Hide resolved

Updated color states.

378f147

harambasicluka previously approved these changes Nov 25, 2020

View reviewed changes

harambasicluka self-assigned this Nov 25, 2020

harambasicluka and others added 2 commits November 25, 2020 17:57

Merge branch 'release/1.8.x' into feature/remove_grey_risk_cards

17b11e6

Fixed card color issues.

1ec9bce

harambasicluka added the maintainers Tag pull requests created by maintainers label Nov 25, 2020

Merge branch 'feature/remove_grey_risk_cards' of https://github.com/c…

d0df086

…orona-warn-app/cwa-app-android into feature/remove_grey_risk_cards

SamuraiKek dismissed harambasicluka’s stale review via d0df086 November 25, 2020 17:17

SamuraiKek and others added 5 commits November 25, 2020 19:23

Fixed detekt issues.

de73afb

Remove test for deleted file.

c00b6cd

Merge remote-tracking branch 'origin/feature/remove_grey_risk_cards' …

303b3ea

…into feature/remove_grey_risk_cards

Fix test regressions.

b491243

Fixed tests and LINTs.

9a15b0b

d4rken approved these changes Nov 25, 2020

View reviewed changes

harambasicluka approved these changes Nov 25, 2020

View reviewed changes

d4rken self-assigned this Nov 25, 2020

d4rken merged commit 86b29f7 into release/1.8.x Nov 25, 2020

d4rken deleted the feature/remove_grey_risk_cards branch November 25, 2020 17:41

dsarkar mentioned this pull request Dec 14, 2020

Layout-Bug for TracingCard UI states resulting in not readable risk card/risk details fragment #1598

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed traces of grey risk card (EXPOSUREAPP-3051) #1699

Removed traces of grey risk card (EXPOSUREAPP-3051) #1699

SamuraiKek commented Nov 23, 2020 •

edited by d4rken

Loading

mlenkeit Nov 24, 2020

SamuraiKek Nov 24, 2020

mlenkeit Nov 24, 2020

SamuraiKek Nov 24, 2020

mlenkeit Nov 24, 2020

SamuraiKek Nov 24, 2020

harambasicluka commented Nov 24, 2020

harambasicluka commented Nov 24, 2020

SamuraiKek commented Nov 25, 2020

harambasicluka left a comment

Oliver-Zimmerman commented Nov 25, 2020

harambasicluka left a comment

harambasicluka left a comment

sonarqubecloud bot commented Nov 25, 2020

vaubaehn commented Nov 26, 2020

		RiskLevelConstants.UNKNOWN_RISK_OUTDATED_RESULTS,
		RiskLevelConstants.UNKNOWN_RISK_OUTDATED_RESULTS_MANUAL ->

Removed traces of grey risk card (EXPOSUREAPP-3051) #1699

Removed traces of grey risk card (EXPOSUREAPP-3051) #1699

Conversation

SamuraiKek commented Nov 23, 2020 • edited by d4rken Loading

Testing

mlenkeit Nov 24, 2020

Choose a reason for hiding this comment

SamuraiKek Nov 24, 2020

Choose a reason for hiding this comment

mlenkeit Nov 24, 2020

Choose a reason for hiding this comment

SamuraiKek Nov 24, 2020

Choose a reason for hiding this comment

mlenkeit Nov 24, 2020

Choose a reason for hiding this comment

SamuraiKek Nov 24, 2020

Choose a reason for hiding this comment

harambasicluka commented Nov 24, 2020

harambasicluka commented Nov 24, 2020

SamuraiKek commented Nov 25, 2020

harambasicluka left a comment

Choose a reason for hiding this comment

Oliver-Zimmerman commented Nov 25, 2020

harambasicluka left a comment

Choose a reason for hiding this comment

harambasicluka left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 25, 2020

vaubaehn commented Nov 26, 2020

SamuraiKek commented Nov 23, 2020 •

edited by d4rken

Loading