django-push-notifications icon indicating copy to clipboard operation
django-push-notifications copied to clipboard

Fix imports of FCM to keep it as an optional dependency

Open sevdog opened this issue 1 year ago • 1 comments

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).

sevdog avatar Feb 23 '24 14:02 sevdog

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.

codecov[bot] avatar Feb 23 '24 15:02 codecov[bot]

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

50-Course avatar Feb 27 '24 12:02 50-Course

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

sevdog avatar Feb 27 '24 13:02 sevdog

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.

50-Course avatar Feb 27 '24 14:02 50-Course