-
-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generic FCM device convenience model #615
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. The GCM module currently uses Firebase, so why add a new model?
name='FCMDevice', | ||
fields=[ | ||
('gcmdevice_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='push_notifications.gcmdevice')), | ||
('platform', models.CharField(blank=True, choices=[('i', 'ios'), ('a', 'android'), ('w', 'web')], help_text='Optional device platform: i - ios, a - android, w - web', max_length=1, null=True)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much rather see a small positive integer field for faster filtering on the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea indeed.
|
||
|
||
class FCMDevice(GCMDevice): | ||
PLATFORM_IOS = 'i' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem to be indented properly
Convenience to use FCM by default plus type of device |
@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. |
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? |
|
||
|
||
class FCMDevice(GCMDevice): | ||
PLATFORM_IOS = 'i' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not indented properly. 4 chars.
@@ -257,3 +256,29 @@ def send_message(self, message, **kwargs): | |||
return webpush_send_message( | |||
uri=self.registration_id, message=message, browser=self.browser, | |||
auth=self.auth, p256dh=self.p256dh, application_id=self.application_id, **kwargs) | |||
|
|||
|
|||
class FCMDevice(GCMDevice): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration for existing users doesn't make sense. You should instead be renaming the current model to GCP no?
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. |
class FCMDeviceSerializer(GCMDeviceSerializer, ModelSerializer): | ||
class Meta(GCMDeviceSerializer.Meta): | ||
model = FCMDevice | ||
fields = GCMDeviceSerializer.Meta.fields + ('platform',) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see https://github.com/jazzband/django-push-notifications/blob/master/CONTRIBUTING.md regarding single quotes
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 |
@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 |
Agreed. Actually https://github.com/xtrinch/fcm-django makes more sense for FCM |
@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. |
@Andrew-Chen-Wang Do you want to do it? You did the work, you should get the credit :) |
Really squeezed for time lately. Will loop back around in a month maybe to implement if necessary. |
I get it. I will be on PTO all next week, but maybe I will take a crack at it after that |
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.