diff --git a/libraries/Microsoft.Bot.Builder/Skills/SkillHandler.cs b/libraries/Microsoft.Bot.Builder/Skills/SkillHandler.cs index 610471ce99..07940f5c02 100644 --- a/libraries/Microsoft.Bot.Builder/Skills/SkillHandler.cs +++ b/libraries/Microsoft.Bot.Builder/Skills/SkillHandler.cs @@ -118,6 +118,40 @@ protected override async Task OnReplyToActivityAsync(ClaimsIde return await ProcessActivityAsync(claimsIdentity, conversationId, activityId, activity, cancellationToken).ConfigureAwait(false); } + /// + protected override async Task OnDeleteActivityAsync(ClaimsIdentity claimsIdentity, string conversationId, string activityId, CancellationToken cancellationToken = default) + { + var skillConversationReference = await GetSkillConversationReferenceAsync(conversationId, cancellationToken).ConfigureAwait(false); + + var callback = new BotCallbackHandler(async (turnContext, ct) => + { + turnContext.TurnState.Add(SkillConversationReferenceKey, skillConversationReference); + await turnContext.DeleteActivityAsync(activityId, cancellationToken).ConfigureAwait(false); + }); + + await _adapter.ContinueConversationAsync(claimsIdentity, skillConversationReference.ConversationReference, skillConversationReference.OAuthScope, callback, cancellationToken).ConfigureAwait(false); + } + + /// + protected override async Task OnUpdateActivityAsync(ClaimsIdentity claimsIdentity, string conversationId, string activityId, Activity activity, CancellationToken cancellationToken = default) + { + var skillConversationReference = await GetSkillConversationReferenceAsync(conversationId, cancellationToken).ConfigureAwait(false); + + ResourceResponse resourceResponse = null; + var callback = new BotCallbackHandler(async (turnContext, ct) => + { + turnContext.TurnState.Add(SkillConversationReferenceKey, skillConversationReference); + activity.ApplyConversationReference(skillConversationReference.ConversationReference); + turnContext.Activity.Id = activityId; + turnContext.Activity.CallerId = $"{CallerIdConstants.BotToBotPrefix}{JwtTokenValidation.GetAppIdFromClaims(claimsIdentity.Claims)}"; + resourceResponse = await turnContext.UpdateActivityAsync(activity, cancellationToken).ConfigureAwait(false); + }); + + await _adapter.ContinueConversationAsync(claimsIdentity, skillConversationReference.ConversationReference, skillConversationReference.OAuthScope, callback, cancellationToken).ConfigureAwait(false); + + return resourceResponse ?? new ResourceResponse(Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture)); + } + private static void ApplyEoCToTurnContextActivity(ITurnContext turnContext, Activity endOfConversationActivity) { // transform the turnContext.Activity to be the EndOfConversation. @@ -153,7 +187,7 @@ private static void ApplyEventToTurnContextActivity(ITurnContext turnContext, Ac turnContext.Activity.Properties = eventActivity.Properties; } - private async Task ProcessActivityAsync(ClaimsIdentity claimsIdentity, string conversationId, string replyToActivityId, Activity activity, CancellationToken cancellationToken) + private async Task GetSkillConversationReferenceAsync(string conversationId, CancellationToken cancellationToken) { SkillConversationReference skillConversationReference; try @@ -162,6 +196,8 @@ private async Task ProcessActivityAsync(ClaimsIdentity claimsI } catch (NotImplementedException) { + _logger.LogWarning("Got NotImplementedException when trying to call GetSkillConversationReferenceAsync() on the ConversationIdFactory, attempting to use deprecated GetConversationReferenceAsync() method instead."); + // Attempt to get SkillConversationReference using deprecated method. // this catch should be removed once we remove the deprecated method. // We need to use the deprecated method for backward compatibility. @@ -177,9 +213,17 @@ private async Task ProcessActivityAsync(ClaimsIdentity claimsI if (skillConversationReference == null) { + _logger.LogError($"Unable to get skill conversation reference for conversationId {conversationId}."); throw new KeyNotFoundException(); } + return skillConversationReference; + } + + private async Task ProcessActivityAsync(ClaimsIdentity claimsIdentity, string conversationId, string replyToActivityId, Activity activity, CancellationToken cancellationToken) + { + var skillConversationReference = await GetSkillConversationReferenceAsync(conversationId, cancellationToken).ConfigureAwait(false); + ResourceResponse resourceResponse = null; var callback = new BotCallbackHandler(async (turnContext, ct) => diff --git a/tests/Microsoft.Bot.Builder.Tests/Skills/SkillHandlerTests.cs b/tests/Microsoft.Bot.Builder.Tests/Skills/SkillHandlerTests.cs index b4b2a2e3c7..f6cde1c55b 100644 --- a/tests/Microsoft.Bot.Builder.Tests/Skills/SkillHandlerTests.cs +++ b/tests/Microsoft.Bot.Builder.Tests/Skills/SkillHandlerTests.cs @@ -20,12 +20,12 @@ namespace Microsoft.Bot.Builder.Tests.Skills { public class SkillHandlerTests { + private readonly string _botId = Guid.NewGuid().ToString("N"); private readonly ClaimsIdentity _claimsIdentity; private readonly string _conversationId; private readonly ConversationReference _conversationReference; private readonly Mock _mockAdapter = new Mock(); private readonly Mock _mockBot = new Mock(); - private readonly string _botId = Guid.NewGuid().ToString("N"); private readonly string _skillId = Guid.NewGuid().ToString("N"); private readonly TestConversationIdFactory _testConversationIdFactory = new TestConversationIdFactory(); @@ -43,11 +43,11 @@ public SkillHandlerTests() var activity = (Activity)Activity.CreateMessageActivity(); activity.ApplyConversationReference(_conversationReference); - var skill = new BotFrameworkSkill() - { - AppId = _skillId, - Id = "skill", - SkillEndpoint = new Uri("http://testbot.com/api/messages") + var skill = new BotFrameworkSkill + { + AppId = _skillId, + Id = "skill", + SkillEndpoint = new Uri("http://testbot.com/api/messages") }; var options = new SkillConversationIdFactoryOptions @@ -57,12 +57,12 @@ public SkillHandlerTests() Activity = activity, BotFrameworkSkill = skill }; - + _conversationId = _testConversationIdFactory.CreateSkillConversationIdAsync(options, CancellationToken.None).Result; } [Fact] - public async Task LegacyConversationIdFactoryTest() + public async Task LegacyConversationIdFactoryWorksTest() { var legacyFactory = new TestLegacyConversationIdFactory(); var conversationReference = new ConversationReference @@ -72,110 +72,191 @@ public async Task LegacyConversationIdFactoryTest() }; // Testing the deprecated method for backward compatibility. -#pragma warning disable 618 +#pragma warning disable 612 var conversationId = await legacyFactory.CreateSkillConversationIdAsync(conversationReference, CancellationToken.None); -#pragma warning restore 618 - BotCallbackHandler botCallback = null; +#pragma warning restore 612 _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((identity, reference, audience, callback, cancellationToken) => + .Callback(async (identity, reference, audience, callback, cancellationToken) => { - botCallback = callback; - Console.WriteLine("blah"); + // Invoke the callback created by the handler so we can assert the rest of the execution. + var turnContext = new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()); + await callback(turnContext, cancellationToken); }); - var sut = CreateSkillHandlerForTesting(legacyFactory); - var activity = Activity.CreateMessageActivity(); activity.ApplyConversationReference(conversationReference); + var sut = CreateSkillHandlerForTesting(legacyFactory); + await sut.TestOnSendToConversationAsync(_claimsIdentity, conversationId, (Activity)activity, CancellationToken.None); - Assert.NotNull(botCallback); - await botCallback.Invoke(new TurnContext(_mockAdapter.Object, (Activity)activity), CancellationToken.None); } - [Fact] - public async Task OnSendToConversationAsyncTest() - { + [Theory] + [InlineData(ActivityTypes.EndOfConversation)] + [InlineData(ActivityTypes.Event)] + [InlineData(ActivityTypes.Message)] + public async Task OnSendToConversationAsyncTest(string activityType) + { + var activity = new Activity(activityType); + _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((identity, reference, audience, callback, cancellationToken) => + .Callback(async (identity, reference, audience, callback, cancellationToken) => { - callback(new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()), CancellationToken.None).Wait(); + // Invoke the callback created by the handler so we can assert the rest of the execution. + var turnContext = new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()); + await callback(turnContext, cancellationToken); + + // Assert the callback set the right properties. + Assert.Equal($"{CallerIdConstants.BotToBotPrefix}{_skillId}", turnContext.Activity.CallerId); }); _mockAdapter.Setup(x => x.SendActivitiesAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Callback((turnContext, activities, cancellationToken) => { + // Messages should not have a caller id set when sent back to the caller. + Assert.Null(activities[0].CallerId); + Assert.Null(activities[0].ReplyToId); }) .Returns(Task.FromResult(new[] { new ResourceResponse { Id = "resourceId" } })); var sut = CreateSkillHandlerForTesting(); + var resourceResponse = await sut.TestOnSendToConversationAsync(_claimsIdentity, _conversationId, activity, CancellationToken.None); - var activity = (Activity)Activity.CreateMessageActivity(); - activity.ApplyConversationReference(_conversationReference); + if (activityType == ActivityTypes.Message) + { + // Assert mock SendActivitiesAsync was called + _mockAdapter.Verify(ma => ma.SendActivitiesAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); - Assert.Null(activity.CallerId); - var resourceResponse = await sut.TestOnSendToConversationAsync(_claimsIdentity, _conversationId, activity, CancellationToken.None); - Assert.Null(activity.CallerId); - Assert.Equal("resourceId", resourceResponse.Id); + Assert.Equal("resourceId", resourceResponse.Id); + } } - [Fact] - public async Task OnOnReplyToActivityAsyncTest() + [Theory] + [InlineData(ActivityTypes.EndOfConversation)] + [InlineData(ActivityTypes.Event)] + [InlineData(ActivityTypes.Message)] + public async Task OnOnReplyToActivityAsyncTest(string activityType) { - BotCallbackHandler botCallback = null; + var activity = new Activity(activityType); + + // Mock ContinueConversationAsync (this is called at the end of ProcessActivityAsync). _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((identity, reference, audience, callback, cancellationToken) => + .Callback(async (identity, reference, audience, callback, cancellationToken) => { - botCallback = callback; + // Invoke the callback created by the handler so we can assert the rest of the execution. + var turnContext = new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()); + await callback(turnContext, cancellationToken); + + // Assert the callback set the right properties. + Assert.Equal($"{CallerIdConstants.BotToBotPrefix}{_skillId}", turnContext.Activity.CallerId); }); - var sut = CreateSkillHandlerForTesting(); + var replyToActivityId = Guid.NewGuid().ToString("N"); - var activity = (Activity)Activity.CreateMessageActivity(); - var activityId = Guid.NewGuid().ToString("N"); - activity.ApplyConversationReference(_conversationReference); + // Mock SendActivitiesAsync, assert values sent and return an arbitrary ResourceResponse. + _mockAdapter.Setup(x => x.SendActivitiesAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((turnContext, activities, cancellationToken) => + { + // Messages should not have a caller id set when sent back to the caller. + Assert.Null(activities[0].CallerId); + Assert.Equal(replyToActivityId, activities[0].ReplyToId); - await sut.TestOnReplyToActivityAsync(_claimsIdentity, _conversationId, activityId, activity, CancellationToken.None); - Assert.NotNull(botCallback); - await botCallback.Invoke(new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()), CancellationToken.None); - Assert.Null(activity.CallerId); - } + // Do nothing, we don't want the activities sent to the channel in the tests. + }) + .Returns(Task.FromResult(new[] { new ResourceResponse { Id = "resourceId" } })); - [Fact] - public async Task EventActivityAsyncTest() - { - var activity = (Activity)Activity.CreateEventActivity(); - await TestActivityCallback(activity); - Assert.Equal($"{CallerIdConstants.BotToBotPrefix}{_skillId}", activity.CallerId); - } + // Call TestOnReplyToActivity on our helper so it calls the OnReply method on the handler and executes our mocks. + var sut = CreateSkillHandlerForTesting(); + var resourceResponse = await sut.TestOnReplyToActivityAsync(_claimsIdentity, _conversationId, replyToActivityId, activity, CancellationToken.None); - [Fact] - public async Task EndOfConversationActivityAsyncTest() - { - var activity = (Activity)Activity.CreateEndOfConversationActivity(); - await TestActivityCallback(activity); - Assert.Equal($"{CallerIdConstants.BotToBotPrefix}{_skillId}", activity.CallerId); + // Assert mock ContinueConversationAsync was called + _mockAdapter.Verify(ma => ma.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); + + if (activityType == ActivityTypes.Message) + { + // Assert mock SendActivitiesAsync was called + _mockAdapter.Verify(ma => ma.SendActivitiesAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); + + Assert.Equal("resourceId", resourceResponse.Id); + } + else + { + // Assert mock SendActivitiesAsync wasn't called + _mockAdapter.Verify(ma => ma.SendActivitiesAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } } [Fact] public async Task OnUpdateActivityAsyncTest() { - var sut = CreateSkillHandlerForTesting(); + // Mock ContinueConversationAsync (this is called at the end of OnUpdateActivityAsync). + _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Callback(async (identity, reference, audience, callback, cancellationToken) => + { + // Create a TurnContext with our mock adapter. + var turnContext = new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()); + + // Execute the callback with our turnContext. + await callback(turnContext, cancellationToken); + + // Assert the callback set the right properties. + Assert.Equal($"{CallerIdConstants.BotToBotPrefix}{_skillId}", turnContext.Activity.CallerId); + }); + var activity = Activity.CreateMessageActivity(); + var message = activity.Text = $"TestUpdate {DateTime.Now}."; var activityId = Guid.NewGuid().ToString("N"); - activity.ApplyConversationReference(_conversationReference); - await Assert.ThrowsAsync(() => - sut.TestOnUpdateActivityAsync(_claimsIdentity, _conversationId, activityId, (Activity)activity, CancellationToken.None)); + // Mock UpdateActivityAsync, assert the activity being sent and return and arbitrary ResourceResponse. + _mockAdapter.Setup(x => x.UpdateActivityAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((context, newActivity, cancellationToken) => + { + // Assert the activity being sent. + Assert.Equal(activityId, newActivity.ReplyToId); + Assert.Equal(message, newActivity.Text); + }) + .Returns(Task.FromResult(new ResourceResponse { Id = "resourceId" })); + + var sut = CreateSkillHandlerForTesting(); + var resourceResponse = await sut.TestOnUpdateActivityAsync(_claimsIdentity, _conversationId, activityId, (Activity)activity, CancellationToken.None); + + // Assert mock methods were called + _mockAdapter.Verify(ma => ma.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); + _mockAdapter.Verify(ma => ma.UpdateActivityAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); + + Assert.Equal("resourceId", resourceResponse.Id); } [Fact] public async Task OnDeleteActivityAsyncTest() { - var sut = CreateSkillHandlerForTesting(); + // Mock ContinueConversationAsync (this is called at the end of OnUpdateActivityAsync). + _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Callback(async (identity, reference, audience, callback, cancellationToken) => + { + // Create a TurnContext with our mock adapter. + var turnContext = new TurnContext(_mockAdapter.Object, _conversationReference.GetContinuationActivity()); + + // Execute the callback with our turnContext. + await callback(turnContext, cancellationToken); + }); + var activityId = Guid.NewGuid().ToString("N"); - await Assert.ThrowsAsync(() => - sut.TestOnDeleteActivityAsync(_claimsIdentity, _conversationId, activityId, CancellationToken.None)); + + // Mock UpdateActivityAsync, assert the activity being sent + _mockAdapter.Setup(x => x.DeleteActivityAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((context, conversationReference, cancellationToken) => + { + // Assert the activityId being deleted. + Assert.Equal(activityId, conversationReference.ActivityId); + }); + + var sut = CreateSkillHandlerForTesting(); + await sut.TestOnDeleteActivityAsync(_claimsIdentity, _conversationId, activityId, CancellationToken.None); + + // Assert mock methods were called + _mockAdapter.Verify(ma => ma.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); + _mockAdapter.Verify(ma => ma.DeleteActivityAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Exactly(1)); } [Fact] @@ -240,7 +321,7 @@ public async Task OnSendConversationHistoryAsyncTest() var conversationId = Guid.NewGuid().ToString("N"); var transcript = new Transcript(); await Assert.ThrowsAsync(() => - sut.TestOnSendConversationHistoryAsync(_claimsIdentity, conversationId, transcript, CancellationToken.None)); + sut.TestOnSendConversationHistoryAsync(_claimsIdentity, conversationId, transcript, CancellationToken.None)); } [Fact] @@ -253,26 +334,6 @@ await Assert.ThrowsAsync(() => sut.TestOnUploadAttachmentAsync(_claimsIdentity, conversationId, attachmentData, CancellationToken.None)); } - private async Task TestActivityCallback(Activity activity) - { - BotCallbackHandler botCallback = null; - _mockAdapter.Setup(x => x.ContinueConversationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Callback((identity, reference, audience, callback, cancellationToken) => - { - botCallback = callback; - Console.WriteLine("blah"); - }); - - var sut = CreateSkillHandlerForTesting(); - - var activityId = Guid.NewGuid().ToString("N"); - activity.ApplyConversationReference(_conversationReference); - - await sut.TestOnReplyToActivityAsync(_claimsIdentity, _conversationId, activityId, activity, CancellationToken.None); - Assert.NotNull(botCallback); - await botCallback.Invoke(new TurnContext(_mockAdapter.Object, activity), CancellationToken.None); - } - private SkillHandlerInstanceForTests CreateSkillHandlerForTesting(SkillConversationIdFactoryBase overrideFactory = null) { return new SkillHandlerInstanceForTests(_mockAdapter.Object, _mockBot.Object, overrideFactory ?? _testConversationIdFactory, new Mock().Object, new AuthenticationConfiguration()); @@ -348,7 +409,8 @@ public override Task DeleteConversationReferenceAsync(string skillConversationId } /// - /// A helper class that provides public methods that allow us to test protected methods in bypassing authentication. + /// A helper class that provides public methods that allow us to test protected methods + /// in bypassing authentication. /// private class SkillHandlerInstanceForTests : SkillHandler {