Skip to content

Adds support for PushScheduling #3722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 15, 2017
Merged

Adds support for PushScheduling #3722

merged 8 commits into from
Apr 15, 2017

Conversation

flovilmart
Copy link
Contributor

Originally #3717

Felipe Andrade and others added 6 commits April 13, 2017 11:40
Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. cool.

body['expiration_time'] = PushController.getExpirationTime(body);
body['push_time'] = PushController.getPushTime(body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just below we use body.push_time notation......

@@ -110,6 +110,8 @@ export function pushStatusHandler(config, objectId = newObjectId()) {
const handler = statusHandler(PUSH_STATUS_COLLECTION, database);
const setInitial = function(body = {}, where, options = {source: 'rest'}) {
const now = new Date();
const pushTime = body.push_time || new Date();
const status = body.push_time ? "scheduled" : "pending";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above we use hasOwnPorperty but not here?

@codecov
Copy link

codecov bot commented Apr 15, 2017

Codecov Report

Merging #3722 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3722      +/-   ##
==========================================
+ Coverage   90.42%   90.44%   +0.02%     
==========================================
  Files         114      114              
  Lines        7412     7438      +26     
==========================================
+ Hits         6702     6727      +25     
- Misses        710      711       +1
Impacted Files Coverage Δ
src/cli/definitions/parse-server.js 50% <ø> (ø) ⬆️
src/Routers/FeaturesRouter.js 100% <ø> (ø) ⬆️
src/ParseServer.js 90.25% <100%> (+0.06%) ⬆️
src/Config.js 95.52% <100%> (+0.03%) ⬆️
src/StatusHandler.js 99.05% <100%> (+0.07%) ⬆️
src/Controllers/PushController.js 95.38% <94.44%> (-0.54%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.58% <0%> (-0.14%) ⬇️
src/RestWrite.js 94.51% <0%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 302a0dd...22dc336. Read the comment docs.

@montymxb
Copy link
Contributor

@flovilmart this looks nifty. If this goes in we'll get a potential scheduled status in _PushStatus? We have ParsePushStatus in the php sdk for reading the volatile class, but I'll have to look into adding some constants for any of those status properties if those won't be changing, would be quite nice to have!

Additionally, but somewhat unrelated, to your knowledge are there any sdks setup for reading _PushStatus, and in particular checking for these statuses? I'm assuming it's mostly for internal purposes, and may not be appropriate in some of the sdks, particularly for mobile devices (unless it's a management app of sorts).

Just airing some thoughts on whether the aforementioned class and the possible status values are/will be safe to rely on in the client sdks, if relied on at all.

@flovilmart
Copy link
Contributor Author

Those values are inherited from the good old parse.com.

You're right, they're not supposed to be consumed from client SDK or even queried for that matter. The _PushStatus entries are protected by masterKey.

This state enum should be stable, from scheduled it should move to sending when consumed by the worker.

@montymxb
Copy link
Contributor

Gotcha, good to hear those values are fairly cemented then.

Hmmm, we're living on the wild side in php land by offering that feature then... I figured it's relatively acceptable given the capacity to send pushes is available there, makes sense to compliment it with some sort of basic monitoring as an optional feature.

Anyways just checking, thanks for the info!

@flovilmart
Copy link
Contributor Author

That makes total sense this feature is available on backend SDK's. That would be a shame it wasn't :)

@flovilmart flovilmart merged commit 907b160 into master Apr 15, 2017
@flovilmart flovilmart deleted the pr/3717 branch April 16, 2017 18:12
@felipeandradebezerra
Copy link
Contributor

@flovilmart what are the prereqs to enable scheduling on Parse-server? I'm a bit confused about this feature since we're not going to rely on timers.

Has anyone already started to change the dashboard?

@flovilmart
Copy link
Contributor Author

@felipemobile didn't you started to change the dashboard? For now, the scheduling won't work on parse-server, unless someone polls the _PushStatus table and runs the push sending. We'll figure it out in time, but I believe this will require some kind of distributed cron based schedule. we'll see how it goes, this will also probably enable the jobs. Not sure I want to implement/manage/maintain those schedules, but we'll see.

@felipeandradebezerra
Copy link
Contributor

So pooling will be the mechanism behind scheduled pushs for now.

On the dashboard I have to put some validations and make minor updates based in this pull request.

Is App Store reviews still relevant? If so, it will also rely on timers/cronjobs or do you have another suggestion?

@flovilmart
Copy link
Contributor Author

Not sure about AppStore reviews, that's quite a chicken and egg problem. Do we want to maintain and manage a cron-ish scheduler when so many projects do it well...

@felipeandradebezerra
Copy link
Contributor

Do you plan to release the push scheduled in parse-server version 2.3.9 later this week?

@flovilmart
Copy link
Contributor Author

Probably!

felipeandradebezerra pushed a commit to felipeandradebezerra/parse-dashboard that referenced this pull request Apr 30, 2017
It's assumed that when push scheduling is available, the dashboard
should allow users to choose the delivery time.

PS: There is no scheduling mechanism on parse-server, so pooling
_PushStatus will be the mechanism behind scheduled pushs for now.

More info: parse-community/parse-server#3722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants