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

RemoteNotification should search for values in "aps" dictionary instead of top level. #6

Closed
NinoScript opened this issue Jul 15, 2016 · 9 comments
Assignees
Labels

Comments

@NinoScript
Copy link

I'm not really sure about this, as I'm new to using remote notifications.
But reading the documentation gives me the impression that the userInfo dictionary has an aps dictionary inside with the values.

So the RemoteNotification's init should actually look more like this:

public init?(remoteNotification: [NSObject : AnyObject]?) {
    guard let remoteNotification = remoteNotification as? [String : AnyObject] else {
        return nil
    }

    let apsKey = "aps"
    let aps = remoteNotification[apsKey] as? [String : AnyObject]

    alert = aps.flatMap(Alert.init(remoteNotification:))
    badge = aps?[badgeKey] as? Int
    sound = aps?[soundKey] as? String
    contentAvailable = aps?[contentAvailableKey] != nil
    categoryIdentifier = aps?[categoryKey] as? String
    remoteNotificationDictionary = remoteNotification

    var customFields = remoteNotification
    customFields.removeValueForKey(apsKey)

    userInfo = customFields
}

At least that seems to work for me, what do you think?

@dfed
Copy link
Collaborator

dfed commented Jul 15, 2016

LaunchItem creates a RemoteNotification with the launchOptions[UIApplicationLaunchOptionsRemoteNotificationKey], which creates a valid remote notification object. When a notification is created via didReceiveRemoteNotification, the notification is not wrapped in an aps key. I believe this works as expected — SuperDelegate is used by Square Cash, and our notification processing works.

Are notifications not working for you with SuperDelegate? If not, can you provide specific reproduction steps and a sample remote notification dictionary payload?

@dfed dfed added the question label Jul 15, 2016
@NinoScript
Copy link
Author

You are supposed to wrap the badge, alert, sound and content-available keys in an aps dictionary when you create the payload in your backend.

Unless you're doing something manually, your notification shouldn't sound or show a badge if those keys aren't inside the aps dictionary.

(At least that's what I understand from the documentation, I also asked in a slack channel and people agree, so I'm not totally crazy.)

@dfed
Copy link
Collaborator

dfed commented Jul 15, 2016

I think you're right re how the server should format the APNS dictionary (I've only done client work, not server work). That said, I'm pretty darned sure we're right on how we're processing RemoteNotifications.

@NinoScript, are you not seeing your RemoteNotifications parsed correctly? Or do you think there's a theoretical bug after reading the code? If the former, can you paste in an NSLog (or print from Swift or po from LLDB) of the remote notification we're having trouble parsing?

Square Cash works just fine with badges, notification sounds, etc. So I'm reasonably convinced that we're doing the right thing on our side. But I'm curious what your experience is!

@NinoScript
Copy link
Author

I'm not seeing RemoteNotifications parsed correctly, this is not theoretical, and the code I posted makes it work.

Here's an example of a working remote notification, where the RemoteNotification object has sound and contentAvailable incorrectly set to nil and false respectively.

(this was printed inside RemoteNotification.init)

(lldb) po remoteNotification
▿ 5 elements
  ▿ [0] : 2 elements
    - .0 : "type"
    - .1 : messages.create
  ▿ [1] : 2 elements
    - .0 : "user_id"
  ▿ [2] : 2 elements
    - .0 : "organization_id"
  ▿ [3] : 2 elements
    - .0 : "aps"
    ▿ .1 : 2 elements
      ▿ [0] : 2 elements
        - .0 : sound
        - .1 : 
      ▿ [1] : 2 elements
        - .0 : content-available
  ▿ [4] : 2 elements
    - .0 : "conversation_id"

(lldb) po remoteNotification.debugDescription
"[\"type\": messages.create, \"user_id\": 30, \"organization_id\": 1, \"aps\": {\n    \"content-available\" = 1;\n    sound = \"\";\n}, \"conversation_id\": 1426]"

I think in your case you're probably re-firing a local notification with that data.

Look, here's some random people printing the contents of userInfo inside application(application:, didReceiveRemoteNotification userInfo:)
http://stackoverflow.com/q/28596295/1904287
http://stackoverflow.com/q/34399848/1904287

You'll notice in both cases they access those properties inside userInfo["aps"]

@dfed
Copy link
Collaborator

dfed commented Jul 15, 2016

I'm not firing a local notification (we rarely ever do that). Looking into this. It's possible we missed something since we don't tend to use alert, badge or sound keys in Cash.

@dfed
Copy link
Collaborator

dfed commented Jul 15, 2016

Fix coming for the 0.9 branch shortly!

dfed pushed a commit that referenced this issue Jul 15, 2016
@dfed
Copy link
Collaborator

dfed commented Jul 15, 2016

0.9.1 has the fix. Waiting for review on 0.8.1.

@dfed dfed closed this as completed in #8 Jul 16, 2016
@dfed
Copy link
Collaborator

dfed commented Jul 16, 2016

0.8.1 has been pushed to cocoapods!

@NinoScript
Copy link
Author

That's great! Now I can remove my fix and pull the latest version. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants