Add support for multiple attachments #8
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,18 @@ | ||
| /* | ||
| * An enumeration of the possible attachment types. | ||
| * | ||
| * `Other` is a catch-all for any attachment type that is not (yet) explicitly | ||
| * supported. The enum values are used to build the regexes for the attachment | ||
| * wildcards, e.g. <IMAGE> is derived from 'IMAGE'. | ||
| */ | ||
| export enum Attachment { | ||
| Image = 'IMAGE', | ||
| Cards = 'CARDS', | ||
| Other = 'OTHER', | ||
| } | ||
|
|
||
| export interface Message { | ||
| attachments: Attachment[]; | ||
| text: string; | ||
| user: string; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,50 +1,80 @@ | ||
| import { Message, Attachment } from './message'; | ||
| import { Translator } from '../translator'; | ||
| import * as program from 'commander'; | ||
|
|
||
| const wildcardRegex: RegExp = /<\*>/g; | ||
| const wordRegex: RegExp = /<WORD>/g; | ||
| const numberRegex: RegExp = /<NUMBER>/g; | ||
| const regexRegex: RegExp = /<\((.+?)\)>/g; | ||
|
|
||
| // Matches an attachment wildcard (e.g. <IMAGE>) at the _end_ of responseData. | ||
| // | ||
| // The matched part is just the word in the brackets ('IMAGE'), which can be | ||
| // directly compared against the Attachment enum (from which it was generated). | ||
| const attachmentAlternatives = Object.values(Attachment).join('|'); | ||
| const attachmentsRegex = new RegExp(`\\s*<(${attachmentAlternatives})>$`); | ||
|
|
||
| export class Response { | ||
| attachments: Attachment[]; | ||
| private textMatchChecker: RegExp; | ||
| private bodyText: string; | ||
| original: string; | ||
|
|
||
| constructor(responseData: string) { | ||
| this.attachments = []; | ||
| this.original = responseData.trim(); | ||
| this.bodyText = this.original; | ||
|
|
||
| // Construct a list of attachments from the attachment wildcards at the | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 You can imagine a client which does support this (e.g. one that uses There was a problem hiding this comment. 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 and it validates against: 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 |
||
| // end of the response data. The end result is that `this.bodyText` | ||
| // contains only the body text and potentially some inline wildcards; | ||
| // and `this.attachments` contains a list of attachment types, in the | ||
| // same order as they originally were in the supplied data. | ||
| let match; | ||
| while ((match = this.bodyText.match(attachmentsRegex)) != null) { | ||
| this.attachments.push(match[1]); | ||
| this.bodyText = this.bodyText.slice(0, match.index); | ||
| } | ||
| this.attachments.reverse(); | ||
|
|
||
| // Construct a regex to match the remaining body text in terms of its | ||
| // inline wildcards. | ||
| this.textMatchChecker = new RegExp(Response.transformTags(this.bodyText)); | ||
| } | ||
|
|
||
| matches(message: Message): boolean { | ||
|
|
||
| // If we are expecting a message body, check that we have one... | ||
| if (this.bodyText && !message.text) { | ||
| return false; | ||
| } | ||
|
|
||
| // ...and check that the content is the same. | ||
| // | ||
| // Note that we ignore the message body altogether if the spec doesn't | ||
| // include one. This is for backwards compatibility with when <IMAGE> | ||
| // and <CARDS> could only be used in place of the entire message. | ||
| if (this.bodyText && message.text.trim().match(this.textMatchChecker) === null) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check that we have the same number, kind, and order of attachments. | ||
| // We can use JSON.stringify here since we know attachments is just a | ||
| // simple array of strings. | ||
|
|
||
| // Check that we have the right number of attachments. | ||
| if (JSON.stringify(this.attachments) !== JSON.stringify(message.attachments)) { | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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? There was a problem hiding this comment. Yeah that sounds like a good idea |
||
| return false; | ||
| } | ||
|
|
||
| // Everything appears to be in order. | ||
| return true; | ||
| } | ||
|
|
||
| static transformTags(text: string): string { | ||
| regexRegex.lastIndex = 0; | ||
|
|
||
| const taggedText = text | ||
| .replace(wildcardRegex, '<([\\s\\S]*?)>') | ||
| .replace(wordRegex, '<([^ ]+?)>') | ||
| .replace(numberRegex, '<([0-9\.,-]+)>'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the docs here. I would just add a comment here that includes buttons, as this might not be obvious to them.