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.

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

Add read receipt message type #41

Merged
merged 3 commits into from
Jun 8, 2016
Merged

Conversation

devchakraborty
Copy link
Contributor

I have no idea why this started happening, but Facebook started sending me messaging payloads that look like this:

{"sender"=>{...}, "recipient"=>{...}, "timestamp"=>1465354035944, "read"=>{"watermark"=>1465354034142, "seq"=>33}}

which raises an UnknownPayload and breaks my app 😱. This behaviour doesn't appear to be documented by Facebook yet.

This PR adds support for this new type of payload.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 98.272% when pulling 5e8e145 on devchakraborty:master into bfff15c on hyperoslo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 98.704% when pulling 3bf69b2 on devchakraborty:master into bfff15c on hyperoslo:master.

@mengqing
Copy link
Contributor

mengqing commented Jun 8, 2016

I was wondering whether Facebook::Messenger::Incoming::UnknowPayload should be raised at all if the incoming payload can not be recognized rather than just ignore the payload. This is so that in case Facebook decides to add new payload in the future and every time they do that, the app would be broken

@jgorset
Copy link
Owner

jgorset commented Jun 8, 2016

Awesome, @devchakraborty! This is why we <3 open source. Thank you.

@jgorset jgorset merged commit 4be960b into jgorset:master Jun 8, 2016
@jgorset
Copy link
Owner

jgorset commented Jun 8, 2016

I agree, @mengqing – let's log a warning, not crash with an exception?

@jgorset
Copy link
Owner

jgorset commented Jun 8, 2016

This might just start happening to everyone, so I released version 0.6.0.

@sorich87 sorich87 mentioned this pull request Jul 21, 2016
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.

None yet

4 participants