feat: migrate CARD actions to LG #28

tpmsr · 2020-01-08T20:46:53Z

Port and update of @coldplaying42 (Zongyang Ma's) changes to replace CARD actions with LG actions.

LG files should be in an lg directory parallel to the previous card directory.

Limitations (will be addressed in a future PR) :

Cards can currently only be rendered in WebChat view (eg, Train/Log dialog views)
- Clicking the eye / preview icon in action list returns an error message
- Clicking the eye / preview icon in "score actions" generates an exception
Bots imported from BF Composer have unhelpful action names like bfdactivity-12345
LG template parameters cannot currently be edited

Example thumbnail card :

Original changes :

Modifications from original changes :

Removed unused dependecies
Const correctness
Updated LG directory to lg rather than lgs
Updated LG directory function to return null if directory is not found, rather than ../../lg(s)
Minor code style changes

mattmazzola · 2020-01-08T23:47:14Z

packages/sdk/package.json

    "@types/supertest": "2.0.4",
    "async-file": "^2.0.2",
    "body-parser": "1.18.3",
    "botbuilder": "4.4.0",


I would expect these versions of botbuilder to all match, all be 4.7.0. Does botbuilder-lg fully work with 4.4?

I'll need to defer this question to @coldplaying42

There is only one public version 4.7.0 of botbuilder-lg, and botbuilder-lg depends on botbuilder-core pkg but not botbuilder pkg (notice that botbuilder also depends on botbuilder-core pkg ).

mattmazzola · 2020-01-08T23:49:36Z

packages/sdk/src/TemplateProvider.ts

+        entityDisplayValues.forEach((value: string, key: string) => { renderingParameters[key] = value })
+
+        // Load the template.  Currently, we assume that each lg file only has one template.
+        const lgFilename = templateDirectory + "//" + templateName + ".lg"


should construct paths using path library. path.join handles OS differences

Missed this one when going through the code port, thanks.

mattmazzola · 2020-01-08T23:53:56Z

packages/sdk/src/TemplateProvider.ts

+                templateBody = templateBody.replace("- ```\r\n", "").replace("```", "")
+                for (let symbol of engine.templates[1].parameters) {
+                    let sourceText = '@{' + symbol + '}'
+                    let replaceText = '{{' + symbol + '}}'


This looks suspicious. I thought the point of using LG was to adopt their templating language instead of using custom mustasch syntax. Does the library not handle this?

TBH, I find this chunk of code confusing also. I don't know why or in what contexts an LG filename would include AdaptiveCard (seems fragile), or why the @{} syntax needs to be changed to {{}}.

There's also an issue that AFAICT, the engine.templates[].parameters property is always empty. I've just been digging into this, and it appears to be why the UI for editing variables in the LG/CARD actions is never rendered.

@coldplaying42 - could you comment on these issues ?

This is the hacking code to render the adaptive card for eye/preview icon. The reason why @{} syntax needs to be changed to {{}} is that in lg file we use @{} to refer to variables (e.g., "text": "@{question}"), while in json file we use {{}} to refer to variables (e.g., "text": "{{question}}"). I made this change to enable AdaptiveCardViewer process the templateString.
https://github.com/microsoft/ConversationLearner-UI/blob/master/src/components/modals/AdaptiveCardViewer/AdaptiveCardViewer.tsx

However, this hacking code could only deal with the rendering of AdaptiveCard. In the future, AdaptiveCardViewer should be replaced with ActivityViewer, which can use some functions in https://github.com/microsoft/BotFramework-WebChat to render all the activities (e.g., thumbnail card, hero card, video card) that LG supports. And this chunk of code could be discarded.

@tpmsr you could try the parameters-lg in https://github.com/coldplaying42/ConversationLearner-Samples/tree/zy_lg/lgs

could you point me to the repo containing ActivityViewer please ?

I think starting from https://github.com/microsoft/BotFramework-WebChat/tree/master/packages/bundle/src/adaptiveCards might be helpful. I am not familiar with BotFramework-WebChat. You could contact with BotFramework-WebChat team by Vishwac. They could help you to solve the eye/preview rendering problem.

mattmazzola · 2020-01-08T23:56:55Z

packages/sdk/src/TemplateProvider.ts

-    public static async RenderTemplate(templateName: string, templateArguments: RenderedActionArgument[]): Promise<any | null> {
-        let template = this.GetTemplate(templateName)
-        if (template === null) {
+    public static async RenderTemplate(templateName: string, templateArguments: RenderedActionArgument[], entityDisplayValues: Map<string, string>): Promise<any | null> {


Could be Record instead of Map. If I remember these entity value types were done when maps were just introduced to JavaScript and wasn't clear when to apply.

I'd prefer to punt on this for this PR, since changing the method that populates this (getEntityDisplayValueMap in FilledEntity) would introduce several unrelated changes across Models, SDK, and UI.

tpmsr · 2020-01-09T20:44:35Z

packages/sdk/src/TemplateProvider.ts

+            if (file.includes('AdaptiveCard')) {
+                templateBody = engine.templates[1].body
+                templateBody = templateBody.replace("- ```\r\n", "").replace("```", "")
+                for (const symbol of engine.templates[1].parameters) {


@coldplaying42 - why is this rewrite only done for AdaptiveCard templates ?

The reason is that the format of other types of cards (e.g., hero, video, audio) is totally different with adaptivecard. For example, it looks like:
[HeroCard
title=Cheese gromit!
subtitle=@{type}
text=This is some text describing the card, it's cool because it's cool
image=https://memegenerator.net/img/instances/500x/73055378/cheese-gromit.jpg
buttons=Option 1| Option 2| Option 3
]
This format could not be directly parsed and rendered by AdaptiveCardViewer.

tpmsr · 2020-01-09T20:45:44Z

packages/sdk/src/TemplateProvider.ts

+            const fileName = path.join(templateDirectory, `${file}.lg`)
+            const engine = new TemplateEngine().addFile(fileName)
+            let templateBody = ''
+            if (file.includes('AdaptiveCard')) {


@coldplaying42 - in what conditions will filenames with this token be generated ? This seems fragile.

This is the hacking way to detect the lg file with adaptivecard. If the lg file has the adaptivecard body, templateBody will be filled and sent to AdaptiveCardViewer for parsing and rendering. I think if the ActivityViewer is implemented, this block of codes could be removed.

Thomas Park and others added 6 commits January 7, 2020 14:32

refactor: packages update after install + build

4e5d269

feat: synced zoma@'s changes for LG action rendering, updated to build

1936d3f

fix: don't return template directory if not found

b8eae03

refactor: const-ify

b322f4b

chore: remove unused dependencies

abf93ff

refactor: update name for renderingParams var

d9eb160

tpmsr added the needs-review PR Needs Review label Jan 8, 2020

tpmsr requested review from LarsLiden and mattmazzola January 8, 2020 21:04

tpmsr added 2 commits January 8, 2020 13:08

feat: port working version of prompt card over

05e5c94

refactor: remove deprecated cards from samples

833777a

mattmazzola approved these changes Jan 8, 2020

View reviewed changes

tpmsr added 2 commits January 9, 2020 09:20

chore: path join and more const

4792698

fix: add promptWithPicture LG

c83b599

tpmsr commented Jan 13, 2020

View reviewed changes

feat: migrate CARD actions to LG #28

Are you sure you want to change the base?

feat: migrate CARD actions to LG #28

Uh oh!

Conversation

tpmsr commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tpmsr commented Jan 8, 2020 •

edited

Loading