Add support for "webpush.fcm_options" (WebpushFcmOptions) #203
Conversation
| Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
| CLAs look good, thanks! |
hiranya911 left a comment
hiranya911 left a comment There was a problem hiding this comment.
Thanks @robermac for the PR. A few changes required before we can merge this.
| 'headers': _Validators.check_string_dict( | ||
| 'WebpushConfig.headers', webpush.headers), | ||
| 'notification': cls.encode_webpush_notification(webpush.notification), | ||
| 'fcm_options': cls.encode_webpush_fcm_options(webpush.fcm_options) |
There was a problem hiding this comment.
The json key should be called fcmOptions. The REST API documentation mentions fcm_options by error and the FCM team is going to fix it.
| return None | ||
| if not isinstance(fcm_options, WebpushFcmOptions): | ||
| raise ValueError('WebpushConfig.fcm_options must be an instance of ' | ||
| 'WebFcmOptions class.') |
| raise ValueError('WebpushConfig.fcm_options must be an instance of ' | ||
| 'WebFcmOptions class.') | ||
| result = { | ||
| 'link': _Validators.check_string( |
There was a problem hiding this comment.
Also check for https:// prefix.
| check_encoding(messaging.Message( | ||
| topic='topic', webpush=messaging.WebpushConfig(fcm_options=fcm_options))) | ||
| return excinfo | ||
| |
There was a problem hiding this comment.
Also add a test case to verify the happy path.
| Closing this in favor of the changes being implemented in https://github.com/firebase/firebase-admin-python/tree/hkj-fcm-options |
3 participants
Resolved #202