Charca / bootbot

Facebook Messenger Bot Framework for Node.js
MIT License
974 stars 252 forks source link

Don't remove buttons without title in _formatButtons #81

Closed LeartS closed 6 years ago

LeartS commented 6 years ago

I've spent a good hour and an half trying to understand why facebook was returning an error when I was trying to send a button template with a single login button. Turns out it was a library bug :)

I'm not sure why the _formatButtons methods would silently discards buttons at all, in my opinion it should send them all and let facebook return an error if some required button attributes are missing; or at the very list show a warning for discarded buttons!

But, to have a behaviour as similar as possible to the current one, I've just changed the check from title (not used in all buttons) to type (required for all button types).

The commit message has more details:

The _formatButton method, called by sendButtonTemplate and sendListTemplate, discarded all buttons without a 'title' key. But not all buttons usable in those methods are required to have a title: notably, the login and logout buttons of the account linking flow must only specify the 'type' and 'url' keys.

This resulted in an error when trying to send a button template with only a login button, for example using:

   bot.say({
     text: 'In order to use the bot, you must authenticate to our website',
     buttons: [{type: 'account_link', url: '<myLoginUrl>'}]
   })

because the bot silently discarded the only button, causing Facebook to return an error due to a request for a button template message without buttons.

LeartS commented 6 years ago

For those afflicted by this bug: in the meantime as a workaround you can use sendGenericTemplate, which doesn't call _formatButtons. Or if you don't want a generic template but a button template, you can go "low-level" and call sendTemplate like this:

bot.sendTemplate({
    template_type: 'button',
    text: 'Please login',
    buttons: [{type: 'account_link', url: login_webview_url}]
})
Charca commented 6 years ago

Thanks, @LeartS!

If I remember correctly the title attribute was required on the first version of the Messenger API, and the login and logout buttons didn't exist back then, that's why we had that condition.

But yes, this needed to be updated. Thanks for the PR.