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

Support for Messenger Platform 1.1 features #26

Merged
merged 3 commits into from
Oct 17, 2016
Merged

Conversation

eXeDK
Copy link
Collaborator

@eXeDK eXeDK commented Jul 6, 2016

No description provided.

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Coverage decreased (-25.6%) to 74.39% when pulling 5bc3b89 on eXeDK:master into 1ac973a on remixz:master.

@danvass
Copy link

danvass commented Jul 14, 2016

@remixz Can you merge this please?

@eXeDK eXeDK mentioned this pull request Jul 14, 2016
3 tasks
@adamjace
Copy link

adamjace commented Aug 2, 2016

please merge @remixz

@victorcotap
Copy link
Contributor

Please merge @remixz

@Naramsim
Copy link

Naramsim commented Aug 6, 2016

Please Merge and huge thanks to @eXeDK 🎆

@Naramsim
Copy link

Naramsim commented Aug 6, 2016

Would be useful to add: https://developers.facebook.com/docs/messenger-platform/thread-settings/greeting-text

To show an initial greeting 😄

@iMicknl
Copy link

iMicknl commented Aug 6, 2016

Coverage decreased (-25.6%) to 74.39% when pulling 5bc3b89 on eXeDK:master into 1ac973a on remixz:master.

Why are there no tests included? I think it is quite important to add some tests, before this will be merged.

@eXeDK
Copy link
Collaborator Author

eXeDK commented Aug 16, 2016

Bump

@remixz
Copy link
Owner

remixz commented Sep 7, 2016

@eXeDK Hey! Thanks for your contribution 😄, sorry I'm so late. I'll agree with @iMicknl that tests should be included for 100% coverage, plus we'd probably want documentation as well. However, I'm gonna add you as a contributor to this repo now, since you've been super helpful in other issues, which I really appreciate. If you want to merge this just to get things started, and work on tests later, go for it. I'll make sure to keep an eye on things, and do a release once everything's good.

@@ -104,6 +162,11 @@ class Bot extends EventEmitter {
this._handleEvent('message', event)
}

// handle echos
if (event.message && event.message.is_echo) {

Choose a reason for hiding this comment

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

if (event.message.is_echo) { ... } would be enough.

Copy link

Choose a reason for hiding this comment

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

not true. you would get a typeError if message was undefined

Choose a reason for hiding this comment

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

Correct.

if (event.message) {
    if (event.message.is_echo) {
         this._handleEvent('echoMessage', event)
    } else {
         this._handleEvent('message', event)
    }
}

@charlesaraya
Copy link

charlesaraya commented Sep 9, 2016

It should be if ... else if ... else instead of a whole bunch of if's. It's a waste of execution time to keep checking falsy conditions, since calbacks types are muatually exclusive. For better performance use switch (Writting Efficient Javascript).

Plus, I would handle unknown events:

if (event.oneCallbackType) {
    ... handle it ...
} else if (event.othercallbackType) {
    ... handle it ...
} else if ( ... ){    // as many as needed
    ... handle these too ...
} else {
    console.error('Webhook received an unknown messaging event: ', JSON.stringify(event))
} 

@coveralls
Copy link

coveralls commented Sep 18, 2016

Coverage Status

Coverage decreased (-7.5%) to 92.473% when pulling f36ecca on eXeDK:master into 1ac973a on remixz:master.

@remixz remixz merged commit f36ecca into remixz:master Oct 17, 2016
@remixz
Copy link
Owner

remixz commented Oct 17, 2016

I went ahead and finished this PR with full test coverage and documentation. Thank you for your hard work on this! Released as 2.4.0.

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

9 participants