Add aioapns version of APNS

[pomali]

Since version python 3.10+ is not supported by apns2 (because of dependency on hyper) we add aioapns version of sending APNs notifications.

We add installation extra [APNS_ASYNC] that installs aioapns and use new version of service if aioapns is installed. Tests are also conditional on installed modules/version of python.

Related to

• https://github.com/jazzband/django-push-notifications/pull/623 (is blocked by apns2)
• https://github.com/jazzband/django-push-notifications/issues/685
• https://github.com/jazzband/django-push-notifications/issues/622
• https://github.com/jazzband/django-push-notifications/issues/675

[pomali]

There are some (maybe) controversial decisions, feel free to discuss them

• choice of aioapns - it looks like only library with active development
• supporting both apns2 and aioapns - because we don't want to break older code
• deciding on which version to use in models.py - sure we can move it to apns.py and move apns2 code to apns_legacy.py

[sivasankarankb]

@pomali You have left print() statements in the code. See apns_async.py.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 11.20690% with 103 lines in your changes missing coverage. Please review.

Project coverage is 64.66%. Comparing base (0f79181) to head (7500caa). Report is 5 commits behind head on master.

:exclamation: Current head 7500caa differs from pull request most recent head fdc90f8

Please upload reports for the commit fdc90f8 to get more accurate results.

Files	Patch %	Lines
push_notifications/apns_async.py	4.62%	103 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (0f79181) and HEAD (7500caa). Click for more details.

HEAD has 6 uploads less than BASE

| Flag | BASE (0f79181) | HEAD (7500caa) | |------|------|------| ||7|1|

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
- Coverage   70.39%   64.66%   -5.73%     
==========================================
  Files          27       28       +1     
  Lines        1101     1214     +113     
  Branches      180      199      +19     
==========================================
+ Hits          775      785      +10     
- Misses        288      391     +103     
  Partials       38       38              

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

[itayAmza]

@pomali, any progress for this one? I had the apns2 0.7.2 requires PyJWT<2.0.0,>=1.4.0, but you have pyjwt 2.1.0 which is incompatible issue, and pointing to your branch (as a temporary solution) completely solves it.

[pomali]

I'm not sure what's missing, maybe someone should review it? And then there is code coverage check failing, but most of failing lines are comments, so I am not sure how important it is.

Is there anyone who could point me in right direction?

@itayAmza glad that it helped :)

[itayAmza]

@jamaalscarlett following this one can you help us figure out who can help review this PR?

[50-Course]

hopefully, I get to have a look into both PRs, tonight. Thank you for the notification @itayAmza

[jamaalscarlett]

1. I can review the PR, but I have never used APNS. Has someone actually run this with an ios/macos app?
2. Regarding the support for both apns2 and aioapns, I think we should add a deprecation warning with this release then completely remove apns2 with the next release.

[pomali]

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

[ssyberg]

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:

ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'

[kupsum]

There is a possible bug with default argument pollution. Below test case can demonstrate the issue. I've also added a review comment about this.

	@mock.patch("push_notifications.apns_async.APNs", autospec=True)
	def test_apns_send_messages_different_priority(self, mock_apns):
		self._create_devices(["abc", "def"])
		device_1 = APNSDevice.objects.get(registration_id="abc")
		device_2 = APNSDevice.objects.get(registration_id="def")

		device_1.send_message("Hello world 1", expiration=time.time() + 1, priority=5, collapse_id="1")
		args_1, _ = mock_apns.return_value.send_notification.call_args

		device_2.send_message("Hello world 2")
		args_2, _ = mock_apns.return_value.send_notification.call_args

		req = args_1[0]
		self.assertEqual(req.device_token, "abc")
		self.assertEqual(req.message["aps"]["alert"], "Hello world 1")
		self.assertAlmostEqual(req.time_to_live, 1, places=-1)
		self.assertEqual(req.priority, 5)
		self.assertEqual(req.collapse_key, "1")

		reg_2 = args_2[0]
		self.assertEqual(reg_2.device_token, "def")
		self.assertEqual(reg_2.message["aps"]["alert"], "Hello world 2")
		self.assertIsNone(reg_2.time_to_live, "No time to live should be specified")
		self.assertIsNone(reg_2.priority, "No priority should be specified")
		self.assertIsNone(reg_2.collapse_key, "No collapse key should be specified")

[pomali]

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:

ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'

do you have aioapns installed? because that is condition on which we use the new code

added mention to README

pip install django-push-notifications[APNS_ASYNC]

[pomali]

There is a possible bug with default argument pollution. Below test case can demonstrate the issue. I've also added a review comment about this.

Thanks for catching this, I added your test and fixed the issue.

[kmasuhr]

I think it is worth to add python 3.10 as testing target in CI, since it should enable support for version 3.10 https://github.com/jazzband/django-push-notifications/blob/820262efedbd71d38e62320b561c4aaed95b1131/.github/workflows/test.yml#L12

[ssyberg]

I ran it with an iOS simulator, but a second pair of eyes with a real device would be good idea

I just tried this and got this error when trying to send a test message via apns:

ModuleNotFoundError at /admin/push_notifications/apnsdevice/
No module named 'apns2'
Request Method:	POST
Request URL:	https://local.a24-vod:8060/admin/push_notifications/apnsdevice/
Django Version:	4.2
Exception Type:	ModuleNotFoundError
Exception Value:	
No module named 'apns2'

do you have aioapns installed? because that is condition on which we use the new code

added mention to README

pip install django-push-notifications[APNS_ASYNC]

I'm pretty sure I did when I tested it, but wouldn't that be handled by pip/piptools?

[50-Course]

Hello @kmasuhr,

I have just updated this PR to bump the version of python being used in CI. However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0 HTTP Client.

Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative. I am open to suggestions, but I would recommend using httpx as a good replacement.

[pomali]

I'm pretty sure I did when I tested it, but wouldn't that be handled by pip/piptools?

@ssyberg it should be handled by pip, but it's optional dependency, so maybe it didn't get installed?

[rib3]

I have just updated this PR to bump the version of python being used in CI. However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0 HTTP Client.

Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative. I am open to suggestions, but I would recommend using httpx as a good replacement.

hyper is a dependency of apns2. This PR adds support for aioapns as an alternative apple push notification library. (because of the problems with apns2/hyper)

There are some github issues with additional details / background linked in the PR description.

[ssyberg]

I have just updated this PR to bump the version of python being used in CI. However, it would seem one of its transitive dependencies is not compatible with Python 3.10 - precisely, the hyper==0.7.0 HTTP Client. Given that the hyper library is not actively maintained, I would like to propose that we replace it with a maintained alternative. I am open to suggestions, but I would recommend using httpx as a good replacement.

hyper is a dependency of apns2. This PR adds support for aioapns as an alternative apple push notification library. (because of the problems with apns2/hyper)

There are some github issues with additional details / background linked in the PR description.

If I'm understanding correctly, the inclusion of hyper in this PR is only to stay backwards compatible with apns2 for people who don't want to upgrade to using aioapns in which case they need to stay <3.10 (which if they are using apns2 I think is implied). IMO this could be noted in the docs but shouldn't block merge, a lot of people are waiting for this to get merged and it's blocking dev for anyone on later version setups.

[ficast]

Hey guys, any news on this? Can we get this merged?

[illymarev]

Hello, are there any updates regarding this issue? Any other changes are needed to merge the PR?

[pomali]

I don't see any outstanding issues from my side.

[50-Course]

haven't followed up much on the development lately, however it would seem we having some conflicts, no?

[illymarev]

Have you determined what causes rebase conflicts? Do you need any additional changes?