-
-
Notifications
You must be signed in to change notification settings - Fork 763
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
fix: Make Mealie Timezone-Aware #3847
fix: Make Mealie Timezone-Aware #3847
Conversation
33986b3
to
060fc04
Compare
060fc04
to
9331437
Compare
ab35f7d
to
7fe6132
Compare
This also reverts #3850, which is a temp fix while this gets reviewed |
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 don't see any issues here. I'd love to get a second opinion before we merge though!
Think this should spend at least a few days on nightly before we do a release.
@michael-genson can you please add a simple example of a before and after api response, into the description?
I've also got a tangential question with you on Discord.
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
Much of our frontend <--> backend logic assumes Mealie stores datetimes in UTC. Well it does in the dev server... because that's just the default. As soon as you set the timezone (e.g. TZ=America/Chicago) we start running into issues. Most notably it breaks the new offline shopping list feature (see #3842, shoutout to @ollywelch for figuring this one out).
Annoyingly, this isn't a super easy fix. This PR makes several changes to how Mealie handles timezones:
datetime.now()
->datetime.now(timezone.utc)
)MealieModel
) to automatically convert all naive datetimes to datetimes with tzinfo=utcOne breaking change this causes is that the API returns datetime strings with a "Z" now (indicating that it is, in fact, in UTC). Some of our frontend logic intentionally adds/strips the Z, so I updated everywhere I could find that logic (which makes the logic simpler).
e.g. currently timestamps look like this from the API:
2024-07-02T16:06:53.431631
and now they'll look like this:2024-07-02T16:06:53.431631Z
Which issue(s) this PR fixes:
(REQUIRED)
Fixes #3842
Special notes for your reviewer:
This is definitely a breaking change because of the API returning a Z, but I don't think there's a cleaner way to fix this problem. In a new application, we should set the database to store TZ info, and we should make Pydantic require TZ-aware datetimes (Pydantic 2.0 supports this) but there's no way to do that now without drastically changing/breaking things.
Also there were a ton of import issues that I found and I fixed those. Adding a model util for getting the current time in UTC exposed them.
Testing
(fill-in or delete this section)
Tested everything I could; frontend seems to be handling everything fine. I'm sure some bugs will pop up but all major functionality works fine, at least.