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__/setting.test.tsx

@@ -5,10 +5,13 @@ import { useRecoilValue } from 'recoil';
 import { act } from '@bfc/test-utils/lib/hooks';
 
 import { renderRecoilHook } from '../../../../__tests__/testUtils';
-import { settingsState, currentProjectIdState } from '../../atoms';
+import { settingsState, currentProjectIdState, skillsState } from '../../atoms';
 import { dispatcherState } from '../../../recoilModel/DispatcherWrapper';
 import { Dispatcher } from '..';
 import { settingsDispatcher } from '../setting';
+import httpClient from '../../../utils/httpUtil';
+
+jest.mock('../../../utils/httpUtil');
 
 const projectId = '1235a.2341';
 
@@ -65,17 +68,19 @@ const settings = {
     maxImbalanceRatio: 10,
     maxUtteranceAllowed: 15000,
   },
+  skill: {},
 };
 
 describe('setting dispatcher', () => {
   let renderedComponent, dispatcher: Dispatcher;
   beforeEach(() => {
     const useRecoilTestHook = () => {
       const settings = useRecoilValue(settingsState(projectId));
+      const skills = useRecoilValue(skillsState(projectId));
       const currentDispatcher = useRecoilValue(dispatcherState);
-
       return {
         settings,
+        skills,
         currentDispatcher,
       };
     };
@@ -84,6 +89,7 @@ describe('setting dispatcher', () => {
       states: [
         { recoilState: settingsState(projectId), initialValue: settings },
         { recoilState: currentProjectIdState, initialValue: projectId },
+        { recoilState: skillsState(projectId), initialValue: [] },
       ],
       dispatcher: {
         recoilState: dispatcherState,
@@ -151,4 +157,38 @@ describe('setting dispatcher', () => {
     });
     expect(renderedComponent.current.settings.runtime.customRuntime).toBeTruthy();
   });
+
+  it('should update skills state', async () => {
+    (httpClient.get as jest.Mock).mockResolvedValue({
+      data: { description: 'description', endpoints: [{ endpointUrl: 'https://test' }] },
+    });
+
+    await act(async () => {
+      await dispatcher.setSettings(projectId, {
+        skill: {
+          foo: {
+            msAppId: '00000000-0000',
+            endpointUrl: 'https://skill-manifest/api/messages',
+            name: 'foo',
+            manifestUrl: 'https://skill-manifest',
+          },
+        },
+      } as any);
+    });
+
+    expect(renderedComponent.current.skills).toEqual(
+      expect.arrayContaining([
+        {
+          id: 'foo',
+          name: 'foo',
+          manifestUrl: 'https://skill-manifest',
+          msAppId: '00000000-0000',
+          endpointUrl: 'https://skill-manifest/api/messages',
+          description: 'description',
+          endpoints: expect.any(Array),
+          content: expect.any(Object),
+        },
+      ])
+    );
+  });
 });

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);