jazzband / django-push-notifications

Send push notifications to mobile devices through GCM or APNS in Django.
MIT License
2.28k stars 618 forks source link

Add FCM v1 API #702

Closed ceoy closed 9 months ago

ceoy commented 10 months ago

Use new FCM v1 API by using firebase_admin SDK.

Resolves #546

codecov[bot] commented 10 months ago

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 :warning:
push_notifications/conf/app.py 71.42% 2 Missing :warning:
push_notifications/models.py 92.00% 0 Missing and 2 partials :warning:
push_notifications/admin.py 93.75% 0 Missing and 1 partial :warning:
push_notifications/conf/base.py 66.66% 1 Missing :warning:
push_notifications/conf/legacy.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ceoy commented 10 months ago

will reopen after i've fixed tests

levinotik commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

Excellent, thanks @ceoy

vitorguima commented 10 months ago

very nice job!! @ceoy

jamaalscarlett commented 10 months ago

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

ceoy commented 10 months ago

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

not sure, i have some time tomorrow to check 👀

ceoy commented 10 months ago

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

anhphamt commented 9 months ago

Thanks @ceoy for this PR. Very nice work.

50-Course commented 9 months ago

@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