10000 fix(site): only show method warning if some template is using it by BrunoQuaresma · Pull Request #14565 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

fix(site): only show method warning if some template is using it #14565

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

Merged
merged 8 commits into from
Sep 6, 2024
Merged

fix(site): only show method warning if some template is using it #14565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a602c72
Fix lint error
BrunoQuaresma Sep 4, 2024
f58660e
Extract notification events to its own component
BrunoQuaresma Sep 4, 2024
8ac097c
Merge branch 'main' of https://github.com/coder/coder into bq/notific…
BrunoQuaresma Sep 4, 2024
20ccc3f
Only show alert if templates has the method enabled
BrunoQuaresma Sep 4, 2024
c4d4afb
Fix fmt
BrunoQuaresma Sep 4, 2024
f54c435
Apply fixes and improvements suggested by @dannykopping
BrunoQuaresma Sep 6, 2024
8cf8478
Update site/src/pages/DeploySettingsPage/NotificationsPage/Notificati…
BrunoQuaresma Sep 6, 2024
db9c136
Update storybookUtils.ts
BrunoQuaresma Sep 6, 2024
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
Prev Previous commit
Next Next commit
Only show alert if templates has the method enabled
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Sep 4, 2024
commit 20ccc3f1b13e894d938da15a8b37d06120d0e9d9
12 changes: 10 additions & 2 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,18 @@ import { API } from "api/api";
import type { DeploymentValues } from "api/typesGenerated";
import { baseMeta } 10000 from "./storybookUtils";
import { NotificationEvents } from "./NotificationEvents";
import { selectTemplatesByGroup } from "api/queries/notifications";
import { MockNotificationTemplates } from "testHelpers/entities";

const meta: Meta<typeof NotificationEvents> = {
title: "pages/DeploymentSettings/NotificationsPage/NotificationEvents",
component: NotificationEvents,
args: {
defaultMethod: "smtp",
availableMethods: ["smtp", "webhook"],
templatesByGroup: selectTemplatesByGroup(MockNotificationTemplates),
deploymentValues: baseMeta.parameters.deploymentValues
},
...baseMeta,
};

Expand All @@ -16,7 +24,7 @@ export default meta;
type Story = StoryObj<typeof NotificationEvents>;

export const NoEmailSmarthost: Story = {
parameters: {
args: {
deploymentValues: {
notifications: {
webhook: {
Expand All @@ -31,7 +39,7 @@ export const NoEmailSmarthost: Story = {
};

export const NoWebhookEndpoint: Story = {
parameters: {
args: {
deploymentValues: {
notifications: {
webhook: {
Expand Down
7 changes: 5 additions & 2 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ export const NotificationEvents: FC<NotificationEventsProps> = ({
templatesByGroup,
deploymentValues,
}) => {
const hasWebhookNotifications = Object.values(templatesByGroup).flat().some(t => t.method === "webhook")
const hasEmailNotifications = Object.values(templatesByGroup).flat().some(t => t.method === "smtp")

return (
<Stack spacing={4}>
{availableMethods.includes("smtp") &&
{hasWebhookNotifications &&
deploymentValues.notifications?.webhook.endpoint === "" && (
<Alert
severity="warning"
Expand All @@ -63,7 +66,7 @@ export const NotificationEvents: FC<NotificationEventsProps> = ({
</Alert>
)}

{availableMethods.includes("smtp") &&
{hasEmailNotifications &&
deploymentValues.notifications?.email.smarthost === "" && (
<Alert
severity="warning"
Expand Down
29 changes: 29 additions & 0 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationsPage.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type { Meta, StoryObj } from "@storybook/react";
import { userEvent, within } from "@storybook/test";
import { NotificationsPage } from "./NotificationsPage";
import { baseMeta } from "./storybookUtils";
import { notificationDispatchMethodsKey, systemNotificationTemplatesKey } from "api/queries/notifications";
import { MockNotificationMethodsResponse, MockNotificationTemplates } from "testHelpers/entities";

const meta: Meta<typeof NotificationsPage> = {
title: "pages/DeploymentSettings/NotificationsPage",
Expand All @@ -13,6 +15,33 @@ export default meta;

type Story = StoryObj<typeof NotificationsPage>;

export const LoadingTemplates: Story = {
parameters: {
queries: [
{
key: systemNotificationTemplatesKey,
data: undefined,
},
{
key: notificationDispatchMethodsKey,
data: MockNotificationMethodsResponse,
},
]
}
};

export const LoadingDispatchMethods: Story = {
parameters: {
queries: [
{ key: systemNotificationTemplatesKey, data: MockNotificationTemplates },
{
key: notificationDispatchMethodsKey,
data: undefined,
},
]
}
};

export const Events: Story = {};

export const Settings: Story = {
Expand Down
4 changes: 2 additions & 2 deletions site/src/pages/DeploySettingsPage/NotificationsPage/storybookUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
import type { NotificationsPage } from "./NotificationsPage";

// Extracted from a real API response
const mockedOptions: SerpentOption[] = [
export const mockNotificationsDeploymentOptions: SerpentOption[] = [
{
name: "Notifications: Dispatch Timeout",
Copy link
Contributor

Choose a reason for hiding this comment

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

This will go out of sync; is there a way we can lint this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only check we have here is the type, as we do for the other mocks as well, but I know it is not sufficient. I've been thinking about how we could improve our mocks and keep them in sync. One option that came to mind was to use a script that communicates with the API and generates the mocks. However, this would require more investigation and is beyond the scope of this pull request. What do you think about this idea? Do you see a simpler way we could achieve better results?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfectly fine to do as a follow-up, yup. Your idea sounds good 👍

description:
Expand Down Expand Up @@ -195,7 +195,7 @@ export const baseMeta = {
],
user: MockUser,
permissions: { viewDeploymentValues: true },
deploymentOptions: mockedOptions,
deploymentOptions: mockNotificationsDeploymentOptions,
deploymentValues: {
notifications: {
webhook: {
Expand Down
0