Add macOS and tvOS push notification support #58

funkenstrahlen · 2017-02-26T20:01:27Z

Make push notifications work for macOS and tvOS.

This is linked to this issue: #11

Add support for osx and tvos deviceTypes to installation
Add support to ParsePushAdapter and APNS.js to accept more than just iOS certificates.
Add support to ParsePushAdapter and APNS.js to send to tvOS, which reuses the same certificate as iOS.
Update spec

funkenstrahlen · 2017-02-26T20:30:26Z

@flovilmart This is what I came up with. Can you please take a look?

funkenstrahlen · 2017-02-27T07:57:47Z

Is tvos actually the correct deviceType for the Apple TV? Or do I have to use appletv?

funkenstrahlen · 2017-02-27T09:50:53Z

I need some help on the last spec test. I do not know why it fails with cannot found valid connection.

I added some debug logging and saw this:

.ERR! parse-server-push-adapter calling send for devices [object Object],[object Object]
ERR! parse-server-push-adapter APNS found conn iosbundleId for: 0d72a1baa92a2febd9a254cbd6584f750c70b2350af5fc9052d1d12584b738e6
ERR! parse-server-push-adapter APNS found conn iosbundleId for: ff3943ed0b2090c47e5d6f07d8f202a10427941d7897fda5a6b18c6d9fd07d48
ERR! parse-server-push-adapter calling send for devices [object Object]
ERR! parse-server-push-adapter APNS found conn osxbundleId for: 5cda62a8d88eb48d9111a6c436f2e326a053eb0cd72dfc3a0893089342602235
ERR! parse-server-push-adapter calling send for devices [object Object]
ERR! parse-server-push-adapter APNS found conn iosbundleId for: 3e72a1baa92a2febd9a254cbd6584f750c70b2350af5fc9052d1d12584b738e6
ERR! parse-server-push-adapter calling send for devices [object Object]
ERR! parse-server-push-adapter APNS cannot find vaild connection for ff3943ed0b2090c47e5d6f07d8f202a10427941d7897fda5a6b18c6d9fd07d48
ERR! parse-server-push-adapter APNS cannot find vaild connection for 0d72a1baa92a2febd9a254cbd6584f750c70b2350af5fc9052d1d12584b738e6
ERR! parse-server-push-adapter APNS cannot find vaild connection for 3e72a1baa92a2febd9a254cbd6584f750c70b2350af5fc9052d1d12584b738e6

It groups devices correctly: ios, osx, android, tvos. It also finds matching connection configuration for each group and uses the ios configurated connection for tvos deviceType.

However it reports invalid connection for ios and tvos devices later. Any idea what part in the code causes this problem?

funkenstrahlen · 2017-03-03T11:29:44Z

I spend hours trying to figure out why this test case does fail. Because I did not know what change caused the test case to fail I switched back to the master branch. Result: The test case does not run either!

npm test

> [email protected] test /Users/funkenstrahlen/Projects/podlive/parse-server-push-adapter
> TESTING=1 babel-node ./node_modules/.bin/isparta cover --root src/ ./node_modules/jasmine/bin/jasmine.js

Started
...................ERR! parse-server-push-adapter GCM send errored: 401
........ERR! parse-server-push-adapter APNS no qualified connections for anotherBundle c5ee8fae0a1c
ERR! parse-server-push-adapter APNS no qualified connections for anotherBundle c5ee8fae0a1c5805e731cf15496d5b2b3f9b9c577353d3239429d3faaee01c79
ERR! parse-server-push-adapter GCM send errored: 401
.


28 specs, 0 failures
Finished in 0.448 seconds

No coverage information was collected, exit without writing coverage information

So I do not see how I can make this test pass if it does not even work in your current master.

@flovilmart I really need help on this PR now. I do not see what I can improve now.

flovilmart · 2017-03-03T13:07:08Z

The print out shows that the tests are passing. (0 failures)

funkenstrahlen · 2017-03-03T13:20:23Z

So this is ready to merge?

…

On 3 Mar 2017, at 14:07, Florent Vilmart ***@***.***> wrote: The print out shows that the tests are passing. (0 failures) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

flovilmart · 2017-03-03T13:28:31Z

I'll check it out on my machine and see what can fail

flovilmart · 2017-03-03T14:02:57Z

I found the reason of the failures (APNs is throwing an exception when the certificate is missing), and I'Ll try to provide a fix on the top of your branch, Is that ok for you?

funkenstrahlen · 2017-03-03T15:09:16Z

I found the reason of the failures (APNs is throwing an exception when the certificate is missing), and I'Ll try to provide a fix on the top of your branch, Is that ok for you?

@flovilmart This is great news! Of course you can add your changes. I granted you push access to my fork in case it is required.

Thank you for looking into it and helping me out! :)

flovilmart · 2017-03-03T16:10:17Z

I granted you push access to my fork in case it is required.

That's not required, I can push on the PR branch directly :) Thanks!

flovilmart · 2017-03-05T20:30:51Z

@funkenstrahlen I uncovered additional inconsistencies with our implementation. I'm still working on it :)

flovilmart · 2017-02-26T21:13:39Z

src/ParsePushAdapter.js

      let devices = deviceMap[pushType];
+
+      if (!sender) {
+        if (pushType == 'tvos') {


not sure this fallback is required. Even if it leads to redundancy for all 'apple' push types.

I added this as it was suggested in the related issue. However I also think it is better to not add redundancy and require the admin to define a certificate for each plattform he wants to support.

flovilmart · 2017-02-26T21:13:56Z

src/ParsePushAdapter.js

+          sender = this.senderMap['ios'];
+        } else if (pushType == 'ios') {
+          // if there is only a tvos push configuration available reuse it for ios
+          sender = this.senderMap['tvos'];


it may also be 'macos'

Are you suggesting to also add macos into this redundancy?

Actually I'm not sure if we should automatically do that. Do you Think of any real world use case?

I personally think it is is more transparent if we do not add this redundancy. If someone needs support for multiple push targets he needs to define all of them in his config. He can use the same certificate of course.

If you agree on this I will remove the redundancy code and adjust the spec.

Fully agreed! less code, better code!

funkenstrahlen · 2017-03-13T08:44:12Z

I uncovered additional inconsistencies with our implementation. I'm still working on it :)

@flovilmart Thanks for fixing the spec! Anything further I can do to help getting this merged? What inconsistencies did you recognize?

funkenstrahlen · 2017-03-14T09:33:04Z

@flovilmart I removed the redundancy push configuration code as we discussed and also adjusted the spec.

flovilmart · 2017-03-14T12:50:11Z

Yeah! Took some commits but we're there!!

funkenstrahlen · 2017-03-14T13:21:29Z

Awesome! Thanks for helping me getting this done! :)

Do you think this is worth a version bump?

flovilmart · 2017-03-14T13:22:23Z

Yep definitely! We should probably release it :)

Do you want to open a PR with the version bump and CHANGELOG?

funkenstrahlen added 3 commits February 26, 2017 20:59

add macos and tvos types

fca23a7

return device type

6d8e1f7

reuse ios configuration for tvos pushes

077d5d9

add osx and tvos to some of the specs

9814917

funkenstrahlen added 11 commits February 27, 2017 09:09

add more specs

7136707

fix spec

027ca49

test for tvos and ios sender reuse in separate test cases

a0cca14

fix typo

84b8a68

fix spec

b8562ff

fix spec

d53e537

fix define args

9bff36f

remove comment

ad8dee4

update spec

7d27b0c

remove todo

133d149

change device token

539549e

flovilmart added 2 commits March 5, 2017 16:08

Merge branch 'master' into feature/macos-tvos-support

756bac3

Update ParsePushAdapter.spec.js

a4bb2ea

flovilmart reviewed Mar 5, 2017

View reviewed changes

funkenstrahlen added 2 commits March 14, 2017 10:31

remove redundancy in push configuration

7daa4f5

fix code style

02b951a

change var back to let

8752872

flovilmart merged commit fd16ccc into parse-community:master Mar 14, 2017

flovilmart mentioned this pull request Mar 15, 2017

Push Notifications support for OS X, tvOS #11

Closed

snyk-bot mentioned this pull request May 6, 2021

[Snyk] Upgrade @parse/node-apn from 4.0.0 to 4.1.0 #186

Merged

cbaker6 mentioned this pull request Jan 17, 2025

feat: Add support for watchOS push notifications #360

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macOS and tvOS push notification support #58

Add macOS and tvOS push notification support #58

funkenstrahlen commented Feb 26, 2017 •

edited

Loading

funkenstrahlen commented Feb 26, 2017

funkenstrahlen commented Feb 27, 2017

funkenstrahlen commented Feb 27, 2017 •

edited

Loading

funkenstrahlen commented Mar 3, 2017 •

edited

Loading

flovilmart commented Mar 3, 2017

funkenstrahlen commented Mar 3, 2017 via email

flovilmart commented Mar 3, 2017

flovilmart commented Mar 3, 2017

funkenstrahlen commented Mar 3, 2017 •

edited

Loading

flovilmart commented Mar 3, 2017

flovilmart commented Mar 5, 2017

flovilmart Feb 26, 2017

funkenstrahlen Mar 6, 2017

flovilmart Feb 26, 2017

funkenstrahlen Mar 13, 2017

flovilmart Mar 13, 2017

funkenstrahlen Mar 13, 2017 •

edited

Loading

funkenstrahlen Mar 13, 2017 •

edited

Loading

flovilmart Mar 13, 2017

funkenstrahlen commented Mar 13, 2017 •

edited

Loading

funkenstrahlen commented Mar 14, 2017

flovilmart commented Mar 14, 2017

funkenstrahlen commented Mar 14, 2017

flovilmart commented Mar 14, 2017 •

edited

Loading

Add macOS and tvOS push notification support #58

Add macOS and tvOS push notification support #58

Conversation

funkenstrahlen commented Feb 26, 2017 • edited Loading

funkenstrahlen commented Feb 26, 2017

funkenstrahlen commented Feb 27, 2017

funkenstrahlen commented Feb 27, 2017 • edited Loading

funkenstrahlen commented Mar 3, 2017 • edited Loading

flovilmart commented Mar 3, 2017

funkenstrahlen commented Mar 3, 2017 via email

flovilmart commented Mar 3, 2017

flovilmart commented Mar 3, 2017

funkenstrahlen commented Mar 3, 2017 • edited Loading

flovilmart commented Mar 3, 2017

flovilmart commented Mar 5, 2017

flovilmart Feb 26, 2017

Choose a reason for hiding this comment

funkenstrahlen Mar 6, 2017

Choose a reason for hiding this comment

flovilmart Feb 26, 2017

Choose a reason for hiding this comment

funkenstrahlen Mar 13, 2017

Choose a reason for hiding this comment

flovilmart Mar 13, 2017

Choose a reason for hiding this comment

funkenstrahlen Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

funkenstrahlen Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

flovilmart Mar 13, 2017

Choose a reason for hiding this comment

funkenstrahlen commented Mar 13, 2017 • edited Loading

funkenstrahlen commented Mar 14, 2017

flovilmart commented Mar 14, 2017

funkenstrahlen commented Mar 14, 2017

flovilmart commented Mar 14, 2017 • edited Loading

funkenstrahlen commented Feb 26, 2017 •

edited

Loading

funkenstrahlen commented Feb 27, 2017 •

edited

Loading

funkenstrahlen commented Mar 3, 2017 •

edited

Loading

funkenstrahlen commented Mar 3, 2017 •

edited

Loading

funkenstrahlen Mar 13, 2017 •

edited

Loading

funkenstrahlen Mar 13, 2017 •

edited

Loading

funkenstrahlen commented Mar 13, 2017 •

edited

Loading

flovilmart commented Mar 14, 2017 •

edited

Loading