Add smart reply to remove boilerplate #53

crccheck · 2017-04-14T05:06:20Z

Why are we doing this?

Multitenant bots will have to specify page id when sending a message. To do that now, they would have to do messenger.send(senderId, responseObj, session._pageId), which is very not ideal. Luckily everything has been backwards compatible and the page routing logic has been hidden. This adds another backwards compatible change so bots only have to do reply(responseObj), and the state required has been tucked into the reply function.

Some other changes:

new test.env. I didn't want to add another .env, but I needed stuff in the test environment that isn't applicable to users. This means we can add comments to example.env

closes #50

Did you document your work?

Some missing docs have been added to the Readme, new functionality is there too, and existing docs are updated.

How can someone test these changes?

Steps to manually verify the change:

npm i
npm t

Link to an existing bot and make sure your existing bot works:

npm link
go to your bot
npm link launch-vehicle-fbm
run and test your bot

What possible risks or adverse effects are there?

The function signature for send is changed. But it's done in a backwards compatible change. That means that the arguments are currently introspected before being used, which adds a mess of code. We just have to hope to mature enough so the page config transition lets us delete the adapter code.
in Add plumbing for page specific operations #49, I made send(senderId, responseObj, pageId) but here I changed my mind and went with send(pageId, senderId, responseObj) because putting pageId at the end looked so hacky. Now the arguments are in a more logical order.
This seems like a lot of LOC, but it's almost all docs and tests. I'm not really happy with how many lines it is, but reading the diff, it seems reasonable.

What are the follow-up tasks?

~~log a deprecation warning when bot is launched w/o page config~~
none

Are there any known issues?

None

Did the test coverage decrease?

new code is covered

stripethree

Didn't try this yet, but going to now. Changes requested re: wanting more info on WIP comments.

stripethree · 2017-04-14T14:29:32Z

src/app.js

@@ -181,6 +202,7 @@ class Messenger extends EventEmitter {
      .then((session) => this.saveSession(session));
  }

+  // WIP


Unclear what this WIP is referring to. Doesn't seem to match anything in the follow-up tasks...?

oh, I was marking that these methods are vestigial from the beauty lens bot and they're kinda dead code at this point but we want to bring them back eventually. I didn't want to start yak shaving to bring them up to "code" 😸 to keep this PR smaller. But I wanted to leave a mark to remind myself to stop trying to work on them.

Ok, cool. Seems like good candidates for linking to issues.

stripethree · 2017-04-14T14:45:28Z

src/app.js

    debug('onAuth for user:%d with param: %j', senderId, optinRef);
  }

  /*
    This is not an event triggered by Messenger, it is the post-back from the
    static Facebook login page that is made to look similar to an 'event'
  */
+  // WIP


Ditto here.

stripethree · 2017-04-14T14:51:21Z

src/app.js

@@ -355,17 +378,25 @@ class Messenger extends EventEmitter {
    return this.cache.set(session._key, session);
  }

-  send(recipientId/*: string|number */, messageData/*: Object */, pageId/*: string|void */)/* Promise<Object> */ {
+  send(arg1/*: string|number */, arg2/*: string|number|Object */, arg3/*: Object|void */)/* Promise<Object> */ {


Since this is going to be refactored into simpler logic, have you considered making the new paradigm a wrapper function for the time being and then we replace send down the line? This would also mean changing the third arg to the pageAccessToken rather than the ID, but I think that's a fair tradeoff since that is currently an optional arg and it would reduce LOC.

pageSend(pageId, recipientId, messageData) { const pageAccessToken = this.pages[pageId]; return this.send(recipientId, messageData, pageAccessToken); }

send(recipientId, messageData, accessToken) { // kills ~10 LOC here const pageAccessToken = accessToken || config.get('messenger.pageAccessToken'); const options = {... } debug('message.send: %j', options); return reqPromise.post(options) ... }

Later on, the contents of send can be merged into pageSend or send can be replace by pageSend.

Both approaches are a bit awkward TBH. I find the above alternative easier to follow and to me, it communicates more about where the code is headed. It does come with downsides though. In either case, I think some additional comments or a link to an issue describe the next steps would provide some useful context.

TL;DR - I'm okay with this even though it is a bit tough to read, but wanted to propose an alternative as a thought exercise.

I've been working on this concept on and off for a few weeks. I think I thought of that approach but I can't remember why I didn't stick with it. I'll give it a shot with pageSend 👍

stripethree · 2017-04-14T15:36:01Z

Tried with my test bot via enterprise-fbm and things are working as expected. Did not try it with the bot deployed on multiple pages.

stripethree · 2017-04-14T19:33:56Z

Noticed that the previous version did not allow a single page to send with reply without a pages arg sent to Messenger. This is remedied with the latest updates; holding off on further review as Chris said there's a few more things coming. I'm liking what the application level bot code looks like (enterprise-fbm, for example below) esp. with the PR that also removes the need to call start.

const { Messenger, responses } = require('launch-vehicle-fbm');
const messenger = new Messenger();

messenger.start(); // gone soon...

messenger.on('text.greeting', ({reply, firstName}) => {
  reply(new responses.Text(`🤖  Beep bop boop. Well hello there, ${firstName}, I am a bot.`));
});

messenger.on('text.help', ({reply}) => {
  reply(new responses.Text('🤖  I am here to offer my services to assist you.'));
});

messenger.on('text', ({reply, senderId, text, source}) => {
  reply(new responses.Text(`Echo: "${text}"`));
});

messenger.on('message.image', ({reply, url}) => {
  reply(new responses.Image(url));
});

crccheck · 2017-04-14T19:57:48Z

I'm moving this back to WIP to change some design. After IRL discussion, moving towards forcing {pages} is drastic since most bots will only have one page. It's a big step to ask people from doing new Messenger() to new Messenger({pages}) so I'm backtracking that and going to keep the config based stuff now so you only need to do new Messenger({pages}) if you need to support multiple pages.

crccheck

ok took off WIP again. I'm not liking how many LOC this is but it's almost all docs and tests.

crccheck · 2017-04-14T21:09:22Z

test.env

+SLACK_WEBHOOK_URL=https://hooks.slack.com/services/T123456/B33Z/Pajamas
+SLACK_CHANNEL=channel
+LOG_FILE=/var/log/my-bot/chat.log
+ALLOW_CONFIG_MUTATIONS=Y


ALLOW_CONFIG_MUTATIONS is the new config I needed. This will let you do PORT=-1 in #54 without potentially confusing users.

stripethree

Working on a test bot with both the old and new way, without passing in pages to the Messenger constructor.

crccheck · 2017-04-15T00:03:45Z

src/app.js

+      this.pages = {[config.get('facebook.pageId')]: config.get('messenger.pageAccessToken')};
+    } else {
+      this.pages = {};
+      debug("MISSING options.pages; you won't be able to reply or get profile information");


This is my favorite line in this PR. It shows how those two config variables are actually optional depending on what your bot is doing. While the SDK doesn't exactly gracefully degrade, we could make it.

crccheck · 2017-04-15T00:27:09Z

I did a test w/ slack ghost just now too and 👍

crccheck added 8 commits April 13, 2017 15:42

add a Response object to hold logic for how to reply

a196f7f

Convert as many emits to use Response as possible

9961a4d

add coverage to Response object

b496bd6

Document new reply() syntax in readme

281e8b4

add failing test case

7975672

get reply to work even when destructured

07b4103

Make send argument more logical while keeping backwards compatibility

f302e29

cleanup

510345f

crccheck requested a review from stripethree April 14, 2017 05:06

crccheck added the in progress label Apr 14, 2017

stripethree suggested changes Apr 14, 2017

View reviewed changes

crccheck added 2 commits April 14, 2017 12:15

make a new .pageSend that legacy .send wraps

84f6376

align warnings

9aed7a8

crccheck changed the title ~~Add smart reply to remove boilerplate~~ WIP Add smart reply to remove boilerplate Apr 14, 2017

crccheck added 2 commits April 14, 2017 15:32

undeprecate setting a config

332de7e

update readme to match code

f6a2da1

crccheck changed the title ~~WIP Add smart reply to remove boilerplate~~ Add smart reply to remove boilerplate Apr 14, 2017

PR feedback: more context on WIP

7a3d910

crccheck commented Apr 14, 2017

View reviewed changes

stripethree approved these changes Apr 14, 2017

View reviewed changes

crccheck added 2 commits April 14, 2017 18:44

delint a simple FlowFixMe

add7f20

save 1 LOC with fancy esnext syntax I can't remember

5c87489

crccheck commented Apr 15, 2017

View reviewed changes

crccheck merged commit 2717a96 into master Apr 15, 2017

crccheck deleted the smart-response/#50 branch April 15, 2017 00:26

crccheck removed the in progress label Apr 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add smart reply to remove boilerplate #53

Add smart reply to remove boilerplate #53

crccheck commented Apr 14, 2017 •

edited

Loading

stripethree left a comment

stripethree Apr 14, 2017

crccheck Apr 14, 2017

stripethree Apr 14, 2017

stripethree Apr 14, 2017

stripethree Apr 14, 2017

crccheck Apr 14, 2017

stripethree commented Apr 14, 2017

stripethree commented Apr 14, 2017 •

edited

Loading

crccheck commented Apr 14, 2017

crccheck left a comment

crccheck Apr 14, 2017

stripethree left a comment

crccheck Apr 15, 2017

crccheck commented Apr 15, 2017

Add smart reply to remove boilerplate #53

Add smart reply to remove boilerplate #53

Conversation

crccheck commented Apr 14, 2017 • edited Loading

Why are we doing this?

Did you document your work?

How can someone test these changes?

What possible risks or adverse effects are there?

What are the follow-up tasks?

Are there any known issues?

Did the test coverage decrease?

stripethree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stripethree commented Apr 14, 2017

stripethree commented Apr 14, 2017 • edited Loading

crccheck commented Apr 14, 2017

crccheck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stripethree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crccheck commented Apr 15, 2017

crccheck commented Apr 14, 2017 •

edited

Loading

stripethree commented Apr 14, 2017 •

edited

Loading