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

alexrecuenco · 2021-10-11T05:04:08Z

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

there is a failed assertion,
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 · 2021-10-11T18:05:57Z

Hi @alexrecuenco

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

alexrecuenco · 2021-10-12T04:05:33Z

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 · 2021-10-12T13:38:01Z

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 · 2021-10-13T03:41:53Z

@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 · 2021-10-13T06:19:34Z

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 · 2021-11-15T19:47:19Z

@EricDahlvang any update on this?

EricDahlvang · 2021-11-15T21:59:06Z

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)

On issue microsoft#3947, relying on the responded flag todecide whether to show the prompt interferes with logic that could be happening either a. Concurrently b. On some middleware c. As part of how we reached the prompt I believe the *intention* is to prevent sending a prompt twice to the user, using a symbol may be enough for that, without relying on global status

alexrecuenco · 2021-11-16T09:50:51Z

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 · 2021-11-16T09:56:38Z

@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 · 2021-11-16T10:14:04Z

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

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.
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)

On issue microsoft#3947, relying on the responded flag todecide whether to show the prompt interferes with logic that could be happening either a. Concurrently b. On some middleware c. As part of how we reached the prompt I believe the *intention* is to prevent sending a prompt twice to the user, using a symbol may be enough for that, without relying on global status

EricDahlvang · 2021-11-16T18:54:32Z

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

On issue microsoft#3947, relying on the responded flag todecide whether to show the prompt interferes with logic that could be happening either a. Concurrently b. On some middleware c. As part of how we reached the prompt I believe the *intention* is to prevent sending a prompt twice to the user, using a symbol may be enough for that, without relying on global status

On issue microsoft#3947, relying on the responded flag todecide whether to show the prompt interferes with logic that could be happening either a. Concurrently b. On some middleware c. As part of how we reached the prompt I believe the *intention* is to *not* send a prompt if the validator has already sent a message. This is in fact not explained anywhere and it leads to confusion, and restricts the ability to write concise explanations to users along side a retry. - The default behavior is kept, *except* if a message was sent *before* the validator (Those are not considered as part of the prompt logic). This still runs into concurrency issues. For that, - A static method in Prompt has been added, setRepromptStatus. This allows the user to choose explicitly if the prompt should re-prompt or not. - Added extra tests for the new functionality *only* for the text prompt, since all other prompts reuse that same logic.

alexrecuenco · 2021-11-30T06:46:56Z

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.

On issue microsoft#3947, relying on the responded flag todecide whether to show the prompt interferes with logic that could be happening either a. Concurrently b. On some middleware c. As part of how we reached the prompt I believe the *intention* is to *not* send a prompt if the validator has already sent a message. This is in fact not explained anywhere and it leads to confusion, and restricts the ability to write concise explanations to users along side a retry. - The default behavior is kept, *except* if a message was sent *before* the validator (Those are not considered as part of the prompt logic). This still runs into concurrency issues. For that, - A static method in Prompt has been added, setRepromptStatus. This allows the user to choose explicitly if the prompt should re-prompt or not. - Added extra tests for the new functionality *only* for the text prompt, since all other prompts reuse that same logic.

alexrecuenco · 2024-02-14T21:22:05Z

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

alexrecuenco added bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team. labels Oct 11, 2021

stevkan added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Oct 11, 2021

EricDahlvang added the customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. label Oct 11, 2021

msomanathan assigned EricDahlvang Oct 27, 2021

johnataylor added ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. and removed needs-triage The issue has just been created and it has not been reviewed by the team. labels Nov 15, 2021

alexrecuenco linked a pull request Nov 16, 2021 that will close this issue

Suggestion for on-responded on prompts #3981

Open

tracyboehrer unassigned EricDahlvang Jul 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alexrecuenco commented Oct 11, 2021 •

edited

Loading

EricDahlvang commented Oct 11, 2021

alexrecuenco commented Oct 12, 2021 •

edited

Loading

EricDahlvang commented Oct 12, 2021

alexrecuenco commented Oct 13, 2021 •

edited

Loading

alexrecuenco commented Oct 13, 2021 •

edited

Loading

johnataylor commented Nov 15, 2021

EricDahlvang commented Nov 15, 2021

alexrecuenco commented Nov 16, 2021

alexrecuenco commented Nov 16, 2021 •

edited

Loading

alexrecuenco commented Nov 16, 2021 •

edited

Loading

EricDahlvang commented Nov 16, 2021

alexrecuenco commented Nov 30, 2021

alexrecuenco commented Feb 14, 2024

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

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

Comments

alexrecuenco commented Oct 11, 2021 • edited Loading

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

Versions

Describe the bug

Expected behavior

PS: TestAdapter is broken

EricDahlvang commented Oct 11, 2021

alexrecuenco commented Oct 12, 2021 • edited Loading

EricDahlvang commented Oct 12, 2021

alexrecuenco commented Oct 13, 2021 • edited Loading

alexrecuenco commented Oct 13, 2021 • edited Loading

johnataylor commented Nov 15, 2021

EricDahlvang commented Nov 15, 2021

alexrecuenco commented Nov 16, 2021

alexrecuenco commented Nov 16, 2021 • edited Loading

alexrecuenco commented Nov 16, 2021 • edited Loading

EricDahlvang commented Nov 16, 2021

alexrecuenco commented Nov 30, 2021

alexrecuenco commented Feb 14, 2024

alexrecuenco commented Oct 11, 2021 •

edited

Loading

alexrecuenco commented Oct 12, 2021 •

edited

Loading

alexrecuenco commented Oct 13, 2021 •

edited

Loading

alexrecuenco commented Oct 13, 2021 •

edited

Loading

alexrecuenco commented Nov 16, 2021 •

edited

Loading

alexrecuenco commented Nov 16, 2021 •

edited

Loading