Add client support for scheduling push notifications #675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
============================================
+ Coverage 52.56% 52.64% +0.08%
- Complexity 1678 1680 +2
============================================
Files 131 131
Lines 10106 10120 +14
Branches 1405 1409 +4
============================================
+ Hits 5312 5328 +16
+ Misses 4353 4352 -1
+ Partials 441 440 -1
Continue to review full report at Codecov.
|
| long now = System.currentTimeMillis() / 1000; | ||
| long twoWeeks = 60*60*24*7*2; | ||
| checkArgument(pushTime > now, "Scheduled push time can not be in the past"); | ||
| checkArgument(pushTime < now + twoWeeks, "Scheduled push time can not be more than " + |
There was a problem hiding this comment.
Is the two week restriction something imposed by the server?
There was a problem hiding this comment.
Yes see here. Same code is in iOS version of this feature so it should be ok
| * Sets a UNIX epoch timestamp at which this notification should be delivered, in seconds (UTC). | ||
| * Scheduled time can not be in the past and must be at most two weeks in the future. | ||
| */ | ||
| public void setPushTime(long time) { |
There was a problem hiding this comment.
Could it be public void setPushTime(Date date)?
(pushTime = date.getTime()/1000)
or change the time unit to milliseconds to match Java's timestamp
(pushTime = time/1000)
Because unit of UNIX epoch timestamp is seconds, but developers probably using Date.getTime() or System.currentTimeMillis() to get timestamp (unit is milliseconds), then they will fail if not converted.
There was a problem hiding this comment.
Yeah seconds are a terrible choice, but we already have setExpirationTime(long) and setExpirationInterval(long) in seconds, it would be confusing to have this in millis or in Date.
Note sure what to do, maybe we should add a Date version for all three methods. What do you think?
There was a problem hiding this comment.
Oh yes, I didn't notice that. We should use the same time unit (seconds) for all relevant methods.
I think it's okay to add a Date version.
| ParsePush.State.Builder builder = new ParsePush.State.Builder(); | ||
|
|
||
| ParsePush.State state = builder | ||
| .pushTime(System.currentTimeMillis() - 1000) |
There was a problem hiding this comment.
It is wrong if pushTime unit not changed as I previously commented.
There was a problem hiding this comment.
time unit is not converted
.pushTime(System.currentTimeMillis() / 1000 - 1)
| ParsePush.State.Builder builder = new ParsePush.State.Builder(); | ||
|
|
||
| ParsePush.State state = builder | ||
| .pushTime(System.currentTimeMillis() - 1000) |
There was a problem hiding this comment.
time unit is not converted
.pushTime(System.currentTimeMillis() / 1000 - 1)
| ParsePush.State.Builder builder = new ParsePush.State.Builder(); | ||
|
|
||
| ParsePush.State state = builder | ||
| .pushTime(System.currentTimeMillis() + 60*60*24*7*3) |
There was a problem hiding this comment.
.pushTime(System.currentTimeMillis() / 1000 + 60*60*24*7*3)
|
Ah, I didn't think about it 🙄 I just wanted to address a very old issue and get on par with iOS. It should do no harm though |
|
Yeah, people may think that 1 we can send push from the clien SDK’s without the master key, and 2 push scheduling is fully supported (only partially IIRC) |
|
Yeah, I think you remember correctly, my bad. I also didn't take it into account in the docs PR here where I just wanted docs to be on par with other clients SDK docs. Same story. @flovilmart should we officially mark as deprecated all the |
|
Enabling client push is dangerous, and already was in 2014, the recommendation from the Parse team was to disable it: http://blog.parse.com/learn/engineering/the-dangerous-world-of-client-push/ |
This adds
ParsePush.setPushTime()which is already present in other client SDKs.Seems that
parse-serverwill be eventually supporting scheduled pushes so we’ll be ready when it happens.Fixes #278 .