-
Notifications
You must be signed in to change notification settings - Fork 339
fix(fcm): Convert event_time to UTC #403
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
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 think this requires a third-party lib. See comments.
firebase_admin/_messaging_encoder.py
Outdated
@@ -324,7 +325,12 @@ def encode_android_notification(cls, notification): | |||
|
|||
event_time = result.get('event_time') | |||
if event_time: | |||
result['event_time'] = str(event_time.isoformat()) + 'Z' | |||
if event_time.tzinfo is None: |
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 think you have to do any of this. At least on my machine, following works as expected.
>>> import datetime
>>> d = datetime.datetime.now()
>>> d
datetime.datetime(2020, 2, 3, 13, 39, 32, 133863)
>>> d.astimezone(datetime.timezone.utc)
datetime.datetime(2020, 2, 3, 21, 39, 32, 133863, tzinfo=datetime.timezone.utc)
The first is in my local timezone (13:39 PST). The second is in UTC (21:39 UTC). Isn't that all we want?
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.
Thanks! You're right. I missed that astimezone
uses the local time zone if the provided datetime object is naive. Will update the code.
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.
LGTM with a suggestion on how to test this.
It turns out that in Python 3.5 |
firebase_admin/_messaging_utils.py
Outdated
@@ -98,7 +98,8 @@ class AndroidNotification: | |||
the user clicks it (optional). | |||
event_timestamp: For notifications that inform users about events with an absolute time | |||
reference, sets the time that the event in the notification occurred as a | |||
``datetime.datetime`` instance. Notifications in the panel are sorted by this time | |||
``datetime.datetime`` instance. If the ``datetime.datetime`` instance is naive it will | |||
be assumed to be in the UTC timezone. Notifications in the panel are sorted by this time |
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.
FYI @egilmorez this is the API docs change. Thanks!
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.
Not critical, butI wonder if there is a more terse way to say "it will be assumed to be in the" . . . would "naive, it defaults to" be correct? Maybe "naive, it is set to" ?
If those aren't accurate, this is fine as/is.
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.
LGTM with a nit. Thanks for the fix 👍
- Check if the datetime object is naive or timezone aware - If a naive datetime object is provided then set the timezone to local timezone - Convert the event_time to UTC Zulu format
4c391f9
to
99bbc05
Compare
datetime
object is naive or timezone awaredatetime
object is provided then set its timezone to the local timezoneevent_time
to UTC Zulu formatRELEASE NOTE:
AndroidNotification
class now correctly formats theevent_time
field sent to the Cloud Messaging service.