search

jazzband / django-push-notifications

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

Fix imports of FCM to keep it as an optional dependency #706

Closed sevdog closed 4 months ago

sevdog commented 4 months ago

As pointed out in this comment https://github.com/jazzband/django-push-notifications/pull/702#discussion_r1489160018 importing firebase_admin or gcm at top level in models.py turns the firebase_admin package as a mandatory requirements while it should not.

To address this I have moved the imports inside the appropriated methods. This allow the codebase to run even without the firebase package (which is optional).

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.28%. Comparing base (9fbab31) to head (c54230f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #706 +/- ## ========================================== - Coverage 70.33% 70.28% -0.06% ========================================== Files 26 26 Lines 1099 1097 -2 Branches 249 249 ========================================== - Hits 773 771 -2 Misses 288 288 Partials 38 38 ```

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

50-Course commented 4 months ago

Thank you for making this MR. The patch was instead resolving two issues, making firebase_admin optional as well as moving the required .gcm methods to appropriate area.

707 resolved the firebase_admin issue, not required. That said, once this conflict herein is fixed. It is good as merged

sevdog commented 4 months ago

I have rebased over #707, now this PR makes a single import statement instead of two to handle FCM in models.py.

50-Course commented 4 months ago

I have rebased over #707, now this PR makes a single import statement instead of two to handle FCM in models.py.

I would make one more quick pass, and merge into the main trunk.

Once again, thank you for pulling in this patch.