FB require_delivery: Remove use of message.delivery.mids because not guaranteed

[cchan]

Fixes #1311

[benbrown]

@cchan can you help me understand how to test this?

[cchan]

@benbrown That's an interesting question; I'm not sure how testing works for botkit. Do you have mocks set up for Facebook?

[cchan]

(oops)

[cchan]

@benbrown Hm, could we create a sinon test similar to this existing one? I'm not familiar at all with how botkit works internally, but it seems like there's a polling loop that checks if the previous message has been marked as delivered before sending the next one. We could... somehow hook into that and make sure messages are returned back in order?

[benbrown]

@cchan my original comment probably confused the issue. what I need to know is what settings to enable and what type of events I need to trigger in order to see this fix in action.

[cchan]

@benbrown Ah, that's what you mean. You just need to set require_delivery: true and send a bunch of messages in a row from Botkit - that's pretty much it. Facebook will respond with delivery events, but once in a while the response will not contain the field mids, which crashes the old code.

I haven't been able to figure out what conditions cause Facebook to not provide mids in the response, but after you try it for enough times / long enough it'll just happen, seemingly at random. I think it's probably a load reduction feature on Facebook's end - the "delivered" feature is probably fundamentally based on the watermark timestamp, and when they don't have the capacity to generate the corresponding list of message IDs, they simply don't.

(The reason I found this bug is that I have a bot on my server (clive-io/statbot) that monitors logs et cetera, and it was crashing at random about once a week.)

[RJiryes]

Hey is this going to be merged anytime soon? Great catch @cchan ;-)

[benbrown]

@cchan @RJiryes merged!

[cchan]

Awesome! :D

[RJiryes]

@benbrown Thanks!