Skip to content

Commit

Permalink
Fixes #3947
Browse files Browse the repository at this point in the history
On issue #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.
  • Loading branch information
alexrecuenco committed Nov 30, 2021
1 parent 2bd9b49 commit 9fe75c5
Show file tree
Hide file tree
Showing 2 changed files with 221 additions and 16 deletions.
84 changes: 69 additions & 15 deletions libraries/botbuilder-dialogs/src/prompts/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,30 @@ export abstract class Prompt<T> extends Dialog {
*/
protected constructor(dialogId: string, private validator?: PromptValidator<T>) {
super(dialogId);

if (!validator) validator = async (prompt) => prompt.recognized.succeeded;

this.validator = async (prompt) => {
const context = prompt.context;

const hasRespondedBeforeValidator = context.responded;
// Making the validator *only* consider as "responded" if responded has been set during a turn
// @ts-expect-error
context._respondedRef.responded = false;
try {
const result = await validator(prompt);
const shouldReprompt = context.turnState.get(Prompt.repromptOnRetry);

// Set reprompt retry status based on whether the bot responded or not while on a validator
Prompt.setRepromptRetry(context, this.shouldRepromptOnRetry(context));
return result;
} finally {
if (hasRespondedBeforeValidator) prompt.context.responded = true;
}
}



}

/**
Expand Down Expand Up @@ -206,6 +230,38 @@ export abstract class Prompt<T> extends Dialog {
return Dialog.EndOfTurn;
}

/**
* By default the bot re-prompts on retries if there has not been any message sent to this user in this turn
*
* However, that might be undesirable. You can set here whether you want it to reprompt or not.
*
* To reset to the default status, you can set it back to undefined or null.
* @param context
* @param shouldReprompt Defaults to {true}. Set to undefined/null to allow the default behaviour.
*/
static setRepromptRetry(context:TurnContext, shouldReprompt: boolean | undefined | null = true) {
context.turnState.set(this.repromptOnRetry, shouldReprompt);
}

/**
* Determined whether to re-prompt on retry, by default it checks whether the context.responded is false or not
*
* You can change that behaviour by either (a) extending this function on your base class or (b) using Prompt.setRepromptRetry
* @param context
* @returns
*/
protected shouldRepromptOnRetry(context: TurnContext): boolean {
const shouldReprompt: null | undefined | boolean = context.turnState.get(Prompt.repromptOnRetry);
if (shouldReprompt == null) return !context.responded;
return shouldReprompt;
}


/**
* Optional symbol to be used to *force* the context whether to reprompt the retry or not based on this flag
*/
private static repromptOnRetry = Symbol('repromptOnRetry');

/**
* Called when a prompt dialog is the active dialog and the user replied with a new activity.
* @param dc The [DialogContext](xref:botbuilder-dialogs.DialogContext) for the current turn of conversation.
Expand Down Expand Up @@ -238,28 +294,26 @@ export abstract class Prompt<T> extends Dialog {

// Validate the return value
let isValid = false;
if (this.validator) {
if (state.state['attemptCount'] === undefined) {
state.state['attemptCount'] = 0;
}
isValid = await this.validator({
context: dc.context,
recognized: recognized,
state: state.state,
options: state.options,
attemptCount: ++state.state['attemptCount'],
});
} else if (recognized.succeeded) {
isValid = true;

if (state.state['attemptCount'] === undefined) {
state.state['attemptCount'] = 0;
}
isValid = await this.validator({
context: dc.context,
recognized: recognized,
state: state.state,
options: state.options,
attemptCount: ++state.state['attemptCount'],
});


// Return recognized value or re-prompt
if (isValid) {
return await dc.endDialog(recognized.value);
} else {
if (!dc.context.responded) {
if (this.shouldRepromptOnRetry(dc.context)) {
await this.onPrompt(dc.context, state.state, state.options, true);
}
}

return Dialog.EndOfTurn;
}
Expand Down
153 changes: 152 additions & 1 deletion libraries/botbuilder-dialogs/tests/textPrompt.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { ActivityTypes, ConversationState, MemoryStorage, TestAdapter } = require('botbuilder-core');
const { DialogSet, TextPrompt, DialogTurnStatus } = require('../');
const { DialogSet, TextPrompt, DialogTurnStatus, Prompt } = require('../');
const assert = require('assert');
const lineReader = require('line-reader');
const path = require('path');
Expand Down Expand Up @@ -191,5 +191,156 @@ describe('TextPrompt', function() {
.send('test')
.assertReply('test')
.startTest();
});

it('should send retryPrompt when a message is sent before validation.', async function() {

const alwaysSentMessage = 'Working on your answer, hold on a second.'
const adapter = new TestAdapter(async (turnContext) => {
await turnContext.sendActivity(alwaysSentMessage);
const dc = await dialogs.createContext(turnContext);

const results = await dc.continueDialog();
if (results.status === DialogTurnStatus.empty) {
await dc.prompt('prompt', { prompt: 'Please say something.', retryPrompt: 'Text is required.' });
} else if (results.status === DialogTurnStatus.complete) {
const reply = results.result;
await turnContext.sendActivity(reply);
}
await convoState.saveChanges(turnContext);
});

const convoState = new ConversationState(new MemoryStorage());

const dialogState = convoState.createProperty('dialogState');
const dialogs = new DialogSet(dialogState);
dialogs.add(new TextPrompt('prompt'));

await adapter.send('Hello')
.assertReply(alwaysSentMessage)
.assertReply('Please say something.')
.send(invalidMessage)
.assertReply(alwaysSentMessage)
.assertReply('Text is required.')
.send('test')
.assertReply(alwaysSentMessage)
.assertReply('test')
.startTest();

});

it('should not send retryPrompt if the validator replies, even when a message is sent before.', async function() {

const alwaysSentMessage = 'Working on your answer, hold on a second.'
const adapter = new TestAdapter(async (turnContext) => {
await turnContext.sendActivity(alwaysSentMessage);
const dc = await dialogs.createContext(turnContext);

const results = await dc.continueDialog();
if (results.status === DialogTurnStatus.empty) {
await dc.prompt('prompt', { prompt: 'Please say something.', retryPrompt: 'Text is required.' });
} else if (results.status === DialogTurnStatus.complete) {
const reply = results.result;
await turnContext.sendActivity(reply);
}
await convoState.saveChanges(turnContext);
});

const convoState = new ConversationState(new MemoryStorage());

const dialogState = convoState.createProperty('dialogState');
const dialogs = new DialogSet(dialogState);
dialogs.add(new TextPrompt('prompt', async (prompt) => {
if (!prompt.recognized.succeeded) {
await prompt.context.sendActivity('dont send an empty text.')
}
return prompt.recognized.succeeded;
}));

await adapter.send('Hello')
.assertReply(alwaysSentMessage)
.assertReply('Please say something.')
.send(invalidMessage)
.assertReply(alwaysSentMessage)
.assertReply('dont send an empty text.')
.send('test')
.assertReply(alwaysSentMessage)
.assertReply('test')
.startTest();

});

it('should not send retryPrompt if repromptRetry status is false.', async function() {

const alwaysSentMessage = 'Working on your answer, hold on a second.'
const adapter = new TestAdapter(async (turnContext) => {
await turnContext.sendActivity(alwaysSentMessage);
Prompt.setRepromptRetry(turnContext, false);
const dc = await dialogs.createContext(turnContext);

const results = await dc.continueDialog();
if (results.status === DialogTurnStatus.empty) {
await dc.prompt('prompt', { prompt: 'Please say something.', retryPrompt: 'Text is required.' });
} else if (results.status === DialogTurnStatus.complete) {
const reply = results.result;
await turnContext.sendActivity(reply);
}
await convoState.saveChanges(turnContext);
});

const convoState = new ConversationState(new MemoryStorage());

const dialogState = convoState.createProperty('dialogState');
const dialogs = new DialogSet(dialogState);
dialogs.add(new TextPrompt('prompt'));

await adapter.send('Hello')
.assertReply(alwaysSentMessage)
.assertReply('Please say something.')
.send(invalidMessage)
.assertReply(alwaysSentMessage)
.send('test')
.assertReply(alwaysSentMessage)
.assertReply('test')
.startTest();

});

it('should send retryPrompt even if validator replies when repromptRetry status is true', async function() {
const adapter = new TestAdapter(async (turnContext) => {
const dc = await dialogs.createContext(turnContext);

const results = await dc.continueDialog();
if (results.status === DialogTurnStatus.empty) {
await dc.prompt('prompt', { prompt: 'Please say something.', retryPrompt: 'Text is required.' });
} else if (results.status === DialogTurnStatus.complete) {
const reply = results.result;
await turnContext.sendActivity(reply);
}
await convoState.saveChanges(turnContext);
});

const convoState = new ConversationState(new MemoryStorage());

const dialogState = convoState.createProperty('dialogState');
const dialogs = new DialogSet(dialogState);
dialogs.add(new TextPrompt('prompt', async (prompt) => {
assert(prompt);
Prompt.setRepromptRetry(prompt.context, true)
const valid = prompt.recognized.value.length >= 3;
if (!valid) {
await prompt.context.sendActivity('too short');
}
return valid;
}));

await adapter.send('Hello')
.assertReply('Please say something.')
.send('i')
.assertReply('too short')
.assertReply('Text is required.')
.send('test')
.assertReply('test')
.startTest();
});
});

0 comments on commit 9fe75c5

Please sign in to comment.