Add support for multiple attachments #8

ecridge · 2018-06-26T17:00:26Z

This changeset allows responses to be checked for multiple attachments, sensitive to their number, order, and type. It also allows for the text body sent along with the attachments to be (optionally) checked just like it is for a plain response, and makes it trivial to add support for more attachment types in the future.

The YAML syntax is unchanged (save for the addition of an <OTHER> attachment type as a catch-all), and these changes should be fully backwards compatible. To achieve this, I made two compromises, which I think are fairly natural for users: (1) attachments must come at the end of the message spec; and (2) the text body of a response is not checked if the message spec consists only of attachments.

ecridge · 2018-06-26T17:03:34Z

@lingz one area I could use your input on is how to make sure that the output is still friendly when a response fails to match

lingz · 2018-06-26T17:05:51Z

Hey Joe,

Thanks a lot for this, I will do a full review tomorrow but cursory inspection looks good. A quick idea I have would be,

Attachment mismatch:
Expected: 2 Images, 1 Button
Got: No Attachments

Something like this might work?

lingz

Joe this looks really nice to me, apart from small comments, the only suggestion would be to make the breaking change now to prevent technical debt accumulation, since the number of users is relatively low. I'll increment the major version number to let people know, but if you add the note to README.md, that would be great. If you fix these, I can merge this and release on npm.

lingz · 2018-06-27T10:09:35Z

README.md

+
+Tag | Meaning
+--- | ---
+`<CARDS>` | Any card attachment


Thanks for the docs here. I would just add a comment here that includes buttons, as this might not be obvious to them.

lingz · 2018-06-27T10:09:41Z

README.md

+--- | ---
+`<CARDS>` | Any card attachment
+`<IMAGE>` | Any image attachment
+`<OTHER>` | Any other type of attachment


Add some examples below here.

lingz · 2018-06-27T10:14:06Z

src/runner.ts

@@ -156,7 +156,7 @@ export class Runner {
      if (response !== null) {
        if (program.verbose) {
          const timing = (new Date().getTime() - this.timing) || 0;
-          if (response.messageTypes.includes(MessageType.Text)) {
+          if (response.attachments.length === 0) {


This seems wrong? Should be if (response.text). Also, for printing, we should probably print the attachments if they exist like:

Bot: Here you go: <IMAGE> <BUTTON> <BUTTON>

I know this is not how it is before but this would bring it into consistency with the new formatting style.

Ah good catch, thanks.

lingz · 2018-06-27T10:42:01Z

src/spec/response.ts

-        this.textMatchChecker = new RegExp(Response.transformTags(this.original));
+    this.bodyText = this.original;
+
+    // Construct a list of attachments from the attachment wildcards at the


I like this solution, it's nice. Though I wonder why you constrain it to happening at the end of the line? Seems a bit artificial to me?

In principle they could go anywhere, but I’m not sure what the use case would be. For example:

Dialogue: - Human: How is my campaign doing? - Bot: According to <IMAGE> and <IMAGE>, it’s going great!

This doesn’t make any sense in bot framework per se, because it doesn’t define a mechanism for embedding attachments inline with the text. Presumably you would expect attachments to be [Attachment.Image, Attachment.Image], but is text supposed to be 'According to', or 'According to and , it’s going great!, or something else?

You can imagine a client which does support this (e.g. one that uses $0, $1, … to reference attachments in the attachments array), in which case you could check that text === 'According to $0 and $1, it’s going great!', and that attachments[0] === Attachment.Image and attachments[1] === Attachment.Image; but in the absence of an established convention I’m not sure how useful this would be.

Actually you are right that the botbuilder platform specifically limits this ordering. However, for other platforms like Slack, it is allowed to have intermingled text + attachments, so for that reason it might be good to allow the tags anywhere, and just concatenate the text.

It could lead to this edge case where someone sends

Hi <IMAGE> there <IMAGE>

and it validates against:

HI there <IMAGE> <IMAGE>

However at the same time, we could introduce this fix as a separate PR as you don't have any example connector right now, so it would be hard to test. For now we can concatenate and it will work for most practical use cases I think.

So to summarize I think for now a good compromise is to just drop the ordering condition (remove the $ on the regex parser).

lingz · 2018-06-27T10:44:20Z

src/spec/response.ts

+    // simple array of strings.
+
+    // Check that we have the right number of attachments.
+    if (JSON.stringify(this.attachments) !== JSON.stringify(message.attachments)) {


Stringify has this tricky behavior where ordering of keys is only guaranteed in Node environment IF the object is the same (same pointer). Even if two objects look the same, they will not necessarily get stringified in the same way.

Use https://github.com/substack/json-stable-stringify to do a sort before the stringify.

Interesting. I assumed that stringifying an Array<String> would be safe. If you like I can just revert ac0dd3b to avoid bringing in another library?

Yeah that sounds like a good idea

lingz · 2018-06-27T10:48:52Z

src/test/spec/response.ts

    })).to.be.true;
  });
+  it('should ignore text with <IMAGE> if no text is expected', () => {


Actually I think this should be a breaking change, and we can increment the major version of autobot to suggest this, and put a note in README.md

lingz · 2018-06-27T10:49:21Z

src/test/spec/response.ts

+      attachments: [Attachment.Image],
+    })).to.be.true;
+  });
+  it('should ignore text with <CARDS> if no text is expected', () => {


Same here, so I would reverse the test condition, and make it .to.be.false

…ingification This reverts commit ac0dd3b.

ecridge added 7 commits June 26, 2018 16:24

Rename {MessageType => Attachment}

04999f6

Remove Text as an attachment type

b86b418

Update Response to handle multiple attachments

a748ad2

Simplify response attachments comparison

ac0dd3b

Add extra tests for backwards compatibility

65f1108

Add tests for the new functionality

dd834c8

Update documentation with new attachment features

8c6ea46

lingz suggested changes Jun 27, 2018

View reviewed changes

ecridge added 4 commits June 27, 2018 16:31

Allow attachments to come anywhere in message text

ed52f83

Revert "Simplify response attachments comparison" to avoid quirky str…

310ac14

…ingification This reverts commit ac0dd3b.

Improve verbose output of attachments

09c3978

Improve output when an attachment mismatch occurs

9c34c6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple attachments #8

Add support for multiple attachments #8

ecridge commented Jun 26, 2018

ecridge commented Jun 26, 2018

lingz commented Jun 26, 2018

lingz left a comment

lingz Jun 27, 2018

lingz Jun 27, 2018

lingz Jun 27, 2018

ecridge Jun 27, 2018

lingz Jun 27, 2018

ecridge Jun 27, 2018

lingz Jun 27, 2018 •

edited

Loading

lingz Jun 27, 2018

ecridge Jun 27, 2018 •

edited

Loading

lingz Jun 27, 2018

lingz Jun 27, 2018

lingz Jun 27, 2018

Add support for multiple attachments #8

Are you sure you want to change the base?

Add support for multiple attachments #8

Conversation

ecridge commented Jun 26, 2018

ecridge commented Jun 26, 2018

lingz commented Jun 26, 2018

lingz 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

lingz Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecridge Jun 27, 2018 • edited Loading

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

lingz Jun 27, 2018 •

edited

Loading

ecridge Jun 27, 2018 •

edited

Loading