superscriptjs / superscript

A dialogue engine for creating chat bots
http://superscriptjs.com
MIT License
1.65k stars 209 forks source link

Conversation matching fails on multi line replies #357

Closed rensdewolf closed 7 years ago

rensdewolf commented 7 years ago

When using the line break in a reply to create multi line answers, the conversation matching fails if the word used to match against is next to the line break (\n). See the following example below:

+ sample
- first reply\n
^ please

    + sample
    % * please *
    - second reply

Expected Behavior

Based on the example provided above, a user types the word "sample" twice and the following is expected:

user: sample bot: first reply please

user: sample bot: second reply

Current Behavior

However, the current behavior results in the following outcome:

user: sample bot: first reply please

user: sample bot:

There is no reply from the bot because there was no conversation match and all answers have been exhausted.

Possible Solution

A possible solution is to replace the line breaks (\n) in the last reply with a white space before doing the conversation matching.

Steps to Reproduce (for bugs)

See the example above.

Context

Creating conversations with multi line answers is not possible now without using a custom line break character sequence in the replies that needs to be replaced before posting the reply to the user.

rensdewolf commented 7 years ago

It seems the reply (and hence lastReply) contains an escaped backslash for the line break, i.e. \\n internally? Does this cause the matching mechanism to fail? Or is something else going on?

rensdewolf commented 7 years ago

I think I found a quick fix for the issue. The ss-parser seems to be the culprit as it does not do proper conversation matching in the post process function. As suggested, replacing the line break \n with a simple space will solve the issue. So the line in function processConversations in parseContents.js

if (pattern.test(reply.string)) {

could be

if (pattern.test(reply.string.replace(/\\n/g, ' '))) {

Note that the original reply remains as is, the replace is only required for the pattern matching in this function.

silentrob commented 7 years ago

Nice work @rensdewolf. Since you have taken it this far, we will happily take a PR too. If you feel up for it. All you would need to do is create a failing test, and confirm your change works, and doesn't regress anything else.

rensdewolf commented 7 years ago

@silentrob I think I have done what you've asked for (be it in separate PRs). The test fails as expected. With the PR for the ss-parser the test should pass.

rensdewolf commented 7 years ago

Thank you @benhjames and @silentrob.