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

kupsum · 2024-01-08T08:21:21Z

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

…INGS

jamaalscarlett · 2024-01-09T16:04:09Z

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

kupsum · 2024-01-10T08:55:58Z

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

codecov · 2024-01-24T07:59:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

push_notifications/models.py

jamaalscarlett · 2024-03-16T15:46:47Z

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

…INGS

kupsum · 2024-03-16T18:29:23Z

@jamaalscarlett I have resolved the merge conflicts

jamaalscarlett · 2024-03-31T01:08:17Z

@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

There is a missing migration

jkoestinger · 2024-04-10T12:22:33Z

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 · 2024-04-13T09:46:28Z

@jamaalscarlett I've added the missing migrations!

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

jamaalscarlett · 2024-04-14T22:58:50Z

Good catch @jkoestinger

tuky · 2024-04-17T08:24:27Z

push_notifications/migrations/0010_alter_gcmdevice_cloud_message_type.py

+                    ("GCM", "Google Cloud Message"),
+                ],
+                default="FCM",
+                help_text="You should choose FCM or GCM",


Could you please rebase and recreate this migration? The help_text changed in #702

tuky · 2024-04-17T08:27:00Z

push_notifications/models.py

@@ -98,7 +102,7 @@ class GCMDevice(Device):
 	registration_id = models.TextField(verbose_name=_("Registration ID"), unique=SETTINGS["UNIQUE_REG_ID"])
 	cloud_message_type = models.CharField(
 		verbose_name=_("Cloud Message Type"), max_length=3,
-		choices=CLOUD_MESSAGE_TYPES, default="FCM",
+		choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type(),


Suggested change

choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type(),

choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type,

having the callable method as default makes it easier to override the setting in tests. also requires the regeneration of the migration

tuky · 2024-04-17T08:29:32Z

tests/test_models.py

+	def _validate_device_cloud_message_type(self, expected_cloud_message_type: str) -> None:
+		# Reload the models so that cached model is evicted and
+		# field default value is re-read from settings
+		import push_notifications.models
+		importlib.reload(push_notifications.models)
+		from push_notifications.models import GCMDevice
+		field_object = GCMDevice._meta.get_field("cloud_message_type")
+		self.assertEqual(field_object.default, expected_cloud_message_type)


all of this should become obsolete and can be removed after switching to the callable default.

instead I would recommend to create model instances in each of the tests and assert they have been created with the correct cloud_message_type (see #699 (comment))

tuky · 2024-04-17T08:33:24Z

push_notifications/migrations/0010_alter_gcmdevice_cloud_message_type.py

+                    ("FCM", "Firebase Cloud Message"),
+                    ("GCM", "Google Cloud Message"),
+                ],
+                default="FCM",


actually, this line is the main reason for a callable default. otherwise the migration would be hard coded to "FCM" and projects using a different default in their settings would complain about a missing migration.

tuky · 2024-04-17T08:44:56Z

tests/test_models.py

+	@override_settings()
+	def test_cloud_message_type_is_set_to_gcm(self) -> None:
+		settings.PUSH_NOTIFICATIONS_SETTINGS.update({
+			"DEFAULT_CLOUD_MESSAGE_TYPE": "GCM",
+		})
+		self._validate_device_cloud_message_type(expected_cloud_message_type="GCM")
+
+	@override_settings()
+	def test_cloud_message_type_is_set_to_fcm(self) -> None:
+		settings.PUSH_NOTIFICATIONS_SETTINGS.update({
+			"DEFAULT_CLOUD_MESSAGE_TYPE": "FCM",
+		})
+		self._validate_device_cloud_message_type(expected_cloud_message_type="FCM")


Suggested change

@override_settings()

def test_cloud_message_type_is_set_to_gcm(self) -> None:

settings.PUSH_NOTIFICATIONS_SETTINGS.update({

"DEFAULT_CLOUD_MESSAGE_TYPE": "GCM",

})

self._validate_device_cloud_message_type(expected_cloud_message_type="GCM")

@override_settings()

def test_cloud_message_type_is_set_to_fcm(self) -> None:

settings.PUSH_NOTIFICATIONS_SETTINGS.update({

"DEFAULT_CLOUD_MESSAGE_TYPE": "FCM",

})

self._validate_device_cloud_message_type(expected_cloud_message_type="FCM")

@override_settings()

def test_cloud_message_type_is_set_to_gcm(self) -> None:

settings.PUSH_NOTIFICATIONS_SETTINGS.update({

"DEFAULT_CLOUD_MESSAGE_TYPE": "GCM",

})

device = GCMDevice.objects.create(registration_id="a valid registration id")

self.assertEqual(device.cloud_message_type, "GCM")

@override_settings()

def test_cloud_message_type_is_set_to_fcm(self) -> None:

settings.PUSH_NOTIFICATIONS_SETTINGS.update({

"DEFAULT_CLOUD_MESSAGE_TYPE": "FCM",

})

device = GCMDevice.objects.create(registration_id="a valid registration id")

self.assertEqual(device.cloud_message_type, "FCM")

kupsum · 2024-04-20T15:19:50Z

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!

Allow default value for cloud message type to be overridden with SETT…

5117e64

…INGS

Add unit tests

1f31b13

vitorguima reviewed Jan 24, 2024

View reviewed changes

push_notifications/models.py Outdated Show resolved Hide resolved

Change default message to method

af2b614

itayAmza mentioned this pull request Jan 25, 2024

Add aioapns version of APNS #696

Closed

VaughnDV approved these changes Feb 2, 2024

View reviewed changes

jamaalscarlett previously approved these changes Mar 10, 2024

View reviewed changes

kupsum added 4 commits March 16, 2024 20:23

Allow default value for cloud message type to be overridden with SETT…

7fe75a7

…INGS

Change default message to method

dba0957

Merge branch 'master' of github.com:jazzband/django-push-notifications

f27bea0

Update README

9558516

kupsum force-pushed the master branch from 7e44ef7 to 9558516 Compare March 16, 2024 18:28

jamaalscarlett mentioned this pull request Mar 31, 2024

Timeline for the next release #705

Closed

jkoestinger mentioned this pull request Apr 10, 2024

Add missing migration for the new default FCM choices #714

Merged

Merge branch 'master' of github.com:jazzband/django-push-notifications

1269c60

kupsum force-pushed the master branch 2 times, most recently from d046596 to 9156078 Compare April 12, 2024 16:32

Add missing migration

53759b1

kupsum force-pushed the master branch from 9156078 to 53759b1 Compare April 12, 2024 16:33

tuky requested changes Apr 17, 2024

View reviewed changes

tuky reviewed Apr 17, 2024

View reviewed changes

kupsum closed this Apr 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kupsum commented Jan 8, 2024

jamaalscarlett commented Jan 9, 2024

kupsum commented Jan 10, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

jamaalscarlett commented Mar 16, 2024

kupsum commented Mar 16, 2024

jamaalscarlett commented Mar 31, 2024 •

edited

Loading

jkoestinger commented Apr 10, 2024 •

edited

Loading

kupsum commented Apr 13, 2024

jamaalscarlett commented Apr 14, 2024

tuky Apr 17, 2024

tuky Apr 17, 2024

tuky Apr 17, 2024 •

edited

Loading

tuky Apr 17, 2024 •

edited

Loading

tuky Apr 17, 2024

tuky Apr 17, 2024

kupsum commented Apr 20, 2024

	choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type(),
	choices=CLOUD_MESSAGE_TYPES, default=get_default_cloud_message_type,

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

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

Conversation

kupsum commented Jan 8, 2024

jamaalscarlett commented Jan 9, 2024

kupsum commented Jan 10, 2024

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

jamaalscarlett commented Mar 16, 2024

kupsum commented Mar 16, 2024

jamaalscarlett commented Mar 31, 2024 • edited Loading

jkoestinger commented Apr 10, 2024 • edited Loading

kupsum commented Apr 13, 2024

jamaalscarlett commented Apr 14, 2024

tuky Apr 17, 2024

Choose a reason for hiding this comment

tuky Apr 17, 2024

Choose a reason for hiding this comment

tuky Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

tuky Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

tuky Apr 17, 2024

Choose a reason for hiding this comment

tuky Apr 17, 2024

Choose a reason for hiding this comment

kupsum commented Apr 20, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

jamaalscarlett commented Mar 31, 2024 •

edited

Loading

jkoestinger commented Apr 10, 2024 •

edited

Loading

tuky Apr 17, 2024 •

edited

Loading

tuky Apr 17, 2024 •

edited

Loading