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 isPushNotificationEnabled #99

Merged
merged 1 commit into from Feb 14, 2017

Conversation

onmyway133
Copy link
Contributor

This method can come in handy when we need to check if push notification can really be delivered to the app

@mention-bot
Copy link

@onmyway133, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zenangst, @vadymmarkov and @RamonGilabert to be potential reviewers.

@zenangst
Copy link
Contributor

@onmyway133 nifty! 👍

@onmyway133 onmyway133 merged commit 0f07e4a into hyperoslo:master Feb 14, 2017
@onmyway133 onmyway133 deleted the fix/push_notification branch February 14, 2017 10:58
Copy link

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

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

Added some comments. Basically this does not cover all cases on iOS 10, for example if the user goes into settings and disables notifications after they've been registered. Happy to help you improve this @onmyway133 🙂 Also, how about adding a test for this new API?


public struct Utilities {

public static var isPushNotificationEnabled: Bool {

Choose a reason for hiding this comment

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

Since there are many push notifications, I think this should be pluralized to become isPushNotificationsEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return false
}

return UIApplication.shared.isRegisteredForRemoteNotifications

Choose a reason for hiding this comment

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

This is not enough on iOS 10. It only checks if the app is registered for notifications, not if they are actually enabled. To check that, you need to call UNUserNotificationCenter.getNotificationSettings(), and check the setting's authorizationStatus.

public struct Utilities {

public static var isPushNotificationEnabled: Bool {
guard let settings = UIApplication.shared.currentUserNotificationSettings else {

Choose a reason for hiding this comment

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

You should also check that settings.types isn't empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have && !settings.types.isEmpty at the bottom

Choose a reason for hiding this comment

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

Oh, you're right. Missed that, sorry 😓

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.

None yet

4 participants