fix: [#9528] Unable to add two PVA bot skills through bot framework composer with the option connect to a skill #9549
Conversation
| // convert zip folder name to skill name | ||
| export const convertFolderNameToSkillName = (path, skillName) => { | ||
| return path.replace(/(\w+)\//, `${skillName}/`); | ||
| const hasFolder = path.match(/(\w+)\//); |
There was a problem hiding this comment.
nit: this doesn't allow hyphens or dashes in folder name
| }; | ||
|
|
||
| // convert zip folder name to skill name | ||
| export const convertFolderNameToSkillName = (path, skillName) => { |
There was a problem hiding this comment.
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);
};| import { addSkillFiles, deleteSkillFiles } from './utils/skills'; | ||
|
|
||
| const urlRegex = /^http[s]?:\/\/\w+/; | ||
| const isExternalLink = (url: string): boolean => { |
There was a problem hiding this comment.
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.
| 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 => { |
There was a problem hiding this comment.
suggestion: put the function to utilities and reuse here and in Composer/packages/client/src/recoilModel/dispatchers/botProjectFile.ts
…om/microsoft/BotFramework-Composer into southworks/fix/multiple-pva-skills
|
Hi @OEvgeny, all feedback applied. Thanks! |
| describe('Convert zip folder name to skill name', () => { | ||
| it('should return path with skillName folder', () => { | ||
| const path = 'manifest/empty-manifest.json'; | ||
| const path = 'manifest-folder/empty-manifest.json'; |
Description
This PR fixes the issue where Composer fails to connect to multiple PVA skills due to a validation error saying "The skill is already part of the Bot Project". This validation happened because a single manifest was saved in the
skillsfolder, and adding another PVA bot would replace it.Therefore, changing the folder structure to where the bot's manifest is saved solved the issue.
Other issues related to PVA skills were addressed:
.zipfile and no endpoint was available.manifest.jsonfile, it shows its content in a modal.projectIdto theuseMemofunction solved the issue..lgand.lufiles, failing to add the skill action trigger to the root bot.Task Item
Fixes #9528
Screenshots
The following images show the validation when adding an existing PVA skill and the folder structure, and the bots working as expected.
