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

Handle a wider variety of messages more gracefully #79

Merged
merged 14 commits into from
Oct 27, 2017

Conversation

crccheck
Copy link
Contributor

@crccheck crccheck commented Oct 25, 2017

Why are we doing this?

Facebook uses webhooks not only for Messenger, but for all sorts of events. Currently, if a request comes into the bot and it's not a Messenger event, it'll throw an error. Instead of throwing an error, it should just ignore non Messenger events.

  • Handles non-Messenger Facebook messages that can come in
  • some YAGNI deletions

closes #73

  • DRY the tests

Did you document your work?

none necessary, internal changes

How can someone test these changes?

Steps to manually verify the change:

  1. npm install
  2. npm test

You can also

  1. npm link this to an existing bot
  2. go the the developer site (example: https://developers.facebook.com/apps/1156126794463877/webhooks/ )
  3. hit the "Test" button
  4. your bot should not throw a 500

What possible risks or adverse effects are there?

  • none

What are the follow-up tasks?

  • none

Are there any known issues?

The Conversation logger could use more robust logic, but skipping that for now because "Slack ghost" might be better approach for logging to Slack.

Did the test coverage decrease?

increases. Some old code is now covered, tests reorganized a bit.

@crccheck crccheck requested a review from stripethree October 26, 2017 20:29
@crccheck crccheck changed the title WIP Handle a wide variety of messages more gracefully Handle a wider variety of messages more gracefully Oct 26, 2017
@crccheck
Copy link
Contributor Author

Ok, did some cleanup so ready for review now

Copy link
Contributor

@stripethree stripethree left a comment

Choose a reason for hiding this comment

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

npm i
npm t

Nice work on the test cleanup, coverage, and migration to supertest!

@crccheck crccheck merged commit b147efa into master Oct 27, 2017
@crccheck crccheck deleted the stricter-conversationLogger branch October 27, 2017 17:16
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.

Add support for the default Facebook webhook debug message
2 participants