[BACK-2520] include pump sleep schedule #678
Conversation
There was a problem hiding this comment.
My Go knowledge is not too great, so I'll leave it to @darinkrauss to comment on the more technical details, but the only issue I see is with the capitalization of days of the week as it's currently implemented.
| package common | ||
|
|
||
| const ( | ||
| DaySunday = "sunday" |
There was a problem hiding this comment.
Looks like I capitalized the days of the week in the API PR and in the implementation. Not sure why we changed that, as I remember having it lowercase initially.
There was a problem hiding this comment.
@gniezen So this is correct here, right? I think we want to keep lowercase to maintain compatability with similar use in Dexcom alert schedules
There was a problem hiding this comment.
@darinkrauss No, Uploader uploads sleep schedules with days of the week capitalized.
Are Dexcom alert schedules currently being uploaded from anywhere? I don't think we have a ticket for Uploader yet.
There was a problem hiding this comment.
Yes, Dexcom alert schedules are currently uploaded via the Dexcom Account Connection (and have been for years).
When you say "Uploader uploads sleep schedules with days of the week capitalized", do you mean to Jellyfish?
It'd be great if we could get this consistent (specifically, all use lowercase), but if that's going to be too much trouble...
There was a problem hiding this comment.
When you say "Uploader uploads sleep schedules with days of the week capitalized", do you mean to Jellyfish?
Yes.
I guess this could be one more thing we add to the migration? If we're changing bolus to boluses, we could also changed Monday to monday. It's an easy change in Uploader.
There was a problem hiding this comment.
Yes, I suppose we should do this. One more thing to add to the list. @jh-bate
I wouldn't be surprised if we find other issues once you (@gniezen) actually start uploading via Platform. Considering the lack of validation in Jellyfish (and the validation in Platform), there will probably be other discrepancies that we'll have to resolve.
| validator.Bool("enabled", s.Enabled).Exists() | ||
| if s.Enabled != nil { | ||
| if *s.Enabled { | ||
| validator.StringArray("days", s.Days).Exists().EachOneOf(common.DaysOfWeek()...).EachUnique() |
| package common | ||
|
|
||
| const ( | ||
| DaySunday = "sunday" |
There was a problem hiding this comment.
@gniezen So this is correct here, right? I think we want to keep lowercase to maintain compatability with similar use in Dexcom alert schedules
| @@ -185,6 +187,9 @@ func (p *Pump) Validate(validator structure.Validator) { | |||
| if p.OverridePresets != nil { | |||
| p.OverridePresets.Validate(validator.WithReference("overridePresets"), unitsBloodGlucose) | |||
| } | |||
| if p.SleepSchedules != nil { | |||
There was a problem hiding this comment.
We should add SleepSchedule to Normalize, too, even if it is a NOP, for consistency.
|
|
||
| "github.com/tidepool-org/platform/data/types/common" | ||
| "github.com/tidepool-org/platform/structure" | ||
|
|
There was a problem hiding this comment.
[nit] Delete blank line.
| ) | ||
|
|
||
| const ( | ||
| SleepSchedulesMidnightOffsetMaximum = 86400 |
There was a problem hiding this comment.
We should hold off on this until we decide the millisecond versus second debate related to the pump settings override duration.
| } | ||
|
|
||
| func (s *SleepScheduleMap) Validate(validator structure.Validator) { | ||
| for _, name := range s.sortedNames() { |
There was a problem hiding this comment.
Do we want to validate the length is less than some maximum. The document says "0 < len <= 10", but that was somewhat arbitrary.
There was a problem hiding this comment.
I ended out not doing that as it didn't seem consistent with the other pumpSettings types from what I could see. I did see any of the maps enforcing a length
| validator.Int("end", s.End).Exists().InRange(SleepSchedulesMidnightOffsetMinimum, SleepSchedulesMidnightOffsetMaximum) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we want a Normalize like:
func (a *AlertScheduleSettings) Normalize(normalizer structure.Normalizer) {
if a.DaysOfWeek != nil {
sort.Sort(DaysOfWeekByAlertScheduleSettingsDayIndex(*a.DaysOfWeek))
}
}
| dataTypesCommon "github.com/tidepool-org/platform/data/types/common" | ||
| dataTypesSettingsPump "github.com/tidepool-org/platform/data/types/settings/pump" | ||
| "github.com/tidepool-org/platform/pointer" | ||
|
|
There was a problem hiding this comment.
[nit] Delete blank line.
| @@ -0,0 +1,88 @@ | |||
| package test | |||
There was a problem hiding this comment.
Name of file to sleep_schedule.go?
| "fmt" | ||
|
|
||
| dataTypesSettingsPump "github.com/tidepool-org/platform/data/types/settings/pump" | ||
|
|
There was a problem hiding this comment.
[nit] Delete blank line.
see tidepool-org/TidepoolApi#78
NOTE:
mapto match how other pump data works in the platformSleepSchedulesMidnightOffsetmillisecond versus second debate related to the pump settings override duration.