Add aioapns version of APNS

pomali commented 8 months ago

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

pomali commented 8 months ago

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 commented 7 months ago

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

codecov[bot] commented 7 months ago

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

```diff @@ 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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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

50-Course commented 4 months ago

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

jamaalscarlett commented 3 months ago

I can review the PR, but I have never used APNS. Has someone actually run this with an ios/macos app?
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 commented 3 months ago

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

ssyberg commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 1 day ago

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

jazzband / django-push-notifications

Add aioapns version of APNS #696

Codecov Report