PushNotificationIOS requestPermission promisified #7900

JAStanton · 2016-06-02T20:31:41Z

Motivation
Today it's hard to build a good flow around requesting permissions if we don't know when the user rejects the push notification permission.

With this PR I wrap PushNotificationIOS#requestPermission in a promise. The promise will return with the updated permissions when the user accepts, rejects or has previously rejected the permission.

An example flow of how an app should handle push notifications with the change proposed:

Show user an explanation of push notification permissions with a button to enable,
User presses the enable push notifications button,
If the user accepts -> take them into the app,
if the user rejects -> explain how to enable permission in the settings app.
My app will now store some state about how it has asked permissions for push notifications so that the next time the user is taken through this flow they will go straight to step 4.

Without this change we could listen to the register event that PushNotificationIOS fires on the success scenario but we would have no clean way to determine if they have rejected the permission. It would break the example flow.

Test plan
In this video I display:

Requesting permission -- if the dialog is currently open. <-- Only available with this PR
Active - if I have all (badge, sound, and alert) push notification permissions or not
Detailed permissions which show device token (truncated), badge, sound, and alert.

ghost · 2016-06-02T20:32:29Z

By analyzing the blame information on this pull request, we identified @nicklockwood and @slycoder to be potential reviewers.

ghost · 2016-06-02T20:32:30Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

ghost · 2016-06-02T20:46:27Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

javache · 2016-06-03T08:35:17Z

Libraries/PushNotificationIOS/RCTPushNotificationManager.m

+{
+  if (self.requestPermissionsResolveBlock == nil) return;
+
+  NSMutableDictionary* notificationTypes = [NSMutableDictionary dictionaryWithObjectsAndKeys:@NO, @"alert", @NO, @"badge", @NO, @"sound", nil];


It would be cleaner to write this as:

NSDictionary *notificationTypes = @{ @"alert": @(notificationSettings.types & UIUserNotificationTypeAlert), ... };

(may need an extra BOOL cast)

Cannot do a BOOL because it's going into an NSDictionary, it needs to be a NSNumber instead. Casted it to that.

@JAStanton I think @javache meant

@((BOOL)(notificationSettings.types & UIUserNotificationTypeAlert))

So that the value of the NSNumber is 1 or 0 instead of the numeric value of the UIUserNotificationTypeAlert enum.

However, that won't work either, since casting to a BOOL doesn't actually affect the value. I think what you'd actually need to write is

@(notificationSettings.types & UIUserNotificationTypeAlert > 0)

That's equivalent to

[NSNumber numberWithBool:notificationSettings.types & UIUserNotificationTypeAlert]

But we prefer the modern @(...) syntax to [NSNumber numberWithX:...] in our codebase.

Gotcha, yeah I updated the PR to achieve the second approach but I agree with the shorthand of the first approach! Updated again! Done

ghost · 2016-06-03T16:19:21Z

@JAStanton updated the pull request.

ghost · 2016-06-03T16:53:15Z

@JAStanton updated the pull request.

nicklockwood · 2016-06-03T18:51:00Z

Libraries/PushNotificationIOS/RCTPushNotificationManager.m

+  }
+  UIUserNotificationSettings *notificationSettings = notification.userInfo;
+  NSDictionary *notificationTypes = @{
+    @"alert": @((BOOL)notificationSettings.types & UIUserNotificationTypeAlert),


I think this won't actually work, for the reason I mentioned - casting to BOOL is basically just casting to char/int8, it won't actually coerce the value to be true or false

I think this needs to be written as

@"alert": @(notificationSettings.types & UIUserNotificationTypeAlert > 0)

Gotcha, alright I played with it on my device and the only change I made was wrapping the bitwise-and in parenthesis before comparing it. Otherwise it works fine 👍

ghost · 2016-06-03T20:14:19Z

@JAStanton updated the pull request.

nicklockwood · 2016-06-03T20:47:26Z

@facebook-github-bot shipit

ghost · 2016-06-03T20:52:37Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

ghost · 2016-06-03T21:18:20Z

@JAStanton updated the pull request.

nicklockwood · 2016-06-03T21:48:13Z

Libraries/PushNotificationIOS/RCTPushNotificationManager.m

+  if (self.requestPermissionsResolveBlock == nil) {
+    return;
+  }
+  UIUserNotificationSettings *notificationSettings = notification.userInfo;


@JAStanton This failed to land due to a type error detected by our build system. You're assigning notification.userInfo (which is defined as being an NSDictionary) to a property of type UIUserNotificationSettings. I'm not sure why this worked for you locally, but it seems unlikely that it's correct.

Oh nice, so it works because I was passing in UIUserNotificationSettings to the userInfo parameter of my NSNotification message instead of setting it a dictionary like how it's supposed to be used. Bummer. Updated, take a look! Thanks.

@JAStanton ah, I totally missed that. Should just be a simple case of wrapping notificationSettings in a dictionary and then unwrapping it then:

@{ @"settings": notificationSettings }

facebook-github-bot · 2016-06-03T22:05:18Z

@JAStanton updated the pull request.

nicklockwood · 2016-06-03T22:11:15Z

@facebook-github-bot shipit

ghost · 2016-06-03T22:16:03Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

chandlervdw · 2016-07-27T16:47:37Z

@JAStanton in the docs, it's not explicit how to use this. Is the following correct?

PushNotificationIOS.requestPermissions().then((permissions) => console.log(permissions));

What am I missing? The above never fires the console.log().

Summary: **Motivation** Today it's hard to build a good flow around requesting permissions if we don't know when the user rejects the push notification permission. With this PR I wrap `PushNotificationIOS#requestPermission` in a promise. The promise will return with the updated permissions when the user accepts, rejects or has previously rejected the permission. An example flow of how an app should handle push notifications with the change proposed: 1) Show user an explanation of push notification permissions with a button to enable, 2) User presses the enable push notifications button, 3) If the user accepts -> take them into the app, 4) if the user rejects -> explain how to enable permission in the settings app. 5) My app will now store some state about how it has asked permissions for push notifications so that the next time the user is taken through this flow they will go straight to step 4. Without this change we could listen to the `register` event that PushNotificationIOS fires on the success sc Closes facebook#7900 Differential Revision: D3387424 Pulled By: nicklockwood fbshipit-source-id: e27df41e83216e4e2a14d1e42c6b26e72236f48c

PushNotificationIOS requestPermission promisified

b8180c9

ghost added the GH Review: review-needed label Jun 2, 2016

Merge branch 'master' into master

1e2f592

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 2, 2016

satya164 added the Platform: iOS iOS applications. label Jun 2, 2016

javache reviewed Jun 3, 2016
View reviewed changes

JAStanton added 2 commits June 3, 2016 09:18

short hand to create notification types

6365175

dont throw, instead use RCTLogError

ef8c066

short hand bool

8344774

nicklockwood reviewed Jun 3, 2016
View reviewed changes

bitwise and then compare then cast to number

afed56b

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jun 3, 2016

Merge branch 'master' into master

dbabb78

nicklockwood reviewed Jun 3, 2016
View reviewed changes

userInfo is a dictionary, dont abuse the system

45c9e15

ghost closed this in f4dbf37 Jun 3, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PushNotificationIOS requestPermission promisified #7900

PushNotificationIOS requestPermission promisified #7900

JAStanton commented Jun 2, 2016 •

edited

Loading

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

javache Jun 3, 2016

JAStanton Jun 3, 2016 •

edited

Loading

nicklockwood Jun 3, 2016 •

edited

Loading

JAStanton Jun 3, 2016 •

edited

Loading

ghost commented Jun 3, 2016

ghost commented Jun 3, 2016

nicklockwood Jun 3, 2016

nicklockwood Jun 3, 2016

JAStanton Jun 3, 2016

ghost commented Jun 3, 2016

nicklockwood commented Jun 3, 2016

ghost commented Jun 3, 2016

ghost commented Jun 3, 2016

nicklockwood Jun 3, 2016 •

edited

Loading

JAStanton Jun 3, 2016 •

edited

Loading

nicklockwood Jun 3, 2016

facebook-github-bot commented Jun 3, 2016

nicklockwood commented Jun 3, 2016

ghost commented Jun 3, 2016

chandlervdw commented Jul 27, 2016 •

edited

Loading

PushNotificationIOS requestPermission promisified #7900

PushNotificationIOS requestPermission promisified #7900

Conversation

JAStanton commented Jun 2, 2016 • edited Loading

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

ghost commented Jun 2, 2016

javache Jun 3, 2016

Choose a reason for hiding this comment

JAStanton Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

nicklockwood Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

JAStanton Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 3, 2016

ghost commented Jun 3, 2016

nicklockwood Jun 3, 2016

Choose a reason for hiding this comment

nicklockwood Jun 3, 2016

Choose a reason for hiding this comment

JAStanton Jun 3, 2016

Choose a reason for hiding this comment

ghost commented Jun 3, 2016

nicklockwood commented Jun 3, 2016

ghost commented Jun 3, 2016

ghost commented Jun 3, 2016

nicklockwood Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

JAStanton Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

nicklockwood Jun 3, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Jun 3, 2016

nicklockwood commented Jun 3, 2016

ghost commented Jun 3, 2016

chandlervdw commented Jul 27, 2016 • edited Loading

JAStanton commented Jun 2, 2016 •

edited

Loading

JAStanton Jun 3, 2016 •

edited

Loading

nicklockwood Jun 3, 2016 •

edited

Loading

JAStanton Jun 3, 2016 •

edited

Loading

nicklockwood Jun 3, 2016 •

edited

Loading

JAStanton Jun 3, 2016 •

edited

Loading

chandlervdw commented Jul 27, 2016 •

edited

Loading