Migrate from apns2 to aioapns #721
Conversation
|
I closed this when I saw #696, but I'm re-opening it because the approaches are very different. This PR does a full replacement rather than adding an alternate APNS implementation. |
|
i'd be following this development closely, and might have to re-base (#711) on this. Let me know if you need some help or require unblocking at some point. |
| # converting to ttl for underlying library | ||
| if expiration: | ||
| time_to_live = expiration - int(time.time()) | ||
| else: | ||
| time_to_live = 2592000 | ||
|
|
There was a problem hiding this comment.
Nice catch here, Looks good! The code handles the case of an unspecified expiration clearly. I might have gone with the ternary approach as an improvement:
time_to_live = expiration - int(time.timeit()) if expiration else 2592000
| key_id=key_id, | ||
| team_id=team_id, | ||
| use_sandbox=get_manager().get_apns_use_sandbox(application_id), | ||
| ) |
There was a problem hiding this comment.
I found two errors. The topic isn't being sent when creating the client and the context processor raises an exception if there's no credential path. I tested it afterwards and works great with Django 4.2 and Python 3.11
if not get_manager().has_auth_token_creds(application_id): cert = get_manager().get_apns_certificate(application_id) with _apns_path_for_cert(cert) as cert_path: client = aioapns.APNs( client_cert=cert_path, team_id=team_id, topic=get_manager().get_apns_topic(application_id), use_sandbox=get_manager().get_apns_use_sandbox(application_id), ) else: key_path, key_id, team_id = get_manager().get_apns_auth_creds(application_id) client = aioapns.APNs( key=key_path, key_id=key_id, team_id=team_id, topic=get_manager().get_apns_topic(application_id), use_sandbox=get_manager().get_apns_use_sandbox(application_id), )
There was a problem hiding this comment.
Here's a fork with the fix
https://github.com/aalbinati/django-push-notifications/tree/remove-apns2
There was a problem hiding this comment.
Awesome. Thank you @aalbinati. Doing some testing with the service was on my to-do, I'll pull in the changes you're suggesting soon.
Disclaimer: I haven't tested this with real interactions with APNs.
The goal is to remove the indirect use of hyper (which is no longer maintained) and unblock compatibility with newer Python versions without breaking too many of the existing contracts.
I did remove the 'creds' parameter that existed in many of the functions. The value that would have been accepted is a part of apns2, so it cannot be realistically supported.
Somewhat surprising.. It seems the indentation is currently set to tabs? This PR currently converts several of the files, which may not be the right approach.