jazzband / django-push-notifications

Send push notifications to mobile devices through GCM or APNS in Django.

MIT License

2.24k stars 609 forks source link

Generic FCM device convenience model #615

Open ayushin opened 3 years ago

ayushin commented 3 years ago

Hi all,

As FCM can now handle ios/android/web we found it convenient to have a generic FCMDevice model that also keep the data about device platform.

Here is my take on how this could be implemented, any feed back is very welcome.

codecov[bot] commented 3 years ago

Codecov Report

Merging #615 (ed34a98) into master (13a2c6f) will increase coverage by 0.63%. The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #615      +/-   ##
==========================================
+ Coverage   68.30%   68.93%   +0.63%     
==========================================
  Files          24       25       +1     
  Lines        1101     1130      +29     
  Branches      173      173              
==========================================
+ Hits          752      779      +27     
- Misses        312      314       +2     
  Partials       37       37

Impacted Files	Coverage Δ
push_notifications/models.py	`78.35% <83.33%> (+0.30%)`	:arrow_up:
push_notifications/admin.py	`36.73% <100.00%> (+2.69%)`	:arrow_up:
push_notifications/api/rest_framework.py	`75.93% <100.00%> (+1.74%)`	:arrow_up:
push_notifications/migrations/0008_fcmdevice.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 13a2c6f...ed34a98. Read the comment docs.

ayushin commented 3 years ago

Convenience to use FCM by default plus type of device

Andrew-Chen-Wang commented 3 years ago

@ayushin got it, understood. I don't think this PR is necessary then since devs should override the GCM model. Plus the name just makes it confusing since the current model is still called GCM even though GCM doesn't exist anymore; we'll probably change the model name in a future migration. So -1.

ayushin commented 3 years ago

Yeah well, perhaps we can just turn around and rename GCM to be FCM (I think it is useful to have a platform field there) and GCM to be a proxy model to FCM for backward compatibility?

Andrew-Chen-Wang commented 2 years ago

One more problem. This currently still uses FCM legacy API rather than v1. Please visit django fcm for how I migrated it. At some point, the legacy API will be deprecated and gone.

ayushin commented 2 years ago

Would it be better to replace the GCMModel with FCMModel instead? We'll have a cleaner code and have a migration renaming one in the previous installs

jamaalscarlett commented 2 years ago

@ayushin, I agree with @Andrew-Chen-Wang that there is currently more work needed to properly support the v1 FCM implementation. I do agree that a new FCMDevice model makes sense, but it should be using the firebase_admin sdk

ayushin commented 2 years ago

Agreed. Actually https://github.com/xtrinch/fcm-django makes more sense for FCM

Andrew-Chen-Wang commented 2 years ago

@ayushin I made the migration for fcm-django. FCM-django is just a fork of django push notifications before this package added fcm support I believe.

In other words, you can basically copy my code and add it to this package.

jamaalscarlett commented 2 years ago

@Andrew-Chen-Wang Do you want to do it? You did the work, you should get the credit :)

Andrew-Chen-Wang commented 2 years ago

Really squeezed for time lately. Will loop back around in a month maybe to implement if necessary.

jamaalscarlett commented 2 years ago

I get it. I will be on PTO all next week, but maybe I will take a crack at it after that