Skip to content
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 Support for mutable_content in aioapns #755

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

HashimJVZ
Copy link
Contributor

@HashimJVZ HashimJVZ commented Jan 11, 2025

Re-do of #745

Changes in the #745 was 314a484

This PR introduces support for the mutable_content property in the aioapns library, which was previously available only in apns2. The mutable_content property allows notifications to be modified by a Notification Service Extension before being delivered to the user.

This is related to #320 and #350.
Earlier support was available in the apns.py file, now added to apns_async.py.

@HashimJVZ HashimJVZ requested a review from 50-Course January 11, 2025 17:23
Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.50%. Comparing base (44977b1) to head (28b8632).
Report is 53 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   70.39%   72.50%   +2.10%     
==========================================
  Files          27       29       +2     
  Lines        1101     1240     +139     
  Branches      180      205      +25     
==========================================
+ Hits          775      899     +124     
- Misses        288      298      +10     
- Partials       38       43       +5     

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

Copy link
Member

@50-Course 50-Course left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @HashimJVZ,

This is great, I had some review in the past four days -- however, i couldn't fully complete it due to one or two. Going through this again, I have one or two questions -- so we can call this in 😄 .

Foremost, this looks great!

def test_push_payload_with_mutable_content(self, mock_apns):
apns_send_message(
"123",
"Hello world",
mutable_content=True,
creds=TokenCredentials(key="aaa", key_id="bbb", team_id="ccc"),
sound="chime",
extra={"custom_data": 12345},
expiration=int(time.time()) + 3,
)
args, kwargs = mock_apns.return_value.send_notification.call_args
req = args[0]
# Assertions
self.assertTrue("mutable-content" in req.message["aps"])
self.assertEqual(req.message["aps"]["mutable-content"], 1) # APNs expects 1 for True
# def test_bad_priority(self):
# with mock.patch("apns2.credentials.init_context"):
# with mock.patch("apns2.client.APNsClient.connect"):
# with mock.patch("apns2.client.APNsClient.send_notification") as s:
# self.assertRaises(APNSUnsupportedPriority, _apns_send, "123",
# "_" * 2049, priority=24)

however, we are introducing mutable_content as int (rightfully so) in this change, as opposed to previous versions built upon apns2 library (mutable_content as boolean - which does the handling underneath, constraining the top-level API to boolean values), i think the former anti-pattern with the boolean values is better here, as it avoid the unnecessary shenanigans with magic numbers from user inputs. However, we with good doc, we good. Can we document this new change?

It would help smooth the smooth transition in the release note. Other than that, this looks ready for join the mainline 👍🏼

@50-Course
Copy link
Member

Hey @HashimJVZ, yes the docstring would suffice. somehow I got the notifications of your reply, but can't find it out here.

@HashimJVZ
Copy link
Contributor Author

@50-Course , I used int instead of bool as type hinting by mistateke. I just realised my mistake in the code after commenting. So I deleted immediately.

Copy link
Member

@50-Course 50-Course left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now marked resolved, let's get this out there 🚀

@50-Course 50-Course merged commit b59fbfb into jazzband:master Feb 4, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants