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

QuotaCalculator v2 #1201

Merged
merged 11 commits into from Sep 22, 2020
Merged

QuotaCalculator v2 #1201

merged 11 commits into from Sep 22, 2020

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Sep 21, 2020

Description

The previous one had to be removed due to unforeseen side-effects in relation to transaction rollbacks.

  • Created a new ENFClient which currently only has a DiagnosisKeyProvider module, but will in the future have further modules until the singleton InternalExposureNotificationClient is empty.
  • Added additional logging statements to better understand transaction failures via logfile.

How to test

  • Review the code
  • Think about whether we have covered all quota edge cases in SubmissionQuotaTest
  • Run the app, force key retrieval via test menu, observe logcat (filter for SubmissionQuota)

@d4rken d4rken added enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers 1.4.0 labels Sep 21, 2020
@d4rken d4rken requested a review from a team September 21, 2020 20:42
@d4rken d4rken marked this pull request as ready for review September 21, 2020 20:42
ralfgehrer
ralfgehrer previously approved these changes Sep 22, 2020
Copy link
Contributor

@ralfgehrer ralfgehrer left a comment

Choose a reason for hiding this comment

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

Looks good overall. Commented on some minor code style things.

@Singleton
class TimeStamper @Inject constructor() {

val nowUTC: Instant
Copy link
Contributor

Choose a reason for hiding this comment

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

not consequently used. I'd prefer Instant.now() instead of this helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should probably also replace all other calls to Instant.now().

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do need this class so that testing is easier and also we can easily fake a timestamp for the whole project

Copy link
Member Author

@d4rken d4rken Sep 22, 2020

Choose a reason for hiding this comment

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

It's not "yet" consequently used, when refactoring please make use of this class where possible. It allows us to inject mock time sources in testing and "timetravel" to test edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this can be improved by switching to the ThreeTen time library entirely injecting a Clock - which in turn can be moved in time - instead of using helper functions to solely determine the current time. Anyway, this is a bigger discussion for another time. PR looks fine to me.

Copy link
Contributor

@chris-cwa chris-cwa left a comment

Choose a reason for hiding this comment

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

I love it.

import com.google.android.gms.nearby.exposurenotification.ExposureConfiguration
import java.io.File

interface DiagnosisKeyProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this called a provider? this class takes something / "consumes" it and does not provide something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it's the interface that the class wrapping GoogleENFClient.provideDiagnosisKeys has to implement. So while it does both, it is more responsible for "providing keys into the ENF" than consuming from our end. 🤔

@Singleton
class TimeStamper @Inject constructor() {

val nowUTC: Instant
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should probably also replace all other calls to Instant.now().

@Singleton
class TimeStamper @Inject constructor() {

val nowUTC: Instant
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do need this class so that testing is easier and also we can easily fake a timestamp for the whole project

@sonarcloud
Copy link

sonarcloud bot commented Sep 22, 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 4 Code Smells

69.0% 69.0% Coverage
5.9% 5.9% Duplication

@d4rken d4rken merged commit 3946e3b into release/1.4.x Sep 22, 2020
@d4rken d4rken deleted the feature/quota-calculator-v2 branch September 22, 2020 12:29
@d4rken d4rken added this to the 1.4.0 milestone Oct 7, 2020
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

3 participants