-
Notifications
You must be signed in to change notification settings - Fork 23
fix: add time offset logic for daylight savings time issue #1039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CK-7vn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That issue we talked about, just leaving this comment as a placeholder since we talked in slack, i'll wait for the ping after that bugs fixed.
CK-7vn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works good, looks good, just one little recommendation to help future proof us I think. I know how annoying this must of been so I commend you for sure!
| } | ||
|
|
||
| // returns the hour and minute from the first occurrence within the slice of time.Time instances | ||
| func getCanonicalHourAndMinute(occurrences []time.Time, timezone string) (int, int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should at least log an error if there are no occurrences or we fall back to UTC, that way if we run into errors down the road with timezone issues etc, we're not blindly wondering what the heck is going on and at least have some logs.
| return &override, nil | ||
| } | ||
|
|
||
| func (db *DB) GetFacilityCalendar(args *models.QueryContext, dtRng *models.DateRange, classID int) ([]models.FacilityProgramClassEvent, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't on you, but after looking through this file, we really should make a note/ticket to clean up this function and a few others in this file, they're violating a good handful of uber style guide stuff, particularly with how huge it is.
corypride
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution is solid and it tests good
Description of the change
Fixes inconsistent timezone handling in calendar components causing incorrect event times during daylight savings transitions.
Screenshot(s)