Generic FCM device convenience model #615

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

Jump to bottom

Open

ayushin wants to merge 1 commit into jazzband:master

from apexlabs-ai:feature-fcm-device

Open

Generic FCM device convenience model #615

ayushin wants to merge 1 commit into jazzband:master from apexlabs-ai:feature-fcm-device

Conversation

@ayushin

Copy link

@ayushin ayushin commented Jul 8, 2021

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.


 initial FCMDevice implementation

ed34a98

@ayushin ayushin mentioned this pull request

Jul 8, 2021

can i use firebase for iOS instead of APNS? #611

Closed

@codecov

Copy link

codecov bot commented Jul 9, 2021 •

edited

Loading

Codecov Report

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

Impacted file tree graph

@@ 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%)`	⬆️
push_notifications/admin.py	`36.73% <100.00%> (+2.69%)`	⬆️
push_notifications/api/rest_framework.py	`75.93% <100.00%> (+1.74%)`	⬆️
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.

Andrew-Chen-Wang

Andrew-Chen-Wang requested changes

Jul 11, 2021

View reviewed changes

Copy link

Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a 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?

push_notifications/migrations/0008_fcmdevice.py

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

Copy link

Member

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 11, 2021

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.

Copy link

Author

@ayushin ayushin Jul 12, 2021

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.

push_notifications/models.py

class FCMDevice(GCMDevice):

PLATFORM_IOS = 'i'

Copy link

Member

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 11, 2021

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

@ayushin

Copy link

Author

ayushin commented Jul 11, 2021

Convenience to use FCM by default plus type of device

@Andrew-Chen-Wang

Copy link

Member

Andrew-Chen-Wang commented Jul 11, 2021 •

edited

Loading

@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

Copy link

Author

ayushin commented Jul 12, 2021

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?

@ayushin ayushin mentioned this pull request

Jul 25, 2021

Switch over from GCM to FCM #618

Open

Andrew-Chen-Wang

Andrew-Chen-Wang requested changes

Jul 31, 2021

View reviewed changes

push_notifications/models.py

class FCMDevice(GCMDevice):

PLATFORM_IOS = 'i'

Copy link

Member

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 31, 2021

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.

push_notifications/models.py

auth=self.auth, p256dh=self.p256dh, application_id=self.application_id, **kwargs)

class FCMDevice(GCMDevice):

Copy link

Member

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 31, 2021

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?

@Andrew-Chen-Wang Andrew-Chen-Wang added the gcm/fcm label

Jul 31, 2021

@Andrew-Chen-Wang Andrew-Chen-Wang linked an issue

Jul 31, 2021

that may be closed by this pull request

Switch over from GCM to FCM #618

Open

@Andrew-Chen-Wang

Copy link

Member

Andrew-Chen-Wang commented Jul 31, 2021

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.

jamaalscarlett

jamaalscarlett requested changes

Jul 1, 2022

View reviewed changes

push_notifications/api/rest_framework.py

class FCMDeviceSerializer(GCMDeviceSerializer, ModelSerializer):

class Meta(GCMDeviceSerializer.Meta):

model = FCMDevice

fields = GCMDeviceSerializer.Meta.fields + ('platform',)

Copy link

Member

@jamaalscarlett jamaalscarlett Jul 1, 2022

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

@ayushin

Copy link

Author

ayushin commented Jul 1, 2022

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

Copy link

Member

jamaalscarlett commented Jul 25, 2022

@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

Copy link

Author

ayushin commented Jul 25, 2022

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

@Andrew-Chen-Wang

Copy link

Member

Andrew-Chen-Wang commented Jul 27, 2022

@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

Copy link

Member

jamaalscarlett commented Jul 27, 2022

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

@Andrew-Chen-Wang

Copy link

Member

Andrew-Chen-Wang commented Jul 27, 2022

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

@jamaalscarlett

Copy link

Member

jamaalscarlett commented Jul 27, 2022

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

@azmeuk azmeuk mentioned this pull request

Jul 24, 2023

State of the maintenance #685

Closed

Labels

gcm/fcm

3 participants

@ayushin @Andrew-Chen-Wang @jamaalscarlett

Uh oh!

Generic FCM device convenience model #615

Are you sure you want to change the base?

Generic FCM device convenience model #615

Uh oh!

Conversation

@ayushin ayushin commented Jul 8, 2021

Uh oh!

codecov bot commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

Uh oh!

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 11, 2021

Choose a reason for hiding this comment

Uh oh!

@ayushin ayushin Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 11, 2021

Choose a reason for hiding this comment

Uh oh!

ayushin commented Jul 11, 2021

Uh oh!

Andrew-Chen-Wang commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayushin commented Jul 12, 2021

Uh oh!

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 31, 2021

Choose a reason for hiding this comment

Uh oh!

@Andrew-Chen-Wang Andrew-Chen-Wang Jul 31, 2021

Choose a reason for hiding this comment

Uh oh!

Andrew-Chen-Wang commented Jul 31, 2021

Uh oh!

@jamaalscarlett jamaalscarlett Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

ayushin commented Jul 1, 2022

Uh oh!

jamaalscarlett commented Jul 25, 2022

Uh oh!

ayushin commented Jul 25, 2022

Uh oh!

Andrew-Chen-Wang commented Jul 27, 2022

Uh oh!

jamaalscarlett commented Jul 27, 2022

Uh oh!

Andrew-Chen-Wang commented Jul 27, 2022

Uh oh!

jamaalscarlett commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jul 9, 2021 •

edited

Loading

Andrew-Chen-Wang commented Jul 11, 2021 •

edited

Loading