fix: Unify skill and skill setting dispatchers

Composer/packages/client/__tests__/components/skill.test.tsx

@@ -22,24 +22,24 @@ jest.mock('../../src/components/Modal/dialogStyle', () => ({}));
 
 const skills: Skill[] = [
   {
+    id: 'email-skill',
+    content: {},
     manifestUrl: 'https://yuesuemailskill0207-gjvga67.azurewebsites.net/manifest/manifest-1.0.json',
     name: 'Email-Skill',
     description: 'The Email skill provides email related capabilities and supports Office and Google calendars.',
     endpointUrl: 'https://yuesuemailskill0207-gjvga67.azurewebsites.net/api/messages',
     msAppId: '79432da8-0f7e-4a16-8c23-ddbba30ae85d',
-    protocol: '',
     endpoints: [],
-    body: '',
   },
   {
+    id: 'point-of-interest-skill',
+    content: {},
     manifestUrl: 'https://hualxielearn2-snskill.azurewebsites.net/manifest/manifest-1.0.json',
     name: 'Point Of Interest Skill',
     description: 'The Point of Interest skill provides PoI search capabilities leveraging Azure Maps and Foursquare.',
     endpointUrl: 'https://hualxielearn2-snskill.azurewebsites.net/api/messages',
     msAppId: 'e2852590-ea71-4a69-9e44-e74b5b6cbe89',
-    protocol: '',
     endpoints: [],
-    body: '',
   },
 ];
 
@@ -101,7 +101,7 @@ describe('<SkillForm />', () => {
   it('should render the skill form, and update skill manifest url', () => {
     jest.useFakeTimers();
 
-    (httpClient.post as jest.Mock).mockResolvedValue({ endpoints: [] });
+    (httpClient.get as jest.Mock).mockResolvedValue({ endpoints: [] });
 
     const onDismiss = jest.fn();
     const onSubmit = jest.fn();
@@ -203,7 +203,7 @@ describe('<SkillForm />', () => {
     });
 
     it('should try and retrieve manifest if manifest url meets other criteria', async () => {
-      (httpClient.post as jest.Mock) = jest.fn().mockResolvedValue({ data: 'skill manifest' });
+      (httpClient.get as jest.Mock) = jest.fn().mockResolvedValue({ data: 'skill manifest' });
 
       const formData = { manifestUrl: 'https://skill' };
       const formDataErrors = { manifestUrl: 'error' };
@@ -223,7 +223,11 @@ describe('<SkillForm />', () => {
           manifestUrl: 'Validating',
         })
       );
-      expect(httpClient.post).toBeCalledWith(`/projects/${projectId}/skill/check`, { url: formData.manifestUrl });
+      expect(httpClient.get).toBeCalledWith(`/projects/${projectId}/skill/retrieve-skill-manifest`, {
+        params: {
+          url: formData.manifestUrl,
+        },
+      });
       expect(setSkillManifest).toBeCalledWith('skill manifest');
       expect(setValidationState).toBeCalledWith(
         expect.objectContaining({
@@ -238,7 +242,7 @@ describe('<SkillForm />', () => {
     });
 
     it('should show error when it could not retrieve skill manifest', async () => {
-      (httpClient.post as jest.Mock) = jest.fn().mockRejectedValue({ message: 'skill manifest' });
+      (httpClient.get as jest.Mock) = jest.fn().mockRejectedValue({ message: 'skill manifest' });
 
       const formData = { manifestUrl: 'https://skill' };
 
@@ -257,7 +261,11 @@ describe('<SkillForm />', () => {
           manifestUrl: 'Validating',
         })
       );
-      expect(httpClient.post).toBeCalledWith(`/projects/${projectId}/skill/check`, { url: formData.manifestUrl });
+      expect(httpClient.get).toBeCalledWith(`/projects/${projectId}/skill/retrieve-skill-manifest`, {
+        params: {
+          url: formData.manifestUrl,
+        },
+      });
       expect(setSkillManifest).not.toBeCalled();
       expect(setValidationState).toBeCalledWith(
         expect.objectContaining({

Composer/packages/client/src/components/CreateSkillModal.tsx

@@ -11,7 +11,7 @@ import { Stack, StackItem } from 'office-ui-fabric-react/lib/Stack';
 import { TextField } from 'office-ui-fabric-react/lib/TextField';
 import { useRecoilValue } from 'recoil';
 import debounce from 'lodash/debounce';
-import { Skill } from '@bfc/shared';
+import { SkillSetting } from '@bfc/shared';
 
 import { addSkillDialog } from '../constants';
 import httpClient from '../utils/httpUtil';
@@ -31,7 +31,7 @@ export const msAppIdRegex = /^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A
 
 export interface CreateSkillModalProps {
   projectId: string;
-  onSubmit: (data: Skill) => void;
+  onSubmit: (data: SkillSetting) => void;
   onDismiss: () => void;
 }
 
@@ -83,7 +83,11 @@ export const validateManifestUrl = async ({
   } else {
     try {
       setValidationState({ ...validationState, manifestUrl: ValidationState.Validating });
-      const { data } = await httpClient.post(`/projects/${projectId}/skill/check`, { url: manifestUrl });
+      const { data } = await httpClient.get(`/projects/${projectId}/skill/retrieve-skill-manifest`, {
+        params: {
+          url: manifestUrl,
+        },
+      });
       setFormDataErrors(errors);
       setSkillManifest(data);
       setValidationState({ ...validationState, manifestUrl: ValidationState.Validated });
@@ -118,7 +122,7 @@ export const validateName = ({
 export const CreateSkillModal: React.FC<CreateSkillModalProps> = ({ projectId, onSubmit, onDismiss }) => {
   const skills = useRecoilValue(skillsState(projectId));
 
-  const [formData, setFormData] = useState<Partial<Skill>>({});
+  const [formData, setFormData] = useState<Partial<SkillSetting>>({});
   const [formDataErrors, setFormDataErrors] = useState<SkillFormDataErrors>({});
   const [validationState, setValidationState] = useState({
     endpoint: ValidationState.NotValidated,
@@ -208,7 +212,7 @@ export const CreateSkillModal: React.FC<CreateSkillModalProps> = ({ projectId, o
       Object.values(validationState).every((validation) => validation === ValidationState.Validated) &&
       !Object.values(formDataErrors).some(Boolean)
     ) {
-      onSubmit(formData as Skill);
+      onSubmit({ name: skillManifest.name, ...formData } as SkillSetting);
     }
   };
 

Composer/packages/client/src/components/Modal/DisplayManifestModal.tsx

@@ -98,7 +98,7 @@ export const DisplayManifestModal: React.FC<DisplayManifestModalProps> = ({
           height={'100%'}
           id={'modaljsonview'}
           options={{ readOnly: true }}
-          value={JSON.parse(selectedSkill.body ?? '')}
+          value={selectedSkill.content}
           onChange={() => {}}
         />
       </div>

Composer/packages/client/src/pages/skills/index.tsx

@@ -7,7 +7,7 @@ import { RouteComponentProps } from '@reach/router';
 import React, { useCallback, useState } from 'react';
 import formatMessage from 'format-message';
 import { useRecoilValue } from 'recoil';
-import { Skill } from '@bfc/shared';
+import { SkillSetting } from '@bfc/shared';
 
 import { dispatcherState, settingsState, botNameState } from '../../recoilModel';
 import { Toolbar, IToolbarItem } from '../../components/Toolbar';
@@ -48,7 +48,7 @@ const Skills: React.FC<RouteComponentProps<{ projectId: string }>> = (props) =>
   ];
 
   const onSubmitForm = useCallback(
-    (skill: Skill) => {
+    (skill: SkillSetting) => {
       addSkill(projectId, skill);
       setShowAddSkillDialogModal(false);
     },

Composer/packages/client/src/pages/skills/skill-list.tsx

@@ -126,22 +126,22 @@ const SkillList: React.FC<SkillListProps> = ({ projectId }) => {
   const [selectedSkillUrl, setSelectedSkillUrl] = useState<string | null>(null);
 
   const handleViewManifest = (item) => {
-    if (item && item.name && item.body) {
+    if (item && item.name && item.content) {
       setSelectedSkillUrl(item.manifestUrl);
     }
   };
 
-  const handleEditSkill = (targetId) => (skillData) => {
-    updateSkill(projectId, { skillData, targetId });
+  const handleEditSkill = (projectId, skillId) => (skillData) => {
+    updateSkill(projectId, skillId, skillData);
   };
 
   const items = useMemo(
     () =>
-      skills.map((skill, index) => ({
+      skills.map((skill) => ({
         skill,
-        onDelete: () => removeSkill(projectId, skill.manifestUrl),
+        onDelete: () => removeSkill(projectId, skill.id),
         onViewManifest: () => handleViewManifest(skill),
-        onEditSkill: handleEditSkill(index),
+        onEditSkill: handleEditSkill(projectId, skill.id),
       })),
     [skills, projectId]
   );

Composer/packages/client/src/recoilModel/dispatchers/__tests__/skill.test.ts

@@ -35,14 +35,17 @@ const mockDialogComplete = jest.fn();
 const projectId = '42345.23432';
 
 const makeTestSkill: (number) => Skill = (n) => ({
+  id: 'id' + n,
   manifestUrl: 'url' + n,
   name: 'skill' + n,
-  protocol: 'GET',
   description: 'test skill' + n,
   endpointUrl: 'url',
   endpoints: [{ test: 'foo' }],
   msAppId: 'ID',
-  body: 'body',
+  content: {
+    description: 'test skill' + n,
+    endpoints: [{ test: 'foo' }],
+  },
 });
 
 describe('skill dispatcher', () => {
@@ -143,19 +146,30 @@ describe('skill dispatcher', () => {
 
   it('updateSkill', async () => {
     await act(async () => {
-      dispatcher.updateSkill(projectId, {
-        targetId: 0,
-        skillData: makeTestSkill(100),
+      dispatcher.updateSkill(projectId, 'id1', {
+        msAppId: 'test',
+        manifestUrl: 'test',
+        endpointUrl: 'test',
+        name: 'test',
       });
     });
 
-    expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1));
-    expect(renderedComponent.current.skills).toContainEqual(makeTestSkill(100));
+    expect(renderedComponent.current.skills[0]).toEqual(
+      expect.objectContaining({
+        id: 'id1',
+        content: {},
+        name: 'test',
+        msAppId: 'test',
+        manifestUrl: 'test',
+        endpointUrl: 'test',
+        endpoints: [],
+      })
+    );
   });
 
   it('removeSkill', async () => {
     await act(async () => {
-      dispatcher.removeSkill(projectId, makeTestSkill(1).manifestUrl);
+      dispatcher.removeSkill(projectId, makeTestSkill(1).id);
     });
     expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1));
   });
@@ -176,32 +190,6 @@ describe('skill dispatcher', () => {
     expect(renderedComponent.current.onAddSkillDialogComplete.func).toBe(undefined);
   });
 
-  describe('addSkillDialogSuccess', () => {
-    it('with a function in onAddSkillDialogComplete', async () => {
-      await act(async () => {
-        dispatcher.addSkillDialogBegin(mockDialogComplete, projectId);
-      });
-      await act(async () => {
-        dispatcher.addSkillDialogSuccess(projectId);
-      });
-      expect(mockDialogComplete).toHaveBeenCalledWith(null);
-      expect(renderedComponent.current.showAddSkillDialogModal).toBe(false);
-      expect(renderedComponent.current.onAddSkillDialogComplete.func).toBe(undefined);
-    });
-
-    it('with nothing in onAddSkillDialogComplete', async () => {
-      await act(async () => {
-        dispatcher.addSkillDialogCancel(projectId);
-      });
-      await act(async () => {
-        dispatcher.addSkillDialogSuccess(projectId);
-      });
-      expect(mockDialogComplete).not.toHaveBeenCalled();
-      expect(renderedComponent.current.showAddSkillDialogModal).toBe(false);
-      expect(renderedComponent.current.onAddSkillDialogComplete.func).toBe(undefined);
-    });
-  });
-
   it('displayManifestModal', async () => {
     await act(async () => {
       dispatcher.displayManifestModal('foo', projectId);

Composer/packages/client/src/recoilModel/dispatchers/project.ts

@@ -143,6 +143,7 @@ const initQnaFilesStatus = (projectId: string, qnaFiles: QnAFile[], dialogs: Dia
   );
   return updateQnaFilesStatus(projectId, qnaFiles);
 };
+
 export const projectDispatcher = () => {
   const initBotState = async (
     callbackHelpers: CallbackInterface,
@@ -152,14 +153,30 @@ export const projectDispatcher = () => {
     qnaKbUrls?: string[]
   ) => {
     const { snapshot, gotoSnapshot, set } = callbackHelpers;
-    const { files, botName, botEnvironment, location, schemas, settings, id: projectId, diagnostics, skills } = data;
+    const {
+      files,
+      botName,
+      botEnvironment,
+      location,
+      schemas,
+      settings,
+      id: projectId,
+      diagnostics,
+      skills: skillContent,
+    } = data;
     const curLocation = await snapshot.getPromise(locationState(projectId));
     const storedLocale = languageStorage.get(botName)?.locale;
     const locale = settings.languages.includes(storedLocale) ? storedLocale : settings.defaultLanguage;
 
     // cache current projectId in session, resolve page refresh caused state lost.
     projectIdCache.set(projectId);
 
+    const mergedSettings = mergeLocalStorage(projectId, settings);
+    if (Array.isArray(mergedSettings.skill)) {
+      const skillsArr = mergedSettings.skill.map((skillData) => ({ ...skillData }));
+      mergedSettings.skill = convertSkillsToDictionary(skillsArr);
+    }
+
     try {
       schemas.sdk.content = processSchema(projectId, schemas.sdk.content);
     } catch (err) {
@@ -169,10 +186,12 @@ export const projectDispatcher = () => {
     }
 
     try {
-      const { dialogs, dialogSchemas, luFiles, lgFiles, qnaFiles, skillManifestFiles } = indexer.index(
+      const { dialogs, dialogSchemas, luFiles, lgFiles, qnaFiles, skillManifestFiles, skills } = indexer.index(
         files,
         botName,
-        locale
+        locale,
+        skillContent,
+        mergedSettings
       );
 
       let mainDialog = '';
@@ -207,15 +226,6 @@ export const projectDispatcher = () => {
         set(botDiagnosticsState(projectId), diagnostics);
 
         refreshLocalStorage(projectId, settings);
-        const mergedSettings = mergeLocalStorage(projectId, settings);
-        if (Array.isArray(mergedSettings.skill)) {
-          const skillsArr = mergedSettings.skill.map((skillData) => {
-            return {
-              ...skillData,
-            };
-          });
-          mergedSettings.skill = convertSkillsToDictionary(skillsArr);
-        }
         set(settingsState(projectId), mergedSettings);
         set(filePersistenceState(projectId), new FilePersistence(projectId));
         set(undoHistoryState(projectId), new UndoHistory(projectId));