search

jazzband / django-push-notifications

Send push notifications to mobile devices through GCM or APNS in Django.
MIT License
2.23k stars 605 forks source link

Allow default value for cloud message type to be overridden with SETTINGS #699

Closed kupsum closed 2 months ago

kupsum commented 6 months ago

Allow default value of GCMDevice.cloud_message_type to be overridden/specified in SETTINGS.

jamaalscarlett commented 5 months ago

@kupsum thanks for this, can you please add a test to validate your change.

kupsum commented 5 months ago

@jamaalscarlett Thanks! I've added unit tests now!

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.33%. Comparing base (9bf5fda) to head (9558516). Report is 1 commits behind head on master.

:exclamation: Current head 9558516 differs from pull request most recent head 53759b1. Consider uploading reports for the commit 53759b1 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #699 +/- ## ========================================== + Coverage 70.28% 70.33% +0.05% ========================================== Files 26 26 Lines 1097 1099 +2 Branches 249 249 ========================================== + Hits 771 773 +2 Misses 288 288 Partials 38 38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jamaalscarlett commented 3 months ago

sorry @kupsum can you fix the merge conflicts, and I will merge this ASAP.

kupsum commented 3 months ago

@jamaalscarlett I have resolved the merge conflicts

jamaalscarlett commented 3 months ago

@kupsum When I use your branch a new migration is generated. Please include it in your PR.

Migrations for 'push_notifications':
  /Users/jamaalscarlett/git/personal/old-mac/git/django-push-notifications-demos/django-push-notifications-server/venv/lib/python3.8/site-packages/push_notifications/migrations/0010_alter_gcmdevice_options_and_more.py
    - Change Meta options on gcmdevice
    - Alter field cloud_message_type on gcmdevice
jkoestinger commented 2 months ago

Hi there, I'm no reviewer so I'm not sure if I should leave a comment directly on the codebase, but I gave it a look since I opened #714 , and I think this line should be changed as follow (not calling the default):

choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type

Otherwise it's effectively not doing anything unless we change the settings, and then it would complain that changes are not reflected in migrations again.

If it's done and has its migration for it, then it would superseed #714 and I can close it.

kupsum commented 2 months ago

@jamaalscarlett I've added the missing migrations!

@jkoestinger Thank you for your effort! I've added the migration here.

jamaalscarlett commented 2 months ago

Good catch @jkoestinger

kupsum commented 2 months ago

I'm closing this PR. Now that FCM is default, my goal is done. I was trying to it with a backwards compatible way but I'm fine the current approach. Thank you everyone who spent time on this PR!