-
Notifications
You must be signed in to change notification settings - Fork 339
FCM Options Support in WebpushConfig API #242
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
Resolves #202 |
firebase_admin/_messaging_utils.py
Outdated
"""Options for features provided by the FCM SDK for Web. | ||
|
||
Args: | ||
link: The link to open when the user clicks on the notification. Must be a HTTPS URL |
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.
s/a HTTPS/an HTTPS/
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.
Done
} | ||
result = cls.remove_null_values(result) | ||
link = result.get('link') | ||
if link is not None and not link.startswith('https://'): |
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.
Should we allow alternate capitalization on the protocol? I mean, "HTTPS://" is probably okay, right?
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.
Yes, but we have always checked only for the lower-cased prefix in this codebase (there are similar URL checks in auth, db etc). So I'll keep this consistent with that. Also developers haven't requested this, so it should be ok.
Adding the
fcm_options
parameter to theWebpushConfig
type. Change ported from firebase/firebase-admin-node#373Resolves #203