From c2e4eb9ee2df7d05b84b2b6d4187bb941343ee68 Mon Sep 17 00:00:00 2001 From: Steven Ickman Date: Wed, 6 May 2020 16:13:34 -0700 Subject: [PATCH] Fixed issues with AdaptiveSkillDialog (#2196) and added unit tests --- .../src/skills/adaptiveSkillDialog.ts | 31 ++-- .../tests/adaptiveSkillDialog.test.js | 152 ++++++++++++++++++ .../botbuilder-dialogs/src/dialogManager.ts | 6 +- 3 files changed, 173 insertions(+), 16 deletions(-) create mode 100644 libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js diff --git a/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts b/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts index 8cc7118811..605e0ae75c 100644 --- a/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts +++ b/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts @@ -8,9 +8,10 @@ import { SkillDialog, SkillDialogOptions, DialogContext, DialogTurnResult, DialogManager, BeginSkillDialogOptions } from 'botbuilder-dialogs'; import { BoolExpression, StringExpression } from 'adaptive-expressions'; import { TemplateInterface } from '../template'; -import { Activity, ActivityTypes } from 'botbuilder-core'; +import { Activity, ActivityTypes, BotFrameworkClient, SkillConversationIdFactoryBase } from 'botbuilder-core'; -const GLOBAL_SKILL_OPTIONS = Symbol('globalSkillOptions'); +const SKILL_CLIENT = Symbol('skillClient'); +const CONVERSATION_ID_FACTORY = Symbol('conversationIdFactory'); export class AdaptiveSkillDialog extends SkillDialog { @@ -83,13 +84,6 @@ export class AdaptiveSkillDialog extends SkillDialog { return await dc.endDialog(); } - // Update options with global options - const globalOptions = dc.context.turnState.get(GLOBAL_SKILL_OPTIONS); - if (globalOptions) { - this.dialogOptions = Object.assign(globalOptions, this.dialogOptions); - } - if (!this.dialogOptions.conversationState) { dc.dialogManager.conversationState } - // Setup the skill to call const botId = this.botId.getValue(dcState); const skillHostEndpoint = this.skillHostEndpoint.getValue(dcState); @@ -98,6 +92,9 @@ export class AdaptiveSkillDialog extends SkillDialog { if (this.skillAppId) { this.dialogOptions.skill.id = this.dialogOptions.skill.appId = this.skillAppId.getValue(dcState) } if (this.skillEndpoint) { this.dialogOptions.skill.skillEndpoint = this.skillEndpoint.getValue(dcState) } if (this.connectionName) { this.dialogOptions.connectionName = this.connectionName.getValue(dcState) } + if (!this.dialogOptions.conversationState) { this.dialogOptions.conversationState = dc.dialogManager.conversationState } + if (!this.dialogOptions.skillClient) { this.dialogOptions.skillClient = dc.context.turnState.get(SKILL_CLIENT) } + if (!this.dialogOptions.conversationIdFactory) { this.dialogOptions.conversationIdFactory = dc.context.turnState.get(CONVERSATION_ID_FACTORY) } // Get the activity to send to the skill. options = {} as BeginSkillDialogOptions; @@ -126,12 +123,20 @@ export class AdaptiveSkillDialog extends SkillDialog { return await super.continueDialog(dc); } + protected onComputeId(): string { + const appId = this.skillAppId ? this.skillAppId.toString() : ''; + const activity = this.activity ? this.activity.toString() : ''; + return `Skill[${appId}:${activity}]`; + } + /** - * Configures the initial skill dialog options to use. + * Configures the skill client and conversation ID factory to use. * @param dm DialogManager to configure. - * @param options Skill dialog options to use. + * @param skillClient Skill client instance to use. + * @param conversationIdFactory Conversation ID factory to use. */ - static setGlobalSkillOptions(dm: DialogManager, options: SkillDialogOptions): void { - dm.initialTurnState.set(GLOBAL_SKILL_OPTIONS, options); + static setSkillHostOptions(dm: DialogManager, skillClient: BotFrameworkClient, conversationIdFactory: SkillConversationIdFactoryBase): void { + dm.initialTurnState.set(SKILL_CLIENT, skillClient); + dm.initialTurnState.set(CONVERSATION_ID_FACTORY, conversationIdFactory); } } \ No newline at end of file diff --git a/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js b/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js new file mode 100644 index 0000000000..b7a86db742 --- /dev/null +++ b/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js @@ -0,0 +1,152 @@ +const { ok, strictEqual } = require('assert'); +const { createHash } = require('crypto'); +const { stub } = require('sinon'); +const { + ActivityTypes, + ConversationState, + MemoryStorage, + TestAdapter, + SkillConversationIdFactoryBase, + StatusCodes, + TurnContext +} = require('botbuilder-core'); +const { BoolExpression, StringExpression } = require('adaptive-expressions'); +const { DialogManager, DialogTurnStatus } = require('botbuilder-dialogs'); +const { AdaptiveSkillDialog } = require('../') + + +class SimpleConversationIdFactory extends SkillConversationIdFactoryBase { + constructor(config = { disableCreateWithOpts: false, disableGetSkillRef: false }) { + super(); + this._conversationRefs = new Map(); + this.disableCreateWithOpts = config.disableCreateWithOpts; + this.disableGetSkillRef = config.disableGetSkillRef; + } + + async createSkillConversationIdWithOptions(opts) { + if (this.disableCreateWithOpts) { + return super.createSkillConversationIdWithOptions(); + } + } + + async createSkillConversationId(options) { + const key = createHash('md5').update(options.activity.conversation.id + options.activity.serviceUrl).digest('hex'); + + const ref = this._conversationRefs.has(key); + if (!ref) { + this._conversationRefs.set(key, { + conversationReference: TurnContext.getConversationReference(options.activity), + oAuthScope: options.fromBotOAuthScope + }); + } + return key; + } + + async getConversationReference() { + + } + + async getSkillConversationReference(skillConversationId) { + return this._conversationRefs.get(skillConversationId); + } + + deleteConversationReference() { + + } +} + +describe('SkillDialog', function() { + this.timeout(3000); + + let activitySent; // Activity + let fromBotIdSent; // string + let toBotIdSent; // string + let toUriSent; // string (URI) + + // Callback to capture the parameters sent to the skill + 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 [skillClient, postActivityStub] = createSkillClientAndStub(captureAction); + + // Setup dialog manager + const conversationState = new ConversationState(new MemoryStorage()); + const dm = new DialogManager(); + dm.conversationState = conversationState; + AdaptiveSkillDialog.setSkillHostOptions(dm, skillClient, new SimpleConversationIdFactory()); + + // Setup skill dialog + const dialog = new AdaptiveSkillDialog(); + setSkillDialogOptions(dialog); + dm.rootDialog = dialog; + + it('should call skill via beginDialog()', async () => { + // Send initial activity + const adapter = new TestAdapter(async (context) => { + const { turnResult } = await dm.onTurn(context); + + // Assert results and data sent to the SkillClient for first turn + strictEqual(fromBotIdSent, 'SkillCallerId'); + strictEqual(toBotIdSent, 'SkillId'); + strictEqual(toUriSent, 'http://testskill.contoso.com/api/messages'); + strictEqual(activitySent.text, 'test'); + strictEqual(turnResult.status, DialogTurnStatus.waiting); + ok(postActivityStub.calledOnce); + }); + + await adapter.send('test'); + }); +}); + +function setSkillDialogOptions(dialog) { + dialog.disabled = new BoolExpression(false); + dialog.botId = new StringExpression('SkillCallerId'); + dialog.skillHostEndpoint = new StringExpression('http://test.contoso.com/skill/messages'); + dialog.skillAppId = new StringExpression('SkillId'); + dialog.skillEndpoint = new StringExpression('http://testskill.contoso.com/api/messages'); +} + +/** + * @remarks + * captureAction should match the below signature: + * `(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity) => void` + * @param {Function} captureAction + * @param {StatusCodes} returnStatusCode Defaults to StatusCodes.OK + * @returns [BotFrameworkHttpClient, postActivityStub] + */ +function createSkillClientAndStub(captureAction, returnStatusCode = StatusCodes.OK) { + // This require should not fail as this method should only be called after verifying that botbuilder is resolvable. + const { BotFrameworkHttpClient } = require('../../botbuilder'); + + if (captureAction && typeof captureAction !== 'function') { + throw new TypeError(`Failed test arrangement - createSkillClientAndStub() received ${typeof captureAction} instead of undefined or a function.`); + } + + // Create ExpectedReplies object for response body + const dummyActivity = { type: ActivityTypes.Message, attachments: [], entities: [] }; + dummyActivity.text = 'dummy activity'; + const activityList = { activities: [dummyActivity] }; + + // Create wrapper for captureAction + function wrapAction(...args) { + captureAction(...args); + return { status: returnStatusCode, body: activityList }; + } + // Create client and stub + const skillClient = new BotFrameworkHttpClient({}); + const postActivityStub = stub(skillClient, 'postActivity'); + + if (captureAction) { + postActivityStub.callsFake(wrapAction); + } else { + postActivityStub.returns({ status: returnStatusCode, body: activityList }); + } + + return [ skillClient, postActivityStub ]; +} \ No newline at end of file diff --git a/libraries/botbuilder-dialogs/src/dialogManager.ts b/libraries/botbuilder-dialogs/src/dialogManager.ts index 9ad9e013cf..b3f76fb98b 100644 --- a/libraries/botbuilder-dialogs/src/dialogManager.ts +++ b/libraries/botbuilder-dialogs/src/dialogManager.ts @@ -111,9 +111,9 @@ export class DialogManager extends Configurable { if (!this.rootDialogId) { throw new Error(`DialogManager.onTurn: the bot's 'rootDialog' has not been configured.`); } // Copy initial turn state to context - for (const key in this.initialTurnState.keys()) { - context.turnState.set(key, this.initialTurnState.get(key)); - } + this.initialTurnState.forEach((value, key) => { + context.turnState.set(key, value); + }); const botStateSet = new BotStateSet();