Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

fix: [#9528] Unable to add two PVA bot skills through bot framework composer with the option connect to a skill #9549

Merged
cwhitten merged 6 commits into from
Apr 27, 2023
Merged

fix: [#9528] Unable to add two PVA bot skills through bot framework composer with the option connect to a skill #9549

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6b1507
Add support for multiple PVA skills
sw-joelmut Mar 28, 2023
1a18e3a
Remove isExternalLink from httpUtil
ceciliaavila Apr 3, 2023
793982d
Merge branch 'main' into southworks/fix/multiple-pva-skills
ceciliaavila Apr 3, 2023
d4dbcc0
Apply feedback
ceciliaavila Apr 10, 2023
72cd093
Merge branch 'southworks/fix/multiple-pva-skills' of https://github.c…
ceciliaavila Apr 10, 2023
0e22f61
Merge branch 'main' into southworks/fix/multiple-pva-skills
cwhitten Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions Composer/packages/client/src/pages/design/ManifestEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { jsx, css } from '@emotion/react';
import formatMessage from 'format-message';
import ErrorBoundary from 'react-error-boundary';
import React from 'react';
import { useRecoilValue } from 'recoil';
import { useShellApi } from '@bfc/extension-client';
import { LoadingTimeout } from '@bfc/adaptive-form/lib/components/LoadingTimeout';
import { FieldLabel } from '@bfc/adaptive-form/lib/components/FieldLabel';
import ErrorInfo from '@bfc/adaptive-form/lib/components/ErrorInfo';
Expand All @@ -15,7 +17,7 @@ import { DetailsList, DetailsListLayoutMode, SelectionMode } from '@fluentui/rea
import { Link } from '@fluentui/react/lib/Link';
import get from 'lodash/get';

import { SkillInfo } from '../../recoilModel';
import { SkillInfo, skillNameIdentifierByProjectIdSelector } from '../../recoilModel';

import { formEditor } from './styles';
import { PropertyEditorHeader } from './PropertyEditorHeader';
Expand Down Expand Up @@ -74,6 +76,10 @@ const styles = {
const helpLink =
'https://docs.microsoft.com/en-us/azure/bot-service/skills-write-manifest-2-1?view=azure-bot-service-4.0';

const isExternalLink = (url: string): boolean => {

@OEvgeny OEvgeny Apr 3, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: put the function to utilities and reuse here and in Composer/packages/client/src/recoilModel/dispatchers/botProjectFile.ts

return /^http[s]?:\/\/\w+/.test(url);
};

export interface ManifestEditorProps {
formData: SkillInfo;
}
Expand All @@ -82,6 +88,12 @@ export const ManifestEditor: React.FC<ManifestEditorProps> = (props) => {
const { formData } = props;
const { manifest } = formData;

const skillsByProjectId = useRecoilValue(skillNameIdentifierByProjectIdSelector);
const { shellApi } = useShellApi();

const skillNameIdentifier = skillsByProjectId[formData.id];
const { displayManifestModal } = shellApi;

if (!manifest) {
return (
<LoadingTimeout timeout={2000}>
Expand Down Expand Up @@ -117,9 +129,13 @@ export const ManifestEditor: React.FC<ManifestEditorProps> = (props) => {
label={formatMessage('Manifest URL')}
/>
<p>
<Link aria-label={formData.location} href={formData.location} rel="noopener noreferrer" target="_blank">
{formData.location}
</Link>
{isExternalLink(formData.location) ? (
<Link aria-label={formData.location} href={formData.location} rel="noopener noreferrer" target="_blank">
{formData.location}
</Link>
) : (
<Link onClick={() => manifest && displayManifestModal(skillNameIdentifier)}>{formData.location}</Link>
)}
</p>
</section>
<section css={styles.section}>
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/pages/design/Modals.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const Modals: React.FC<ModalsProps> = ({ projectId = '', rootBotId = '' }) => {
await addRemoteSkillToBotProject(manifestUrl, endpointName, zipContent);
}}
addTriggerToRoot={async (dialogId, formData, skillId) => {
await createTriggerForRemoteSkill(projectId, dialogId, formData, skillId);
await createTriggerForRemoteSkill(rootBotId, dialogId, formData, skillId);
commitChanges();
}}
projectId={rootBotId}
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/pages/design/PropertyPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const PropertyPanel: React.FC<PropertyViewProps> = React.memo(({ projectId = '',

const skillNameIdentifier = skillsByProjectId[projectId];
return skills[skillNameIdentifier];
}, [skills, isSkill, skillsByProjectId]);
}, [projectId, skills, isSkill, skillsByProjectId]);

const shellForPropertyEditor = useShell('PropertyEditor', projectId);

Expand Down
6 changes: 4 additions & 2 deletions Composer/packages/client/src/recoilModel/dispatchers/botProjectFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import { rootBotProjectIdSelector } from '../selectors';
import { setRootBotSettingState } from './setting';
import { addSkillFiles, deleteSkillFiles } from './utils/skills';

const urlRegex = /^http[s]?:\/\/\w+/;
const isExternalLink = (url: string): boolean => {

@OEvgeny OEvgeny Apr 3, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

const isExternalLink = (url: string): boolean => {
    const { origin } = location;
    const parsedUrl = new URL(url, origin);
    return parsedUrl.origin !== origin;
};

Note that in case the URL provided is invalid, this will throw.

My only concern is about protocols limiting to http(s).

Also noted the implementation does not support relative protocol //. Though this is not supported in the code suggested as well, and I don't think it should be. Just raising the point in case there are known use-cases.

return /^http[s]?:\/\/\w+/.test(url);
};

export const botProjectFileDispatcher = () => {
const addLocalSkill = useRecoilCallback(({ set, snapshot }: CallbackInterface) => async (skillId: string) => {
Expand Down Expand Up @@ -52,7 +54,7 @@ export const botProjectFileDispatcher = () => {
}
const botName = await snapshot.getPromise(botNameIdentifierState(skillId));
let finalManifestUrl: string | undefined;
if (urlRegex.test(manifestUrl)) {
if (isExternalLink(manifestUrl)) {
finalManifestUrl = manifestUrl;
} else {
const data = await addSkillFiles(rootBotProjectId, botName, manifestUrl, zipContent);
Expand Down
7 changes: 7 additions & 0 deletions Composer/packages/client/src/recoilModel/dispatchers/project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import formatMessage from 'format-message';
import findIndex from 'lodash/findIndex';
import { PublishTarget, QnABotTemplateId, RootBotManagedProperties } from '@bfc/shared';
import get from 'lodash/get';
import camelCase from 'lodash/camelCase';
import { CallbackInterface, useRecoilCallback } from 'recoil';

import { BotStatus, CreationFlowStatus, FEEDVERSION } from '../../constants';
Expand Down Expand Up @@ -196,6 +197,11 @@ export const projectDispatcher = () => {
if (!rootBotProjectId) return;

const manifestFromZip = getManifestJsonFromZip(zipContent);
let botNameIdentifier;
if (manifestFromZip.content?.name) {
botNameIdentifier = camelCase(manifestFromZip.content.name);
manifestFromZip.name = `skills/${botNameIdentifier}/${manifestFromZip.name}`;
}
const botExists = await checkIfBotExistsInBotProjectFile(
callbackHelpers,
manifestFromZip.name ? manifestFromZip.name : manifestUrl,
Expand All @@ -213,6 +219,7 @@ export const projectDispatcher = () => {
manifestUrl,
manifestFromZip,
rootBotProjectId,
botNameIdentifier,
});
set(botProjectIdsState, (current) => [...current, projectId]);
await dispatcher.addRemoteSkillToBotProjectFile(projectId, manifestUrl, zipContent, endpointName);
Expand Down
2 changes: 1 addition & 1 deletion Composer/packages/client/src/recoilModel/utils/skill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const getManifestJsonFromZip = (zipContent) => {
try {
const manifestUrl = Object.keys(zipContent).find((key) => zipContent[key] && isManifestJson(zipContent[key]));
return manifestUrl
? { name: `skills/${manifestUrl}`, content: JSON.parse(zipContent[manifestUrl]) }
? { name: manifestUrl, content: JSON.parse(zipContent[manifestUrl]) }
: { name: '', content: null };
} catch (e) {
return { name: '', content: null };
Expand Down
8 changes: 7 additions & 1 deletion Composer/packages/server/src/models/utilities/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,11 @@ export const getRemoteFile = async (url): Promise<string> => {

// convert zip folder name to skill name
export const convertFolderNameToSkillName = (path, skillName) => {

@OEvgeny OEvgeny Apr 3, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion use path module to operate with paths. Not tested but something like this should work:

export const convertFolderNameToSkillName = (pathStr, skillName) => {
  const folderName = path.parse(pathStr).dir;
  const relativePath = path.relative(folderName, pathStr);
  
  return path.join(skillName, relativePath);
};

return path.replace(/(\w+)\//, `${skillName}/`);
const hasFolder = path.match(/(\w+)\//);

@OEvgeny OEvgeny Apr 3, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this doesn't allow hyphens or dashes in folder name


if (hasFolder === null) {
return `${skillName}/${path}`;
}

return path.replace(hasFolder[0], `${skillName}/`);
};