8000 feat: turn off notification via email by joobisb · Pull Request #14520 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

feat: turn off notification via email #14520

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 9 commits into from
Sep 11, 2024
Merged

feat: turn off notification via email #14520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8568812
feat: turn off notification via email
joobisb Sep 1, 2024
5309f0a
remove unused imports
joobisb Sep 1, 2024
254f4dc
addressed the comments
joobisb Sep 3, 2024
f9ac6d2
chore: refactored notifications page
joobisb Sep 4, 2024
18b3f9a
refactored and added tests
joobisb Sep 5, 2024
8538ec4
Merge remote-tracking branch 'origin/main' into email-notification-di…
joobisb Sep 11, 2024
025c633
Simplify effect
BrunoQuaresma Sep 11, 2024
8af93fd
Fix stories
BrunoQuaresma Sep 11, 2024
7f0a831
Fix fmt
BrunoQuaresma Sep 11, 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
refactored and added tests
  • Loading branch information
@joobisb
joobisb committed Sep 5, 2024
commit 18b3f9a1229ff71f1b5996e16722a44b07d7ba6c
21 changes: 0 additions & 21 deletions site/src/api/queries/notifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,28 +149,7 @@ export const disableNotification = (
[templateId]: true,
},
});

// Invalidate the user notification preferences query
queryClient.invalidateQueries(userNotificationPreferencesKey(userId));

return result;
},
onSuccess: (_, templateId) => {
const allTemplates = queryClient.getQueryData<NotificationTemplate[]>(
systemNotificationTemplatesKey,
);
const template = allTemplates?.find((t) => t.id === templateId);

if (template) {
displaySuccess(`${template.name} notification has been disabled`);
} else {
displaySuccess("Notification has been disabled");
}
},
onError: () => {
displayError(
"An error occurred when attempting to disable the requested notification",
);
},
} satisfies UseMutationOptions<NotificationPreference[], unknown, string>;
};
73 changes: 72 additions & 1 deletion site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import type { Meta, StoryObj } from "@storybook/react";
import { spyOn, userEvent, within } from "@storybook/test";
import { spyOn, userEvent, waitFor, within } from "@storybook/test";
import { API } from "api/api";
import {
notificationDispatchMethodsKey,
systemNotificationTemplatesKey,
userNotificationPreferencesKey,
} from "api/queries/notifications";
import { http, HttpResponse } from "msw";
import {
MockNotificationMethodsResponse,
MockNotificationPreferences,
Expand Down Expand Up @@ -76,3 +77,73 @@ export const NonAdmin: Story = {
permissions: { viewDeploymentValues: false },
},
};

export const DisableValidTemplate: Story = {
parameters: {
msw: {
handlers: [
http.put("/api/v2/users/:userId/notifications/preferences", () => {
return HttpResponse.json([
{ id: "valid-template-id", disabled: true },
]);
}),
],
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);

const validTemplateId = "valid-template-id";
const validTemplateName = "Valid Template Name";

window.history.pushState({}, "", `?disabled=${validTemplateId}`);

await waitFor(
async () => {
const successMessage = await canvas.findByText(
`${validTemplateName} notification has been disabled`,
);
expect(successMessage).toBeInTheDocument();
},
{ timeout: 10000 },
);

await waitFor(
async () => {
const templateSwitch = await canvas.findByLabelText(validTemplateName);
expect(templateSwitch).not.toBeChecked();
},
{ timeout: 10000 },
);
},
};

export const DisableInvalidTemplate: Story = {
parameters: {
msw: {
handlers: [
http.put("/api/v2/users/:userId/notifications/preferences", () => {
// Mock failed API response
return new HttpResponse(null, { status: 400 });
}),
],
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);

const invalidTemplateId = "invalid-template-id";

window.history.pushState({}, "", `?disabled=${invalidTemplateId}`);

await waitFor(
async () => {
const errorMessage = await canvas.findByText(
"An error occurred when attempting to disable the requested notification",
);
expect(errorMessage).toBeInTheDocument();
},
{ timeout: 10000 },
);
},
};
27 changes: 26 additions & 1 deletion site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.tsx
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to remember to check for the following scenarios:

  1. When a valid template ID is disabled and the request is successful, check if the success message is displayed. Also, ensure that the UI is updated correctly. Verify if the request is being sent properly. Additionally, confirm if the error message is displayed when the request fails.

  2. When a not found template ID is disabled, check if an error message is displayed.

You can check how we do that using Storybook interaction tests on the NotificationsPage.stories.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have verified these scnearios

Copy link
Contributor

Choose a reason for hiding this comment

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

@joobisb can you add a test please? Manual verification is good but it only helps us know if this works currently, and doesn't catch future degradations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@joobisb we are close, we just need to automate these tests using the way I shared before. Thanks for your hard work! 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BrunoQuaresma the tests needed to be added to NotificationsPage.stories.tsx right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joobisb we are close, we just need to automate these tests using the way I shared before. Thanks for your hard work! 🙏

done, please have a look

Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,38 @@ export const NotificationsPage: FC = () => {

useEffect(() => {
if (disabledId && templatesByGroup.isSuccess && templatesByGroup.data) {
disableMutation.mutate(disabledId);
searchParams.delete("disabled");
disableMutation
.mutateAsync(disabledId)
.then(() => {
const allTemplates = Object.values(
templatesByGroup.data ?? {},
).flat();
const template = allTemplates.find((t) => t.id === disabledId);

if (template) {
displaySuccess(`${template.name} notification has been disabled`);
} else {
displaySuccess("Notification has been disabled");
}
queryClient.invalidateQueries(
userNotificationPreferences(user.id).queryKey,
);
})
.catch(() => {
displayError(
"An error occurred when attempting to disable the requested notification",
);
});
}
}, [
disabledId,
templatesByGroup.isSuccess,
templatesByGroup.data,
disableMutation,
queryClient,
user.id,
searchParams,
]);

const ready =
Expand Down
Loading
0