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

Family Test Repository (EXPOSUREAPP-12330) #4986

Merged
merged 17 commits into from
Mar 24, 2022
Merged

Conversation

chiljamgossow
Copy link
Contributor

@chiljamgossow chiljamgossow commented Mar 23, 2022

  • unit tests -> will be in a separate PR

@chiljamgossow chiljamgossow added the maintainers Tag pull requests created by maintainers label Mar 23, 2022
@chiljamgossow chiljamgossow added this to the 2.21.0 milestone Mar 23, 2022
@chiljamgossow chiljamgossow requested a review from a team March 23, 2022 15:15
@chiljamgossow chiljamgossow marked this pull request as draft March 23, 2022 15:16
@chiljamgossow chiljamgossow changed the title Repository (EXPOSUREAPP-12330) Family Test Repository (EXPOSUREAPP-12330) Mar 23, 2022
mtwalli
mtwalli previously approved these changes Mar 24, 2022
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

LGTM

@mtwalli mtwalli self-assigned this Mar 24, 2022
@chiljamgossow chiljamgossow marked this pull request as ready for review March 24, 2022 09:26
mtwalli
mtwalli previously approved these changes Mar 24, 2022
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

LGTM

@SamuraiKek SamuraiKek self-assigned this Mar 24, 2022
Copy link
Contributor

@SamuraiKek SamuraiKek left a comment

Choose a reason for hiding this comment

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

LGTM. Will approve once you decide what you wish to do with the naming.

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

11.7% 11.7% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

Nice work!

@mtwalli mtwalli merged commit e02aa0b into release/2.21.x Mar 24, 2022
@mtwalli mtwalli deleted the feature/12330-repo branch March 24, 2022 12:45

@Singleton
class FamilyTestRepository @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

FamilyTestRepository is NOT thread-safe!

Copy link
Contributor

Choose a reason for hiding this comment

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

Please elaborate more? you mean because the flow is not shared ?

@@ -276,3 +261,19 @@ private fun CoronaTestResult.toValidatedResult(): CoronaTestResult {
RAT_INVALID
}
}

// After 60 days, the previously EXPIRED test is deleted from the server, and it may return pending again.
fun check60DaysRAT(test: BaseCoronaTest, newResult: CoronaTestResult, now: Instant): CoronaTestResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Create only 1 method for both types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both methods are different from each other, even if the name looks the same

return if ((newResult == PCR_OR_RAT_PENDING || newResult == RAT_PENDING) &&
testAge > VerificationServer.TestAvailabilityDuration
) {
Timber.tag(RATestProcessor.TAG).d("$testAge is exceeding the test availability.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont use string templates for logging with timber. Make use of the lazy loading.
And VerificationServer.TestAvailabilityDuration should be logged to

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

else -> false
}

val CoronaTestResult?.isPending
Copy link
Contributor

Choose a reason for hiding this comment

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

and rat pending?

Copy link
Contributor

Choose a reason for hiding this comment

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

The state says RAT_OR_PCR_PENDING, and this is not new, it was just moved from one place to another
see https://github.com/corona-warn-app/cwa-app-android/pull/4986/files#diff-74d8a3af0a20d2b26b9c921ae7b3bbf64dd14995365a3d31f7b3b483d7f970d9L76-L87

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants