feat(fcm): Add 12 new Android Notification Parameters Support #363

lahirumaramba · 2019-11-08T16:57:50Z

Discussion

Introduce support for the new Android notification parameters in FCM API to AdminSDK.

Testing

Added unit tests and integration tests to reflect this change.

API Changes

In Messaging added new fields to AndroidNotification class.
Introduced LightSettings class

RELEASE NOTE: Added a series of new parameters to the AndroidNotification class that allow further customization of notifications that target Android devices.

firebase_admin/_messaging_utils.py

lahirumaramba · 2019-11-11T00:32:54Z

@hiranya911 :
I moved Message and MulticastMessage to the new _mesaage_encoder.py for now to avoid circular dependencies. I hope this is okay for now.

hiranya911

Looks mostly good. Just a few clarifications needed in the docs, plus a few readability improvements.

firebase_admin/_messaging_utils.py

firebase_admin/_messaging_encoder.py

tests/test_messaging.py

hiranya911

Looks pretty solid. Just a few nits from me, and then I'll be at an LGTM.

hiranya911 · 2019-11-11T22:24:18Z

tests/test_messaging.py

-    @pytest.mark.parametrize('data', NON_STRING_ARGS + ['', 'topic', 'priority', 'foo'])
-    def test_invalid_visibility(self, data):
-        notification = messaging.AndroidNotification(visibility=data)
+    @pytest.mark.parametrize('visibility', NON_STRING_ARGS + ['', 'topic', 'priority', 'foo'])


Remove topic and priority

hiranya911 · 2019-11-11T22:25:52Z

tests/test_messaging.py

@@ -32,6 +32,7 @@
 NON_DICT_ARGS = ['', list(), tuple(), True, False, 1, 0, {1: 'foo'}, {'foo': 1}]
 NON_OBJECT_ARGS = [list(), tuple(), dict(), 'foo', 0, 1, True, False]
 NON_LIST_ARGS = ['', tuple(), dict(), True, False, 1, 0, [1], ['foo', 1]]
+NON_UNUM_ARGS = ['1.23s', list(), tuple(), dict(), -1.23]


Nit: NON_UINT_ARGS?

hiranya911 · 2019-11-11T22:29:57Z

firebase_admin/_messaging_encoder.py

+        result = {
+            'color': _Validators.check_string(
+                'LightSettings.color', light_settings.color, non_empty=True),
+            'light_on_duration': cls.encode_milliseconds(


I think this is fine. But let's just document it as a milliseconds API for simplicity.

egilmorez · 2019-11-12T01:14:18Z

firebase_admin/_messaging_encoder.py

+class _Validators(object):
+    """A collection of data validation utilities.
+
+    Methods provided in this class raise ValueErrors if any validations fail.


Is this a literal Lahiru? If so, let's code-font it (with backticks probably).

egilmorez · 2019-11-12T01:15:11Z

firebase_admin/_messaging_encoder.py

+
+
+class MessageEncoder(json.JSONEncoder):
+    """A custom JSONEncoder implementation for serializing Message instances into JSON."""


Should this be capped? I'm thinking not.

Thank you! I think JSONEncoder is correct here. I will code-font it. These were from one of our existing modules (not new code) they appear here as new because I had moved them to a new module file. It is good to fix the code-fonts though! :)

egilmorez · 2019-11-12T01:15:36Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_android_fcm_options(cls, fcm_options):
+        """Encodes an AndroidFCMOptions instance into a json."""


Looks like a literal.

egilmorez · 2019-11-12T01:16:21Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_android_fcm_options(cls, fcm_options):
+        """Encodes an AndroidFCMOptions instance into a json."""


Should this be either "a JSON object" or maybe just "JSON?" Unless json is actually a literal.

egilmorez · 2019-11-12T01:16:49Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_ttl(cls, ttl):
+        """Encodes a AndroidConfig TTL duration into a string."""


"an AndroidConfig (if literal)

egilmorez · 2019-11-12T01:17:34Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_android_notification(cls, notification):
+        """Encodes an AndroidNotification instance into JSON."""


egilmorez · 2019-11-12T01:19:09Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_webpush_notification_actions(cls, actions):
+        """Encodes a list of WebpushNotificationActions into JSON."""


Literal? If not, then "Web Push notification actions" could even work.

egilmorez · 2019-11-12T01:19:23Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_webpush_fcm_options(cls, options):
+        """Encodes a WebpushFCMOptions instance into JSON."""


egilmorez · 2019-11-12T01:19:43Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_apns(cls, apns):
+        """Encodes an APNSConfig instance into JSON."""


egilmorez · 2019-11-12T01:19:59Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_apns_fcm_options(cls, fcm_options):
+        """Encodes an APNSFCMOptions instance into JSON."""


egilmorez · 2019-11-12T15:11:48Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_notification(cls, notification):
+        """Encodes an Notification instance into JSON."""


Suggest "a notification"

egilmorez · 2019-11-12T15:12:09Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_fcm_options(cls, fcm_options):
+        """Encodes an FCMOptions instance into JSON."""


egilmorez · 2019-11-12T15:12:20Z

firebase_admin/_messaging_encoder.py

+
+    @classmethod
+    def encode_aps_alert(cls, alert):
+        """Encodes an ApsAlert instance into JSON."""


egilmorez · 2019-11-12T15:15:05Z

firebase_admin/_messaging_utils.py

+            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
+            (optional).
+        local_only: Set whether or not this notification is relevant only to the current device.


Suggest sticking with "Sets"

egilmorez · 2019-11-12T15:16:04Z

firebase_admin/_messaging_utils.py

+            watch. This hint can be set to recommend this notification not be bridged (optional).
+            See Wear OS guides:
+            https://developer.android.com/training/wearables/notifications/bridger#existing-method-of-preventing-bridging
+        priority: Set the relative priority for this notification. Low-priority notifications may be


Here too: "Sets"

egilmorez

Thanks Lahiru!

A couple of global thoughts:

-- Where we refer to literals, those should be code-fonted, with double backticks if that's what works with our Python doc parsing.
-- Total nit, but for setters we may want to stick with "Sets" whenever we are talking about a single method or property.

I tried but may not have caught all these nits.

hiranya911

LGTM 👍

egilmorez

Thanks Lahiru!

chong-shao

LGTM. Thanks!

feat(fcm): Add 12 new Android Notification Parameters Support

a965841

lahirumaramba added the release-note label Nov 8, 2019

lahirumaramba requested a review from hiranya911 November 8, 2019 16:57

lahirumaramba assigned hiranya911 Nov 8, 2019

lahirumaramba requested review from chong-shao and egilmorez November 8, 2019 16:58

chong-shao requested changes Nov 8, 2019

View reviewed changes

firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved

firebase_admin/_messaging_utils.py Outdated Show resolved Hide resolved

Move message encoders and validators to a separate module

c8d86e6

lahirumaramba force-pushed the lm-fcm-android-12 branch from 71e60bf to d39bc45 Compare November 9, 2019 01:57

Fix cyclic import

ad0bebe

lahirumaramba force-pushed the lm-fcm-android-12 branch from d39bc45 to ad0bebe Compare November 10, 2019 23:38

PR Fixes

53aa966

hiranya911 suggested changes Nov 11, 2019

View reviewed changes

hiranya911 assigned lahirumaramba and unassigned hiranya911 Nov 11, 2019

PR Fixes

484d1d9

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Nov 11, 2019

hiranya911 suggested changes Nov 11, 2019

View reviewed changes

hiranya911 assigned egilmorez and lahirumaramba and unassigned hiranya911 Nov 11, 2019

egilmorez reviewed Nov 12, 2019

View reviewed changes

lahirumaramba assigned hiranya911 and unassigned lahirumaramba Nov 12, 2019

Code-font class names and other PR fixes

04a9d50

lahirumaramba force-pushed the lm-fcm-android-12 branch from aabafe3 to 04a9d50 Compare November 12, 2019 17:23

hiranya911 approved these changes Nov 12, 2019

View reviewed changes

hiranya911 removed their assignment Nov 12, 2019

egilmorez approved these changes Nov 13, 2019

View reviewed changes

lahirumaramba requested a review from chong-shao November 13, 2019 14:09

lahirumaramba assigned chong-shao and unassigned egilmorez Nov 13, 2019

chong-shao approved these changes Nov 13, 2019

View reviewed changes

lahirumaramba merged commit 8580361 into master Nov 13, 2019

lahirumaramba deleted the lm-fcm-android-12 branch November 13, 2019 17:05



		class MessageEncoder(json.JSONEncoder):
		"""A custom JSONEncoder implementation for serializing Message instances into JSON."""

feat(fcm): Add 12 new Android Notification Parameters Support #363

feat(fcm): Add 12 new Android Notification Parameters Support #363

Uh oh!

Conversation

lahirumaramba commented Nov 8, 2019

Discussion

Testing

API Changes

Uh oh!

Uh oh!

Uh oh!

lahirumaramba commented Nov 11, 2019

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

chong-shao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!