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
Fix fmt
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Sep 4, 2024
commit c4d4afb024934f842c3dad49ac4bb9776ecd7b5a
11 changes: 5 additions & 6 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationEvents.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import type { Meta, StoryObj } from "@storybook/react";
import { spyOn, userEvent, within } from "@storybook/test";
import { API } from "api/api";
import type { DeploymentValues } from "api/typesGenerated";
import { baseMeta } from "./storybookUtils";
import { NotificationEvents } from "./NotificationEvents";
import { selectTemplatesByGroup } from "api/queries/notifications";
import type { DeploymentValues } from "api/typesGenerated";
import { MockNotificationTemplates } from "testHelpers/entities";
import { NotificationEvents } from "./NotificationEvents";
import { baseMeta } from "./storybookUtils";

const meta: Meta<typeof NotificationEvents> = {
title: "pages/DeploymentSettings/NotificationsPage/NotificationEvents",
Expand All @@ -14,7 +14,7 @@ const meta: Meta<typeof NotificationEvents> = {
defaultMethod: "smtp",
availableMethods: ["smtp", "webhook"],
templatesByGroup: selectTemplatesByGroup(MockNotificationTemplates),
deploymentValues: baseMeta.parameters.deploymentValues
deploymentValues: baseMeta.parameters.deploymentValues,
},
...baseMeta,
};
Expand Down Expand Up @@ -60,7 +60,7 @@ export const Toggle: Story = {
const canvas = within(canvasElement);
const option = await canvas.findByText("Workspace Marked as Dormant");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should include the template ID as a data attribute and select on that? It'll be more stable than the name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, it doesn't matter too much since we control the data. I prefer to use the name in UI tests to match the user behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

We control it, yes, but we may rename this template, which will cause this test to fail mysteriously.
IDs will likely never be changed, so it's more resilient.

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This test won't fail if we rename the template because we're using mocked data. It's not an end-to-end test. I understand that using IDs makes the test more resilient, but this is a UI test in a controlled context, so I think we should test it as the user would. However, if you strongly prefer using IDs, we can switch to that approach and start a discussion with the front-end guild to establish it as a convention, since the community uses a different method. The goal is to test the UI as the user would.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but when you implement what we discussed in https://github.com/coder/coder/pull/14565/files#r1745353934 then it will go out of sync, and this is an entirely preventable problem.
I'm not part of the front-end guild so it's up to you.

const li = option.closest("li");
if(!li) {
if (!li) {
throw new Error("Could not find li");
}
const toggleButton = within(li).getByRole("button", {
Expand All @@ -69,4 +69,3 @@ export const Toggle: Story = {
await user.click(toggleButton);
},
};

10 changes: 7 additions & 3 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 @@ -9,8 +9,8 @@ import ToggleButton from "@mui/material/ToggleButton";
import ToggleButtonGroup from "@mui/material/ToggleButtonGroup";
import Tooltip from "@mui/material/Tooltip";
import {
updateNotificationTemplateMethod,
type selectTemplatesByGroup,
updateNotificationTemplateMethod,
} from "api/queries/notifications";
import type { DeploymentValues } from "api/typesGenerated";
import { Alert } from "components/Alert/Alert";
Expand Down Expand Up @@ -39,8 +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")
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}>
Expand Down
18 changes: 12 additions & 6 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationsPage.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import type { Meta, StoryObj } from "@storybook/react";
import { userEvent, within } from "@storybook/test";
import {
notificationDispatchMethodsKey,
systemNotificationTemplatesKey,
} from "api/queries/notifications";
import {
MockNotificationMethodsResponse,
MockNotificationTemplates,
} from "testHelpers/entities";
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 @@ -26,8 +32,8 @@ export const LoadingTemplates: Story = {
key: notificationDispatchMethodsKey,
data: MockNotificationMethodsResponse,
},
]
}
],
},
};

export const LoadingDispatchMethods: Story = {
Expand All @@ -38,8 +44,8 @@ export const LoadingDispatchMethods: Story = {
key: notificationDispatchMethodsKey,
data: undefined,
},
]
}
],
},
};

export const Events: Story = {};
Expand Down
5 changes: 1 addition & 4 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import {
} from "api/queries/notifications";
import { Loader } from "components/Loader/Loader";
import { TabLink, Tabs, TabsList } from "components/Tabs/Tabs";
import {
castNotificationMethod,
} from "modules/notifications/utils";
import { castNotificationMethod } from "modules/notifications/utils";
import { Section } from "pages/UserSettingsPage/Section";
import type { FC } from "react";
import { Helmet } from "react-helmet-async";
Expand Down Expand Up @@ -86,7 +84,6 @@ export const NotificationsPage: FC = () => {
);
};


export default NotificationsPage;

const styles = {
Expand Down
5 changes: 2 additions & 3 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
export const mockNotificationsDeploymentOptions: 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 @@ -213,5 +213,4 @@ export const baseMeta = {
withDashboardProvider,
withDeploySettings,
],
} satisfies Meta<typeof NotificationsPage>;

} satisfies Meta<typeof NotificationsPage>;
Loading