Apply fixes and improvements suggested by @dannykopping

BrunoQuaresma · BrunoQuaresma · commit f54c43591de4 · 2024-09-06T13:31:10.000Z
diff --git a/site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx b/site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx
@@ -23,7 +23,7 @@ export default meta;
 
 type Story = StoryObj<typeof NotificationEvents>;
 
-export const NoEmailSmarthost: Story = {
+export const SMTPNotConfigured: Story = {
 	args: {
 		deploymentValues: {
 			notifications: {
@@ -38,7 +38,7 @@ export const NoEmailSmarthost: Story = {
 	},
 };
 
-export const NoWebhookEndpoint: Story = {
+export const WebhookNotConfigured: Story = {
 	args: {
 		deploymentValues: {
 			notifications: {
@@ -47,6 +47,8 @@ export const NoWebhookEndpoint: Story = {
 				},
 				email: {
 					smarthost: "smtp.example.com",
+					from: "localhost",
+					hello: "localhost",
 				},
 			},
 		} as DeploymentValues,
@@ -58,7 +60,8 @@ export const Toggle: Story = {
 		spyOn(API, "updateNotificationTemplateMethod").mockResolvedValue();
 		const user = userEvent.setup();
 		const canvas = within(canvasElement);
-		const option = await canvas.findByText("Workspace Marked as Dormant");
+		const tmpl = MockNotificationTemplates[4];
+		const option = await canvas.findByText(tmpl.name);
 		const li = option.closest("li");
 		if (!li) {
 			throw new Error("Could not find li");
@@ -67,5 +70,27 @@ export const Toggle: Story = {
 			name: "Webhook",
 		});
 		await user.click(toggleButton);
+		await within(document.body).findByText("Notification method updated");
+	},
+};
+
+export const ToggleError: Story = {
+	play: async ({ canvasElement }) => {
+		spyOn(API, "updateNotificationTemplateMethod").mockRejectedValue({});
+		const user = userEvent.setup();
+		const canvas = within(canvasElement);
+		const tmpl = MockNotificationTemplates[4];
+		const option = await canvas.findByText(tmpl.name);
+		const li = option.closest("li");
+		if (!li) {
+			throw new Error("Could not find li");
+		}
+		const toggleButton = within(li).getByRole("button", {
+			name: "Webhook",
+		});
+		await user.click(toggleButton);
+		await within(document.body).findByText(
+			"Failed to update notification method",
+		);
 	},
 };
diff --git a/site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx b/site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx
@@ -8,13 +8,14 @@ import ListItemText, { listItemTextClasses } from "@mui/material/ListItemText";
 import ToggleButton from "@mui/material/ToggleButton";
 import ToggleButtonGroup from "@mui/material/ToggleButtonGroup";
 import Tooltip from "@mui/material/Tooltip";
+import { getErrorMessage } from "api/errors";
 import {
 	type selectTemplatesByGroup,
 	updateNotificationTemplateMethod,
 } from "api/queries/notifications";
 import type { DeploymentValues } from "api/typesGenerated";
 import { Alert } from "components/Alert/Alert";
-import { displaySuccess } from "components/GlobalSnackbar/utils";
+import { displayError, displaySuccess } from "components/GlobalSnackbar/utils";
 import { Stack } from "components/Stack/Stack";
 import {
 	type NotificationMethod,
@@ -39,58 +40,67 @@ export const NotificationEvents: FC<NotificationEventsProps> = ({
 	templatesByGroup,
 	deploymentValues,
 }) => {
+	// Webhook
 	const hasWebhookNotifications = Object.values(templatesByGroup)
 		.flat()
 		.some((t) => t.method === "webhook");
-	const hasEmailNotifications = Object.values(templatesByGroup)
+	const webhookValues = deploymentValues.notifications?.webhook ?? {};
+	const isWebhookConfigured = requiredFieldsArePresent(webhookValues, [
+		"endpoint",
+	]);
+
+	// SMTP
+	const hasSMTPNotifications = Object.values(templatesByGroup)
 		.flat()
 		.some((t) => t.method === "smtp");
+	const smtpValues = deploymentValues.notifications?.email ?? {};
+	const isSMTPConfigured = requiredFieldsArePresent(smtpValues, [
+		"smarthost",
+		"from",
+		"hello",
+	]);
 
 	return (
 		<Stack spacing={4}>
-			{hasWebhookNotifications &&
-				deploymentValues.notifications?.webhook.endpoint === "" && (
-					<Alert
-						severity="warning"
-						actions={
-							<Button
-								variant="text"
-								size="small"
-								component="a"
-								target="_blank"
-								rel="noreferrer"
-								href={docs("/cli/server#--notifications-webhook-endpoint")}
-							>
-								Read the docs
-							</Button>
-						}
-					>
-						Webhook notifications are enabled, but no endpoint has been
-						configured.
-					</Alert>
-				)}
+			{hasWebhookNotifications && !isWebhookConfigured && (
+				<Alert
+					severity="warning"
+					actions={
+						<Button
+							variant="text"
+							size="small"
+							component="a"
+							target="_blank"
+							rel="noreferrer"
+							href={docs("/admin/notifications#webhook")}
+						>
+							Read the docs
+						</Button>
+					}
+				>
+					Webhook notifications are enabled, but not properly configured.
+				</Alert>
+			)}
 
-			{hasEmailNotifications &&
-				deploymentValues.notifications?.email.smarthost === "" && (
-					<Alert
-						severity="warning"
-						actions={
-							<Button
-								variant="text"
-								size="small"
-								component="a"
-								target="_blank"
-								rel="noreferrer"
-								href={docs("/cli/server#--notifications-email-smarthost")}
-							>
-								Read the docs
-							</Button>
-						}
-					>
-						SMTP notifications are enabled, but no smarthost has been
-						configured.
-					</Alert>
-				)}
+			{hasSMTPNotifications && !isSMTPConfigured && (
+				<Alert
+					severity="warning"
+					actions={
+						<Button
+							variant="text"
+							size="small"
+							component="a"
+							target="_blank"
+							rel="noreferrer"
+							href={docs("/admin/notifications#smtp-email")}
+						>
+							Read the docs
+						</Button>
+					}
+				>
+					SMTP notifications are enabled but not properly configured.
+				</Alert>
+			)}
 
 			{Object.entries(templatesByGroup).map(([group, templates]) => (
 				<Card
@@ -131,6 +141,13 @@ export const NotificationEvents: FC<NotificationEventsProps> = ({
 	);
 };
 
+function requiredFieldsArePresent(
+	obj: Record<string, string | undefined>,
+	fields: string[],
+): boolean {
+	return fields.every((field) => Boolean(obj[field]));
+}
+
 type MethodToggleGroupProps = {
 	templateId: string;
 	options: NotificationMethod[];
@@ -155,10 +172,16 @@ const MethodToggleGroup: FC<MethodToggleGroupProps> = ({
 			aria-label="Notification method"
 			css={styles.toggleGroup}
 			onChange={async (_, method) => {
-				await updateMethodMutation.mutateAsync({
-					method,
-				});
-				displaySuccess("Notification method updated");
+				try {
+					await updateMethodMutation.mutateAsync({
+						method,
+					});
+					displaySuccess("Notification method updated");
+				} catch (error) {
+					displayError(
+						getErrorMessage(error, "Failed to update notification method"),
+					);
+				}
 			}}
 		>
 			{options.map((method) => {
diff --git a/site/src/pages/DeploySettingsPage/NotificationsPage/storybookUtils.ts b/site/src/pages/DeploySettingsPage/NotificationsPage/storybookUtils.ts
@@ -203,6 +203,8 @@ export const baseMeta = {
 				},
 				email: {
 					smarthost: "smtp.example.com",
+					from: "localhost",
+					hello: "localhost",
 				},
 			},
 		} as DeploymentValues,
