Skip to content

Push Status ID and Hash fix #2067

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 4 commits into from
Jul 18, 2016
Merged

Conversation

mihai-iorga
Copy link
Contributor

pushStatusHandler should encode only data.alert to create MD5 Hash

PushController should also send pushStatus.objectId to adapter: adapter can map sends with ids

@codecov-io
Copy link

codecov-io commented Jun 15, 2016

Current coverage is 91.93%

Merging #2067 into master will increase coverage by <.01%

@@             master      #2067   diff @@
==========================================
  Files            93         93          
  Lines          6537       6538     +1   
  Methods        1169       1169          
  Messages          0          0          
  Branches       1370       1371     +1   
==========================================
+ Hits           6010       6011     +1   
  Misses          527        527          
  Partials          0          0          

Powered by Codecov. Last updated by 39f11a5...2017913

@flovilmart
Copy link
Contributor

flovilmart commented Jun 16, 2016

thanks for the PR!
Just to check, why change the hash to just the alert? I may break analytics on parse.com as I understand. I'll let Nikita check on that.

@mihai-iorga
Copy link
Contributor Author

Because push_hash generated from app does not match pushHash generated by pushStatusHandler. I see no other logic assuming they both generate a correlation string between _PushStatus and AppOpened event.

@flovilmart
Copy link
Contributor

@nlutsenko the code look good to me. can you confirm the push_hash is computed that way? given the spec here, the full payload should be hashed: https://gist.github.com/flovilmart/bf64276507d685ae41f6#file-_pushstatus-L44

@nlutsenko
Copy link
Contributor

Will double check and get back to you guys tomorrow.
Off the top of my head - yes, this is correct, but Android can have a different payload structure by default.

Also, I would love to kill push hash completely and replace with push id, so we can get deterministic analytics.

@flovilmart
Copy link
Contributor

flovilmart commented Jun 27, 2016

I'd move for that too, killing the hash, that will require update in the client SDK's which is not that bad.
Also we don't collect analytics on that project, so that won't affect much anyone running parse-server only

@mihai-iorga
Copy link
Contributor Author

@nlutsenko iOS just hashes that payload.alert and posts to /AppOpened, Android receives the hash from payload.push_hash and posts to /AppOpened, so it's legit. You can calculate that md5 and everything works well. You can correlate in db.

So I do not see any problems.

@flovilmart flovilmart modified the milestone: 2.2.17 Jul 12, 2016
@flovilmart flovilmart merged commit 676d2e2 into parse-community:master Jul 18, 2016
yuzeh pushed a commit to yuzeh/parse-server that referenced this pull request Jul 19, 2016
rsouzas pushed a commit to back4app/parse-server that referenced this pull request Mar 15, 2017
rsouzas pushed a commit to back4app/parse-server that referenced this pull request Mar 16, 2017
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.

6 participants