parse-community / parse-server-push-adapter

A push notification adapter for Parse Server
https://parseplatform.org
MIT License
87 stars 99 forks source link

Add macOS and tvOS push notification support #58

Closed funkenstrahlen closed 7 years ago

funkenstrahlen commented 7 years ago

Make push notifications work for macOS and tvOS.

This is linked to this issue: https://github.com/parse-server-modules/parse-server-push-adapter/issues/11

funkenstrahlen commented 7 years ago

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

funkenstrahlen commented 7 years ago

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

funkenstrahlen commented 7 years ago

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 commented 7 years ago

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

> parse-server-push-adapter@1.2.0 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 commented 7 years ago

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

funkenstrahlen commented 7 years ago

So this is ready to merge?

On 3 Mar 2017, at 14:07, Florent Vilmart notifications@github.com 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 commented 7 years ago

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

flovilmart commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

funkenstrahlen commented 7 years ago

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 commented 7 years ago

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

flovilmart commented 7 years ago

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

funkenstrahlen commented 7 years ago

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

Do you think this is worth a version bump?

flovilmart commented 7 years ago

Yep definitely! We should probably release it :)

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