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 new apns-push-type header #226

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

essen
Copy link
Contributor

@essen essen commented Sep 13, 2019

As documented here:

https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns

The header is required on iOS 13+ and a default value had
to be chosen. I set it to "alert" as I think it is the
most common. Indeed the documentation about "background"
said those should be sent two to three times per hour.

Please review but note that I do not have an Apple device personally and this has not been extensively tested yet. Still it's just a header so it's probably good enough.

@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #226 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #226   +/-   ##
======================================
  Coverage    87.5%   87.5%           
======================================
  Files           7       7           
  Lines         216     216           
======================================
  Hits          189     189           
  Misses         27      27
Impacted Files Coverage Δ
src/apns_connection.erl 79.68% <ø> (ø) ⬆️
src/apns.erl 97.67% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34290fc...5ba10b8. Read the comment docs.

@ferigis
Copy link
Member

ferigis commented Sep 13, 2019

hi @essen ! thanks for your contribution to apns4erl. Your code looks good to me but I can't merge it if we don't fix the coverage issue. Could you help with that? after that I will merge it for sure!

@essen
Copy link
Contributor Author

essen commented Sep 13, 2019

Probably.

@essen
Copy link
Contributor Author

essen commented Sep 13, 2019

Let's see what it says. I've changed a little how the default is handled for the new header, simpler like this.

src/apns.erl Show resolved Hide resolved
As documented here:

  https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns

The header is required on iOS 13+ and a default value had
to be chosen. I set it to "alert" as I think it is the
most common. Indeed the documentation about "background"
said those should be sent two to three times per hour.
@essen
Copy link
Contributor Author

essen commented Sep 13, 2019

All good.

@ferigis ferigis merged commit dee8403 into inaka:master Sep 13, 2019
@ferigis
Copy link
Member

ferigis commented Sep 13, 2019

Merged! We are going to create a new release with this change, thanks for your contribution!

@essen
Copy link
Contributor Author

essen commented Sep 13, 2019

Perfect! Thanks a lot!

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.

3 participants