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

Remote Notification Type #60

Merged
merged 8 commits into from
Apr 24, 2018
Merged

Remote Notification Type #60

merged 8 commits into from
Apr 24, 2018

Conversation

lukabratos
Copy link
Contributor

@lukabratos lukabratos commented Apr 9, 2018

What?

Usage:

// Example userInfo payload.
let userInfo = ["aps" : ["alert": ["title": "Hello", "body": "Hello, world!"], "content-available": 1], "data": ["pusher": ["publishId": "pubid-33f3f68e-b0c5-438f-b50f-fae93f6c48df", "userShouldIgnore": true]]]

// Get remote notification type.
// `handleNotification` now returns notification type: `RemoteNotificationType` which can be either `ShouldIgnore` or `ShouldProcess`.
let remoteNotificationType = self.pushNotifications.handleNotification(userInfo: userInfo)
if remoteNotificationType == .ShouldIgnore {
    return
}

CC @pusher/mobile

@codecov-io
Copy link

codecov-io commented Apr 9, 2018

Codecov Report

Merging #60 into master will increase coverage by 13.83%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #60       +/-   ##
===========================================
+ Coverage   83.18%   97.02%   +13.83%     
===========================================
  Files          29       16       -13     
  Lines        1249      538      -711     
  Branches       38        0       -38     
===========================================
- Hits         1039      522      -517     
+ Misses        210       16      -194
Impacted Files Coverage Δ
Tests/SDKTests.swift 100% <100%> (ø) ⬆️
Tests/EventTypeHandlerTests.swift 100% <100%> (+2.94%) ⬆️
Tests/MetadataTests.swift 89.65% <100%> (-1.7%) ⬇️
Tests/InterestNameValidationTests.swift 75% <0%> (-7.23%) ⬇️
Tests/InstanceTests.swift 91.3% <0%> (-1.8%) ⬇️
Tests/ReasonTests.swift 100% <0%> (ø) ⬆️
Tests/RegisterTests.swift 100% <0%> (ø) ⬆️
Tests/PublishIdTests.swift 100% <0%> (ø) ⬆️
Tests/PushNotificationsNetworkableTests.swift 100% <0%> (ø) ⬆️
... and 19 more

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 1297818...35dd774. Read the comment docs.

let pusher = data["pusher"] as? Dictionary<String, Any>
else { return .Other }

return pusher["isInternalOnly"] != nil ? .Internal : .Other
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have isInternalOnly value in the payload we know that it's internal remote notification.
Other doesn't sound right to me - it needs to be renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

User?

@lukabratos lukabratos changed the base branch from update-to-recommended-settings to master April 11, 2018 13:38
@lukabratos lukabratos force-pushed the remote-notification-type branch from a33b90f to a65e998 Compare April 11, 2018 13:44
@@ -67,4 +67,13 @@ struct EventTypeHandler {
// Returns `true` if there is any additional information provided.
return data.count > 1
}

static func isInternalNotification(_ userInfo: [AnyHashable: Any]) -> RemoteNotificationType {
Copy link
Contributor

Choose a reason for hiding this comment

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

getRemoteNotificationType

Copy link
Contributor

@luismfonseca luismfonseca left a comment

Choose a reason for hiding this comment

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

LGTM

@lukabratos lukabratos force-pushed the remote-notification-type branch from 3f45908 to 35dd774 Compare April 24, 2018 09:26
@lukabratos lukabratos merged commit e9d21e4 into master Apr 24, 2018
@lukabratos lukabratos deleted the remote-notification-type branch April 24, 2018 09:35
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