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

ENFClient v2 - ExposureDetectionTracker without identifier (EXPOSUREAPP-3540) #1605

Conversation

BMItr
Copy link
Contributor

@BMItr BMItr commented Nov 13, 2020

This PR implements one additional part of the new ExposureWindowMode (#1584 ).

We don't have a token anymore and can't provide one to the ExposureDetectionTracker.
This PR implements the opportunity to use ExposureDetectionTracker with & without identifier.
Note: ExposureDetectionTracker was CalculationTracker some days ago (just to prevent confusion).

@BMItr BMItr requested review from a team and removed request for a team November 13, 2020 22:59
exposureDetectionTracker.trackNewExposureDetection(token)
diagnosisKeyProvider.provideDiagnosisKeys(keyFiles, configuration, token)
}
// NO-OP
Copy link
Member

Choose a reason for hiding this comment

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

Maybe throw UnsupportedOperationException() until this is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throwing UnsupportedOperationException() now

@@ -93,34 +94,16 @@ class DefaultExposureDetectionTracker @Inject constructor(
}
}

override fun finishExposureDetection(identifier: String, result: Result) {
Timber.i("finishExposureDetection(token=%s, result=%s)", identifier, result)
Copy link
Member

Choose a reason for hiding this comment

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

Please re-add a log statement directly at function start, the following methods using updateSafely are async, if there is any issue there we'd still like to know that the function was called, but failed somewhere later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@@ -134,6 +117,54 @@ class DefaultExposureDetectionTracker @Inject constructor(
}
}

private fun finishExposureDetectionWithoutIdentifier(
Copy link
Member

Choose a reason for hiding this comment

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

Would an extension function improve readability here?

Something like fun Map<String, TrackedExposureDetection>.findOrCreateByIdentifier(identifier: String?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to:
private fun Map<String, TrackedExposureDetection>.findUnfinishedOrCreateIdentifier(): String {

.map { it.value }
.filter { it.finishedAt == null }
.maxByOrNull { it.startedAt.millis }
?.identifier ?: kotlin.run {
Copy link
Member

Choose a reason for hiding this comment

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

A bit difficult to read. How could we make this easier on the 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point.
?.identifier ?: kotlin.run is out.
readability improved

@harambasicluka harambasicluka added this to the 1.7.0 milestone Nov 16, 2020
@chris-cwa chris-cwa modified the milestones: 1.7.0, 1.8.0 Nov 16, 2020
@harambasicluka harambasicluka added the maintainers Tag pull requests created by maintainers label Nov 16, 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.

LGTM 👍 all handleable changes seem solved, lets get this merged to get the main branch ready, tests can be fixed later before we merge into release

@sonarcloud
Copy link

sonarcloud bot commented Nov 16, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 3 Code Smells

45.8% 45.8% Coverage
0.0% 0.0% Duplication

@BMItr BMItr merged commit dcf361a into feature/3456-enfv2-main-branch Nov 16, 2020
@BMItr BMItr deleted the feature/3540-ENFclient-ExposureDetectionTracker branch November 16, 2020 12:06
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.

6 participants