Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) #1751

d4rken · 2020-11-27T17:30:17Z

Removed RiskLevelConstants which we not useful anymore and just unnecessary complicated
Simplified RiskLevel to RiskState which just has 3 states now INCREASED_RISK, LOW_RISK, FAILED
Simplified state behavior of the risk cards on the homescreen.
- Disabled tracing overrides all other cards, as it's a current state we want the user to fix
- Otherwise if the last calculation failed (for any reason), we show the "tracing failed" card
- If the last calculation did not fail we display either a high or low risk card

TODO

No data yet, fresh install, airplane mode, manually press "restart", card seems to be just empty (light mode)
No data yet, fresh install, airplane mode, manually press "restart", card seems to be just empty, now enable WIFI, card is empty then green card appears (light mode)
Tracing disabled, light mode, divider is missing or invisible
Missing "contacts" when displaying low risk card
Negative days when time traveling backwards
Remove "minimum tracing time active" restrictions which should be removed according to EXPOSUREAPP-3051 (moved to extra PR Remove minimum tracing time requirement (EXPOSUREAPP-3051) #1762)

Testing

Make sure all 4 cards look correct (compare with figma, though it's not 100% current)
- Red card (high rish)
- Green card (low risk)
- Tracing stopped
- Tracing failed
Check all designs in dark mode too

Replaced RiskLevel and RiskLevelConstants with RiskState { LOW; HIGH, FAILED }.

# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/storage/BaseRiskLevelStorage.kt # Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/storage/internal/PersistedRiskResultDaoTest.kt

chiljamgossow

Left some suggestions on how to make is easier to understand

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

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

chiljamgossow · 2020-11-30T14:32:43Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsState.kt

@@ -26,28 +29,26 @@ data class TracingDetailsState(
     * in all cases when risk level is not increased
     */
    fun isBehaviorNormalVisible(): Boolean =
-        riskLevelScore != RiskLevelConstants.INCREASED_RISK
+        riskState != INCREASED_RISK


Is there a reason why we do not simply call it HIGH_RISK? (independent of the wording we use to display it to the user) As far as I understand, it's either low = green or high = red

I had it previously named like that and @harambasicluka expressed the desire to keep it INCREASED_RISK.

I wanted to have it consistently used in our app and as I think it's called increased risk in the protobuff files and since the beginning I would stick to increased risk.

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsState.kt

chiljamgossow · 2020-11-30T14:42:01Z

...arn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsStateProvider.kt

        )

+        val isRestartButtonEnabled = !isBackgroundJobEnabled || latestCalc.riskState == RiskState.CALCULATION_FAILED


Same here isBackgroundJosDisabled instead of negation

Then we would have to negate isAutoModeEnabled and negate it again in other places.

I thought about having both, like val isBackgroundJobDisabled with get() =!isBackgroundJobEnabled, but it's just a suggestion

Corona-Warn-App/src/main/res/layout/fragment_risk_details.xml

Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/RiskLevelChangeDetectorTest.kt

# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelChangeDetector.kt

harambasicluka

Coding LGTM, just one question, we did you choose such a dramatic title with: ... Chapter1? :D

Will do device testing.

d4rken · 2020-11-30T15:48:56Z

Coding LGTM, just one question, we did you choose such a dramatic title with: ... Chapter1? :D

The implementation is still not where it should be. It's still one multipurpose card doing everything, and I want to have separate cards and layouts, the home screen should be using a list with dynamic elements. Time did not allow me yet to do this, so Chapter 2: RiskCards strike back! ™️ 😉.

ralfgehrer

Did some serious device testing! Great work. Works very smoothly.

harambasicluka · 2020-11-30T16:40:31Z

Is this a typo?
Begegnungen an 1 Tag mit erhöhtem Risiko

I would suggest Begegnungen an einem Tag mit erhöhtem Risiko <- also this sounds strange

harambasicluka · 2020-11-30T16:49:40Z

I now have a flickering when navigating from home to risk details as the download tries to start.

As it's converted to a gif you only see the loading indicator as I navigate from home to details, but I see it one device every time.

sonarqubecloud · 2020-11-30T16:53:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

70.2% Coverage
0.0% Duplication

harambasicluka · 2020-11-30T16:56:03Z

I deactivated wifi during downloading got in the correct state, also disabled tracing and this was visible. Then I enabled tracing and wifi and the download started. as it was showing the download it turned from the failure to green, but still was in the loading state and displaying that it's downloading and the second state of the loading state for the calculation was visible but seemed to be executes.

harambasicluka

have noted down some follow up tasks:

card flickering: tracingrepository.refreshRisklevel
color change before apparent risk calculation
string singular high risk

vaubaehn · 2020-12-01T10:47:56Z

@harambasicluka

Is this a typo?
Begegnungen an 1 Tag mit erhöhtem Risiko

I would suggest Begegnungen an einem Tag mit erhöhtem Risiko <- also this sounds strange

image

I would propose:
'An 1 Tag Begegnungen mit erhöhtem Risiko'
Or
'An einem Tag Begegnungen mit erhöhtem Risiko'
Or
'Sie hatten an einem Tag Begegnungen mit erhöhtem Risiko'

OT: What is the beautiful 'Corona-C' inside the ~~snack~~statusbar in the screenshot? ForegroundService? Looks nice!

harambasicluka · 2020-12-01T11:53:02Z

@harambasicluka

Is this a typo?
Begegnungen an 1 Tag mit erhöhtem Risiko
I would suggest Begegnungen an einem Tag mit erhöhtem Risiko <- also this sounds strange
image

I would propose:
'An 1 Tag Begegnungen mit erhöhtem Risiko'
Or
'An einem Tag Begegnungen mit erhöhtem Risiko'
Or
'Sie hatten an einem Tag Begegnungen mit erhöhtem Risiko'

Thank's for your proposal, we currently discussing it with our User Assistance colleagues :) But I think we'll stick to the structure, even if I would prefer your idea :)

OT: What is the beautiful 'Corona-C' inside the ~~snack~~statusbar in the screenshot? ForegroundService? Looks nice!

It's one of our notifications, so that's one of my test devices not sure if I got an reminder to update the more more frequently or my risk changed from low to high :) Both is already in production ;)

vaubaehn · 2020-12-01T14:56:09Z

@harambasicluka

we currently discussing it with our User Assistance colleagues :) But I think we'll stick to the structure

Let's see, I found UA being quite exact wrt language, maybe they'll also see the need for a change :)

OT: What is the beautiful 'Corona-C' inside the ~~snack~~statusbar in the screenshot? ForegroundService? Looks nice!

It's one of our notifications, so that's one of my test devices not sure if I got an reminder to update the more more frequently or my risk changed from low to high :) Both is already in production ;)

Ha, never saw a notification yet, that's why I d didn't get it immediately ;)

d4rken added 4 commits November 25, 2020 22:15

Fix displayed details in tracing inactive states.

ef8712d

Merge branch 'release/1.8.x' into fix/riskcard-states

ba53f66

Merge branch 'release/1.8.x' into fix/riskcard-states

636780e

Simplified UI states.

90cc361

Replaced RiskLevel and RiskLevelConstants with RiskState { LOW; HIGH, FAILED }.

d4rken added bug Something isn't working enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers labels Nov 27, 2020

d4rken added this to the 1.8.0 milestone Nov 27, 2020

d4rken requested a review from a team November 27, 2020 17:30

d4rken marked this pull request as draft November 27, 2020 17:51

d4rken added 9 commits November 27, 2020 18:53

Fix low risk card missing the encounters if there have been more than 0.

ee2abb6

Fix missing card divider/border on error cards in light mode.

4e2368d

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

3187060

# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/storage/BaseRiskLevelStorage.kt # Corona-Warn-App/src/test/java/de/rki/coronawarnapp/risk/storage/internal/PersistedRiskResultDaoTest.kt

Fix merge regression.

44ac058

LINTs

4da2d66

Fix progress card colors on white card background color.

a22daf0

Fix progress card colors on white card background color.

e39aa66

Don't allow negative active tracing days.

485a937

Revert sync behavior (extra PR)

9e75ad2

d4rken mentioned this pull request Nov 28, 2020

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

Closed

3 tasks

d4rken changed the title ~~Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049)~~ Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) Nov 28, 2020

d4rken marked this pull request as ready for review November 29, 2020 21:16

chiljamgossow self-assigned this Nov 30, 2020

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

5ca52a5

chiljamgossow reviewed Nov 30, 2020

View reviewed changes

d4rken added 2 commits November 30, 2020 15:55

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

ae74bb7

# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/risk/RiskLevelChangeDetector.kt

Address PR comments.

12c54b7

harambasicluka reviewed Nov 30, 2020

View reviewed changes

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

9a38fee

ralfgehrer reviewed Nov 30, 2020

View reviewed changes

harambasicluka self-assigned this Nov 30, 2020

ralfgehrer approved these changes Nov 30, 2020

View reviewed changes

harambasicluka added 2 commits November 30, 2020 17:35

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

1438b39

Merge branch 'release/1.8.x' into fix/4049-refactor-riskcards

5c47d64

harambasicluka approved these changes Nov 30, 2020

View reviewed changes

harambasicluka merged commit 7ce4be5 into release/1.8.x Nov 30, 2020

harambasicluka deleted the fix/4049-refactor-riskcards branch November 30, 2020 17:14

d4rken mentioned this pull request Dec 1, 2020

Fix refresh check and improve test menu (EXPOSUREAPP-4049) #1772

Merged

dsarkar mentioned this pull request Dec 11, 2020

Indeterminate risk status after onboarding with no network #1830

Closed

3 tasks

This was referenced Jan 24, 2021

Unknown Risk status no longer used corona-warn-app/cwa-website#802

Closed

Unused Unknown Risk appears in Overview #2198

Closed

MikeMcC399 mentioned this pull request Mar 25, 2021

RKI: Outdated app risk screenshots corona-warn-app/cwa-website#1026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) #1751

Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) #1751

d4rken commented Nov 27, 2020 •

edited

Loading

chiljamgossow left a comment

chiljamgossow Nov 30, 2020

d4rken Nov 30, 2020

harambasicluka Nov 30, 2020

chiljamgossow Nov 30, 2020

d4rken Nov 30, 2020

chiljamgossow Nov 30, 2020 •

edited

Loading

harambasicluka left a comment

d4rken commented Nov 30, 2020 •

edited

Loading

ralfgehrer left a comment

harambasicluka commented Nov 30, 2020

harambasicluka commented Nov 30, 2020

sonarqubecloud bot commented Nov 30, 2020

harambasicluka commented Nov 30, 2020

harambasicluka left a comment

vaubaehn commented Dec 1, 2020 •

edited

Loading

harambasicluka commented Dec 1, 2020

vaubaehn commented Dec 1, 2020

		)

		val isRestartButtonEnabled = !isBackgroundJobEnabled \|\| latestCalc.riskState == RiskState.CALCULATION_FAILED

Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) #1751

Refactor riskcard & states; Chapter1 (EXPOSUREAPP-4049, EXPOSUREAPP-3834) #1751

Conversation

d4rken commented Nov 27, 2020 • edited Loading

TODO

Testing

chiljamgossow left a comment

Choose a reason for hiding this comment

chiljamgossow Nov 30, 2020

Choose a reason for hiding this comment

d4rken Nov 30, 2020

Choose a reason for hiding this comment

harambasicluka Nov 30, 2020

Choose a reason for hiding this comment

chiljamgossow Nov 30, 2020

Choose a reason for hiding this comment

d4rken Nov 30, 2020

Choose a reason for hiding this comment

chiljamgossow Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

harambasicluka left a comment

Choose a reason for hiding this comment

d4rken commented Nov 30, 2020 • edited Loading

ralfgehrer left a comment

Choose a reason for hiding this comment

harambasicluka commented Nov 30, 2020

harambasicluka commented Nov 30, 2020

sonarqubecloud bot commented Nov 30, 2020

harambasicluka commented Nov 30, 2020

harambasicluka left a comment

Choose a reason for hiding this comment

vaubaehn commented Dec 1, 2020 • edited Loading

harambasicluka commented Dec 1, 2020

vaubaehn commented Dec 1, 2020

d4rken commented Nov 27, 2020 •

edited

Loading

chiljamgossow Nov 30, 2020 •

edited

Loading

d4rken commented Nov 30, 2020 •

edited

Loading

vaubaehn commented Dec 1, 2020 •

edited

Loading