From dfe23162e7f59efc75e11f9c6ba2fe4a3ce3930f Mon Sep 17 00:00:00 2001 From: Tom Laird-McConnell Date: Mon, 5 Apr 2021 17:47:47 -0700 Subject: [PATCH] ResumeDialog is not called after resumption with adaptivedialog (#5426) * add unit test. * Added logic to ensure "turn.interrupted" is set before an interrupted action is continued. * Update SkillConversationFactory to return a different ID every time CreateSkillConversationIdAsync is called. Added check to InputDialog to ignore incoming activity if this is a reprompt. Fixed broken tests. Co-authored-by: Steven Ickman Co-authored-by: Gabo Gilabert --- .../AdaptiveDialog.cs | 26 ++- .../Input/InputDialog.cs | 10 +- .../SkillDialog.cs | 9 ++ .../Skills/SkillConversationIdFactory.cs | 8 +- .../AdaptiveDialogTests.cs | 6 + ...veDialog_ParentBotInterruption.test.dialog | 148 ++++++++++++++++++ .../Skills/SkillConversationIdFactoryTests.cs | 34 +++- 7 files changed, 222 insertions(+), 19 deletions(-) create mode 100644 tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/Tests/AdaptiveDialogTests/AdaptiveDialog_ParentBotInterruption.test.dialog diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialog.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialog.cs index 8939540a9e..c0028d8a92 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialog.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/AdaptiveDialog.cs @@ -653,22 +653,35 @@ protected async Task ContinueActionsAsync(DialogContext dc, ob // from the stack and we want to detect this so we can stop processing actions. var instanceId = GetUniqueInstanceId(actionContext); + // Initialize local interruption detection + // - Any steps containing a dialog stack after the first step indicates the action was interrupted. We + // want to force a re-prompt and then end the turn when we encounter an interrupted step. + var interrupted = false; + // Execute queued actions var actionDC = CreateChildContext(actionContext); while (actionDC != null) { - // Continue current step // DEBUG: To debug step execution set a breakpoint on line below and add a watch // statement for actionContext.Actions. - var result = await actionDC.ContinueDialogAsync(cancellationToken).ConfigureAwait(false); - - // Start step if not continued - if (result.Status == DialogTurnStatus.Empty && GetUniqueInstanceId(actionContext) == instanceId) + DialogTurnResult result; + if (actionDC.Stack.Count == 0) { - // Call begin dialog on our next step, passing the effective options we computed + // Start step var nextAction = actionContext.Actions.First(); result = await actionDC.BeginDialogAsync(nextAction.DialogId, nextAction.Options, cancellationToken).ConfigureAwait(false); } + else + { + // Set interrupted flag + if (interrupted && !actionDC.State.TryGetValue(TurnPath.Interrupted, out _)) + { + actionDC.State.SetValue(TurnPath.Interrupted, true); + } + + // Continue step execution + result = await actionDC.ContinueDialogAsync(cancellationToken).ConfigureAwait(false); + } // Is the step waiting for input or were we cancelled? if (result.Status == DialogTurnStatus.Waiting || GetUniqueInstanceId(actionContext) != instanceId) @@ -712,6 +725,7 @@ protected async Task ContinueActionsAsync(DialogContext dc, ob // Apply any local changes and fetch next action await actionContext.ApplyChangesAsync(cancellationToken).ConfigureAwait(false); actionDC = CreateChildContext(actionContext); + interrupted = true; } return await OnEndOfActionsAsync(actionContext, cancellationToken).ConfigureAwait(false); diff --git a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Input/InputDialog.cs b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Input/InputDialog.cs index 4dd38700e7..dff6784972 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Input/InputDialog.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/Input/InputDialog.cs @@ -230,16 +230,18 @@ public abstract class InputDialog : Dialog public override async Task ContinueDialogAsync(DialogContext dc, CancellationToken cancellationToken = default(CancellationToken)) { var activity = dc.Context.Activity; - if (activity.Type != ActivityTypes.Message) + + // Interrupted dialogs reprompt so we can ignore the incoming activity. + var interrupted = dc.State.GetValue(TurnPath.Interrupted, () => false); + if (!interrupted && activity.Type != ActivityTypes.Message) { - return Dialog.EndOfTurn; + return EndOfTurn; } - var interrupted = dc.State.GetValue(TurnPath.Interrupted, () => false); var turnCount = dc.State.GetValue(TURN_COUNT_PROPERTY, () => 0); // Perform base recognition - var state = await this.RecognizeInputAsync(dc, interrupted ? 0 : turnCount, cancellationToken).ConfigureAwait(false); + var state = await RecognizeInputAsync(dc, interrupted ? 0 : turnCount, cancellationToken).ConfigureAwait(false); if (state == InputState.Valid) { diff --git a/libraries/Microsoft.Bot.Builder.Dialogs/SkillDialog.cs b/libraries/Microsoft.Bot.Builder.Dialogs/SkillDialog.cs index 8046889dfa..295fa1852e 100644 --- a/libraries/Microsoft.Bot.Builder.Dialogs/SkillDialog.cs +++ b/libraries/Microsoft.Bot.Builder.Dialogs/SkillDialog.cs @@ -91,6 +91,15 @@ public override async Task BeginDialogAsync(DialogContext dc, /// return value. public override async Task ContinueDialogAsync(DialogContext dc, CancellationToken cancellationToken = default) { + // with adaptive dialogs, ResumeDialog is not called directly. Instead the Interrupted flag is set, which + // acts as the signal to the SkillDialog to resume the skill. + if (dc.State.TryGetValue(TurnPath.Interrupted, out bool interrupted) && interrupted) + { + // resume dialog execution + dc.State.SetValue(TurnPath.Interrupted, false); + return await this.ResumeDialogAsync(dc, DialogReason.EndCalled, cancellationToken: cancellationToken).ConfigureAwait(false); + } + if (!OnValidateActivity(dc.Context.Activity)) { return EndOfTurn; diff --git a/libraries/Microsoft.Bot.Builder/Skills/SkillConversationIdFactory.cs b/libraries/Microsoft.Bot.Builder/Skills/SkillConversationIdFactory.cs index 5f97f2f190..0ccb3074e4 100644 --- a/libraries/Microsoft.Bot.Builder/Skills/SkillConversationIdFactory.cs +++ b/libraries/Microsoft.Bot.Builder/Skills/SkillConversationIdFactory.cs @@ -47,13 +47,7 @@ public override async Task CreateSkillConversationIdAsync( // Create the storage key based on the SkillConversationIdFactoryOptions. var conversationReference = options.Activity.GetConversationReference(); - string skillConversationId = string.Join( - "-", - options.FromBotId, - options.BotFrameworkSkill.AppId, - conversationReference.Conversation.Id, - conversationReference.ChannelId, - "skillconvo"); + var skillConversationId = Guid.NewGuid().ToString(); // Create the SkillConversationReference instance. var skillConversationReference = new SkillConversationReference diff --git a/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/AdaptiveDialogTests.cs b/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/AdaptiveDialogTests.cs index db91467dcf..690f328bbc 100644 --- a/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/AdaptiveDialogTests.cs +++ b/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/AdaptiveDialogTests.cs @@ -225,6 +225,12 @@ public async Task AdaptiveDialog_TopLevelFallbackMultipleActivities() await TestUtils.RunTestScript(_resourceExplorerFixture.ResourceExplorer); } + [Fact] + public async Task AdaptiveDialog_ParentBotInterruption() + { + await TestUtils.RunTestScript(_resourceExplorerFixture.ResourceExplorer); + } + [Fact] public async Task TestBindingTwoWayAcrossAdaptiveDialogs() { diff --git a/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/Tests/AdaptiveDialogTests/AdaptiveDialog_ParentBotInterruption.test.dialog b/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/Tests/AdaptiveDialogTests/AdaptiveDialog_ParentBotInterruption.test.dialog new file mode 100644 index 0000000000..7b96bdd9b1 --- /dev/null +++ b/tests/Microsoft.Bot.Builder.Dialogs.Adaptive.Tests/Tests/AdaptiveDialogTests/AdaptiveDialog_ParentBotInterruption.test.dialog @@ -0,0 +1,148 @@ +{ + "$schema": "../../../tests.schema", + "$kind": "Microsoft.Test.Script", + "dialog": { + "$kind": "Microsoft.AdaptiveDialog", + "id": "AdaptiveDialog", + "recognizer": { + "$kind": "Microsoft.RegexRecognizer", + "intents": [ + { + "intent": "dialoga", + "pattern": "(?i)dialoga" + }, + { + "intent": "dialogb", + "pattern": "(?i)dialogb" + } + ] + }, + "triggers": [ + { + "$kind": "Microsoft.OnIntent", + "intent": "dialoga", + "actions": [ + { + "$kind": "Microsoft.BeginDialog", + "dialog": { + "id": "dialogA", + "$kind": "Microsoft.AdaptiveDialog", + "recognizer": { + "$kind": "Microsoft.RegexRecognizer" + }, + "triggers": [ + { + "$kind": "Microsoft.OnBeginDialog", + "actions": [ + { + "$kind": "Microsoft.TextInput", + "allowInterruptions": true, + "property": "dialog.value", + "prompt": "DialogA Prompt" + }, + { + "$kind": "Microsoft.SendActivity", + "activity": "DialogA: ${dialog.value}" + } + ] + } + ] + } + } + ] + }, + { + "$kind": "Microsoft.OnIntent", + "intent": "dialogb", + "actions": [ + { + "$kind": "Microsoft.BeginDialog", + "dialog": { + "$kind": "Microsoft.AdaptiveDialog", + "id": "dialogB", + "recognizer": { + "$kind": "Microsoft.RegexRecognizer" + }, + "triggers": [ + { + "$kind": "Microsoft.OnBeginDialog", + "actions": [ + { + "$kind": "Microsoft.TextInput", + "allowInterruptions": true, + "property": "dialog.value", + "prompt": "DialogB Prompt" + }, + { + "$kind": "Microsoft.SendActivity", + "activity": "DialogB: ${dialog.value}" + } + ] + } + ] + } + } + ] + }, + { + "$kind": "Microsoft.OnIntent", + "intent": "Santa", + "actions": [ + { + "$kind": "Microsoft.SendActivity", + "activity": "I love you santa." + } + ] + }, + { + "$kind": "Microsoft.OnUnknownIntent", + "actions": [ + { + "$kind": "Microsoft.SendActivity", + "activity": "In None..." + } + ] + } + ], + "autoEndDialog": false, + "defaultResultProperty": "dialog.result" + }, + "script": [ + { + "$kind": "Microsoft.Test.UserSays", + "text": "dialoga" + }, + { + "$kind": "Microsoft.Test.AssertReply", + "text": "DialogA Prompt" + }, + { + "$kind": "Microsoft.Test.UserSays", + "text": "dialogb" + }, + { + "$kind": "Microsoft.Test.AssertReply", + "text": "DialogB Prompt" + }, + { + "$kind": "Microsoft.Test.UserSays", + "text": "testb" + }, + { + "$kind": "Microsoft.Test.AssertReply", + "text": "DialogB: testb" + }, + { + "$kind": "Microsoft.Test.AssertReply", + "text": "DialogA Prompt" + }, + { + "$kind": "Microsoft.Test.UserSays", + "text": "testa" + }, + { + "$kind": "Microsoft.Test.AssertReply", + "text": "DialogA: testa" + } + ] +} \ No newline at end of file diff --git a/tests/Microsoft.Bot.Builder.Tests/Skills/SkillConversationIdFactoryTests.cs b/tests/Microsoft.Bot.Builder.Tests/Skills/SkillConversationIdFactoryTests.cs index 42f989afaf..d11ca0a011 100644 --- a/tests/Microsoft.Bot.Builder.Tests/Skills/SkillConversationIdFactoryTests.cs +++ b/tests/Microsoft.Bot.Builder.Tests/Skills/SkillConversationIdFactoryTests.cs @@ -23,10 +23,10 @@ public class SkillConversationIdFactoryTests [Fact] public async Task SkillConversationIdFactoryHappyPath() { - ConversationReference conversationReference = BuildConversationReference(); + var conversationReference = BuildConversationReference(); // Create skill conversation - string skillConversationId = await _skillConversationIdFactory.CreateSkillConversationIdAsync( + var skillConversationId = await _skillConversationIdFactory.CreateSkillConversationIdAsync( options: new SkillConversationIdFactoryOptions { Activity = BuildMessageActivity(conversationReference), @@ -53,6 +53,36 @@ public async Task SkillConversationIdFactoryHappyPath() Assert.Null(deletedConversationReference); } + [Fact] + public async Task IdIsUniqueEachTime() + { + var conversationReference = BuildConversationReference(); + + // Create skill conversation + var firstId = await _skillConversationIdFactory.CreateSkillConversationIdAsync( + options: new SkillConversationIdFactoryOptions + { + Activity = BuildMessageActivity(conversationReference), + BotFrameworkSkill = this.BuildBotFrameworkSkill(), + FromBotId = _botId, + FromBotOAuthScope = _botId, + }, + cancellationToken: CancellationToken.None); + + var secondId = await _skillConversationIdFactory.CreateSkillConversationIdAsync( + options: new SkillConversationIdFactoryOptions + { + Activity = BuildMessageActivity(conversationReference), + BotFrameworkSkill = this.BuildBotFrameworkSkill(), + FromBotId = _botId, + FromBotOAuthScope = _botId, + }, + cancellationToken: CancellationToken.None); + + // Ensure that we get a different conversationId each time we call CreateSkillConversationIdAsync + Assert.NotEqual(firstId, secondId); + } + private static ConversationReference BuildConversationReference() { return new ConversationReference