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

Add download entire period bundle feature #176

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Conversation

maxneuvians
Copy link
Contributor

This is a feature that will allow a person to download all the key for the period defined by numberOfDaysToServe, currently 14, in one package.

The access the feature,enableEntirePeriodBundle needs to be set to true.

Instead of the regular day number since unix time, ex: 18440 we use 00000 because that is not a valid date but does not require the creation of a new API endpoint or changing the authorization logic behind retrieving bundles. The mobile client can use that as a default before replacing it with the current day.

@maxneuvians maxneuvians requested a review from a team as a code owner July 13, 2020 22:02
@@ -25,6 +25,7 @@ type Constants struct {
HmacKeyLength int
CORSAccessControlAllowOrigin string
DisableCurrentDateCheckFeatureFlag bool
EnableEntirePeriodBundle bool

Choose a reason for hiding this comment

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

Super minor, and maybe this is just the github editor, but it doesn't line up with the others. Do we have a linter / formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

dateNumber = endDate

startTimestamp = time.Unix(int64(startDate*86400), 0)
endTimestamp = time.Unix(int64((endDate+1)*86400), 0)

Choose a reason for hiding this comment

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

Maybe I'm reading this wrong, but it seems like we're doing "yesterday" on L77, then taking numberOfDaysToServe in the past, and then on L83 we're adding back the day we removed so we're back to "today"
This seems to imply that we're also serving today as part of the bundle, is there ever a use case where we'd want DisableCurrentDateCheckFeatureFlag to be false and EnableEntirePeriodBundle to be true? It seems like we're assuming that EnableEntirePeriodBundle means DisableCurrentDateCheckFeatureFlag is also true.
It's also entirely possible that my timeMath is just bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

endTimestamp is just the outer bound of the period, so you go up to the end days midnight. We need to ensure that yesterdays day is used in startDate and dateNumber and that the correct endTimestamp is then calculated.


currentRSIN := pb.CurrentRollingStartIntervalNumber()
currentDateNumber := timemath.CurrentDateNumber()

if config.AppConstants.DisableCurrentDateCheckFeatureFlag == false && dateNumber == currentDateNumber {
return s.fail(log(ctx, err), w, "request for current date", "cannot serve data for current period for privacy reasons", http.StatusNotFound)
return s.fail(log(ctx, nil), w, "request for current date", "cannot serve data for current period for privacy reasons", http.StatusNotFound)

Choose a reason for hiding this comment

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

Got it, we're checking it here, so we do need both to be true for it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

active_at = time_in_date('10:00', today_utc.prev_day(8))
two_days_ago = yesterday_utc.prev_day(1)

# Our retrieve endpoint returns keys SUBMITTED within the given period.

Choose a reason for hiding this comment

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

Should we check the bounds? Like 14 days ago and today as well?

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!

@maxneuvians maxneuvians merged commit 56a6a57 into master Jul 14, 2020
@henrytao-me
Copy link
Contributor

Link cds-snc/covid-alert-app#642

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants