Skip to content
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

Add FCM v1 API #702

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Add FCM v1 API #702

merged 4 commits into from
Feb 14, 2024

Conversation

ceoy
Copy link
Contributor

@ceoy ceoy commented Jan 16, 2024

Use new FCM v1 API by using firebase_admin SDK.

  • Removed GCM type
  • Added firebase-admin
  • Updated documentation
  • Updated tests

Resolves #546

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (7d28052) 69.51% compared to head (a8bfcd5) 70.28%.

Files Patch % Lines
push_notifications/gcm.py 88.05% 4 Missing and 4 partials ⚠️
push_notifications/conf/app.py 71.42% 2 Missing ⚠️
push_notifications/models.py 92.00% 0 Missing and 2 partials ⚠️
push_notifications/admin.py 93.75% 0 Missing and 1 partial ⚠️
push_notifications/conf/base.py 66.66% 1 Missing ⚠️
push_notifications/conf/legacy.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   69.51%   70.28%   +0.76%     
==========================================
  Files          26       26              
  Lines        1122     1097      -25     
  Branches      245      249       +4     
==========================================
- Hits          780      771       -9     
+ Misses        304      288      -16     
  Partials       38       38              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ceoy ceoy closed this Jan 16, 2024
@ceoy
Copy link
Contributor Author

ceoy commented Jan 16, 2024

will reopen after i've fixed tests

@ceoy ceoy reopened this Jan 17, 2024
@levinotik
Copy link

@jamaalscarlett @jleclanche Any sense of when this might be reviewed? Would be really great to have FCM v1 support merged in soon with the legacy API going away in 2024/06/20

@levinotik
Copy link

@ceoy nice work. In your changes to the docs, you have:

		firebase_app = firebase_admin.initialize_app()
	PUSH_NOTIFICATIONS_SETTINGS = {
		# Load and process all PUSH_NOTIFICATIONS_SETTINGS using the AppConfig manager.
		"CONFIG": "push_notifications.conf.AppConfig",
		# collection of all defined applications
		"APPLICATIONS": {
			"my_fcm_app": {
				# PLATFORM (required) determines what additional settings are required.
				"PLATFORM": "FCM",
				# FCM settings
				"FIREBASE_APP": firebase_app,
			},
			"my_ios_app": {
				  # PLATFORM (required) determines what additional settings are required.
				  "PLATFORM": "APNS",
				  # required APNS setting
				  "CERTIFICATE": "/path/to/your/certificate.pem",
			},
			"my_wns_app": {
				# PLATFORM (required) determines what additional settings are required.
				"PLATFORM": "WNS",
				# required WNS settings
				"PACKAGE_SECURITY_ID": "[your package security id, e.g: 'ms-app://e-3-4-6234...']",
				"SECRET_KEY": "[your app secret key, e.g.: 'KDiejnLKDUWodsjmewuSZkk']",
			},
		}

Would this allow for setting up and initializing multiple firebase_admin apps, each with different service account credentials and then being to dynamically choose which app to send the push with at the time of sending the message? Or would this work some other way? I'm trying to get a sense for how we can use multiple Firebase apps with this.

@ceoy
Copy link
Contributor Author

ceoy commented Jan 18, 2024

Yes, exactly.

This part is basically the same as before (old documentation here https://github.com/jazzband/django-push-notifications), you just need to set the FIREBASE_APP instead of an API_KEY.

The config is then selected depending on the value of the application_id field in the GCMDevice model.

@levinotik
Copy link

Excellent, thanks @ceoy

@vitorguima
Copy link

very nice job!! @ceoy

@jamaalscarlett
Copy link
Member

@ceoy any idea why the python 3.6 test is failing?

@ceoy
Copy link
Contributor Author

ceoy commented Jan 26, 2024

@ceoy any idea why the python 3.6 test is failing?

not sure, i have some time tomorrow to check 👀

@ceoy
Copy link
Contributor Author

ceoy commented Jan 28, 2024

@jamaalscarlett fixed now, bit of a weird issue and there is probably a better way to fix it 😆

@anhphamt
Copy link

anhphamt commented Feb 3, 2024

Thanks @ceoy for this PR. Very nice work.

@50-Course
Copy link
Member

@ceoy, good work out here! thanks for sending in this patch, re-running the pipeline all test now passes with the new update 👍🏼

cc: @jamaalscarlett

@jamaalscarlett jamaalscarlett merged commit c23e49d into jazzband:master Feb 14, 2024
7 checks passed
@@ -1,7 +1,9 @@
from django.db import models
from django.utils.translation import gettext_lazy as _
from firebase_admin import messaging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import should be placed in the gcm package, since if placed here it turns firebase_admin as a mandatory requirements for everyone.

50-Course pushed a commit that referenced this pull request Oct 3, 2024
* Add FCM v1 API

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Unit Tests for GCMDeviceAdmin and fix FCM v1 tests

* fixes admin test for python 3.6

---------

Co-authored-by: Tim Jahn <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
50-Course pushed a commit that referenced this pull request Oct 6, 2024
* Add FCM v1 API

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add Unit Tests for GCMDeviceAdmin and fix FCM v1 tests

* fixes admin test for python 3.6

---------

Co-authored-by: Tim Jahn <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use new FCM APIs
7 participants