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

Corona test result fetching, storage & wiring (EXPOSUREAPP-6007) #2824

Merged
merged 34 commits into from Apr 16, 2021

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Apr 13, 2021

  • This PR replaces the mix of SharedPreference properties used to store the data related to the current PCR test with a CoronaTestRepository that allows to work with implementations of CoronaTest.
  • Each CoronaTest has a CoronaTestProcessor that hides the test specific actions
  • Any test related actions are performed via the repository, access to the current test data is only possible via Flow<Set<CoronaTest>> to allow threadsafe observation from various areas of the app.
  • CoronaTestMigration will take the old data and construct a data class PCRTest : CoronaTest out of it, after that has been stored in the new CoronaTestStorage the old attributes are deleted.
  • The tests themselves are stored in memory via a HotDataFlow<Set<CoronaTest>>. This flow is observed and on every data change a new snapshot of the data classes is serialized and persisted. The last snapshots is loaded from storage and used to initialize HotDataFlow if our app is killed.
  • On migration of old data, the user may see the app refreshing the tests for a short moment or PENDING if there is no internet until we were able to determine the current test result state for the migrated test data.
  • Unit tests for migration
  • Unit tests for processors
  • Unit tests for repo
  • Unit tests for storage
  • Implement State determination within new test data classes
  • Refactor remaining access to old test attributes
  • Implement specific repository methods for updating test properties (viewed/consent etc)

@d4rken d4rken added enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers labels Apr 13, 2021
@d4rken d4rken added this to the 2.1.0 milestone Apr 13, 2021
@d4rken d4rken requested a review from a team April 13, 2021 16:32
@d4rken d4rken marked this pull request as draft April 13, 2021 16:33
@mtwalli mtwalli self-assigned this Apr 14, 2021
@d4rken d4rken changed the title Corona test result fetching & storage (EXPOSUREAPP-6007) Corona test result fetching, storage & wiring (EXPOSUREAPP-6007) Apr 14, 2021
@mtwalli
Copy link
Contributor

mtwalli commented Apr 14, 2021

In general looks good , I like the separation for handling test lifecycle

@chris-cwa chris-cwa self-requested a review April 15, 2021 14:42
@chris-cwa chris-cwa self-assigned this Apr 15, 2021
…l related UI elements.

Fix dagger injection issues (loop) related to worker scheduling.
# Conflicts:
#	Corona-Warn-App/src/androidTest/java/de/rki/coronawarnapp/ui/main/home/HomeData.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/task/SubmissionTask.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/PcrTestErrorCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/PcrTestInvalidCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/PcrTestPendingCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/PcrTestPositiveCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/PcrTestReadyCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/RapidTestErrorCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/submission/ui/homecards/TestSubmissionDoneCard.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragmentViewModel.kt
@d4rken d4rken marked this pull request as ready for review April 16, 2021 09:08
@chiljamgossow chiljamgossow self-assigned this Apr 16, 2021
chris-cwa
chris-cwa previously approved these changes Apr 16, 2021
@mtwalli
Copy link
Contributor

mtwalli commented Apr 16, 2021

I got a crash while registering a test:

2021-04-16 11:58:09.770 27053-27144/de.rki.coronawarnapp.test E/AndroidRuntime: FATAL EXCEPTION: DefaultDispatcher-worker-12 @coroutine#874
    Process: de.rki.coronawarnapp.test, PID: 27053
    java.lang.IllegalStateException: No test of type PCR available
        at de.rki.coronawarnapp.submission.SubmissionRepository$giveConsentToSubmission$1.invokeSuspend(SubmissionRepository.kt:50)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

@d4rken
Copy link
Member Author

d4rken commented Apr 16, 2021

I got a crash while registering a test:

Currently expected, test registration will only work after additional PRs from other team members.
Because some data (like consent) is stored "per test", we need to forward the data until we can actually create the test (with QR Code).

I can make it not crash, but I think it does not matter, because it will not be able to register a test until PRs from @chiljamgossow @axelherbstreith and @SamuraiKek are merged.

@SamuraiKek
Copy link
Contributor

I can make it not crash, but I think it does not matter, because it will not be able to register a test until PRs from @chiljamgossow @axelherbstreith and @SamuraiKek are merged.

I'm not sure if my PR would need to be merged for this to work. I think I would actually need to be able to register a test in order to test my PR.

@d4rken
Copy link
Member Author

d4rken commented Apr 16, 2021

I fixed the crashes, and registering a PCR test will now work, consent is hardcoded for now, this will be fixed by @SamuraiKek .
Also fixed an issue with storing the tests, and a collision between stableIds on the homescreen.

@sonarcloud
Copy link

sonarcloud bot commented Apr 16, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 32 Code Smells

17.9% 17.9% Coverage
3.6% 3.6% Duplication

@d4rken d4rken merged commit 8abd175 into release/2.1.x Apr 16, 2021
@d4rken d4rken deleted the feature/6007-test-result-fetching branch April 16, 2021 11:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants