APNS icon indicating copy to clipboard operation
APNS copied to clipboard
verified publisher icon jpoz

Update core.rb

Open theosp opened this issue 12 years ago • 14 comments

Isn't this loop redundant and make the bulk of notifications to be sent more than once?!

theosp avatar Aug 27 '13 11:08 theosp

We just experienced some serious multi-notification spam via the new library, so I think it may well be...

wlipa avatar Aug 28 '13 20:08 wlipa

Originally from the @toto checkin I think.

wlipa avatar Aug 28 '13 21:08 wlipa

@wlipa indeed, that is a bug I introduced when updating the lib. Will have a look why the tests did not catch this, they should have.

toto avatar Aug 28 '13 21:08 toto

I verified that this is indeed the cause of the multi-notification problem. @theosp can you please bump the bundle version as well (https://github.com/toto/APNS/commit/f134e9057765716a42dc6bfb086e96e1daa1a249), so @jpoz can quickly merge this in.

Building a regression test for this needs a bit more time, but it should be fixed ASAP.

toto avatar Aug 28 '13 21:08 toto

The library is very dangerous to use as is. 1.1 should be pulled unless a fix is forthcoming.

wlipa avatar Aug 29 '13 21:08 wlipa

True, unfortunately only @jpoz can do that. As he does not seem to notice this thread, I will try him via mail.

toto avatar Aug 30 '13 06:08 toto

@toto , @wlipa , @toto In my opinion 1.0 should be set as the production version of the gem for at least a couple of weeks. 1.1 needs more testing even with this fix.

theosp avatar Aug 30 '13 08:08 theosp

I wonder if we can get the gem pulled some other way if @jpoz is out of action. It really shouldn't be distributed in the current state as it very easily leads to an absolute nightmare of multiple alerts and very annoyed users.

wlipa avatar Sep 10 '13 18:09 wlipa

bad time for me to take a vacation i guess. I'll get this merged in. This maybe a a good time to have a collaborator on this gem. Any takers? @toto @theosp

jpoz avatar Sep 11 '13 23:09 jpoz

yanked 1.1.0

jpoz avatar Sep 11 '13 23:09 jpoz

Thanks for merging. would be glad to help.

I will make sure to build a regression test for this problem. Currently only the single notification is tested, not what get's written to the socket in the end. That is how this slipped through.

toto avatar Sep 12 '13 06:09 toto

Can this be pushed out, or is there still work to do?

mkonecny avatar Oct 04 '14 23:10 mkonecny

I have cleaned up a few things and added patches. Take a look at https://github.com/toto/APNS. I will do a new PR, so @jpoz can take a look. I bumped the version to 1.1.1 though, to ensure that everything get's updated correctly after the 1.1.0 debacle.

toto avatar Oct 05 '14 09:10 toto

Also I should note that I use my 1.1.1 code in production sending out push to >100k devices - including content_availiblepushes. So the format implementation is robust.

toto avatar Oct 05 '14 23:10 toto