botmasterai / botmaster-messenger

The Facebook Messenger Botmaster integration
MIT License
15 stars 9 forks source link

Enables hearing/replying to more than one page #6

Closed andremw closed 7 years ago

andremw commented 7 years ago

(Closes https://github.com/botmasterai/botmaster-messenger/issues/5)

This method will get the pageToken either from settings.accessToken or from the page corresponding to the bot id (in case settings.pages is passed).

This will enable one bot instance to receive from and send updates to more than one page in the same platform, using the same webhook.

We'll instantiate the plugin the way we already do or this way:

botmaster.addBot(new MessengerBot({
  credentials: {
    verifyToken: '[verifyToken]',
    pages: {
      ['page1Id']: { pageToken: '[page1Token]' },
      ['page2Id']: { pageToken: '[page2Token]' }
    },
    fbAppSecret: '[appSecret]'
  },
  webhookEndpoint: '[webhook]'
}));

I did not update the unit tests for this change because I got some errors with the _config.js module. Can you help me with this?

andremw commented 7 years ago

I still need to change some things.

jdwuarin commented 7 years ago

Continuing conversation from : https://github.com/botmasterai/botmaster/pull/28.

Once the changes mentioned in the above PR are done. Another issue would need to be fixed:

this code:

+  __getAccessToken() {
 +    if (this.credentials.pages) {
 +      return this.credentials.pages[this.id].pageToken;
 +    }
 +    return this.credentials.pageToken;
 +  }

won't work. As you are referencing this.id which is set conditionnaly in __setBotIdIfNotSet:

which has the following implementation

  __setBotIdIfNotSet(update) {
    if (!this.id) {
      debug(`setting id: ${update.recipient.id} to ${this.type} bot`);
      this.id = update.recipient.id;
    }
  }

I.e. the id is set only on the first request. => you will be stuck on the first page. And the issue you had before will still be present. Now, one might think that fixing it by removing the condition in __setBotIdIfNotSet would do the trick, but that's unfortunately not the case, as the instance might be working on two requests from different pages in parallel. When that occurs, it will try to answer to both users using the last id it set (i.e. from the last request that actually came in).

The best way to solve this is really to do something like I mentioned in https://github.com/botmasterai/botmaster-messenger/issues/5 (although it won't work as I wrote it there either...). This will however: . I.e:

  __sendMessage(message, sendOptions) {
    const sendAsId = sendOptions.sendAsPageId || sendOptions.__update.recipient.id
    const options = {
      uri: baseMessageURL,
      qs: { access_token: this.credentials.pages[sendAsId].pageToken },
      method: 'POST',
      json: message,
    };

    return request(options);
  }

In this situation, you can either pass in an option (to, say, send a message as a page different from the one the update was received on). Or use the __update in sendOptions (which is always passed to __sendMessage and was just ignored as wasn't useful) to send the message as the page that received the update

andremw commented 7 years ago

This is correct. I don't know what tests I was doing that led me to think my changes were enough when I opened the PR, but now I see what's wrong and how to fix it. I'm gonna reopen it as soon as it's fixed.

One question: couldn't we do const sendAsId = sendOptions.sendAsPageId || this.id, as the latter usually corresponds to what would be update.recipient.id?

andremw commented 7 years ago

@jdwuarin unfortunately using sendOptions wouldn't work for the other methods such as __getUserInfo and _messengerProfileRequest, unless we change the botmaster, because they don't have something such as sendOptions. What do you think?

jdwuarin commented 7 years ago

You're absolutely right. I think the solution would have to lie in changing the monkey patch that is going on : https://github.com/botmasterai/botmaster/blob/master/lib/base_bot.js#L772. And changing it to take it out of a patched sendMessage function to something like this:

  __createBotPatchedWithUpdate(update) {
    const newBot = Object.create(this);
    newBot.__associatedUpdate = update;
    return newBot;
  }

Then here: https://github.com/botmasterai/botmaster/blob/master/lib/base_bot.js#L253

Update that line to:

        outgoingMiddlewarePromise = this.master.middleware.__runOutgoingMiddleware(
          this, this.__associatedUpdate, outgoingMessage);

where __associatedUpdate may be undefined.

Then instead of using sendOptions in the above solution, using simply this.__associatedUpdate should do the trick. I.e.:

  __sendMessage(message, sendOptions) {
    const sendAsId = sendOptions.sendAsPageId || this.__associatedUpdate.recipient.id
    const options = {
      uri: baseMessageURL,
      qs: { access_token: this.credentials.pages[sendAsId].pageToken },
      method: 'POST',
      json: message,
    };

    return request(options);
  }

(where you'll want to handle an error when both `sendAsPageId and __associatedUpdate is not provided). This can happen when trying to push a message to a user (without having received an associatedUpdate).

You could then use an option in the other methods (getUserInfo in botmaster will have to be updated to accept an options object that it passes down to the platform specific implementation. That is a change I'm fine with in the core)

Does that make sense?

andremw commented 7 years ago

Yep, it sounds good! I'm gonna try it.