Skip to content

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

Merged
merged 6 commits into from
Feb 12, 2020
Merged

Conversation

lahirumaramba
Copy link
Member

@lahirumaramba lahirumaramba commented Feb 3, 2020

  • Check whether the datetime object is naive or timezone aware
  • If a naive datetime object is provided then set its timezone to the local timezone
  • Convert the event_time to UTC Zulu format

RELEASE NOTE: AndroidNotification class now correctly formats the event_time field sent to the Cloud Messaging service.

Copy link
Contributor

@hiranya911 hiranya911 left a 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.

@@ -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:
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@hiranya911 hiranya911 left a 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.

@lahirumaramba
Copy link
Member Author

It turns out that in Python 3.5 astimezone() cannot be used with naive datetimes. Therefore, we will assume that all naive datetimes are already to be in the UTC timezone. The API docs are updated to reflect this change.

@hiranya911 @egilmorez

@@ -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
Copy link
Member Author

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!

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.

Copy link
Contributor

@hiranya911 hiranya911 left a 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 👍

@hiranya911 hiranya911 assigned egilmorez and unassigned hiranya911 Feb 5, 2020
- 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
@lahirumaramba lahirumaramba merged commit 5b244a2 into master Feb 12, 2020
@lahirumaramba lahirumaramba deleted the fix_eventtime_fcm branch February 12, 2020 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants