Skip to content
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

[BACK-3030] Persist latest time zone offset for Dexcom Connection into data set #726

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

darinkrauss
Copy link
Contributor

  • Determine time zone offset for Dexcom Connection from most recent datum
  • Persist time zone offset for Dexcom Connection in data set
  • Use display time zone offset, if available, in time zone calculation
  • https://tidepool.atlassian.net/browse/BACK-3030

Note: I attempted to add tests for the key piece of this change, but ran into challenges. First, there are currently no tests at all for this code (ultimately, my bad). Second, I now have a whole set of tests that incorporate this change, as well as significant other changes (for related Dexcom work), but changing those tests to allow them to work just for this one change (without the rest) would be a significant lift and effectively throw-away (since it would be soon replaced by the new tests).

…o data set

- Determine time zone offset for Dexcom Connection from most recent datum
- Persist time zone offset for Dexcom Connection in data set
- Use display time zone offset, if available, in time zone calculation
- https://tidepool.atlassian.net/browse/BACK-3030
@darinkrauss darinkrauss requested a review from jh-bate May 29, 2024 03:35
jh-bate
jh-bate previously approved these changes May 29, 2024
Copy link
Contributor

@jh-bate jh-bate left a comment

Choose a reason for hiding this comment

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

one tiny question and you have already answered the other "where the tests at" question :)

data/types/base_test.go Show resolved Hide resolved
- Fix Dexcom time handling with and without time zone specifier
- Updates based upon PR review
@darinkrauss
Copy link
Contributor Author

This adds considerably more work in order to parse Dexcom date/time strings correctly and, more importantly, to capture whether the original date/time string included a time zone specifier or not. While the attempt to parse the date/time string is a tad over-lenient, given what I've seen with real production Dexcom data, hopefully this will prevent any date/time parsing issues (and parse them as correctly as possible).

If the time zone was specified on the display time, then the time zone offset is forced to that with an appropriate change to the conversion offset (so the UTC bootstrapping calculations remain correct). This gives us the best change of have the time zone offset correctly represent
"where the user is". It also fixes a current bug where all data from iOS (likely Android, too) has a zero time zone offset.

dexcom/fetch/runner.go Show resolved Hide resolved
dexcom/fetch/translate.go Show resolved Hide resolved
dexcom/fetch/translate.go Show resolved Hide resolved
dexcom/fetch/translate.go Show resolved Hide resolved
dexcom/fetch/translate.go Show resolved Hide resolved
jh-bate
jh-bate previously approved these changes Jun 11, 2024
Copy link
Contributor

@jh-bate jh-bate left a comment

Choose a reason for hiding this comment

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

One query re a param name but LGTM

Well done on untangling as best as possible the mess that is dexcom dates!

dexcom/time.go Show resolved Hide resolved
dexcom/fetch/translate.go Show resolved Hide resolved
toddkazakov
toddkazakov previously approved these changes Jun 11, 2024
dexcom/time.go Show resolved Hide resolved
@darinkrauss darinkrauss dismissed stale reviews from toddkazakov and jh-bate via 7e04432 June 11, 2024 16:28
@darinkrauss
Copy link
Contributor Author

@toddkazakov @jh-bate Just need a quick approval after I merged in Todd's alternate implementation of time parsing.

@toddkazakov toddkazakov self-requested a review June 11, 2024 18:44
toddkazakov
toddkazakov previously approved these changes Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants