-
Notifications
You must be signed in to change notification settings - Fork 374
Handle copy LG activity in visual editor #1096
Changes from all commits
28c7599
972e553
2217d46
a47d52e
3eebc55
a16d7d1
e6e5268
f880cf2
301b850
4372400
5b9a245
20deb95
0ab66e0
78393e6
9b7075d
3b5a8f1
acba6eb
c6c7a91
84c954b
f2d7417
0d25c59
29ab0c4
a628c69
78815ac
500be04
594d1ed
b92aebc
72fbe49
9ed7aa6
026000b
9335d44
dda7fb1
e369844
35adcdf
4b368db
7f8b749
96788fe
fbb74ee
df52ffa
8166737
b3174b4
e6a2130
f420982
039d255
c0b48d5
3d571d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import { copyAdaptiveAction } from '../src/copyUtils'; | ||
|
|
||
| describe('copyAdaptiveAction', () => { | ||
| const lgTemplate = [{ Name: 'bfdactivity-1234', Body: '-hello' }]; | ||
| const externalApi = { | ||
| updateDesigner: data => { | ||
| data.$designer = { id: '5678' }; | ||
| }, | ||
| lgApi: { | ||
| getLgTemplates: (fileId, activityId) => { | ||
| return Promise.resolve(lgTemplate); | ||
| }, | ||
| updateLgTemplate: (filedId, activityId, activityBody) => { | ||
| return Promise.resolve(true); | ||
| }, | ||
| }, | ||
| }; | ||
| const externalApiWithFailure = { | ||
| ...externalApi, | ||
| lgApi: { | ||
| ...externalApi.lgApi, | ||
| updateLgTemplate: () => Promise.reject(), | ||
| }, | ||
| }; | ||
|
|
||
| it('should return {} when input is invalid', async () => { | ||
| expect(await copyAdaptiveAction(null, externalApi)).toEqual({}); | ||
| expect(await copyAdaptiveAction({}, externalApi)).toEqual({}); | ||
| expect(await copyAdaptiveAction({ name: 'hi' }, externalApi)).toEqual({}); | ||
| }); | ||
|
|
||
| it('can copy BeginDialog', async () => { | ||
| const beginDialog = { | ||
| $type: 'Microsoft.BeginDialog', | ||
| dialog: 'AddToDo', | ||
| }; | ||
|
|
||
| expect(await copyAdaptiveAction(beginDialog, externalApi)).toEqual({ | ||
| $type: 'Microsoft.BeginDialog', | ||
| $designer: { id: '5678' }, | ||
| dialog: 'AddToDo', | ||
| }); | ||
| }); | ||
|
|
||
| it('can copy SendActivity', async () => { | ||
| const sendActivity = { | ||
| $type: 'Microsoft.SendActivity', | ||
| activity: '[bfdactivity-1234]', | ||
| }; | ||
|
|
||
| expect(await copyAdaptiveAction(sendActivity, externalApi)).toEqual({ | ||
| $type: 'Microsoft.SendActivity', | ||
| $designer: { id: '5678' }, | ||
| activity: '[bfdactivity-5678]', | ||
| }); | ||
|
|
||
| expect(await copyAdaptiveAction(sendActivity, externalApiWithFailure)).toEqual({ | ||
| $type: 'Microsoft.SendActivity', | ||
| $designer: { id: '5678' }, | ||
| activity: '-hello', | ||
| }); | ||
| }); | ||
|
|
||
| it('can copy TextInput', async () => { | ||
| const promptText = { | ||
| $type: 'Microsoft.TextInput', | ||
| $designer: { | ||
| id: '844184', | ||
| name: 'Prompt for text', | ||
| }, | ||
| maxTurnCount: 3, | ||
| alwaysPrompt: false, | ||
| allowInterruptions: 'true', | ||
| outputFormat: 'none', | ||
| prompt: '[bfdactivity-1234]', | ||
| }; | ||
|
|
||
| expect(await copyAdaptiveAction(promptText, externalApi)).toEqual({ | ||
| $type: 'Microsoft.TextInput', | ||
| $designer: { | ||
| id: '5678', | ||
| }, | ||
| maxTurnCount: 3, | ||
| alwaysPrompt: false, | ||
| allowInterruptions: 'true', | ||
| outputFormat: 'none', | ||
| prompt: '[bfdactivity-5678]', | ||
| }); | ||
|
|
||
| expect(await copyAdaptiveAction(promptText, externalApiWithFailure)).toEqual({ | ||
| $type: 'Microsoft.TextInput', | ||
| $designer: { | ||
| id: '5678', | ||
| }, | ||
| maxTurnCount: 3, | ||
| alwaysPrompt: false, | ||
| allowInterruptions: 'true', | ||
| outputFormat: 'none', | ||
| prompt: '-hello', | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| const NestedFieldNames = { | ||
| Actions: 'actions', | ||
| ElseActions: 'elseActions', | ||
| DefaultCase: 'default', | ||
| Cases: 'cases', | ||
| }; | ||
|
|
||
| const DEFAULT_CHILDREN_KEYS = [NestedFieldNames.Actions]; | ||
| const childrenMap = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. those 3 properties are inventing a new abstraction which i'm not sure it's necessary, and make this part less readable. Can you refer to "jsonWalker" in server package, which is generic json walking implementation, and the concept of "json walk" is already a well-known concept, we don't have re-invent one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where is it? do we have plan on how can we share things in shared with server? I tend to no to do that here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point was "JsonWalk" is a well-solved problem, and has many implementation out there, and we also have one in server package. We don't have to choose the one in server, the point is implementing a customized xxWalk and not based on a generic JsonWalk looks like we are reinventing something, and making code less readable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in my mind, json walk is a simple problem could be solved by DFS, do we have any reference link?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. google walk-json, json walk npm, or any keyword. JsonWalk is a simple problem, can be achieved by DFS or other approach. The point is if we do some walk on json. A good pattern is use a generic jsonWalk which you trusted and tested seperately, plus your sepecific interested type of node. It's the classic "visitor pattern". So "walkAdaptiveJson" can be done as "walkJson + adapativeVisitor". your walkJson method can be tested and verifiy with 100% coverage, your adpativeVisitor can be tested with 100% coverage. and they are both relative small, relaitvely focused. You don't care about specific node type when you are testing "jsonWalk",you don't care "DFS" when you testing "visitors".
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is how the code will looks like function visitor(path, value) { function copyNode(data) { it can be simplified so much. |
||
| ['Microsoft.IfCondition']: [NestedFieldNames.Actions, NestedFieldNames.ElseActions], | ||
| ['Microsoft.SwitchCondition']: [NestedFieldNames.Cases, NestedFieldNames.DefaultCase], | ||
| }; | ||
|
|
||
| /** | ||
| * Considering that an Adaptive Action could be nested with other actions, | ||
| * for example, the IfCondition and SwitchCondition and Foreach, we need | ||
| * this helper to visit all possible action nodes recursively. | ||
| * | ||
| * @param {any} input The input Adaptive Action which has $type field. | ||
| * @param {function} visitor The callback function called on each action node. | ||
| */ | ||
| async function walkAdaptiveAction(input: any, visitor: (data: any) => Promise<any>) { | ||
| if (!input || !input.$type) return; | ||
|
|
||
| await visitor(input); | ||
|
|
||
| let childrenKeys = DEFAULT_CHILDREN_KEYS; | ||
| if (input.$type && childrenMap[input.$type]) { | ||
| childrenKeys = childrenMap[input.$type]; | ||
| } | ||
|
|
||
| for (const childrenKey of childrenKeys) { | ||
| const children = input[childrenKey]; | ||
| if (Array.isArray(children)) { | ||
| Promise.all(children.map(async x => await walkAdaptiveAction(x, visitor))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function isLgActivity(activity: string) { | ||
| return activity && (activity.includes('bfdactivity-') || activity.includes('bfdprompt-')); | ||
yeze322 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| async function copyLgActivity(activity: string, designerId: string, lgApi: any): Promise<string> { | ||
| if (!activity) return ''; | ||
| if (!isLgActivity(activity) || !lgApi) return activity; | ||
|
|
||
| const { getLgTemplates, updateLgTemplate } = lgApi; | ||
| if (!getLgTemplates) return activity; | ||
|
|
||
| let rawLg: any[] = []; | ||
| try { | ||
| rawLg = await getLgTemplates('common', activity); | ||
| } catch (error) { | ||
| return activity; | ||
| } | ||
|
|
||
| const currentLg = rawLg.find(lg => `[${lg.Name}]` === activity); | ||
|
|
||
| if (currentLg) { | ||
| // Create new lg activity. | ||
| const newLgContent = currentLg.Body; | ||
| const newLgId = `bfdactivity-${designerId}`; | ||
| try { | ||
| await updateLgTemplate('common', newLgId, newLgContent); | ||
| return `[${newLgId}]`; | ||
| } catch (e) { | ||
| return newLgContent; | ||
| } | ||
| } | ||
| return activity; | ||
| } | ||
yeze322 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const overrideLgActivity = async (data, { lgApi }) => { | ||
| data.activity = await copyLgActivity(data.activity, data.$designer.id, lgApi); | ||
| }; | ||
|
|
||
| const overrideLgPrompt = async (data, { lgApi }) => { | ||
| data.prompt = await copyLgActivity(data.prompt, data.$designer.id, lgApi); | ||
| }; | ||
|
|
||
| // TODO: use $type from SDKTypes (after solving circular import issue). | ||
| const OverriderByType = { | ||
| 'Microsoft.SendActivity': overrideLgActivity, | ||
| 'Microsoft.AttachmentInput': overrideLgPrompt, | ||
| 'Microsoft.ConfirmInput': overrideLgPrompt, | ||
| 'Microsoft.DateTimeInput': overrideLgPrompt, | ||
| 'Microsoft.NumberInput': overrideLgPrompt, | ||
| 'Microsoft.OAuthInput': overrideLgPrompt, | ||
| 'Microsoft.TextInput': overrideLgPrompt, | ||
| 'Microsoft.ChoiceInput': overrideLgPrompt, | ||
| }; | ||
|
|
||
| const needsOverride = data => !!(data && OverriderByType[data.$type]); | ||
|
|
||
| export async function copyAdaptiveAction(data, externalApi) { | ||
| if (!data || !data.$type) return {}; | ||
|
|
||
| // Deep copy the original data. | ||
| const copy = JSON.parse(JSON.stringify(data)); | ||
yeze322 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const { updateDesigner } = externalApi; | ||
| // Create copy handler for rewriting fields which need to be handled specially. | ||
| const copyHandler = async data => { | ||
| updateDesigner(data); | ||
| if (needsOverride(data)) { | ||
| const overrider = OverriderByType[data.$type]; | ||
| await overrider(data, externalApi); | ||
| } | ||
yeze322 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| // Walk action and rewrite needs copy fields | ||
| await walkAdaptiveAction(copy, copyHandler); | ||
|
|
||
| return copy; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.