[botbuilder-dialogs] Dialog does not prompt on continue dialog if any activity is sent on the turn

[alexrecuenco]

Github issues should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.

Versions

What package version of the SDK are you using: 4.14.1
What nodejs version are you using: node14
What browser version are you using: NA
What os are you using: MacOS

Describe the bug

On continueDialog a prompt checks first if the context.responded is false before showing the prompt. This behavior is incorrect in most cases. I show here an example with a typing activity (Which is very common)

/* eslint-disable no-console */
import { ActivityTypes, ConversationState, MemoryStorage, TestAdapter } from 'botbuilder';
import { ChoicePrompt, DialogSet, DialogTurnStatus, ListStyle, WaterfallDialog } from 'botbuilder-dialogs';

const STATEMENT = 'Hello, I will ask you a question.';
const QUESTION = 'Do you like me?';
const prompt = new ChoicePrompt('choices', async (p) => {
  if (!p.recognized && p.attemptCount < 2) {
    return false;
  }

  return true;
});
const dialog = new WaterfallDialog('testDialog', [
  async (step) => {
    await step.context.sendActivity(STATEMENT);
    return step.prompt('choices', { prompt: QUESTION, choices: ['Yes', 'Of course'], style: ListStyle.list });
  },
]);

const memory = new MemoryStorage();

const dialogMemory = new ConversationState(memory);

const dialogSet = new DialogSet(dialogMemory.createProperty('dialogs'));

dialogSet.add(prompt);
dialogSet.add(dialog);

const testAdapter = new TestAdapter(async (ctx) => {
  const dc = await dialogSet.createContext(ctx);
  // Comment out the typing activity to see how it changes the behavior of the script.
  await ctx.sendActivity({ type: ActivityTypes.Typing });
  const result = await dc.continueDialog();
  if (result.status === DialogTurnStatus.empty) {
    await dc.beginDialog('testDialog');
  }
  await dialogMemory.saveChanges(ctx);
});

(async () => {
  await testAdapter.send('hi');
  testAdapter.activeQueue.splice(0, Infinity);
  await testAdapter.send('sorry, what?');
  const texts = testAdapter.activeQueue.map((a) => a.text);
  if (!texts.some((t) => t?.includes(QUESTION))) {
    throw new Error('Question not included in replies');
  }
})()
  .then(() => console.log('Well done'))
  .catch((err) => console.error(err));

// (async () => {
//   await testAdapter
//     .send('hi')
//     .assertReply({ type: ActivityTypes.Typing })
//     .assertReply(STATEMENT)
//     .assertReply((r) => {
//       r.text?.includes(QUESTION);
//     });
//   await testAdapter
//     .send('sorry, what')
//     .assertReply({ type: ActivityTypes.Typing })
//     .assertReply((r) => {
//       r.text?.includes(QUESTION);
//     });
// })()
//   .then(() => console.log('Well done'))
//   .catch((err) => console.error(err));

Expected behavior

On continueDialog, the question should be shown, regardless of whether the application sends message before the prompt continueDialog is called

If backwards compatibility for this is necessary, I would recommend to add an option parameter that specifies whether to reply on continue or not. And if this parameter is empty, continue doing the current behavior

PS: TestAdapter is broken

Also, note that if you run the test with the assertReply, the test adapter throws a unhandledpromiserejection. I believe currently the test adapter throws unhandled exceptions whenever

1. there is a failed assertion,
2. If the test adapter handler itself throws an error.

That makes it really hard to use the TestAdapter on tests to test error throwing

[EricDahlvang]

Hi @alexrecuenco

Thank you for opening this issue. Does this also occur during runtime of the bot, or only with the TestAdapter?

[alexrecuenco]

Good morning/afternoon,

Yes, it occurs with any adapter. I just provided a simplified example. I was hoping it could be used to make it a test case when reviewing the issue

It is related to this line, I am not sure what was the intention for it, maybe to prevent the prompt prompting twice on the same turn when the dialog is resumed twice?

botbuilder-js/libraries/botbuilder-dialogs/src/prompts/prompt.ts

Line 260 in cb0718c

if (!dc.context.responded) {

[EricDahlvang]

It seems the sentNonTraceActivity flag should also be excluding activity types of Typing:

            if (result.type !== ActivityTypes.Trace) {
                sentNonTraceActivity = true;
            }

https://github.com/microsoft/botbuilder-js/blob/main/libraries/botbuilder-core/src/turnContext.ts#L513

[alexrecuenco]

@EricDahlvang , that is one issue, the "delay" type might also need to be included in that list of excluded activities, but I would say that is independent to this issue we are discussing.

• As an example, you might send a message external to the prompt that is meant to be read by the user based on some previous logic. (i.e., some warning about the conversation being "recorded", or some async "it is ready" message). And you will get different behavior on the prompt showing up depending on the ordering of these actions, which is really hard to debug. The typing activity is just an example, but you can imagine a circumstance where you send any other message.

Overall, I am not sure when you would want to prompt the user, but not show the prompt to the user.

[alexrecuenco]

In my opinion, If the goal is to prevent the bot saying twice the same prompt in a turn, a better way to achieve that would be to use a private symbol within the turnState, exclusive to the prompt itself.

I can try to propose a PR. But I am not sure that is the current intent

[johnataylor]

@EricDahlvang any update on this?

[EricDahlvang]

I've looked into this a bit more, and our historical guidance is to use middleware and response event handlers: https://docs.microsoft.com/en-us/azure/bot-service/bot-builder-concept-middleware?view=azure-bot-service-4.0#response-event-handlers Perhaps middleware could reset the responded flag to false under your specific business conditions?

This old comment thread provides a bit more context:
microsoft/botbuilder-dotnet#822 (comment)

[alexrecuenco]

Since this is taking a while, I tried to write down my suggestion for one possible solution for this issue on #3981

I hope this suggestion is not considered intrusive.

[alexrecuenco]

@EricDahlvang any update on this?

Our fix currently is to dig into the private properties on the context on the validator function, and to set it to false over there, then execute the validator.

Something like,

wrapValidator = (validator) => (prompt) => { prompt.context._respondedRef.responded=false; return validator(prompt) }

Obviously, we would like to remove that hack

[alexrecuenco]

I am not sure yet why it is even checking the responded flag because:

1. It is a concurrency issue, you can't guarantee that it won't get turned, and it will create huge headaches with timing of async tasks.
2. Even if no concurrency issue was present, it is still a private property. I hope you would understand how hacking it is not preferable

Notice how the suggestion (#3981) breaks the retry tests.

For backwards compatibility, we can wrap the validator to set the "shouldRetry" flag based on the "responded" flag change within the validator, unless you get a value of false instead of undefined. (And wrap all of this into some simple static functions on the Prompt class)

[EricDahlvang]

@Stevenic do you have any feedback on this discussion regarding the context.responded flag?

[alexrecuenco]

Good afternoon, I modified #3981 to address the backwards compatibility issues we were discussing.

Pushing a change like that (or similar to that if that is considered too intrusive) would minimize the harm of using the "responded" flag.

In essence, it understands the intention to check only if it responded within the validator.

[alexrecuenco]

It's been a while, was this ever fixed?