django-push-notifications
django-push-notifications copied to clipboard
Fix imports of FCM to keep it as an optional dependency
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 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
@@ 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.
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
I have rebased over #707, now this PR makes a single import statement instead of two to handle FCM in models.py.
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.