-
Notifications
You must be signed in to change notification settings - Fork 192
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
Use Firebase v1 and Upgrade to v1.0.0 #160
Conversation
* Changed indents from tabs to spaces in README.rst
* Added firebase-admin-sdk * Linters via pre-commit * Added type hinting * Added cron job to constantly check for new max batch number of sending notifications Changed: * Updated send_message to be more flexible * Updated models.py to adjust to new migration * Updated version numbers for package to 1.0.0 * Updated docs Dropped: * Dropped Python 3.5 and Python 2.X since Firebase-admin-sdk only supports Python 3.6+
* Remove deprecated setting FCM_SERVER_KEY from doc
* send_message for the QuerySet now sends back a dictionary with the response, registration_ids_sent, and deactivated_registration_ids * Made arguments more flexible for send and send_all Firebase messaging APIs in case they add any new APIs * Added message to docs talking about how v1 will be using the new API
* Added support in admin.py * Added documentation for topic subscription handling
* Merged README with docs/index.rst
I've created a branch on the demo project using this PR to facilitate the testing: |
Other than that I seem to be able to send notifications just fine, so looking promising! |
@Andrew-Chen-Wang Thanks for working on this. |
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 have reviewed the code, mostly looks good to me. I have few queries/suggestions:
- Looks like for migration, no changes need to be done (to code or data)? Am I right? If not then we should write migration guide (for <v1 to v1 users). except "google credential file" instead of key in settings?
I haven't tested the code yet, this is just from reading the code.
* The fix here avoids a new migration
@xtrinch xtrinch/fcm-django-web-demo#23 adjusted demo readme. Thanks for creating that new branch! No migrations needed @daadu. Updated docs for migration to v1. Yes, seems like the only "version migration" needed is to include the new firebase init in the settings.py and configure their google credential. Besides that, the only new things is to adjust to using firebase_admin.messaging.Message and Notification (same API, but you aren't limited to the kwargs we provided, you can use the kwargs provided by firebase_admin package itself). Admin.py now properly formats all the responses properly. |
* Tested only the messages. Could not identify an actual error from the TopicManagementResponse * Added note on incorrect terminology of registration IDs actually being called registration tokens, but the entire * Added note on how to get the errors from the responses
* Updated docs and README.rst
A couple of merge conflicts arose from one of your other PRs. Other than that it looks good to me, thanks! Will merge after that and create the 1.0.0 release |
Alright. I'll also create an archive branch for the v0 so we don't lose it. |
Released last pyfcm version under 0.3.11 and the 1.0.0 is now live 🚀 Thanks to @Andrew-Chen-Wang |
thanks for the quick responses and testing @xtrinch ! |
Fixes #148
Fixes #144
Fixes #122
Fixes #119
Also adds topic subscription handling
This needs a little bit of testing (I didn't test this at all...)
Note: I took out fcm.py since people can just use
firebase_admin.messaging import send, send_all
whenever they need. There's no need to do it here since we have no API key to use. I've made the queryset object for deactivating public, however, if people wanted to maintain compatibility. If requested, I can reimplement fcm.py as an API file.