-
-
Notifications
You must be signed in to change notification settings - Fork 613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aioapns version of APNS #696
base: master
Are you sure you want to change the base?
Conversation
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.
There are some (maybe) controversial decisions, feel free to discuss them
|
@pomali You have left |
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
@pomali, any progress for this one? |
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 :) |
@jamaalscarlett following this one can you help us figure out who can help review this PR? |
hopefully, I get to have a look into both PRs, tonight. Thank you for the notification @itayAmza |
|
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:
|
collapse_id: str = None, | ||
aps_kwargs: dict = {}, | ||
message_kwargs: dict = {}, | ||
notification_request_kwargs: dict = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since notification_request_kwargs
argument has a default value and it's content is changed in the function logic, the default state can change during execution. If any previous call has set values expiration
, priority
or collapse_id
, these are persisted to any next call and if the next call does not have these values or has the value set to None
, the values are still passed to sent NotificationRequest
. A simple test case to demonstrate this is to send first message with expiration
and second without expiration
set. The value from first call is passed to second call NotificationRequest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved it by creating copy of notification_request_kwargs
, but if you consider it code smell I could rework it to having default argument None
and replacing them in function with {}
. Added your test (test_apns_send_messages_different_priority
)
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.
|
do you have added mention to README
|
Thanks for catching this, I added your test and fixed the issue. |
for more information, see https://pre-commit.ci
I think it is worth to add python 3.10 as testing target in CI, since it should enable support for version 3.10
|
I'm pretty sure I did when I tested it, but wouldn't that be handled by pip/piptools? |
Hello @kmasuhr, I have just updated this PR to bump the version of python being used in CI. Given that the |
@ssyberg it should be handled by pip, but it's optional dependency, so maybe it didn't get installed? |
hyper is a dependency of There are some github issues with additional details / background linked in the PR description. |
If I'm understanding correctly, the inclusion of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments when trying to install this in Python 3.11 system
.github/workflows/test.yml
Outdated
@@ -9,7 +9,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
python-version: ['3.6', '3.7', '3.8', '3.9'] | |||
python-version: ['3.6', '3.7', '3.8', '3.9', '3.10'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you consider adding 3.11 too?
python-version: ['3.6', '3.7', '3.8', '3.9', '3.10'] | |
python-version: ['3.6', '3.7', '3.8', '3.9', '3.10', '3.11'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted change
@@ -22,6 +22,7 @@ classifiers = | |||
Programming Language :: Python :: 3.7 | |||
Programming Language :: Python :: 3.8 | |||
Programming Language :: Python :: 3.9 | |||
Programming Language :: Python :: 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you consider adding Python 3.11? Either way, Python 3.10 was missing
Programming Language :: Python :: 3.11 | |
Programming Language :: Python :: 3.10 | |
Programming Language :: Python :: 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted change
tox.ini
Outdated
@@ -41,6 +45,8 @@ deps = | |||
dj32: Django>=3.2,<3.3 | |||
dj40: Django>=4.0,<4.0.5 | |||
dj405: Django>=4.0.5,<4.1 | |||
dj42: Django>=4.2,<4.3 | |||
dj42: aioapns>=3.1,<3.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make send to have apns2
for Python 3.9 or less and aioapns
Python 3.10+
dj42: aioapns>=3.1,<3.2 | |
py{36,37,38,39}: apns2 | |
py{310,311}: aioapns>=3.1,<3.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated deps, removed apns2 from default leaving it only in py{36,37,38,39}
tests/test_apns_push_payload.py
Outdated
from apns2.client import NotificationPriority | ||
from push_notifications.apns import _apns_send | ||
from push_notifications.exceptions import APNSUnsupportedPriority | ||
except AttributeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apns2
and aioapns
are not supported together with conflict
The conflict is caused by:
aioapns 3.1 depends on pyjwt>=2.0.0
apns2 0.7.2 depends on PyJWT<2.0.0 and >=1.4.0
If apns2
is not installed, ModuleNotFoundError
is raised.
except AttributeError: | |
expect (AttributeError, ModuleNotFoundError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted change
tests/test_apns_models.py
Outdated
class APNSModelTestCase(TestCase): | ||
from push_notifications.exceptions import APNSError | ||
from push_notifications.models import APNSDevice | ||
except AttributeError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apns2
and aioapns
are not supported together with conflict
The conflict is caused by:
aioapns 3.1 depends on pyjwt>=2.0.0
apns2 0.7.2 depends on PyJWT<2.0.0 and >=1.4.0
If apns2
is not installed, ModuleNotFoundError
is raised.
except AttributeError: | |
expect (AttributeError, ModuleNotFoundError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted change
tox.ini
Outdated
py{38,39,310}-dj{40,405} | ||
py311-dj42 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django 4.2 supports still Python 3.8
py{38,39,310}-dj{40,405} | |
py311-dj42 | |
py{38,39,310,311}-dj{40,405,42} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted change
(fix missing space)
…ge, add ruff formatting config
to conform with Core metadata specification https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use
|
||
from . import models | ||
from .conf import get_manager | ||
from .exceptions import APNSServerError | ||
|
||
ErrFunc = Optional[Callable[[NotificationRequest, NotificationResult], Awaitable[None]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of office right now, Awaitable
is that some form of Future
result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#Update: Interesting,
https://docs.python.org/3/library/typing.html#annotating-callable-objects
I have always took a different approach to this!
notification_request_kwargs_out["time_to_live"] = expiration - int( | ||
time.time() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruff
botched this formatting in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 looks good to me, can we have a test for this?
Hey guys, any news on this? Can we get this merged? |
Hello, are there any updates regarding this issue? Any other changes are needed to merge the PR? |
I don't see any outstanding issues from my side. |
haven't followed up much on the development lately, however it would seem we having some conflicts, no? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks greenlit, still on the lookout what's causing the rebase conflicts.
Have you determined what causes rebase conflicts? Do you need any additional changes? |
Anyone maintaining this package?.. its not usable at the moment on python 3.10+ |
Since version python 3.10+ is not supported by
apns2
(because of dependency onhyper
) we addaioapns
version of sending APNs notifications.We add installation extra
[APNS_ASYNC]
that installs aioapns and use new version of service ifaioapns
is installed. Tests are also conditional on installed modules/version of python.Related to