Add support for push scheduling#3717
Add support for push scheduling#3717felipeandradebezerra wants to merge 4 commits intoparse-community:masterfrom Colubris:master
Conversation
Add a configuration flag on the server to handle the availability of push scheduling.
src/Controllers/PushController.js
Outdated
| * @returns {Number|undefined} The push time if it exists in the request | ||
| */ | ||
| static getPushTime(body = {}) { | ||
| var hasPushTime = !!body['push_time']; |
There was a problem hiding this comment.
use body.hasOwnProperty('push_time')
There was a problem hiding this comment.
I've used the same convention found in the getExpirationTime function.
There was a problem hiding this comment.
Yeah, bad conventions from when I started a year ago
flovilmart
left a comment
flovilmart
left a comment
There was a problem hiding this comment.
This is looking great!
Can you add the option to the CLI definitions as well?
I know it's a lot but...
src/Controllers/PushController.js
Outdated
| onPushStatusSaved(pushStatus.objectId); | ||
| return badgeUpdate(); | ||
| }).then(() => { | ||
| if (body.push_time) { |
There was a problem hiding this comment.
if body.push_time is set and the config is not enabled for scheduling, we should warn that the message will be delivered right away no? Only skip if configured, what do you think? I'm thinking of all other users :)
There was a problem hiding this comment.
That makes sense. I've updated the code to only allow skipping on the right configuration.
There was a problem hiding this comment.
@flovilmart What do you mean about CLI definitions?
There was a problem hiding this comment.
Only skip push sending if scheduling is configured
Codecov Report
@@ Coverage Diff @@
## master #3717 +/- ##
==========================================
- Coverage 90.42% 90.31% -0.11%
==========================================
Files 114 114
Lines 7412 7432 +20
==========================================
+ Hits 6702 6712 +10
- Misses 710 720 +10
Continue to review full report at Codecov.
|
|
@flovilmart the code is updated with additional commits :) |
| * @param {Object} request A request object | ||
| * @returns {Number|undefined} The push time if it exists in the request | ||
| */ | ||
| static getPushTime(body = {}) { |
There was a problem hiding this comment.
could we add a test for that piece?
There is a bit of logic there, even trivial but that would help not introduce any regression.
I could add them on the top of your branch if you don't have much time :)
There was a problem hiding this comment.
Sure, we've to add a test. Any help is appreciated.
There was a problem hiding this comment.
i wish i knew enough to help.. i will check your test to help further in any future contribution. Tks in advance @flovilmart
|
@FelipeMobile thanks for getting that one started, I just merged #3722 |
* Fixes #3717 This fixes PR #3717. Sending push with parse-server@2.4.0 returns error 504 GATEWAY_TIMEOUT. This happens when push_time is not set (default). * Fix lint issues * Fix in PushController and add tests Add a test to check push_time format and if it should schedule push when the parse-server is configured
2 participants
Add a configuration flag on the server to handle the availability of
push scheduling.