Fixes BadRequest message in Teams (#3966)

gabog · gabog · commit 68e1408ec51a · 2020-05-21T17:27:13.000-04:00
* Added check to ensure we don't send EoC when a skill is done and we are at the root bot.
Updated tests to handle this case and removed duplicated test after refactoring.
Fixed some typos and trace message names.
diff --git a/libraries/Microsoft.Bot.Builder.Dialogs/DialogManager.cs b/libraries/Microsoft.Bot.Builder.Dialogs/DialogManager.cs
@@ -56,7 +56,7 @@ public DialogManager(Dialog rootDialog = null, string dialogStateProperty = null
         public UserState UserState { get; set; }
 
         /// <summary>
-        /// Gets InitialTurnState collection to copy into the turnstate on every turn.
+        /// Gets InitialTurnState collection to copy into the TurnState on every turn.
         /// </summary>
         /// <value>
         /// TurnState.
@@ -136,7 +136,7 @@ public async Task<DialogManagerResult> OnTurnAsync(ITurnContext context, Cancell
                 context.TurnState.Set(pair.Key, pair.Value);
             }
 
-            // register DialogManager with turnstate.
+            // register DialogManager with TurnState.
             context.TurnState.Set(this);
 
             if (ConversationState == null)
@@ -185,13 +185,13 @@ public async Task<DialogManagerResult> OnTurnAsync(ITurnContext context, Cancell
             // Create DialogContext
             var dc = new DialogContext(Dialogs, context, dialogState);
 
-            // promote initial turnstate into dc.services for contextual services
+            // promote initial TurnState into dc.services for contextual services
             foreach (var service in dc.Services)
             {
                 dc.Services[service.Key] = service.Value;
             }
 
-            // map turnstate into root dialog context.services
+            // map TurnState into root dialog context.services
             foreach (var service in context.TurnState)
             {
                 dc.Services[service.Key] = service.Value;
@@ -295,7 +295,7 @@ private async Task<DialogTurnResult> HandleSkillOnTurnAsync(DialogContext dc, Ca
                 var activeDialogContext = GetActiveDialogContext(dc);
 
                 var remoteCancelText = "Skill was canceled through an EndOfConversation activity from the parent.";
-                await turnContext.TraceActivityAsync($"{typeof(Dialog).Name}.RunAsync()", label: $"{remoteCancelText}", cancellationToken: cancellationToken).ConfigureAwait(false);
+                await turnContext.TraceActivityAsync($"{GetType().Name}.OnTurnAsync()", label: $"{remoteCancelText}", cancellationToken: cancellationToken).ConfigureAwait(false);
 
                 // Send cancellation message to the top dialog in the stack to ensure all the parents are canceled in the right order. 
                 return await activeDialogContext.CancelAllDialogsAsync(true, cancellationToken: cancellationToken).ConfigureAwait(false);
@@ -321,20 +321,23 @@ private async Task<DialogTurnResult> HandleSkillOnTurnAsync(DialogContext dc, Ca
             {
                 // restart root dialog
                 var startMessageText = $"Starting {_rootDialogId}.";
-                await turnContext.TraceActivityAsync($"{typeof(Dialog).Name}.RunAsync()", label: $"{startMessageText}", cancellationToken: cancellationToken).ConfigureAwait(false);
+                await turnContext.TraceActivityAsync($"{GetType().Name}.OnTurnAsync()", label: $"{startMessageText}", cancellationToken: cancellationToken).ConfigureAwait(false);
                 turnResult = await dc.BeginDialogAsync(_rootDialogId, cancellationToken: cancellationToken).ConfigureAwait(false);
             }
 
             await SendStateSnapshotTraceAsync(dc, "Skill State", cancellationToken).ConfigureAwait(false);
 
-            // Send end of conversation if it is completed or cancelled.
-            if (turnResult.Status == DialogTurnStatus.Complete || turnResult.Status == DialogTurnStatus.Cancelled)
+            if (ShouldSendEndOfConversationToParent(turnContext, turnResult))
             {
                 var endMessageText = $"Dialog {_rootDialogId} has **completed**. Sending EndOfConversation.";
-                await turnContext.TraceActivityAsync($"{typeof(Dialog).Name}.RunAsync()", label: $"{endMessageText}", value: turnResult.Result, cancellationToken: cancellationToken).ConfigureAwait(false);
+                await turnContext.TraceActivityAsync($"{GetType().Name}.OnTurnAsync()", label: $"{endMessageText}", value: turnResult.Result, cancellationToken: cancellationToken).ConfigureAwait(false);
 
                 // Send End of conversation at the end.
-                var activity = new Activity(ActivityTypes.EndOfConversation) { Value = turnResult.Result, Locale = turnContext.Activity.Locale };
+                var activity = new Activity(ActivityTypes.EndOfConversation)
+                {
+                    Value = turnResult.Result,
+                    Locale = turnContext.Activity.Locale
+                };
                 await turnContext.SendActivityAsync(activity, cancellationToken).ConfigureAwait(false);
             }
 
@@ -368,5 +371,33 @@ private async Task<DialogTurnResult> HandleBotOnTurnAsync(DialogContext dc, Canc
 
             return turnResult;
         }
+
+        /// <summary>
+        /// Helper to determine if we should send an EndOfConversation to the parent or not.
+        /// </summary>
+        private bool ShouldSendEndOfConversationToParent(ITurnContext context, DialogTurnResult turnResult)
+        {
+            if (!(turnResult.Status == DialogTurnStatus.Complete || turnResult.Status == DialogTurnStatus.Cancelled))
+            {
+                // The dialog is still going, don't return EoC.
+                return false;
+            }
+
+            if (context.TurnState.Get<IIdentity>(BotAdapter.BotIdentityKey) is ClaimsIdentity claimIdentity && SkillValidation.IsSkillClaim(claimIdentity.Claims))
+            {
+                // EoC Activities returned by skills are bounced back to the bot by SkillHandler.
+                // In those cases we will have a SkillConversationReference instance in state.
+                var skillConversationReference = context.TurnState.Get<SkillConversationReference>(SkillHandler.SkillConversationReferenceKey);
+                if (skillConversationReference != null)
+                {
+                    // If the skillConversationReference.OAuthScope is for one of the supported channels, we are at the root and we should not send an EoC.
+                    return skillConversationReference.OAuthScope != AuthenticationConstants.ToChannelFromBotOAuthScope && skillConversationReference.OAuthScope != GovernmentAuthenticationConstants.ToChannelFromBotOAuthScope;
+                }
+
+                return true;
+            }
+
+            return false;
+        }
     }
 }
diff --git a/tests/Microsoft.Bot.Builder.Dialogs.Tests/DialogManagerTests.cs b/tests/Microsoft.Bot.Builder.Dialogs.Tests/DialogManagerTests.cs
@@ -4,6 +4,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Security.Claims;
 using System.Threading;
 using System.Threading.Tasks;
@@ -27,9 +28,38 @@ public class DialogManagerTests
         // An App ID for a skill bot.
         private readonly string _skillBotId = Guid.NewGuid().ToString();
 
+        // Captures an EndOfConversation if it was sent to help with assertions.
+        private Activity _eocSent;
+
         // Property to capture the DialogManager turn results and do assertions.
         private DialogManagerResult _dmTurnResult;
 
+        /// <summary>
+        /// Enum to handle different skill test cases.
+        /// </summary>
+        public enum SkillFlowTestCase
+        {
+            /// <summary>
+            /// DialogManager is executing on a root bot with no skills (typical standalone bot).
+            /// </summary>
+            RootBotOnly,
+
+            /// <summary>
+            /// DialogManager is executing on a root bot handling replies from a skill.
+            /// </summary>
+            RootBotConsumingSkill,
+
+            /// <summary>
+            /// DialogManager is executing in a skill that is called from a root and calling another skill.
+            /// </summary>
+            MiddleSkill,
+
+            /// <summary>
+            /// DialogManager is executing in a skill that is called from a parent (a root or another skill) but doesn't call another skill.
+            /// </summary>
+            LeafSkill
+        }
+
         public TestContext TestContext { get; set; }
 
         [TestMethod]
@@ -210,26 +240,35 @@ public async Task DialogManager_DialogSet()
         }
 
         [TestMethod]
-        public async Task SkillSendsEoCAndValuesAtDialogEnd()
+        [DataRow(SkillFlowTestCase.RootBotOnly, false)]
+        [DataRow(SkillFlowTestCase.RootBotConsumingSkill, false)]
+        [DataRow(SkillFlowTestCase.MiddleSkill, true)]
+        [DataRow(SkillFlowTestCase.LeafSkill, true)]
+        public async Task HandlesBotAndSkillsTestCases(SkillFlowTestCase testCase, bool shouldSendEoc)
         {
             var firstConversationId = Guid.NewGuid().ToString();
             var storage = new MemoryStorage();
 
             var adaptiveDialog = CreateTestDialog(property: "conversation.name");
-
-            await CreateFlow(adaptiveDialog, storage, firstConversationId, isSkillFlow: true, locale: "en-GB")
-                .Send("hi")
+            await CreateFlow(adaptiveDialog, storage, firstConversationId, testCase: testCase, locale: "en-GB").Send("Hi")
                 .AssertReply("Hello, what is your name?")
-                .Send("Carlos")
-                .AssertReply("Hello Carlos, nice to meet you!")
-                .AssertReply(activity =>
-                {
-                    Assert.AreEqual(activity.Type, ActivityTypes.EndOfConversation);
-                    Assert.AreEqual(((Activity)activity).Value, "Carlos");
-                    Assert.AreEqual(((Activity)activity).Locale, "en-GB");
-                })
+                .Send("SomeName")
+                .AssertReply("Hello SomeName, nice to meet you!")
                 .StartTestAsync();
+            
             Assert.AreEqual(DialogTurnStatus.Complete, _dmTurnResult.TurnResult.Status);
+
+            if (shouldSendEoc)
+            {
+                Assert.IsNotNull(_eocSent, "Skills should send EndConversation to channel");
+                Assert.AreEqual(ActivityTypes.EndOfConversation, _eocSent.Type);
+                Assert.AreEqual("SomeName", _eocSent.Value);
+                Assert.AreEqual("en-GB", _eocSent.Locale);
+            }
+            else
+            {
+                Assert.IsNull(_eocSent, "Root bot should not send EndConversation to channel");
+            }
         }
 
         [TestMethod]
@@ -242,7 +281,7 @@ public async Task SkillHandlesEoCFromParent()
 
             var eocActivity = new Activity(ActivityTypes.EndOfConversation);
 
-            await CreateFlow(adaptiveDialog, storage, firstConversationId, isSkillFlow: true, isSkillResponse: false)
+            await CreateFlow(adaptiveDialog, storage, firstConversationId, testCase: SkillFlowTestCase.LeafSkill)
                 .Send("hi")
                 .AssertReply("Hello, what is your name?")
                 .Send(eocActivity)
@@ -261,7 +300,7 @@ public async Task SkillHandlesRepromptFromParent()
 
             var repromptEvent = new Activity(ActivityTypes.Event) { Name = DialogEvents.RepromptDialog };
 
-            await CreateFlow(adaptiveDialog, storage, firstConversationId, isSkillFlow: true)
+            await CreateFlow(adaptiveDialog, storage, firstConversationId, testCase: SkillFlowTestCase.LeafSkill)
                 .Send("hi")
                 .AssertReply("Hello, what is your name?")
                 .Send(repromptEvent)
@@ -281,7 +320,7 @@ public async Task SkillShouldReturnEmptyOnRepromptWithNoDialog()
 
             var repromptEvent = new Activity(ActivityTypes.Event) { Name = DialogEvents.RepromptDialog };
 
-            await CreateFlow(adaptiveDialog, storage, firstConversationId, isSkillFlow: true)
+            await CreateFlow(adaptiveDialog, storage, firstConversationId, testCase: SkillFlowTestCase.LeafSkill)
                 .Send(repromptEvent)
                 .StartTestAsync();
 
@@ -293,7 +332,7 @@ private Dialog CreateTestDialog(string property)
             return new AskForNameDialog(property.Replace(".", string.Empty), property);
         }
 
-        private TestFlow CreateFlow(Dialog dialog, IStorage storage, string conversationId, string dialogStateProperty = null, bool isSkillFlow = false, bool isSkillResponse = true, string locale = null)
+        private TestFlow CreateFlow(Dialog dialog, IStorage storage, string conversationId, string dialogStateProperty = null, SkillFlowTestCase testCase = SkillFlowTestCase.RootBotOnly, string locale = null)
         {
             var convoState = new ConversationState(storage);
             var userState = new UserState(storage);
@@ -312,7 +351,7 @@ private TestFlow CreateFlow(Dialog dialog, IStorage storage, string conversation
             var dm = new DialogManager(dialog, dialogStateProperty: dialogStateProperty);
             return new TestFlow(adapter, async (turnContext, cancellationToken) =>
             {
-                if (isSkillFlow)
+                if (testCase != SkillFlowTestCase.RootBotOnly)
                 {
                     // Create a skill ClaimsIdentity and put it in TurnState so SkillValidation.IsSkillClaim() returns true.
                     var claimsIdentity = new ClaimsIdentity();
@@ -321,14 +360,28 @@ private TestFlow CreateFlow(Dialog dialog, IStorage storage, string conversation
                     claimsIdentity.AddClaim(new Claim(AuthenticationConstants.AuthorizedParty, _parentBotId));
                     turnContext.TurnState.Add(BotAdapter.BotIdentityKey, claimsIdentity);
 
-                    if (isSkillResponse)
+                    if (testCase == SkillFlowTestCase.RootBotConsumingSkill)
+                    {
+                        // Simulate the SkillConversationReference with a channel OAuthScope stored in TurnState.
+                        // This emulates a response coming to a root bot through SkillHandler. 
+                        turnContext.TurnState.Add(SkillHandler.SkillConversationReferenceKey, new SkillConversationReference { OAuthScope = AuthenticationConstants.ToChannelFromBotOAuthScope });
+                    }
+
+                    if (testCase == SkillFlowTestCase.MiddleSkill)
                     {
                         // Simulate the SkillConversationReference with a parent Bot ID stored in TurnState.
                         // This emulates a response coming to a skill from another skill through SkillHandler. 
                         turnContext.TurnState.Add(SkillHandler.SkillConversationReferenceKey, new SkillConversationReference { OAuthScope = _parentBotId });
                     }
                 }
 
+                // Interceptor to capture the EoC activity if it was sent so we can assert it in the tests.
+                turnContext.OnSendActivities(async (tc, activities, next) =>
+                {
+                    _eocSent = activities.FirstOrDefault(activity => activity.Type == ActivityTypes.EndOfConversation);
+                    return await next().ConfigureAwait(false);
+                });
+
                 // Capture the last DialogManager turn result for assertions.
                 _dmTurnResult = await dm.OnTurnAsync(turnContext, cancellationToken: cancellationToken).ConfigureAwait(false);
             });
@@ -368,7 +421,7 @@ public override async Task<DialogTurnResult> BeginDialogAsync(DialogContext oute
                                 Text = "Hello, what is your name?"
                             }
                         },
-                        cancellationToken: cancellationToken)
+                        cancellationToken)
                     .ConfigureAwait(false);
             }
 
