Skip to content

feat(site): implement notification ui #14175

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 34 commits into from
Aug 9, 2024
Merged

feat(site): implement notification ui #14175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
fc282e7
Add base notification settings page to deployment
BrunoQuaresma Jul 29, 2024
997e0d3
Add base notifications components
BrunoQuaresma Jul 29, 2024
73f8471
Bind notifications into the user account notifications page
BrunoQuaresma Aug 2, 2024
33c9ab0
Remove deployment notifications page
BrunoQuaresma Aug 2, 2024
bbc8cbb
Add test for toggling notifications
BrunoQuaresma Aug 2, 2024
7227a42
Update migration
BrunoQuaresma Aug 5, 2024
788de88
Update template notification methods
BrunoQuaresma Aug 5, 2024
1262f7b
Fix types
BrunoQuaresma Aug 5, 2024
e449e3d
Fix remaining type issues
BrunoQuaresma Aug 5, 2024
0154b94
Experience improvements
BrunoQuaresma Aug 5, 2024
aedf159
Fix validation
BrunoQuaresma Aug 5, 2024
1a2efae
Remove BE changes
BrunoQuaresma Aug 6, 2024
a1f363c
Fix FE types
BrunoQuaresma Aug 6, 2024
7412eb9
Fix notifications permissions
BrunoQuaresma Aug 6, 2024
1ff0973
Display webhook info
BrunoQuaresma Aug 6, 2024
2acde04
Merge branch 'main' of https://github.com/coder/coder into bq/user-no…
BrunoQuaresma Aug 6, 2024
7cc7bdb
Add tests to the notifications page
BrunoQuaresma Aug 6, 2024
4956409
Remove unecessary migration
BrunoQuaresma Aug 6, 2024
1c62242
Don't show deployment wide method
BrunoQuaresma Aug 6, 2024
a020619
Fix templates sorting
BrunoQuaresma Aug 6, 2024
0efc40d
Add nav tabs
BrunoQuaresma Aug 6, 2024
1aedb92
Update titles
BrunoQuaresma Aug 6, 2024
786b005
Add tests
BrunoQuaresma Aug 6, 2024
2ecbe5f
Improve product copy
BrunoQuaresma Aug 7, 2024
ec7ab40
Fix notifications visibility
BrunoQuaresma Aug 7, 2024
c986e51
Minor improvements
BrunoQuaresma Aug 7, 2024
e037423
Remove alerts
BrunoQuaresma Aug 8, 2024
341f550
Add alerts when SMTP or Webhook config are enabled but not set
BrunoQuaresma Aug 8, 2024
d97dd82
Apply a few Michaels suggestions
BrunoQuaresma Aug 8, 2024
4399cd6
Merge branch 'main' of https://github.com/coder/coder into bq/user-no…
BrunoQuaresma Aug 9, 2024
d403eed
Simplify state logic for the switch component
BrunoQuaresma Aug 9, 2024
fb02aec
Update copy
BrunoQuaresma Aug 9, 2024
013ccff
Apply PR comments
BrunoQuaresma Aug 9, 2024
7858a5a
Add docs
BrunoQuaresma Aug 9, 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
Add nav tabs
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Aug 6, 2024
commit 0efc40d753cb1cf1202bff25ae2d3d01985dbb61
181 changes: 100 additions & 81 deletions site/src/pages/DeploySettingsPage/NotificationsPage/NotificationsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import type { Interpolation, Theme } from "@emotion/react";
import AlertTitle from "@mui/material/AlertTitle";
import Button from "@mui/material/Button";
import Card from "@mui/material/Card";
import Divider from "@mui/material/Divider";
import List from "@mui/material/List";
Expand All @@ -11,25 +9,29 @@ import ToggleButtonGroup from "@mui/material/ToggleButtonGroup";
import Tooltip from "@mui/material/Tooltip";
import { Fragment, type FC } from "react";
import { useMutation, useQueries, useQueryClient } from "react-query";
import { useSearchParams } from "react-router-dom";
import {
notificationDispatchMethods,
selectTemplatesByGroup,
systemNotificationTemplates,
updateNotificationTemplateMethod,
} from "api/queries/notifications";
import { Alert, AlertDetail } from "components/Alert/Alert";
import type { NotificationsConfig } from "api/typesGenerated";
import { Alert } from "components/Alert/Alert";
import { displaySuccess } from "components/GlobalSnackbar/utils";
import { Loader } from "components/Loader/Loader";
import { Stack } from "components/Stack/Stack";
import { useClipboard } from "hooks";
import { TabLink, Tabs, TabsList } from "components/Tabs/Tabs";
import {
castNotificationMethod,
methodIcons,
methodLabels,
type NotificationMethod,
} from "modules/notifications/utils";
import { Section } from "pages/UserSettingsPage/Section";
import { deploymentGroupHasParent } from "utils/deployOptions";
import { useDeploySettings } from "../DeploySettingsLayout";
import OptionsTable from "../OptionsTable";

type MethodToggleGroupProps = {
templateId: string;
Expand Down Expand Up @@ -89,6 +91,7 @@ const MethodToggleGroup: FC<MethodToggleGroupProps> = ({
};

export const NotificationsPage: FC = () => {
const [searchParams] = useSearchParams();
const { deploymentValues } = useDeploySettings();
const [templatesByGroup, dispatchMethods] = useQueries({
queries: [
Expand All @@ -99,104 +102,120 @@ export const NotificationsPage: FC = () => {
notificationDispatchMethods(),
],
});
const ready = templatesByGroup.data && dispatchMethods.data;

const isUsingWebhook = dispatchMethods.data?.available.includes("webhook");
const webhookEndpoint =
deploymentValues?.config.notifications?.webhook.endpoint;
const ready =
templatesByGroup.data && dispatchMethods.data && deploymentValues;
const tab = searchParams.get("tab") || "events";
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this might be a good use case for useSearchParamsKey? It'd have to be changed a little bit to account for the || instead of the ??, though

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it would make too much difference since I'm using regukar links to update the search params. I'm using || instead of ?? to grab empty strings as well.


return (
<Section
title="Notifications"
description="Control delivery methods for notifications. Settings applied to this deployment."
layout="fluid"
>
{ready ? (
<Stack spacing={3}>
{isUsingWebhook &&
(webhookEndpoint ? (
<WebhookInfo endpoint={webhookEndpoint} />
) : (
<Alert severity="warning">
Webhook method is enabled, but the endpoint is not configured.
</Alert>
))}
{Object.entries(templatesByGroup.data).map(([group, templates]) => (
<Card
key={group}
variant="outlined"
css={{ background: "transparent", width: "100%" }}
>
<List>
<ListItem css={styles.listHeader}>
<ListItemText css={styles.listItemText} primary={group} />
</ListItem>
<Tabs active={tab}>
<TabsList>
<TabLink to="?tab=events" value="events">
Events
</TabLink>
<TabLink to="?tab=settings" value="settings">
Settings
</TabLink>
</TabsList>
</Tabs>

{templates.map((tpl) => {
const value = castNotificationMethod(
tpl.method || dispatchMethods.data.default,
);
const options = dispatchMethods.data.available.map(
castNotificationMethod,
);

return (
<Fragment key={tpl.id}>
<ListItem>
<ListItemText
css={styles.listItemText}
primary={tpl.name}
/>
<MethodToggleGroup
templateId={tpl.id}
options={options}
value={value}
/>
</ListItem>
<Divider css={styles.divider} />
</Fragment>
);
})}
</List>
</Card>
))}
</Stack>
) : (
<Loader />
)}
<div css={styles.content}>
{ready ? (
tab === "events" ? (
<EventsView
defaultMethod={castNotificationMethod(
dispatchMethods.data.default,
)}
availableMethods={dispatchMethods.data.available.map(
castNotificationMethod,
)}
notificationsConfig={deploymentValues.config.notifications}
templatesByGroup={templatesByGroup.data}
/>
) : (
<OptionsTable
options={deploymentValues?.options.filter((o) =>
deploymentGroupHasParent(o.group, "Notifications"),
)}
/>
)
) : (
<Loader />
)}
</div>
</Section>
);
};

export default NotificationsPage;

type WebhookInfoProps = {
endpoint: string;
type EventsViewProps = {
defaultMethod: NotificationMethod;
availableMethods: NotificationMethod[];
notificationsConfig?: NotificationsConfig;
templatesByGroup: ReturnType<typeof selectTemplatesByGroup>;
};

const WebhookInfo = ({ endpoint }: WebhookInfoProps) => {
const clipboard = useClipboard({ textToCopy: endpoint });
const EventsView: FC<EventsViewProps> = ({
defaultMethod,
availableMethods,
notificationsConfig,
templatesByGroup,
}) => {
const isUsingWebhook = availableMethods.includes("webhook");
const webhookEndpoint = notificationsConfig?.webhook.endpoint;

return (
<Alert
severity="info"
actions={
<Button
variant="text"
onClick={clipboard.copyToClipboard}
disabled={clipboard.showCopiedSuccess}
<Stack spacing={3}>
Copy link
Member

Choose a reason for hiding this comment

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

I think the tables could use a little more spacing between them. They have the borders, so there's no real risk of them getting confused for each other, but they feel a little tense right now

{isUsingWebhook && !webhookEndpoint && (
<Alert severity="warning">
Webhook method is enabled, but the endpoint is not configured.
</Alert>
)}
{Object.entries(templatesByGroup).map(([group, templates]) => (
Copy link
Member

Choose a reason for hiding this comment

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

Could this have a .sort tacked on to make sure there's no risk of the UI elements jumping around? Mainly worried that as the objects gets updated over time, the entries array could serialize the values in different orders across re-renders

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going to implement this in the selectTemplatesByGroup

<Card
key={group}
variant="outlined"
css={{ background: "transparent", width: "100%" }}
>
{clipboard.showCopiedSuccess ? "Copied!" : "Copy"}
</Button>
}
>
<AlertTitle>Webhook Endpoint</AlertTitle>
<AlertDetail>{endpoint}</AlertDetail>
</Alert>
<List>
<ListItem css={styles.listHeader}>
<ListItemText css={styles.listItemText} primary={group} />
</ListItem>

{templates.map((tpl) => {
const value = castNotificationMethod(tpl.method || defaultMethod);
Copy link
Member

Choose a reason for hiding this comment

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

Just making sure: this is basically a stopgap, since the more specific types aren't being auto-generated just yet, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes!


return (
<Fragment key={tpl.id}>
<ListItem>
<ListItemText
css={styles.listItemText}
primary={tpl.name}
/>
<MethodToggleGroup
templateId={tpl.id}
options={availableMethods}
value={value}
/>
</ListItem>
<Divider css={styles.divider} />
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but could this changed so that we check the array index and conditionally choose not to render out the divider if it's the last one? I know we have the last-child styling, but it feels like it could be a bit more fragile over time

</Fragment>
);
})}
</List>
</Card>
))}
</Stack>
);
};

export default NotificationsPage;

const styles = {
content: { paddingTop: 24 },
listHeader: (theme) => ({
background: theme.palette.background.paper,
borderBottom: `1px solid ${theme.palette.divider}`,
Expand Down
1 change: 1 addition & 0 deletions site/src/pages/UserSettingsPage/Section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const styles = {
description: (theme) => ({
color: theme.palette.text.secondary,
fontSize: 16,
margin: 0,
marginTop: 4,
lineHeight: "140%",
}),
Expand Down