Skip to content

Change pushHash with stringified data #2397

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
Merged
flovilmart merged 4 commits into from
Aug 9, 2016
Merged

Change pushHash with stringified data #2397

flovilmart merged 4 commits into from
Aug 9, 2016

Conversation

@alenoir
Copy link
Contributor

@alenoir alenoir commented Jul 26, 2016

Notification with loc-key and loc-args crash because data.alert is not a string.

e.g.

alert: {
  'loc-key': '....',
  'loc-args': '...',
}

@codecov-io
Copy link

codecov-io commented Jul 26, 2016 •
edited
Loading

Current coverage is 92.47% (diff: 100%)

Merging #2397 into master will increase coverage by 0.63%

@@             master      #2397   diff @@
==========================================
  Files            95         93     -2   
  Lines         10708      10622    -86   
  Methods        1309       1311     +2   
  Messages          0          0          
  Branches       1741       1727    -14   
==========================================
- Hits           9835       9823    -12   
+ Misses          873        799    -74   
  Partials          0          0

Powered by Codecov. Last update 831b29b...7ca6190

@ghost
Copy link

ghost commented Jul 26, 2016

@alenoir updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jul 26, 2016

@alenoir updated the pull request.

@ghost
Copy link

ghost commented Jul 26, 2016

@alenoir updated the pull request.

@ghost
Copy link

ghost commented Jul 27, 2016

@alenoir updated the pull request.

@alenoir
Copy link
Contributor Author

alenoir commented Jul 27, 2016

as we said in 676d2e2 i replace payloadstring by stringify data.alert for localization format

alenoir referenced this pull request Jul 27, 2016
@mihai-iorga @flovilmart
pushStatusHandler wrong payload md5, PushController send pushId to ad…
676d2e2 
…apter also (#2067)
@protspace
Copy link

protspace commented Aug 3, 2016

the same for me.
info: warning: error while sending push TypeError: Not a string or buffer
and then

Error: {"code":141,"message":{"code":107,"message":"Received an error with invalid JSON from Parse: <html>\r\n<head><title>502 Bad Gateway</title></head>\r\n<body bgcolor=\"white\">\r\n<center><h1>502 Bad Gateway</h1></center>\r\n<hr><center>nginx/1.6.2</center>\r\n</body>\r\n</html>\r\n"}}

@0xthk
Copy link

0xthk commented Aug 6, 2016 •
edited
Loading

This should be merged soon... All iOS localized push will fail on version 2.2.17!

flovilmart
flovilmart reviewed Aug 6, 2016
View reviewed changes
src/pushStatusHandler.js Outdated
status: "pending",
numSent: 0,
pushHash: md5Hash(data.alert || ''),
pushHash: md5Hash(JSON.stringify(data.alert) || ''),
Copy link
Contributor

@flovilmart flovilmart Aug 6, 2016

Choose a reason for hiding this comment

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

in the case alert is a string, that will wrap the string into another string, including the quotes...
This should be updated to:

let pushHash;
if (typeof data.alert === 'string') {
  pushHash = md5Hash(data.alert);
} else if (typeof data.alert === 'object') {
  pushHash = md5Hash(JSON.stringify(data.alert));
} else {
   pushHash = "d41d8cd98f00b204e9800998ecf8427e"; // empty string hash
}

@flovilmart
Copy link
Contributor

flovilmart commented Aug 9, 2016

thanks @alenoir. Tests would have been nice too.

@facebook-github-bot
Copy link

facebook-github-bot commented Aug 9, 2016

@alenoir updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Aug 9, 2016

@alenoir updated the pull request.

@flovilmart
Copy link
Contributor

flovilmart commented Aug 9, 2016

Thanks @alenoir, merging after travis

@flovilmart flovilmart added this to the 2.2.18 milestone Aug 9, 2016
@flovilmart flovilmart merged commit 452887b into parse-community:master Aug 9, 2016
@cbaker6 cbaker6 mentioned this pull request Jun 16, 2022
Closed
4 tasks
@parseplatformorg parseplatformorg mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.2.18

Development

Successfully merging this pull request may close these issues.

6 participants

@alenoir @codecov-io @protspace @0xthk @flovilmart @facebook-github-bot