fix: Improve LG response editor parsing for text/speech variations (m…

…icrosoft#7434) * fix * added comments * minor * lint * lint Co-authored-by: Soroush <[email protected]> Co-authored-by: TJ Durnford <[email protected]> Co-authored-by: Dong Lei <[email protected]>
lei9444 · Apr 28, 2021 · c114f64 · c114f64
1 parent 0b9eebf
commit c114f64
Show file tree

Hide file tree

Showing 7 changed files with 275 additions and 73 deletions.
diff --git a/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts b/Composer/packages/lib/code-editor/src/lg/hooks/useStringArray.ts
@@ -1,8 +1,10 @@
+/* eslint-disable security/detect-unsafe-regex */
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
 import { LgTemplate } from '@bfc/shared';
 import React from 'react';
+import { TemplateBodyItem, parseTemplateBody, templateBodyItemsToString } from '@bfc/shared';
 
 import { ArrayBasedStructuredResponseItem, PartialStructuredResponse } from '../types';
 import { getTemplateId } from '../../utils/structuredResponse';
@@ -14,28 +16,25 @@ const getInitialItems = <T extends ArrayBasedStructuredResponseItem>(
   response: T,
   lgTemplates?: readonly LgTemplate[],
   focusOnMount?: boolean
-): string[] => {
+): TemplateBodyItem[] => {
   const templateId = getTemplateId(response);
   const template = lgTemplates?.find(({ name }) => name === templateId);
   return response?.value && template?.body
-    ? template?.body
-        // Split by non-escaped -
-        // eslint-disable-next-line security/detect-unsafe-regex
-        ?.split(/(?<!\\)- /g)
-        // Ignore empty or newline strings
-        .filter((s) => s !== '' && s !== '\n')
-        .map((s) => s.replace(/\r?\n$/g, ''))
+    ? parseTemplateBody(template?.body)
         // Remove LG template multiline block symbol
-        .map((s) => s.replace(/```/g, '')) || []
-    : response?.value || (focusOnMount ? [''] : []);
+        .map((s) => ({ ...s, value: s.value.replace(/```/g, '') })) ?? []
+    : response?.value.map((v) => ({ kind: 'variation', value: v })) ||
+        (focusOnMount ? [{ kind: 'variation', value: '' }] : []);
 };
 
-const fixMultilineItems = (items: string[]) => {
+const fixMultilineItems = (items: TemplateBodyItem[]) => {
   return items.map((item) => {
-    if (/\r?\n/g.test(item)) {
-      // Escape all un-escaped -
-      // eslint-disable-next-line security/detect-unsafe-regex
-      return `${multiLineBlockSymbol}${item.replace(/(?<!\\)-/g, '\\-')}${multiLineBlockSymbol}`;
+    if (item.kind === 'variation' && /\r?\n/g.test(item.value)) {
+      return {
+        ...item,
+        // Escape all un-escaped -
+        value: `${multiLineBlockSymbol}${item.value.replace(/(?<!\\)-/g, '\\-')}${multiLineBlockSymbol}`,
+      };
     }
 
     return item;
@@ -62,10 +61,12 @@ export const useStringArray = <T extends ArrayBasedStructuredResponseItem>(
   const { lgOption, lgTemplates, focusOnMount } = options || {};
 
   const [templateId, setTemplateId] = React.useState(getTemplateId(structuredResponse));
-  const [items, setItems] = React.useState<string[]>(getInitialItems(structuredResponse, lgTemplates, focusOnMount));
+  const [items, setItems] = React.useState<TemplateBodyItem[]>(
+    getInitialItems(structuredResponse, lgTemplates, focusOnMount)
+  );
 
   const onChange = React.useCallback(
-    (newItems: string[]) => {
+    (newItems: TemplateBodyItem[]) => {
       setItems(newItems);
       // Fix variations that are multiline
       // If only one item but it's multiline, still use helper LG template
@@ -75,13 +76,13 @@ export const useStringArray = <T extends ArrayBasedStructuredResponseItem>(
         setTemplateId(id);
         onUpdateResponseTemplate({ [kind]: { kind, value: [], valueType: 'direct' } });
         onRemoveTemplate(id);
-      } else if (fixedNewItems.length === 1 && !/\r?\n/g.test(fixedNewItems[0]) && lgOption?.templateId) {
-        onUpdateResponseTemplate({ [kind]: { kind, value: fixedNewItems, valueType: 'direct' } });
+      } else if (fixedNewItems.length === 1 && !/\r?\n/g.test(fixedNewItems[0].value) && lgOption?.templateId) {
+        onUpdateResponseTemplate({ [kind]: { kind, value: [fixedNewItems[0].value], valueType: 'direct' } });
         onTemplateChange(id, '');
       } else {
         setTemplateId(id);
         onUpdateResponseTemplate({ [kind]: { kind, value: [`\${${id}()}`], valueType: 'template' } });
-        onTemplateChange(id, fixedNewItems.map((item) => `- ${item}`).join('\n'));
+        onTemplateChange(id, templateBodyItemsToString(fixedNewItems));
       }
     },
     [kind, newTemplateNameSuffix, lgOption, templateId, onRemoveTemplate, onTemplateChange, onUpdateResponseTemplate]

diff --git a/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx b/Composer/packages/lib/code-editor/src/lg/modalityEditors/StringArrayEditor.tsx
@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-import { LgTemplate, TelemetryClient } from '@bfc/shared';
+import { LgTemplate, TelemetryClient, TemplateBodyItem } from '@bfc/shared';
 import { FluentTheme } from '@uifabric/fluent-theme';
 import formatMessage from 'format-message';
 import { Callout, DirectionalHint } from 'office-ui-fabric-react/lib/Callout';
@@ -55,12 +55,12 @@ const getSSMLProps = (tag: 'prosody' | 'audio' | 'break'): string => {
 
 type StringArrayEditorProps = {
   addButtonText?: string;
-  items: string[];
+  items: TemplateBodyItem[];
   lgTemplates?: readonly LgTemplate[];
   memoryVariables?: readonly string[];
   lgOption?: LGOption;
   isSpeech?: boolean;
-  onChange: (items: string[]) => void;
+  onChange: (items: TemplateBodyItem[]) => void;
   telemetryClient: TelemetryClient;
 };
 
@@ -76,13 +76,15 @@ export const StringArrayEditor = React.memo(
   }: StringArrayEditorProps) => {
     const containerRef = useRef<HTMLDivElement | null>(null);
 
-    const [currentIndex, setCurrentIndex] = useState<number | null>(items.length === 1 && items[0] === '' ? 0 : null);
+    const [currentIndex, setCurrentIndex] = useState<number | null>(
+      items.length === 1 && items[0].value === '' ? 0 : null
+    );
     const [calloutTargetElement, setCalloutTargetElement] = useState<HTMLTextAreaElement | null>(null);
 
     const onItemChange = useCallback(
       (index: number) => (_, newValue?: string) => {
         const updatedItems = [...items];
-        updatedItems[index] = newValue ?? '';
+        updatedItems[index].value = newValue ?? '';
         onChange(updatedItems);
       },
       [items, onChange]
@@ -108,7 +110,7 @@ export const StringArrayEditor = React.memo(
     );
 
     const onClickAddVariation = useCallback(() => {
-      onChange([...items, '']);
+      onChange([...items, { kind: 'variation', value: '' }]);
       setCurrentIndex(items.length);
     }, [items, onChange]);
 
@@ -129,15 +131,15 @@ export const StringArrayEditor = React.memo(
           }
 
           setCalloutTargetElement(null);
-          // Filter our empty or newline strings
-          const filteredItems = items.filter((s) => s !== '' && s !== '\n');
+          // Filter out empty variations
+          const filteredItems = items.filter((item) => item.kind !== 'variation' || !!item.value);
           if (e.key === 'Enter' && containerRef.current?.contains(e.target as Node)) {
             // If the value is not filtered, go to the next entry
             // Otherwise cancel editing
             if (items.length === filteredItems.length) {
               e.preventDefault();
-              onChange([...filteredItems, '']);
-              setCurrentIndex(filteredItems.length);
+              onChange([...items, { kind: 'variation', value: '' }]);
+              setCurrentIndex(items.length);
             } else {
               onChange(filteredItems);
               setCurrentIndex(null);
@@ -167,8 +169,8 @@ export const StringArrayEditor = React.memo(
           setCalloutTargetElement(null);
           setCurrentIndex(null);
           // Remove empty variations only if necessary
-          if (items.some((item) => !item)) {
-            onChange(items.filter(Boolean));
+          if (items.some((item) => item.kind === 'variation' && !item.value)) {
+            onChange(items.filter((item) => item.kind !== 'variation' || !!item.value));
           }
         }
       };
@@ -194,15 +196,15 @@ export const StringArrayEditor = React.memo(
               typeof calloutTargetElement?.selectionEnd === 'number'
                 ? calloutTargetElement.selectionEnd
                 : calloutTargetElement.selectionStart;
-            const context = getCursorContextWithinLine(item.substring(0, start));
+            const context = getCursorContextWithinLine(item.value.substring(0, start));
             const insertText = context === 'expression' ? text : `\${${text}}`;
-            updatedItems[currentIndex] = [item.slice(0, start), insertText, item.slice(end)].join('');
+            updatedItems[currentIndex].value = [item.value.slice(0, start), insertText, item.value.slice(end)].join('');
             onChange(updatedItems);
 
             setTimeout(() => {
               calloutTargetElement.setSelectionRange(
-                updatedItems[currentIndex].length,
-                updatedItems[currentIndex].length
+                updatedItems[currentIndex].value.length,
+                updatedItems[currentIndex].value.length
               );
             }, 0);
           }
@@ -238,26 +240,26 @@ export const StringArrayEditor = React.memo(
                 typeof calloutTargetElement?.selectionEnd === 'number'
                   ? calloutTargetElement.selectionEnd
                   : calloutTargetElement.selectionStart;
-              updatedItems[currentIndex] = [
-                item.slice(0, start),
+              updatedItems[currentIndex].value = [
+                item.value.slice(0, start),
                 `<${ssmlTagType} ${getSSMLProps(ssmlTagType)}/>`,
-                item.slice(end),
+                item.value.slice(end),
               ].join('');
             } else {
-              updatedItems[currentIndex] = [
-                item.slice(0, start),
+              updatedItems[currentIndex].value = [
+                item.value.slice(0, start),
                 `<${ssmlTagType} ${getSSMLProps(ssmlTagType)}>`,
-                item.slice(start, end),
+                item.value.slice(start, end),
                 `</${ssmlTagType}>`,
-                item.slice(end),
+                item.value.slice(end),
               ].join('');
             }
             onChange(updatedItems);
 
             setTimeout(() => {
               calloutTargetElement.setSelectionRange(
-                updatedItems[currentIndex].length,
-                updatedItems[currentIndex].length
+                updatedItems[currentIndex].value.length,
+                updatedItems[currentIndex].value.length
               );
             }, 0);
           }
@@ -296,18 +298,20 @@ export const StringArrayEditor = React.memo(
 
     return (
       <div ref={containerRef}>
-        {items.map((value, idx) => (
-          <StringArrayItem
-            key={`item-${idx}`}
-            mode={idx === currentIndex ? 'edit' : 'view'}
-            telemetryClient={telemetryClient}
-            value={value}
-            onChange={onItemChange(idx)}
-            onFocus={onItemFocus(idx)}
-            onRemove={onItemRemove(idx)}
-            onShowCallout={onShowCallout}
-          />
-        ))}
+        {items.map((item, idx) =>
+          item.kind === 'variation' ? (
+            <StringArrayItem
+              key={`item-${idx}`}
+              mode={idx === currentIndex ? 'edit' : 'view'}
+              telemetryClient={telemetryClient}
+              value={item.value}
+              onChange={onItemChange(idx)}
+              onFocus={onItemFocus(idx)}
+              onRemove={onItemRemove(idx)}
+              onShowCallout={onShowCallout}
+            />
+          ) : null
+        )}
         {currentIndex === null && (
           <Link as="button" styles={styles.link} onClick={onClickAddVariation}>
             {addButtonText ?? formatMessage('Add new variation')}

diff --git a/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx b/Composer/packages/lib/code-editor/src/lg/modalityEditors/SuggestedActionsModalityEditor.tsx
@@ -3,6 +3,7 @@
 
 import formatMessage from 'format-message';
 import React from 'react';
+import { TemplateBodyItem } from '@bfc/shared';
 
 import { CommonModalityEditorProps, SuggestedActionsStructuredResponseItem } from '../types';
 
@@ -25,9 +26,10 @@ const SuggestedActionsModalityEditor = React.memo(
     const [items, setItems] = React.useState<string[]>(response?.value || []);
 
     const onChange = React.useCallback(
-      (newItems: string[]) => {
-        setItems(newItems);
-        onUpdateResponseTemplate({ SuggestedActions: { kind: 'SuggestedActions', value: newItems } });
+      (newItems: TemplateBodyItem[]) => {
+        const newValues = newItems.map((s) => s.value);
+        setItems(newValues);
+        onUpdateResponseTemplate({ SuggestedActions: { kind: 'SuggestedActions', value: newValues } });
       },
       [onUpdateResponseTemplate]
     );
@@ -45,7 +47,7 @@ const SuggestedActionsModalityEditor = React.memo(
       >
         <StringArrayEditor
           addButtonText={formatMessage('Add suggested action')}
-          items={items}
+          items={items.map((s) => ({ kind: 'variation', value: s }))}
           lgOption={lgOption}
           lgTemplates={lgTemplates}
           memoryVariables={memoryVariables}

diff --git a/Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts b/Composer/packages/lib/shared/__tests__/lgUtils/stringBuilders/lgStringUtils.test.ts
@@ -0,0 +1,90 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+import { parseTemplateBody } from '../../../src';
+
+const normalTemplateBody = `- variation0
+- variation1
+- variation2
+- variation3`;
+
+const parsedNormalTemplateBody = [
+  { kind: 'variation', value: 'variation0' },
+  { kind: 'variation', value: 'variation1' },
+  { kind: 'variation', value: 'variation2' },
+  { kind: 'variation', value: 'variation3' },
+];
+
+const templateBody = `> comment 0
+- variation1
+- variation2
+> comment 1
+- variation3
+- \`\`\` multiline
+variation
+- with dash
+\\- and escaped dash
+and normal text
+> and this comment!
+\\> and escaped comment
+\`\`\`
+
+
+> comment 2`;
+
+const parsedTemplateBody = [
+  { kind: 'comment', value: 'comment 0' },
+  { kind: 'variation', value: 'variation1' },
+  { kind: 'variation', value: 'variation2' },
+  { kind: 'comment', value: 'comment 1' },
+  { kind: 'variation', value: 'variation3' },
+  {
+    kind: 'variation',
+    value: `\`\`\` multiline
+variation
+- with dash
+\\- and escaped dash
+and normal text
+> and this comment!
+\\> and escaped comment
+\`\`\``,
+  },
+  { kind: 'newline', value: '' },
+  { kind: 'newline', value: '' },
+  { kind: 'comment', value: 'comment 2' },
+];
+
+const oneMultilineVariation = `- \`\`\` multiline
+variation
+- with dash
+\\- and escaped dash
+and normal text
+> and this comment!
+\\> and escaped comment
+\`\`\``;
+
+describe('lgStringUtils', () => {
+  it('Should parse template body with normal variations', () => {
+    const items = parseTemplateBody(normalTemplateBody);
+    expect(items.length).toBe(4);
+    expect(items).toEqual(parsedNormalTemplateBody);
+  });
+
+  it('Should parse template body with variations, multiline variations and comments', () => {
+    const items = parseTemplateBody(templateBody);
+    expect(items.length).toBe(9);
+    expect(items).toEqual(parsedTemplateBody);
+  });
+
+  it('Should parse template body with one multiline variation', () => {
+    const items = parseTemplateBody(oneMultilineVariation);
+    expect(items.length).toBe(1);
+    expect(items).toEqual([parsedTemplateBody[5]]);
+  });
+
+  it('Should return empty array for undefined template body', () => {
+    const items = parseTemplateBody(undefined);
+    expect(items.length).toBe(0);
+    expect(items).toEqual([]);
+  });
+});
diff --git a/Composer/packages/lib/shared/src/lgUtils/index.ts b/Composer/packages/lib/shared/src/lgUtils/index.ts
@@ -7,3 +7,4 @@ export { default as LgTemplateRef } from './models/LgTemplateRef';
 export { default as LgType } from './models/LgType';
 export { extractLgTemplateRefs } from './parsers/parseLgTemplateRef';
 export { LgNamePattern } from './parsers/lgPatterns';
+export { TemplateBodyItem, parseTemplateBody, templateBodyItemsToString } from './stringBuilders/lgStringUtils';