From f4ba9e8d692a78f0b776dec67b1bb26a1f679a7f Mon Sep 17 00:00:00 2001 From: TJ Durnford Date: Wed, 16 Sep 2020 17:06:10 -1000 Subject: [PATCH] feat: Add endpoint dropdown to the add skill modal (#4163) * feat: Add endpoint dropdown to the add skill modal * update duplicate criteria * prject id * updated dispatchers * fix test --- .../__tests__/components/skill.test.tsx | 414 +++++++++++++++--- .../src/components/CreateSkillModal.tsx | 406 ++++++++--------- .../client/src/pages/design/DesignPage.tsx | 15 +- .../client/src/pages/skills/index.tsx | 50 +-- .../client/src/pages/skills/skill-list.tsx | 161 +++---- .../dispatchers/__tests__/skill.test.ts | 69 ++- .../src/recoilModel/dispatchers/skill.ts | 99 +++-- .../server/src/models/bot/skillManager.ts | 15 +- .../src/BeginSkillDialogField.tsx | 10 +- 9 files changed, 765 insertions(+), 474 deletions(-) diff --git a/Composer/packages/client/__tests__/components/skill.test.tsx b/Composer/packages/client/__tests__/components/skill.test.tsx index dec5633a25..200a2e5fbd 100644 --- a/Composer/packages/client/__tests__/components/skill.test.tsx +++ b/Composer/packages/client/__tests__/components/skill.test.tsx @@ -2,24 +2,28 @@ // Licensed under the MIT License. import * as React from 'react'; -import { fireEvent, getByLabelText, getByTestId } from '@bfc/test-utils'; +import { act, fireEvent, getByLabelText, getByTestId, getByText } from '@bfc/test-utils'; import { Skill } from '@bfc/shared'; import httpClient from '../../src//utils/httpUtil'; -import Skills from '../../src/pages/skills'; import SkillList from '../../src/pages/skills/skill-list'; import { renderWithRecoil } from '../testUtils'; -import CreateSkillModal from '../../src/components/CreateSkillModal'; -import { settingsState } from '../../src/recoilModel'; +import CreateSkillModal, { + validateEndpoint, + validateManifestUrl, + validateName, +} from '../../src/components/CreateSkillModal'; +import { settingsState, projectIdState, skillsState } from '../../src/recoilModel'; +import Skills from '../../src/pages/skills'; jest.mock('../../src//utils/httpUtil'); jest.mock('../../src/components/Modal/dialogStyle', () => ({})); -const items: Skill[] = [ +const skills: Skill[] = [ { manifestUrl: 'https://yuesuemailskill0207-gjvga67.azurewebsites.net/manifest/manifest-1.0.json', - name: 'Email Skill', + 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', @@ -39,91 +43,369 @@ const items: Skill[] = [ }, ]; -const recoilInitState = ({ set }) => { - set(settingsState, { - luis: { - name: '', - authoringKey: '12345', - authoringEndpoint: 'testAuthoringEndpoint', - endpointKey: '12345', - endpoint: 'testEndpoint', - authoringRegion: 'westus', - defaultLanguage: 'en-us', - environment: 'composer', - }, - qna: { - subscriptionKey: '12345', - qnaRegion: 'westus', - endpointKey: '', - }, - }); -}; +let recoilInitState; describe('Skill page', () => { - it('can add a new skill', () => { - const { getByText } = renderWithRecoil(, recoilInitState); + beforeEach(() => { + recoilInitState = ({ set }) => { + set(projectIdState, '243245'); + set(skillsState, skills), + set(settingsState, { + luis: { + name: '', + authoringKey: '12345', + authoringEndpoint: 'testAuthoringEndpoint', + endpointKey: '12345', + endpoint: 'testEndpoint', + authoringRegion: 'westus', + defaultLanguage: 'en-us', + environment: 'composer', + }, + qna: { + subscriptionKey: '12345', + qnaRegion: 'westus', + endpointKey: '', + }, + }); + }; + }); + + it('can add a new skill', async () => { + const { baseElement } = renderWithRecoil(, recoilInitState); - const button = getByText('Connect to a new skill'); - fireEvent.click(button); + const button = getByText(baseElement, 'Connect to a new skill'); + act(() => { + fireEvent.click(button); + }); - const manifestUrl = getByLabelText(document.body, 'Manifest url'); + const manifestUrl = getByLabelText(baseElement, 'Manifest url'); expect(manifestUrl).toBeTruthy(); - const cancel = getByTestId(document.body, 'SkillFormCancel'); - fireEvent.click(cancel); + const cancel = getByTestId(baseElement, 'SkillFormCancel'); + act(() => { + fireEvent.click(cancel); + }); }); }); describe('', () => { it('should render the SkillList', () => { - const { container } = renderWithRecoil( - - ); - expect(container).toHaveTextContent('Email Skill'); + const { container } = renderWithRecoil(, recoilInitState); + expect(container).toHaveTextContent('Email-Skill'); expect(container).toHaveTextContent('Point Of Interest Skill'); }); - - it('can edit the skill', () => { - const onEdit = jest.fn(); - const { getAllByTestId } = renderWithRecoil( - - ); - - const editBtns = getAllByTestId('EditSkill'); - editBtns.forEach((btn, i) => { - fireEvent.click(btn); - expect(onEdit).toHaveBeenCalledWith(i); - }); - }); }); describe('', () => { - it('should render the skill form, and do update', () => { + it('should render the skill form, and update skill manifest url', () => { jest.useFakeTimers(); - const onSubmit = jest.fn((formData) => { - expect(formData.manifestUrl).toBe('http://AwesomeSkill'); - }); - (httpClient.post as jest.Mock).mockResolvedValue(undefined); + (httpClient.post as jest.Mock).mockResolvedValue({ endpoints: [] }); - const onDismiss = jest.fn(() => {}); + const onDismiss = jest.fn(); + const onSubmit = jest.fn(); const { getByLabelText, getByText } = renderWithRecoil( - + , + recoilInitState ); const urlInput = getByLabelText('Manifest url'); - expect(urlInput.getAttribute('value')).toBe(items[0].manifestUrl); - fireEvent.change(urlInput, { target: { value: 'http://AwesomeSkill' } }); + act(() => { + fireEvent.change(urlInput, { target: { value: skills[0].manifestUrl } }); + }); + + expect(urlInput.getAttribute('value')).toBe(skills[0].manifestUrl); const submitButton = getByText('Confirm'); - fireEvent.click(submitButton); + act(() => { + fireEvent.click(submitButton); + }); + expect(onSubmit).not.toBeCalled(); }); + + let formDataErrors; + let projectId; + let validationState; + let setFormDataErrors; + let setSkillManifest; + let setValidationState; + + beforeEach(() => { + formDataErrors = {}; + projectId = '123'; + validationState = {}; + setFormDataErrors = jest.fn(); + setSkillManifest = jest.fn(); + setValidationState = jest.fn(); + }); + + describe('validateManifestUrl', () => { + it('should set the error for an invalid url', async () => { + const formData = { manifestUrl: 'invalid url' }; + + await validateManifestUrl({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ manifestUrl: 'Url should start with http[s]://' }) + ); + expect(setSkillManifest).not.toBeCalled(); + expect(setValidationState).not.toBeCalled(); + }); + }); + + describe('validateManifestUrl', () => { + it('should set an error for duplicate skill manifest url', () => { + const formData = { + manifestUrl: 'https://yuesuemailskill0207-gjvga67.azurewebsites.net/MANIFEST/MANIFEST-1.0.json', + }; + + validateManifestUrl({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ manifestUrl: 'Duplicate skill manifest Url' }) + ); + expect(setSkillManifest).not.toBeCalled(); + expect(setValidationState).not.toBeCalled(); + }); + + it('should set an error for a missing manifest url', () => { + const formData = {}; + + validateManifestUrl({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith(expect.objectContaining({ manifestUrl: 'Please input a manifest Url' })); + }); + + it('should try and retrieve manifest if manifest url meets other criteria', async () => { + (httpClient.post as jest.Mock) = jest.fn().mockResolvedValue({ data: 'skill manifest' }); + + const formData = { manifestUrl: 'https://skill' }; + const formDataErrors = { manifestUrl: 'error' }; + + await validateManifestUrl({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, + }); + expect(setValidationState).toBeCalledWith( + expect.objectContaining({ + manifestUrl: 'Validating', + }) + ); + expect(httpClient.post).toBeCalledWith('/projects/123/skill/check', { url: formData.manifestUrl }); + expect(setSkillManifest).toBeCalledWith('skill manifest'); + expect(setValidationState).toBeCalledWith( + expect.objectContaining({ + manifestUrl: 'Validated', + }) + ); + expect(setFormDataErrors).toBeCalledWith( + expect.not.objectContaining({ + manifestUrl: 'error', + }) + ); + }); + + it('should show error when it could not retrieve skill manifest', async () => { + (httpClient.post as jest.Mock) = jest.fn().mockRejectedValue({ message: 'skill manifest' }); + + const formData = { manifestUrl: 'https://skill' }; + + await validateManifestUrl({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, + }); + expect(setValidationState).toBeCalledWith( + expect.objectContaining({ + manifestUrl: 'Validating', + }) + ); + expect(httpClient.post).toBeCalledWith('/projects/123/skill/check', { url: formData.manifestUrl }); + expect(setSkillManifest).not.toBeCalled(); + expect(setValidationState).toBeCalledWith( + expect.objectContaining({ + manifestUrl: 'NotValidated', + }) + ); + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ + manifestUrl: 'Manifest url can not be accessed', + }) + ); + }); + }); + + describe('validateName', () => { + it('should set error for invalid name', () => { + const formData = { name: 'Email Skill' }; + + validateName({ + formData, + formDataErrors, + skills, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ name: 'Name cannot include special characters or spaces' }) + ); + }); + + it('should set error for duplicate name', () => { + const formData = { name: 'email-skill' }; + + validateName({ + formData, + formDataErrors, + skills, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith(expect.objectContaining({ name: 'Duplicate skill name' })); + }); + }); + + describe('validateEndpoint', () => { + it('should set an error for missing msAppId', () => { + const formData = { endpointUrl: 'https://skill/api/messages' }; + + validateEndpoint({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ + endpoint: 'Please select a valid endpoint', + }) + ); + expect(setValidationState).not.toBeCalled(); + }); + + it('should set an error for missing endpointUrl', () => { + const formData = { msAppId: '00000000-0000-0000-0000-000000000000' }; + + validateEndpoint({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ + endpoint: 'Please select a valid endpoint', + }) + ); + expect(setValidationState).not.toBeCalled(); + }); + + it('should set an error for malformed msAppId', () => { + const formData = { endpointUrl: 'https://skill/api/messages', msAppId: 'malformed app id' }; + + validateEndpoint({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ + endpoint: 'Skill manifest endpoint is configured improperly', + }) + ); + expect(setValidationState).not.toBeCalled(); + }); + + it('should set an error for malformed endpointUrl', () => { + const formData = { endpointUrl: 'malformed endpoint', msAppId: '00000000-0000-0000-0000-000000000000' }; + + validateEndpoint({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.objectContaining({ + endpoint: 'Skill manifest endpoint is configured improperly', + }) + ); + expect(setValidationState).not.toBeCalled(); + }); + + it('should not set an error', () => { + const formData = { endpointUrl: 'https://skill/api/messages', msAppId: '00000000-0000-0000-0000-000000000000' }; + + validateEndpoint({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, + }); + + expect(setFormDataErrors).toBeCalledWith( + expect.not.objectContaining({ + endpoint: expect.any(String), + }) + ); + expect(setValidationState).toBeCalledWith( + expect.objectContaining({ + endpoint: 'Validated', + }) + ); + }); + }); }); diff --git a/Composer/packages/client/src/components/CreateSkillModal.tsx b/Composer/packages/client/src/components/CreateSkillModal.tsx index 01affe1bda..167892bcb8 100644 --- a/Composer/packages/client/src/components/CreateSkillModal.tsx +++ b/Composer/packages/client/src/components/CreateSkillModal.tsx @@ -1,265 +1,277 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -/** @jsx jsx */ -import { jsx, css } from '@emotion/core'; -import React, { useState, FormEvent, useEffect, useCallback, useRef } from 'react'; +import React, { useRef, useState, useMemo } from 'react'; import formatMessage from 'format-message'; +import { Dropdown, IDropdownOption } from 'office-ui-fabric-react/lib/Dropdown'; +import { Label } from 'office-ui-fabric-react/lib/Label'; import { PrimaryButton, DefaultButton } from 'office-ui-fabric-react/lib/Button'; import { Spinner, SpinnerSize } from 'office-ui-fabric-react/lib/Spinner'; import { Stack, StackItem } from 'office-ui-fabric-react/lib/Stack'; import { TextField } from 'office-ui-fabric-react/lib/TextField'; -import { assignDefined, Skill } from '@bfc/shared'; +import { useRecoilValue } from 'recoil'; import debounce from 'lodash/debounce'; -import { FontSizes } from 'office-ui-fabric-react/lib/Styling'; +import { Skill } from '@bfc/shared'; import { addSkillDialog } from '../constants'; import httpClient from '../utils/httpUtil'; +import { skillsState } from '../recoilModel'; import { DialogWrapper, DialogTypes } from './DialogWrapper'; -// -------------------- Styles -------------------- // - -const FormModalBody = css` - padding: 24px; -`; - -const FormFieldManifestUrl = css` - width: 40rem; -`; - -const FormFieldEditName = css` - width: 20rem; -`; - -const MarginLeftSmall = css` - margin-left: ${FontSizes.small}; -`; - -const SpinnerLabel = css` - justify-content: left; - margin-top: 0.4rem; -`; - -// -------------------- CreateSkillModal -------------------- // - -async function validateManifestUrl(projectId: string, manifestUrl: string): Promise { - // skip validation if there are other local errors. - if (manifestUrl == null || manifestUrl === '') { - return; - } - - try { - await httpClient.post(`/projects/${projectId}/skill/check`, { url: manifestUrl }); - } catch (err) { - return err.response?.data.message ?? err; - } -} - -export interface ISkillFormData { - manifestUrl: string; - name?: string; -} - -export interface ISkillFormDataErrors { +export interface SkillFormDataErrors { + endpoint?: string; manifestUrl?: string; - manifestUrlFetch?: string; name?: string; } -export const skillUrlRegex = /^http[s]?:\/\/\w+/; +export const urlRegex = /^http[s]?:\/\/\w+/; export const skillNameRegex = /^\w[-\w]*$/; +export const msAppIdRegex = /^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}$/; -export interface ICreateSkillModalProps { - isOpen: boolean; - editIndex?: number; - skills: Skill[]; +export interface CreateSkillModalProps { projectId: string; - onSubmit: (skillFormData: ISkillFormData, editIndex: number) => void; + onSubmit: (data: Skill) => void; onDismiss: () => void; } -const defaultFormData = { - manifestUrl: '', -}; - -export const CreateSkillModal: React.FC = (props) => { - const { editIndex = -1, skills, onSubmit, onDismiss, isOpen, projectId } = props; - const originFormData = skills[editIndex]; - const initialFormData = originFormData - ? assignDefined(defaultFormData, { manifestUrl: originFormData.manifestUrl, name: originFormData.name }) - : { ...defaultFormData }; - const [formData, setFormData] = useState(initialFormData); - const [formDataErrors, setFormDataErrors] = useState({}); - const [isValidating, setIsValidating] = useState(false); - - const isModify = editIndex >= 0 && editIndex < skills.length; - useEffect(() => { - setFormData(initialFormData); - }, [editIndex]); - - const asyncManifestUrlValidation = async (projectId: string, manifestUrl: string) => { - const err = await validateManifestUrl(projectId, manifestUrl); - if (err) { - setFormDataErrors((current) => ({ ...current, manifestUrl: err })); - } - setIsValidating(false); - }; - - const debouncedManifestValidation = useRef(debounce(asyncManifestUrlValidation, 300)).current; - - const validateForm = (newData: ISkillFormData): ISkillFormDataErrors => { - const errors: ISkillFormDataErrors = { ...formDataErrors }; - - if (newData.manifestUrl != null) { - const { manifestUrl } = newData; - - let currentError = ''; - if (manifestUrl) { - if (!skillUrlRegex.test(manifestUrl)) { - currentError = formatMessage('Url should start with http[s]://'); - } +enum ValidationState { + NotValidated = 'NotValidated', + Validating = 'Validating', + Validated = 'Validated', +} - const duplicatedItemIndex = skills.findIndex((item) => item.manifestUrl === manifestUrl); - if (duplicatedItemIndex !== -1 && (!isModify || (isModify && duplicatedItemIndex !== editIndex))) { - currentError = formatMessage('Duplicate skill manifest Url'); - } - } else { - currentError = formatMessage('Please input a manifest url'); - } +export const validateEndpoint = ({ + formData, + formDataErrors, + setFormDataErrors, + setValidationState, + validationState, +}) => { + const { msAppId, endpointUrl } = formData; + const { endpoint: _, ...errors } = formDataErrors; + + if (!msAppId || !endpointUrl) { + setFormDataErrors({ ...errors, endpoint: formatMessage('Please select a valid endpoint') }); + } else if (!urlRegex.test(endpointUrl) || !msAppIdRegex.test(msAppId)) { + setFormDataErrors({ ...errors, endpoint: formatMessage('Skill manifest endpoint is configured improperly') }); + } else { + setFormDataErrors(errors); + setValidationState({ ...validationState, endpoint: ValidationState.Validated }); + } +}; - if (currentError) { - errors.manifestUrl = currentError; - } else { - delete errors.manifestUrl; - } +export const validateManifestUrl = async ({ + formData, + formDataErrors, + projectId, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, +}) => { + const { manifestUrl } = formData; + const { manifestUrl: _, ...errors } = formDataErrors; + + if (!manifestUrl) { + setFormDataErrors({ ...errors, manifestUrl: formatMessage('Please input a manifest Url') }); + } else if (!urlRegex.test(manifestUrl)) { + setFormDataErrors({ ...errors, manifestUrl: formatMessage('Url should start with http[s]://') }); + } else if (skills.some((skill) => skill.manifestUrl.toLowerCase() === manifestUrl.toLowerCase())) { + setFormDataErrors({ ...errors, manifestUrl: formatMessage('Duplicate skill manifest Url') }); + } else { + try { + setValidationState({ ...validationState, manifestUrl: ValidationState.Validating }); + const { data } = await httpClient.post(`/projects/${projectId}/skill/check`, { url: manifestUrl }); + setFormDataErrors(errors); + setSkillManifest(data); + setValidationState({ ...validationState, manifestUrl: ValidationState.Validated }); + } catch (error) { + setFormDataErrors({ ...errors, manifestUrl: formatMessage('Manifest url can not be accessed') }); + setValidationState({ ...validationState, manifestUrl: ValidationState.NotValidated }); } + } +}; - if (newData.name != null) { - const { name } = newData; - let currentError = ''; - if (name) { - if (!skillNameRegex.test(name)) { - currentError = formatMessage('Name contains invalid charactors'); - } - - const duplicatedItemIndex = skills.findIndex((item) => item.name === name); - if (duplicatedItemIndex !== -1 && (!isModify || (isModify && duplicatedItemIndex !== editIndex))) { - currentError = formatMessage('Duplicate skill name'); - } - } - - if (currentError) { - errors.name = currentError; - } else { - delete errors.name; - } - } +export const validateName = ({ + formData, + formDataErrors, + skills, + setFormDataErrors, + setValidationState, + validationState, +}) => { + const { name } = formData; + const { name: _, ...errors } = formDataErrors; + + if (name && !skillNameRegex.test(name)) { + setFormDataErrors({ ...errors, name: formatMessage('Name cannot include special characters or spaces') }); + } else if (name && skills.some((skill) => skill.name.toLowerCase() === name.toLowerCase())) { + setFormDataErrors({ ...errors, name: formatMessage('Duplicate skill name') }); + } else { + setFormDataErrors(errors); + setValidationState({ ...validationState, name: ValidationState.Validated }); + } +}; - return errors; +export const CreateSkillModal: React.FC = ({ projectId, onSubmit, onDismiss }) => { + const skills = useRecoilValue(skillsState); + + const [formData, setFormData] = useState>({}); + const [formDataErrors, setFormDataErrors] = useState({}); + const [validationState, setValidationState] = useState({ + endpoint: ValidationState.NotValidated, + manifestUrl: ValidationState.NotValidated, + name: ValidationState.Validated, + }); + const [selectedEndpointKey, setSelectedEndpointKey] = useState(null); + const [skillManifest, setSkillManifest] = useState(null); + + const endpointOptions = useMemo(() => { + return (skillManifest?.endpoints || [])?.map(({ name, endpointUrl, msAppId }, key) => ({ + key, + text: name, + data: { + endpointUrl, + msAppId, + }, + })); + }, [skillManifest]); + + const debouncedValidateName = useRef(debounce(validateName, 500)).current; + const debouncedValidateManifestURl = useRef(debounce(validateManifestUrl, 500)).current; + + const validationHelpers = { + formDataErrors, + skills, + setFormDataErrors, + setValidationState, + setSkillManifest, + validationState, }; - // fetch manifest url - useEffect(() => { - if (!formDataErrors.manifestUrl) { - setIsValidating(true); - debouncedManifestValidation(projectId, formData.manifestUrl); - } - - () => { - debouncedManifestValidation.cancel(); - }; - }, [formData.manifestUrl, formDataErrors.manifestUrl]); + const handleManifestUrlChange = (_, manifestUrl = '') => { + const { msAppId, endpointUrl, ...rest } = formData; + setValidationState((validationState) => ({ + ...validationState, + manifestUrl: ValidationState.NotValidated, + endpoint: ValidationState.NotValidated, + })); + debouncedValidateManifestURl({ + formData: { ...rest, manifestUrl }, + projectId, + ...validationHelpers, + }); + setFormData({ + ...rest, + manifestUrl, + }); + setSkillManifest(null); + setSelectedEndpointKey(null); + }; - const updateForm = (field: string) => (e: FormEvent, newValue: string | undefined) => { - const newData: ISkillFormData = { + const handleNameChange = (_, name = '') => { + setValidationState((validationState) => ({ ...validationState, name: ValidationState.NotValidated })); + debouncedValidateName({ + formData: { ...formData, name }, + ...validationHelpers, + }); + setFormData({ ...formData, - [field]: newValue, - }; - setFormData(newData); - const errors = validateForm(newData); - setFormDataErrors(errors); + name, + }); }; - const handleSubmit = useCallback( - (e) => { - e.preventDefault(); - if (isValidating) return; - setIsValidating(true); - - const errors = validateForm(formData); - setFormDataErrors(errors); - if (Object.keys(errors).length > 0) { - setIsValidating(false); - return; - } - - // do async validation - validateManifestUrl(projectId, formData.manifestUrl).then((error) => { - if (error) { - setFormDataErrors((current) => ({ ...current, manifestUrl: error })); - setIsValidating(false); - return; - } - const newFormData = { ...formData }; - onSubmit(newFormData, editIndex); - setIsValidating(false); + const handleEndpointUrlChange = (_, option?: IDropdownOption) => { + if (option) { + const { data, key } = option; + validateEndpoint({ + formData: { + ...data, + ...formData, + }, + ...validationHelpers, }); - }, - [formData, formDataErrors] - ); + setFormData({ + ...data, + ...formData, + }); + setSelectedEndpointKey(key as number); + } + }; + + const handleSubmit = (event) => { + event.preventDefault(); - const formTitles = - editIndex === -1 ? { ...addSkillDialog.SKILL_MANIFEST_FORM } : { ...addSkillDialog.SKILL_MANIFEST_FORM_EDIT }; + if ( + Object.values(validationState).every((validation) => validation === ValidationState.Validated) && + !Object.values(formDataErrors).some(Boolean) + ) { + onSubmit(formData as Skill); + } + }; - const isDisabled = !Object.values(formData).some(Boolean) || Object.values(formDataErrors).some(Boolean); + const isDisabled = + !formData.manifestUrl || + Object.values(formDataErrors).some(Boolean) || + !Object.values(validationState).every((validation) => validation === ValidationState.Validated); return ( - -
+ + - {isValidating && ( + {validationState.manifestUrl === ValidationState.Validating && ( )} + + - + + - diff --git a/Composer/packages/client/src/pages/design/DesignPage.tsx b/Composer/packages/client/src/pages/design/DesignPage.tsx index 87bb9a8678..f1c0b12362 100644 --- a/Composer/packages/client/src/pages/design/DesignPage.tsx +++ b/Composer/packages/client/src/pages/design/DesignPage.tsx @@ -44,7 +44,6 @@ import { visualEditorSelectionState, focusPathState, showAddSkillDialogModalState, - skillsState, actionsSeedState, userSettingsState, localeState, @@ -121,13 +120,13 @@ const DesignPage: React.FC addSkillDialogCancel()} - onSubmit={handleAddSkillDialogSubmit} + onDismiss={addSkillDialogCancel} + onSubmit={(skill) => addSkill(projectId, skill)} /> )} {exportSkillModalVisible && ( diff --git a/Composer/packages/client/src/pages/skills/index.tsx b/Composer/packages/client/src/pages/skills/index.tsx index 2edfe8dd15..15d21378c0 100644 --- a/Composer/packages/client/src/pages/skills/index.tsx +++ b/Composer/packages/client/src/pages/skills/index.tsx @@ -7,24 +7,24 @@ 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 { skillsState, botNameState, settingsState, projectIdState, dispatcherState } from '../../recoilModel'; +import { botNameState, settingsState, projectIdState, dispatcherState } from '../../recoilModel'; import { Toolbar, IToolbarItem } from '../../components/Toolbar'; import { TestController } from '../../components/TestController/TestController'; -import { CreateSkillModal, ISkillFormData } from '../../components/CreateSkillModal'; +import { CreateSkillModal } from '../../components/CreateSkillModal'; import { ContainerStyle, ContentHeaderStyle, HeaderText } from './styles'; import SkillSettings from './skill-settings'; import SkillList from './skill-list'; const Skills: React.FC = () => { - const [editIndex, setEditIndex] = useState(); + const [showAddSkillDialogModal, setShowAddSkillDialogModal] = useState(false); const botName = useRecoilValue(botNameState); const settings = useRecoilValue(settingsState); const projectId = useRecoilValue(projectIdState); - const skills = useRecoilValue(skillsState); - const { setSettings, updateSkill } = useRecoilValue(dispatcherState); + const { addSkill, setSettings } = useRecoilValue(dispatcherState); const toolbarItems: IToolbarItem[] = [ { @@ -35,7 +35,7 @@ const Skills: React.FC = () => { iconName: 'Add', }, onClick: () => { - setEditIndex(-1); + setShowAddSkillDialogModal(true); }, }, align: 'left', @@ -47,33 +47,16 @@ const Skills: React.FC = () => { }, ]; - const onItemDelete = useCallback( - (index) => { - const payload = { - projectId, - targetId: index, - skillData: null, - }; - updateSkill(payload); - }, - [projectId] - ); - const onSubmitForm = useCallback( - (submitFormData: ISkillFormData, editIndex: number) => { - const payload = { - projectId, - targetId: editIndex, - skillData: submitFormData, - }; - updateSkill(payload); - setEditIndex(undefined); + (skill: Skill) => { + addSkill(projectId, skill); + setShowAddSkillDialogModal(false); }, [projectId] ); const onDismissForm = useCallback(() => { - setEditIndex(undefined); + setShowAddSkillDialogModal(false); }, []); return ( @@ -93,15 +76,10 @@ const Skills: React.FC = () => { skillHostEndpoint={settings.skillHostEndpoint as string | undefined} /> - setEditIndex(idx)} /> - + + {showAddSkillDialogModal && ( + + )} ); }; diff --git a/Composer/packages/client/src/pages/skills/skill-list.tsx b/Composer/packages/client/src/pages/skills/skill-list.tsx index 0cd7cece13..390b009efe 100644 --- a/Composer/packages/client/src/pages/skills/skill-list.tsx +++ b/Composer/packages/client/src/pages/skills/skill-list.tsx @@ -10,27 +10,22 @@ import { CheckboxVisibility, IColumn, } from 'office-ui-fabric-react/lib/DetailsList'; -import React, { useState, useCallback } from 'react'; +import React, { useState, useCallback, useMemo } from 'react'; +import { Dropdown, IDropdownOption } from 'office-ui-fabric-react/lib/Dropdown'; import { IconButton } from 'office-ui-fabric-react/lib/Button'; import { TooltipHost } from 'office-ui-fabric-react/lib/Tooltip'; import { ScrollablePane, ScrollbarVisibility } from 'office-ui-fabric-react/lib/ScrollablePane'; import { Sticky, StickyPositionType } from 'office-ui-fabric-react/lib/Sticky'; import { Stack } from 'office-ui-fabric-react/lib/Stack'; import { FontSizes } from '@uifabric/fluent-theme'; +import { useRecoilValue } from 'recoil'; import formatMessage from 'format-message'; -import { Skill } from '@bfc/shared'; import { DisplayManifestModal } from '../../components/Modal/DisplayManifestModal'; +import { dispatcherState, skillsState } from '../../recoilModel'; import { TableView, TableCell } from './styles'; -export interface ISkillListProps { - skills: Skill[]; - projectId: string; - onEdit: (index?: number) => void; - onDelete: (index?: number) => void; -} - const columns: IColumn[] = [ { key: 'name', @@ -40,20 +35,8 @@ const columns: IColumn[] = [ maxWidth: 150, isResizable: true, data: 'string', - onRender: (item: Skill) => { - return
{item.name}
; - }, - }, - { - key: 'msAppId', - name: formatMessage('App Id'), - fieldName: 'msAppId', - minWidth: 150, - maxWidth: 280, - isResizable: true, - data: 'string', - onRender: (item: Skill) => { - return
{item.msAppId}
; + onRender: ({ skill: { name } }) => { + return
{name}
; }, }, { @@ -64,8 +47,26 @@ const columns: IColumn[] = [ maxWidth: 400, isResizable: true, data: 'string', - onRender: (item: Skill) => { - return
{item.endpointUrl}
; + onRender: ({ skill, onEditSkill }) => { + const { endpoints, endpointUrl: selectedEndpointUrl } = skill; + + const options = (endpoints || []).map(({ name, endpointUrl, msAppId }, key) => ({ + key, + text: name, + data: { + endpointUrl, + msAppId, + }, + selected: endpointUrl === selectedEndpointUrl, + })); + + const handleChange = (_, option?: IDropdownOption) => { + if (option) { + onEditSkill({ ...skill, ...option.data }); + } + }; + + return ; }, }, { @@ -76,71 +77,79 @@ const columns: IColumn[] = [ maxWidth: 400, isResizable: true, data: 'string', - onRender: (item: Skill) => { - return
{item.description}
; + onRender: ({ skill: { description } }) => { + return
{description}
; + }, + }, + { + key: 'buttons', + name: '', + minWidth: 120, + maxWidth: 120, + fieldName: 'buttons', + data: 'string', + onRender: ({ onDelete, onViewManifest }) => { + return ( +
+ + onDelete()} + /> + onViewManifest()} + /> + +
+ ); }, }, ]; -const SkillList: React.FC = (props) => { - const { skills, projectId, onEdit, onDelete } = props; +interface SkillListProps { + projectId: string; +} + +const SkillList: React.FC = ({ projectId }) => { + const { removeSkill, updateSkill } = useRecoilValue(dispatcherState); + const skills = useRecoilValue(skillsState); const [selectedSkillUrl, setSelectedSkillUrl] = useState(null); - const onViewManifest = (item) => { + const handleViewManifest = (item) => { if (item && item.name && item.body) { setSelectedSkillUrl(item.manifestUrl); } }; + const handleEditSkill = (targetId) => (skillData) => { + updateSkill(projectId, { skillData, targetId }); + }; + + const items = useMemo( + () => + skills.map((skill, index) => ({ + skill, + onDelete: () => removeSkill(projectId, skill.manifestUrl), + onViewManifest: () => handleViewManifest(skill), + onEditSkill: handleEditSkill(index), + })), + [skills, projectId] + ); + const onDismissManifest = () => { setSelectedSkillUrl(null); }; - const getColumns = useCallback(() => { - return columns.concat({ - key: 'buttons', - name: '', - minWidth: 120, - maxWidth: 120, - fieldName: 'buttons', - data: 'string', - onRender: (item, index) => { - return ( -
- - onEdit(index)} - /> - onDelete(index)} - /> - onViewManifest(item)} - /> - -
- ); - }, - }); - }, [projectId]); - const onRenderDetailsHeader = useCallback((props, defaultRender) => { return (
@@ -161,8 +170,8 @@ const SkillList: React.FC = (props) => { { const mockDialogComplete = jest.fn(); const makeTestSkill: (number) => Skill = (n) => ({ - manifestUrl: 'url', + manifestUrl: 'url' + n, name: 'skill' + n, protocol: 'GET', description: 'test skill' + n, @@ -48,6 +49,7 @@ describe('skill dispatcher', () => { mockDialogComplete.mockClear(); const useRecoilTestHook = () => { + const projectId = useRecoilValue(projectIdState); const skillManifests = useRecoilValue(skillManifestsState); const onAddSkillDialogComplete = useRecoilValue(onAddSkillDialogCompleteState); const skills: Skill[] = useRecoilValue(skillsState); @@ -58,6 +60,7 @@ describe('skill dispatcher', () => { const currentDispatcher = useRecoilValue(dispatcherState); return { + projectId, skillManifests, onAddSkillDialogComplete, skills, @@ -85,6 +88,7 @@ describe('skill dispatcher', () => { { recoilState: settingsState, initialValue: {} }, { recoilState: showAddSkillDialogModalState, initialValue: false }, { recoilState: displaySkillManifestState, initialValue: undefined }, + { recoilState: projectIdState, initialValue: '123' }, ], dispatcher: { recoilState: dispatcherState, @@ -126,51 +130,32 @@ describe('skill dispatcher', () => { ]); }); - describe('updateSkill', () => { - it('adds a skill', async () => { - await act(async () => { - dispatcher.updateSkill({ - projectId: 'projectId', - targetId: -1, - skillData: makeTestSkill(3), - }); - }); - expect(renderedComponent.current.showAddSkillDialogModal).toBe(false); - expect(renderedComponent.current.onAddSkillDialogComplete.func).toBeUndefined(); - // expect(renderedComponent.current.settingsState.skill).toContain({ - // manifestUrl: 'url', - // name: 'skill3', - // }); - expect(renderedComponent.current.skills).toContainEqual(makeTestSkill(3)); + it('addsSkill', async () => { + await act(async () => { + dispatcher.addSkill('123', makeTestSkill(3)); }); - it('modifies a skill', async () => { - await act(async () => { - dispatcher.updateSkill({ - projectId: 'projectId', - targetId: 0, - skillData: makeTestSkill(100), - }); + expect(renderedComponent.current.showAddSkillDialogModal).toBe(false); + expect(renderedComponent.current.onAddSkillDialogComplete.func).toBeUndefined(); + expect(renderedComponent.current.skills).toContainEqual(makeTestSkill(3)); + }); + + it('updateSkill', async () => { + await act(async () => { + dispatcher.updateSkill('123', { + targetId: 0, + skillData: makeTestSkill(100), }); - expect(renderedComponent.current.showAddSkillDialogModal).toBe(false); - expect(renderedComponent.current.onAddSkillDialogComplete.func).toBeUndefined(); - // expect(renderedComponent.current.settingsState.skill).toContain({ - // manifestUrl: 'url', - // name: 'skill100', - // }); - expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1)); - expect(renderedComponent.current.skills).toContainEqual(makeTestSkill(100)); }); - it('deletes a skill', async () => { - await act(async () => { - dispatcher.updateSkill({ - projectId: 'projectId', - targetId: 0, - }); - }); - expect(renderedComponent.current.showAddSkillDialogModal).toBe(false); - expect(renderedComponent.current.onAddSkillDialogComplete.func).toBeUndefined(); - expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1)); + + expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1)); + expect(renderedComponent.current.skills).toContainEqual(makeTestSkill(100)); + }); + + it('removeSkill', async () => { + await act(async () => { + dispatcher.removeSkill('123', makeTestSkill(1).manifestUrl); }); + expect(renderedComponent.current.skills).not.toContain(makeTestSkill(1)); }); it('addSkillDialogBegin', async () => { diff --git a/Composer/packages/client/src/recoilModel/dispatchers/skill.ts b/Composer/packages/client/src/recoilModel/dispatchers/skill.ts index 2301a213e3..249c116075 100644 --- a/Composer/packages/client/src/recoilModel/dispatchers/skill.ts +++ b/Composer/packages/client/src/recoilModel/dispatchers/skill.ts @@ -3,7 +3,7 @@ /* eslint-disable react-hooks/rules-of-hooks */ import { CallbackInterface, useRecoilCallback } from 'recoil'; -import { SkillManifest, convertSkillsToDictionary } from '@bfc/shared'; +import { SkillManifest, convertSkillsToDictionary, Skill } from '@bfc/shared'; import httpClient from '../../utils/httpUtil'; @@ -39,50 +39,71 @@ export const skillDispatcher = () => { } ); - const updateSkill = useRecoilCallback( - (callbackHelpers: CallbackInterface) => async ({ projectId, targetId, skillData }) => { + const updateSkillState = async ( + callbackHelpers: CallbackInterface, + projectId: string, + updatedSkills: Skill[] + ): Promise => { + try { + const { set } = callbackHelpers; + + const { data: skills } = await httpClient.post(`/projects/${projectId}/skills/`, { skills: updatedSkills }); + + set(settingsState, (settings) => ({ + ...settings, + skill: convertSkillsToDictionary(skills), + })); + set(skillsState, skills); + + return skills; + } catch (error) { + logMessage(callbackHelpers, error.message); + } + }; + + const addSkill = useRecoilCallback( + (callbackHelpers: CallbackInterface) => async (projectId: string, skillData: Skill) => { const { set, snapshot } = callbackHelpers; - const onAddSkillDialogComplete = (await snapshot.getPromise(onAddSkillDialogCompleteState)).func; + const { func: onAddSkillDialogComplete } = await snapshot.getPromise(onAddSkillDialogCompleteState); + const skills = await updateSkillState(callbackHelpers, projectId, [ + ...(await snapshot.getPromise(skillsState)), + skillData, + ]); + + const skill = (skills || []).find(({ manifestUrl }) => manifestUrl === skillData.manifestUrl); + + if (typeof onAddSkillDialogComplete === 'function') { + onAddSkillDialogComplete(skill || null); + } + + set(showAddSkillDialogModalState, false); + set(onAddSkillDialogCompleteState, {}); + } + ); + + const removeSkill = useRecoilCallback( + (callbackHelpers: CallbackInterface) => async (projectId: string, manifestUrl?: string) => { + const { snapshot } = callbackHelpers; + const skills = [...(await snapshot.getPromise(skillsState))].filter((skill) => skill.manifestUrl !== manifestUrl); + await updateSkillState(callbackHelpers, projectId, skills); + } + ); + + const updateSkill = useRecoilCallback( + (callbackHelpers: CallbackInterface) => async ( + projectId: string, + { targetId, skillData }: { targetId: number; skillData?: any } + ) => { + const { snapshot } = callbackHelpers; const originSkills = [...(await snapshot.getPromise(skillsState))]; - // add - if (targetId === -1 && skillData) { - originSkills.push(skillData); - } else if (targetId >= 0 && targetId < originSkills.length) { - // modify - if (skillData) { - originSkills.splice(targetId, 1, skillData); - - // delete - } else { - originSkills.splice(targetId, 1); - } - // error + if (targetId >= 0 && targetId < originSkills.length && skillData) { + originSkills.splice(targetId, 1, skillData); } else { throw new Error(`update out of range, skill not found`); } - try { - const response = await httpClient.post(`/projects/${projectId}/skills/`, { skills: originSkills }); - - if (typeof onAddSkillDialogComplete === 'function') { - const skill = response.data.find(({ manifestUrl }) => manifestUrl === skillData.manifestUrl); - onAddSkillDialogComplete(skill ? skill : null); - } - - const skills = response.data; - - set(showAddSkillDialogModalState, false); - set(onAddSkillDialogCompleteState, { func: undefined }); - set(settingsState, (settings) => ({ - ...settings, - skill: convertSkillsToDictionary(skills), - })); - set(skillsState, skills); - } catch (err) { - //TODO: error - logMessage(callbackHelpers, err.message); - } + updateSkillState(callbackHelpers, projectId, originSkills); } ); @@ -115,6 +136,7 @@ export const skillDispatcher = () => { }); return { + addSkill, createSkillManifest, removeSkillManifest, updateSkillManifest, @@ -124,5 +146,6 @@ export const skillDispatcher = () => { addSkillDialogSuccess, displayManifestModal, dismissManifestModal, + removeSkill, }; }; diff --git a/Composer/packages/server/src/models/bot/skillManager.ts b/Composer/packages/server/src/models/bot/skillManager.ts index f15d3fceab..0fbfefdb06 100644 --- a/Composer/packages/server/src/models/bot/skillManager.ts +++ b/Composer/packages/server/src/models/bot/skillManager.ts @@ -17,7 +17,12 @@ const token = process.env.ACCESS_TOKEN || 'token'; const creds = new msRest.TokenCredentials(token); const client = new msRest.ServiceClient(creds, clientOptions); -export const getSkillByUrl = async (url: string, name?: string): Promise => { +export const getSkillByUrl = async ( + url: string, + name?: string, + msAppId?: string, + endpointUrl?: string +): Promise => { try { const req: msRest.RequestPrepareOptions = { url, @@ -36,9 +41,9 @@ export const getSkillByUrl = async (url: string, name?: string): Promise name: name || resBody?.name || '', description: resBody?.description || '', endpoints: get(resBody, 'endpoints', []), - endpointUrl: get(resBody, 'endpoints[0].endpointUrl', ''), // needs more invesment on endpoint + endpointUrl: endpointUrl || get(resBody, 'endpoints[0].endpointUrl', ''), // needs more investment on endpoint protocol: get(resBody, 'endpoints[0].protocol', ''), - msAppId: get(resBody, 'endpoints[0].msAppId', ''), + msAppId: msAppId || get(resBody, 'endpoints[0].msAppId', ''), body: res.bodyAsText, }; } catch (error) { @@ -52,9 +57,9 @@ export const extractSkillManifestUrl = async ( const skillsParsed: Skill[] = []; const diagnostics: Diagnostic[] = []; for (const skill of skills) { - const { manifestUrl, name } = skill; + const { manifestUrl, name, msAppId, endpointUrl } = skill; try { - const parsedSkill = await getSkillByUrl(manifestUrl, name); + const parsedSkill = await getSkillByUrl(manifestUrl, name, msAppId, endpointUrl); skillsParsed.push(parsedSkill); } catch (error) { const notify = new Diagnostic( diff --git a/Composer/packages/ui-plugins/select-skill-dialog/src/BeginSkillDialogField.tsx b/Composer/packages/ui-plugins/select-skill-dialog/src/BeginSkillDialogField.tsx index 4cb5857c09..df1af439d4 100644 --- a/Composer/packages/ui-plugins/select-skill-dialog/src/BeginSkillDialogField.tsx +++ b/Composer/packages/ui-plugins/select-skill-dialog/src/BeginSkillDialogField.tsx @@ -16,6 +16,11 @@ const referBySettings = (skillName: string, property: string) => { return `=settings.skill['${skillName}'].${property}`; }; +const settingReferences = (skillName: string) => ({ + skillEndpoint: referBySettings(skillName, 'endpointUrl'), + skillAppId: referBySettings(skillName, 'msAppId'), +}); + const handleBackwardCompatibility = (skills: Skill[], value): { name: string; endpointName: string } | undefined => { const { skillEndpoint } = value; const foundSkill = skills.find(({ manifestUrl }) => manifestUrl === value.id); @@ -107,8 +112,9 @@ export const BeginSkillDialogField: React.FC = (props) => { }; const onSkillSelectionChange = (option: IComboBoxOption | null) => { - if (option) { - setSelectedSkill(option?.text); + if (option?.text) { + setSelectedSkill(option.text); + onChange({ ...value, ...settingReferences(option.text) }); } };