Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add GCM client #227

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Add GCM client #227

merged 1 commit into from
Feb 5, 2016

Conversation

wangmengyan95
Copy link
Contributor

Add GCM client for push notification. For the expirationTime, we use a fixed length string in go, do we need to follow this?

@wangmengyan95 wangmengyan95 changed the title Add GCM sender Add GCM client Feb 4, 2016
@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_gcm_client branch 2 times, most recently from 8f03c5f to 8e38a04 Compare February 4, 2016 08:29
@wangmengyan95
Copy link
Contributor Author

cc @fantasist for review

grantland
grantland reviewed Feb 4, 2016
View reviewed changes
GCM.js
'data': JSON.stringify(coreData)
}
var payload = {
priority: 'high',
Copy link

Choose a reason for hiding this comment

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

We shouldn't default to high on GCM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason besides the battery concern?

Copy link

Choose a reason for hiding this comment

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

It's also not the current default for hosted Parse

@bnham
Copy link

bnham commented Feb 4, 2016

Mostly LGTM.

For the date, you get to define the parameters here right? It seems like expiration_date should just be a regular JS date or null.

For priority, here was our plan for supporting GCM priorities in the API server:

  1. If the user specifies "priority": "high" to /1/push, then send with high priority.
  2. If the user specifices "priority": "normal" to /1/push, then send with normal priority.
  3. Otherwise, send with high priority only if the alert key is present in the payload.

If you do anything with priority here, I vote for either always sending with low priority (which we currently do in the API), or using the simple policy described in (3).

@wangmengyan95
Copy link
Contributor Author

@bnham , for the expiration_time, we handle the expiration_time convertion in push.js, so expiration_time is a valid date in Unix epoch time in milliseconds here

@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

bnham
bnham reviewed Feb 5, 2016
View reviewed changes
GCM.js
*/
var generateGCMPayload = function(coreData, pushId, timeStamp, expirationTime) {
var payloadData = {
'time': timeStamp.toString(),
Copy link

Choose a reason for hiding this comment

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

The current API server sends an ISO8601-formatted date like "2016-02-05T13:08:00.000Z" here, not an integer. I think we should match that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

wangmengyan95 added a commit that referenced this pull request Feb 5, 2016
@wangmengyan95
Merge pull request #227 from ParsePlatform/wangmengyan.add_gcm_client
116c632 
Add GCM client
@wangmengyan95 wangmengyan95 merged commit 116c632 into master Feb 5, 2016
@wangmengyan95 wangmengyan95 deleted the wangmengyan.add_gcm_client branch February 5, 2016 21:58
@snyk-bot snyk-bot mentioned this pull request May 21, 2023
Closed
@yozef yozef mentioned this pull request Mar 2, 2024
Open
@Arul- Arul- mentioned this pull request Mar 2, 2024
Open
@RaschidJFR RaschidJFR mentioned this pull request Mar 2, 2024
Open
@kiarza2543 kiarza2543 mentioned this pull request Mar 2, 2024
Open
@ajesse11x ajesse11x mentioned this pull request Mar 3, 2024
Open
@skmezanul skmezanul mentioned this pull request Mar 3, 2024
Open
@pradyum07 pradyum07 mentioned this pull request Mar 4, 2024
Open
@pradyum07 pradyum07 mentioned this pull request Mar 30, 2024
Open
@Arul- Arul- mentioned this pull request Mar 30, 2024
Open
@skmezanul skmezanul mentioned this pull request Mar 31, 2024
Open
@kiarza2543 kiarza2543 mentioned this pull request Mar 31, 2024
Open
@RaschidJFR RaschidJFR mentioned this pull request Mar 31, 2024
Open
@pradyum07 pradyum07 mentioned this pull request May 21, 2024
Open
@ajesse11x ajesse11x mentioned this pull request May 21, 2024
Open
@RaschidJFR RaschidJFR mentioned this pull request May 21, 2024
Open
@Arul- Arul- mentioned this pull request May 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bnham bnham

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wangmengyan95 @bnham @facebook-github-bot @grantland