Delete Conversation Reference from ConversationIdFactory when SkillDi…

…alog receives EndOfConversation while using ExpectReplies (#2787) Co-authored-by: Gabo Gilabert <gabog@users.noreply.github.com>
microsoft · Sep 26, 2020 · 621fd44 · 621fd44
1 parent 6fe5179
commit 621fd44
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 3 deletions.
diff --git a/libraries/botbuilder-dialogs/src/skillDialog.ts b/libraries/botbuilder-dialogs/src/skillDialog.ts
@@ -213,6 +213,9 @@ export class SkillDialog extends Dialog<Partial<BeginSkillDialogOptions>> {
                 if (activityFromSkill.type === ActivityTypes.EndOfConversation) {
                     // Capture the EndOfConversation activity if it was sent from skill
                     eocActivity = activityFromSkill;
+
+                    // The conversation has ended, so cleanup the conversation id.
+                    await this.dialogOptions.conversationIdFactory.deleteConversationReference(skillConversationId);
                 } else if (await this.interceptOAuthCards(context, activityFromSkill, this.dialogOptions.connectionName)) {
                     // Do nothing. The token exchange succeeded, so no OAuthCard needs to be shown to the user.
                 } else {

diff --git a/libraries/botbuilder-dialogs/tests/skillDialog.test.js b/libraries/botbuilder-dialogs/tests/skillDialog.test.js
@@ -126,6 +126,42 @@ describe('SkillDialog', function() {
                 strictEqual(await dialogOptions.conversationIdFactory.getSkillConversationReference(undefined), undefined, 'no test should use TestAdapter ConversationId as SkillConversationId.');
             };
 
+            it('should delete conversation from conversationIdFactory when receiving EndOfConversation from skill and ExpectReplies is true', async () => {
+                const adapter = new TestAdapter(/* logic param not required */);
+
+                const activity = { type: ActivityTypes.Message, deliveryMode: DeliveryModes.ExpectReplies, channelId: Channels.Directline, conversation: { id: '1' } };
+                const context = new TurnContext(adapter, activity);
+                context.turnState.set(adapter.OAuthScopeKey, DEFAULT_OAUTHSCOPE);
+
+                const captureAction = (fromBotId, toBotId, toUri, serviceUrl, conversationId, activity) => {
+                    // Capture values sent to the skill so we can assert the right parameters were used.
+                    fromBotIdSent = fromBotId;
+                    toBotIdSent = toBotId;
+                    toUriSent = toUri;
+                    activitySent = activity;
+                }
+
+                // Create BotFrameworkHttpClient and postActivityStub
+                const expectedReply = { type: ActivityTypes.EndOfConversation, attachments: [], entities: [] };
+                const expectedReplies = { activities: [expectedReply] };
+                const [skillClient, postActivityStub] = createSkillClientAndStub(captureAction, undefined, expectedReplies );
+                const conversationState = new ConversationState(new MemoryStorage());
+                const dialogOptions = createSkillDialogOptions(conversationState, skillClient, undefined, false);
+
+                const dialog = new SkillDialog(dialogOptions, 'SkillDialog');
+                dialog.state = {};
+                const dialogState = conversationState.createProperty('dialogState');
+                const dialogs = new DialogSet(dialogState);
+                dialogs.add(dialog);
+                const dc = await dialogs.createContext(context);
+                dc.stack = [dialog];
+
+                await dialog.beginDialog(dc, { activity });
+
+                strictEqual(1, dialogOptions.conversationIdFactory.createCount);
+                strictEqual(0, dialogOptions.conversationIdFactory._conversationRefs.size);
+            });
+
             // "Data Rows"
             it('when deliveryMode is undefined', async () => {
                 await beginDialogShouldCallSkill();
@@ -492,7 +528,7 @@ describe('SkillDialog', function() {
  * @param {StatusCodes} returnStatusCode Defaults to StatusCodes.OK
  * @returns [BotFrameworkHttpClient, postActivityStub]
  */
-function createSkillClientAndStub(captureAction, returnStatusCode = StatusCodes.OK) {
+function createSkillClientAndStub(captureAction, returnStatusCode = StatusCodes.OK, expectedReplies) {
     // This require should not fail as this method should only be called after verifying that botbuilder is resolvable.
     const { BotFrameworkHttpClient } = require('../../botbuilder');
 
@@ -503,7 +539,7 @@ function createSkillClientAndStub(captureAction, returnStatusCode = StatusCodes.
     // Create ExpectedReplies object for response body
     const dummyActivity = { type: ActivityTypes.Message, attachments: [], entities: [] };
     dummyActivity.text = 'dummy activity';
-    const activityList = { activities: [dummyActivity] };
+    const activityList =  expectedReplies ? expectedReplies : { activities: [dummyActivity] };
 
     // Create wrapper for captureAction
     function wrapAction(...args) {
@@ -621,7 +657,11 @@ class SimpleConversationIdFactory extends SkillConversationIdFactoryBase {
 
     async getSkillConversationReference(skillConversationId) { return this.getConversationReference(skillConversationId) }
 
-    async deleteConversationReference() { /* not used in SkillDialog */ }
+    async deleteConversationReference(skillConversationId) { 
+        if (!this.useCreateSkillConversationId) {
+            this._conversationRefs.delete(skillConversationId);
+        }
+    }
 }
 
 /**