Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(bot): bot name conflicts #1098

Merged
merged 5 commits into from
May 19, 2021
Merged

fix(bot): bot name conflicts #1098

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c73da6
fix(bot): add specific error message for bot name conflicts
IvanJobs May 18, 2021
9cfa15b
chore(bot): add log for manually updating message endpoint
IvanJobs May 18, 2021
dbab746
chore(bot): refine errors according to review
IvanJobs May 19, 2021
233f044
chore(bot): adapt ut cases
IvanJobs May 19, 2021
eaf559a
chore(bot): refine error design, name, reduce duplicated code
IvanJobs May 19, 2021
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
4 changes: 2 additions & 2 deletions packages/fx-core/src/plugins/resource/bot/appStudio/appStudio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { IAADDefinition } from "./interfaces/IAADDefinition";

import { AxiosInstance, default as axios } from "axios";
import {
CallAppStudioError,
AADAppCheckingError,
ConfigUpdatingError,
ProvisionError,
SomethingMissingError,
Expand Down Expand Up @@ -96,7 +96,7 @@ export class AppStudio {
axiosInstance.get(`${AppStudio.baseUrl}/api/aadapp/v2/${objectId}`)
);
} catch (e) {
throw new CallAppStudioError(LifecycleFuncNames.CHECK_AAD_APP, e);
throw new AADAppCheckingError(e);
}

if (!response || !response.data) {
Expand Down
7 changes: 7 additions & 0 deletions packages/fx-core/src/plugins/resource/bot/azureOps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ import {
MessageEndpointUpdatingError,
MissingSubscriptionRegistrationError,
FreeServerFarmsQuotaError,
BotNameRegisteredError,
} from "./errors";
import { CommonStrings, ConfigNames } from "./resources/strings";
import * as utils from "./utils/common";
import { default as axios } from "axios";
import { ErrorMessagesForChecking } from "./constants";
import { Messages } from "./resources/messages";

export class AzureOperations {
public static async CreateBotChannelRegistration(
Expand All @@ -38,6 +40,11 @@ export class AzureOperations {
} catch (e) {
if (e.code === "MissingSubscriptionRegistration") {
throw new MissingSubscriptionRegistrationError();
} else if (
e.code === "InvalidBotData" &&
e.message.includes(Messages.BotNameAlreadyRegistered)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the error message is changed from SDK?

) {
throw new BotNameRegisteredError();
} else {
throw new ProvisionError(CommonStrings.BOT_CHANNEL_REGISTRATION, e);
}
Expand Down
1 change: 1 addition & 0 deletions packages/fx-core/src/plugins/resource/bot/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export class ErrorNames {
public static readonly MISSING_SUBSCRIPTION_REGISTRATION_ERROR =
"MissingSubscriptionRegistrationError";
public static readonly FREE_SERVER_FARMS_QUOTA_ERROR = "FreeServerFarmsQuotaError";
public static readonly BOT_NAME_REGISTERED_ERROR = "BotNameRegisteredError";
}

export class Links {
Expand Down
54 changes: 33 additions & 21 deletions packages/fx-core/src/plugins/resource/bot/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class PluginError extends Error {

export class PreconditionError extends PluginError {
constructor(message: string, suggestions: string[]) {
super(ErrorType.System, ErrorNames.PRECONDITION_ERROR, message, suggestions);
super(ErrorType.User, ErrorNames.PRECONDITION_ERROR, message, suggestions);
}
}

Expand Down Expand Up @@ -71,12 +71,12 @@ export class UserInputsError extends PluginError {
}
}

export class CallAppStudioError extends PluginError {
constructor(apiName: string, innerError?: Error) {
export class AADAppCheckingError extends PluginError {
constructor(innerError?: Error) {
super(
ErrorType.System,
ErrorNames.CALL_APPSTUDIO_API_ERROR,
Messages.FailToCallAppStudio(apiName),
Messages.FailToCallAppStudioForCheckingAADApp,
[Messages.RetryTheCurrentStep],
innerError
);
Expand All @@ -86,10 +86,10 @@ export class CallAppStudioError extends PluginError {
export class ClientCreationError extends PluginError {
constructor(clientName: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.CLIENT_CREATION_ERROR,
Messages.FailToCreateSomeClient(clientName),
[Messages.RetryTheCurrentStep],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand All @@ -98,10 +98,10 @@ export class ClientCreationError extends PluginError {
export class ProvisionError extends PluginError {
constructor(resource: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.PROVISION_ERROR,
Messages.FailToProvisionSomeResource(resource),
[Messages.RetryTheCurrentStep],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand All @@ -123,10 +123,10 @@ export class MissingSubscriptionRegistrationError extends PluginError {
export class ConfigUpdatingError extends PluginError {
constructor(configName: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.CONFIG_UPDATING_ERROR,
Messages.FailToUpdateConfigs(configName),
[Messages.RetryTheCurrentStep],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand Down Expand Up @@ -157,10 +157,10 @@ export class PackDirExistenceError extends PluginError {
export class ListPublishingCredentialsError extends PluginError {
constructor(innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.LIST_PUBLISHING_CREDENTIALS_ERROR,
Messages.FailToListPublishingCredentials,
[Messages.RetryTheCurrentStep],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand All @@ -169,10 +169,10 @@ export class ListPublishingCredentialsError extends PluginError {
export class ZipDeployError extends PluginError {
constructor(innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.ZIP_DEPLOY_ERROR,
Messages.FailToDoZipDeploy,
["Please retry the deploy command."],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand All @@ -181,10 +181,10 @@ export class ZipDeployError extends PluginError {
export class MessageEndpointUpdatingError extends PluginError {
constructor(endpoint: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.MSG_ENDPOINT_UPDATING_ERROR,
Messages.FailToUpdateMessageEndpoint(endpoint),
[Messages.RetryTheCurrentStep],
[Messages.CheckOutputLogAndTryToFix, Messages.RetryTheCurrentStep],
innerError
);
}
Expand All @@ -193,7 +193,7 @@ export class MessageEndpointUpdatingError extends PluginError {
export class DownloadError extends PluginError {
constructor(url: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.DOWNLOAD_ERROR,
Messages.FailToDownloadFrom(url),
["Please check your network status and retry."],
Expand All @@ -205,7 +205,7 @@ export class DownloadError extends PluginError {
export class TemplateProjectNotFoundError extends PluginError {
constructor() {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.TEMPLATE_PROJECT_NOT_FOUND_ERROR,
Messages.SomethingIsNotFound("Template project for scaffold"),
[Messages.RetryTheCurrentStep]
Expand All @@ -214,11 +214,11 @@ export class TemplateProjectNotFoundError extends PluginError {
}

export class CommandExecutionError extends PluginError {
constructor(cmd: string, message: string, innerError?: Error) {
constructor(cmd: string, innerError?: Error) {
super(
ErrorType.System,
ErrorType.User,
ErrorNames.COMMAND_EXECUTION_ERROR,
Messages.CommandFailWithMessage(cmd, message),
Messages.CommandExecutionFailed(cmd),
[Messages.CheckCommandOutputAndTryToFixIt, Messages.RetryTheCurrentStep],
innerError
);
Expand All @@ -237,3 +237,15 @@ export class FreeServerFarmsQuotaError extends PluginError {
);
}
}

export class BotNameRegisteredError extends PluginError {
constructor(innerError?: Error) {
super(
ErrorType.User,
ErrorNames.BOT_NAME_REGISTERED_ERROR,
Messages.BotNameAlreadyRegistered,
[Messages.DeleteExistingBotChannelRegistration, Messages.DeleteBotAfterAzureAccountSwitching],
innerError
);
}
}
8 changes: 2 additions & 6 deletions packages/fx-core/src/plugins/resource/bot/languageStrategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ export class LanguageStrategy {
await utils.execute("npm install", packDir);
await utils.execute("npm run build", packDir);
} catch (e) {
throw new CommandExecutionError(
`${Commands.NPM_INSTALL},${Commands.NPM_BUILD}`,
e.message,
e
);
throw new CommandExecutionError(`${Commands.NPM_INSTALL},${Commands.NPM_BUILD}`, e);
}
}

Expand All @@ -107,7 +103,7 @@ export class LanguageStrategy {
// fail to npm install @microsoft/teamsfx on azure web app, so pack it locally.
await utils.execute("npm install @microsoft/teamsfx", packDir);
} catch (e) {
throw new CommandExecutionError(`${Commands.NPM_INSTALL}`, e.message, e);
throw new CommandExecutionError(`${Commands.NPM_INSTALL}`, e);
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions packages/fx-core/src/plugins/resource/bot/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,9 @@ export class TeamsBotImpl {
`Before running this bot, please manually update bot's message endpoint(${this.config.provision.siteEndpoint}${CommonStrings.MESSAGE_ENDPOINT_SUFFIX}). Click 'Get Help' button for more details.`,
Links.UPDATE_MESSAGE_ENDPOINT
);
Logger.info(
`Please manually update bot's message endpoint(${this.config.provision.siteEndpoint}${CommonStrings.MESSAGE_ENDPOINT_SUFFIX}).`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this sentence is the same as above, I'd like to have a variable to for it to remove duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

);
break;
}
}
Expand Down
18 changes: 13 additions & 5 deletions packages/fx-core/src/plugins/resource/bot/resources/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

export class Messages {
public static readonly SomethingIsInvalidWithValue = (something: string, value: string): string =>
`'${something}' is invalid with '${value}'.`;
`'${something}' is invalid with value '${value}'.`;
public static readonly InputValidValueForSomething = (something: string): string =>
`Please select valid values for '${something}'.`;
public static readonly SomethingIsMissing = (something: string): string =>
Expand Down Expand Up @@ -32,12 +32,12 @@ export class Messages {
"Please click 'Get Help' button for more details.";
public static readonly ClickIssueButtonToReportIssue =
"Please click 'Report Issue' button to report the issue.";
public static readonly CommandFailWithMessage = (command: string, message: string): string =>
`Run '${command}' failed with message: ${message}`;
public static readonly CommandExecutionFailed = (command: string): string =>
`Run '${command}' failed.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "Failed to run ${}" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok for me.

public static readonly DoSthBeforeSth = (sth: string, beforeSth: string): string =>
`Perform command '${sth}' before '${beforeSth}'.`;
public static readonly FailToCallAppStudio = (apiName: string): string =>
`Failed to execute '${apiName}'.`;
public static readonly FailToCallAppStudioForCheckingAADApp =
"Failed to call app studio's api to check aad app's existence.";
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to use "App Studio" which is aligned with official name? Same for "AAD application existence"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay for me.

public static readonly SuccessfullyRetrievedTemplateZip = (zipUrl: string): string =>
`Successfully retrieved zip package from ${zipUrl}.`;
public static readonly FallingBackToUseLocalTemplateZip =
Expand Down Expand Up @@ -96,6 +96,8 @@ export class Messages {
"The subscription didn't register to use namespace 'Microsoft.BotService'.";
public static readonly MaxFreeAppServicePlanIsTen =
"The maximum number of Free App Service Plan allowed in a Subscription is 10.";
public static readonly BotNameAlreadyRegistered =
"The bot name is already registered to another bot application.";

// Suggestions
public static readonly RetryTheCurrentStep = "Please retry the current step.";
Expand All @@ -107,4 +109,10 @@ export class Messages {
public static readonly RecreateTheProject = "Please recreate the project.";
public static readonly CheckCommandOutputAndTryToFixIt =
"Please check the command output and try to fix it.";
public static readonly DeleteExistingBotChannelRegistration =
"Please delete existing azure bot channel registrations.";
Copy link
Contributor

@YitongFeng-git YitongFeng-git May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is proper noun, better to use "Azure Bot Channels Registration", you can confirm with PM.
image

public static readonly DeleteBotAfterAzureAccountSwitching =
"If azure account is switched, don't forget to delete azure bot channel registration under the previous account.";
public static readonly CheckOutputLogAndTryToFix =
"Please check log in output channel and try to fix this issue.";
}