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

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

Closed

Conversation

BMItr
Copy link
Contributor

@BMItr BMItr commented Nov 20, 2020

Quick fix for grey card state (1.8.x RC)

@BMItr BMItr requested a review from a team November 20, 2020 16:03
@harambasicluka harambasicluka changed the title Grey card undetermine state without wifi Grey card undetermine state without wifi (DEV) Nov 20, 2020
@harambasicluka harambasicluka added this to the 1.8.0 milestone Nov 20, 2020
@harambasicluka harambasicluka added the maintainers Tag pull requests created by maintainers label Nov 20, 2020
@BMItr
Copy link
Contributor Author

BMItr commented Nov 20, 2020

issue is under investigation. (DRAFT)

@d4rken d4rken marked this pull request as draft November 20, 2020 16:23
@BMItr BMItr marked this pull request as ready for review November 20, 2020 17:09
kolyaopahle
kolyaopahle previously approved these changes Nov 20, 2020
Copy link
Contributor

@kolyaopahle kolyaopahle left a comment

Choose a reason for hiding this comment

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

Worked on my device 👍

@ralfgehrer ralfgehrer self-assigned this Nov 23, 2020
Copy link
Contributor

@Oliver-Zimmerman Oliver-Zimmerman left a comment

Choose a reason for hiding this comment

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

Works for me and seems to resolve EXPOSUREAPP-3892 as well. 👍

@harambasicluka harambasicluka changed the title Grey card undetermine state without wifi (DEV) Grey card undetermine state without wifi (DEV, EXPOSUREAPP-3892) Nov 23, 2020
…core instead of calling method multiple times
@Oliver-Zimmerman Oliver-Zimmerman dismissed stale reviews from kolyaopahle and themself via 25c1d40 November 23, 2020 14:59
kolyaopahle
kolyaopahle previously approved these changes Nov 23, 2020
* Set the internal risk level score [internalRisklevelScore]
*/
private fun setInternalRiskLevelScore(): MutableStateFlow<Int> {
val lastSuccessfullyCalculatedScore = getLastCalculatedScore().raw
Copy link
Contributor

@ralfgehrer ralfgehrer Nov 23, 2020

Choose a reason for hiding this comment

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

Thanks! I still don't get though, why we do this here in the Repo and not at the source in LocalData.lastCalculatedRiskLevel().
Currently, we have overwriting behavior in multiple places. Am I missing sth?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think you're correct and I have adjusted the code to match this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ralfgehrer @InterestedParties
what do you think about the UNDETERMINED state now?

Copy link
Contributor

@ralfgehrer ralfgehrer Nov 24, 2020

Choose a reason for hiding this comment

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

@BMItter not quite sure I understand your question.

This is what it says in our code. Hence, initializing sth with that value is probably not the best idea.

    // mapped to no UI state
    // this should never happen
    UNDETERMINED(RiskLevelConstants.UNDETERMINED);

chris-cwa
chris-cwa previously approved these changes Nov 23, 2020
@BMItr
Copy link
Contributor Author

BMItr commented Nov 23, 2020

device Tests looking good so far (with & without connection). We still need to check for sideeffects...

@@ -14,7 +14,6 @@ object RiskLevelRepository {
MutableStateFlow(LocalData.lastSuccessfullyCalculatedRiskLevel().raw)
Copy link
Contributor

@ralfgehrer ralfgehrer Nov 24, 2020

Choose a reason for hiding this comment

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

Here we access LocalData.lastSuccessfullyCalculatedRiskLevel().raw directly whereas we use getLastSuccessfullyCalculatedScore().raw in line 10 doing the same thing.
We should align this.

@harambasicluka
Copy link
Contributor

In #1699 the UNKOWN_RISKS ARE removed, so do we even need this PR?

@BMItter @SamuraiKek @ralfgehrer @d4rken

@d4rken
Copy link
Member

d4rken commented Nov 24, 2020

Maybe wait with this until #1705 is merged. The whole RiskLevelRepository will be removed. There will be single source of truth for all risk level things.

@d4rken
Copy link
Member

d4rken commented Nov 24, 2020

After #1705 and #1699 is merged, we should reevaluate this bug again. Most of the bugs of this type were caused by weird states due to some values being stale, most of that will be fixed with these 2 PRs.

@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 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@harambasicluka
Copy link
Contributor

Ticket was set to "obsolete", will close this PR.

@d4rken d4rken deleted the fix/grey-card-undetermine-state-without-wifi branch March 12, 2021 15:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do not merge maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants