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
Fix stories
  • Loading branch information
@BrunoQuaresma
BrunoQuaresma committed Sep 11, 2024
commit 8af93fd2d36281b5abe287cb4166113aa18c8253
7 changes: 2 additions & 5 deletions site/src/api/queries/notifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import type {
UpdateNotificationTemplateMethod,
UpdateUserNotificationPreferences,
} from "api/typesGenerated";
import { displayError, displaySuccess } from "components/GlobalSnackbar/utils";
import type { QueryClient, UseMutationOptions } from "react-query";

export const userNotificationPreferencesKey = (userId: string) => [
Expand Down Expand Up @@ -151,10 +150,8 @@ export const disableNotification = (
});
return result;
},
onSuccess: () => {
queryClient.invalidateQueries(
userNotificationPreferences(userId).queryKey,
);
onSuccess: (data) => {
queryClient.setQueryData(userNotificationPreferencesKey(userId), data);
},
} satisfies UseMutationOptions<NotificationPreference[], unknown, string>;
};
122 changes: 64 additions & 58 deletions site/src/pages/UserSettingsPage/NotificationsPage/NotificationsPage.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Meta, StoryObj } from "@storybook/react";
import { spyOn, userEvent, waitFor, within } from "@storybook/test";
import { spyOn, userEvent, waitFor, within, expect } from "@storybook/test";
import { API } from "api/api";
import {
notificationDispatchMethodsKey,
Expand All @@ -19,8 +19,9 @@ import {
withGlobalSnackbar,
} from "testHelpers/storybook";
import { NotificationsPage } from "./NotificationsPage";
import { reactRouterParameters } from "storybook-addon-remix-react-router";

const meta: Meta<typeof NotificationsPage> = {
const meta = {
title: "pages/UserSettingsPage/NotificationsPage",
component: NotificationsPage,
parameters: {
Expand All @@ -43,7 +44,7 @@ const meta: Meta<typeof NotificationsPage> = {
permissions: { viewDeploymentValues: true },
},
decorators: [withGlobalSnackbar, withAuthProvider, withDashboardProvider],
};
} satisfies Meta<typeof NotificationsPage>;

export default meta;
type Story = StoryObj<typeof NotificationsPage>;
Expand Down Expand Up @@ -78,72 +79,77 @@ export const NonAdmin: Story = {
},
};

// Ensure the selected notification template is enabled before attempting to
// disable it.
const enabledPreference = MockNotificationPreferences.find(
(pref) => pref.disabled === false,
);
if (!enabledPreference) {
throw new Error(
"No enabled notification preference available to test the disabling action.",
);
}
const templateToDisable = MockNotificationTemplates.find(
(tpl) => tpl.id === enabledPreference.id,
);
if (!templateToDisable) {
throw new Error(" No notification template matches the enabled preference.");
}

export const DisableValidTemplate: Story = {
parameters: {
msw: {
handlers: [
http.put("/api/v2/users/:userId/notifications/preferences", () => {
return HttpResponse.json([
{ id: "valid-template-id", disabled: true },
]);
reactRouter: reactRouterParameters({
location: {
searchParams: { disabled: templateToDisable.id },
},
}),
},
decorators: [
(Story) => {
// Since the action occurs during the initial render, we need to spy on
// the API call before the story is rendered. This is done using a
// decorator to ensure the spy is set up in time.
spyOn(API, "putUserNotificationPreferences").mockResolvedValue(
MockNotificationPreferences.map((pref) => {
if (pref.id === templateToDisable.id) {
return {
...pref,
disabled: true,
};
}
return pref;
}),
],
);
return <Story />;
},
},
],
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 },
await within(document.body).findByText("Notification has been disabled");
const switchEl = await within(canvasElement).findByLabelText(
templateToDisable.name,
);
expect(switchEl).not.toBeChecked();
},
};

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();
reactRouter: reactRouterParameters({
location: {
searchParams: { disabled: "invalid-template-id" },
},
{ timeout: 10000 },
);
}),
},
decorators: [
(Story) => {
// Since the action occurs during the initial render, we need to spy on
// the API call before the story is rendered. This is done using a
// decorator to ensure the spy is set up in time.
spyOn(API, "putUserNotificationPreferences").mockRejectedValue({});
return <Story />;
},
],
play: async () => {
await within(document.body).findByText("Error disabling notification");
},
};
4 changes: 1 addition & 3 deletions 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 @@ -83,9 +83,7 @@ export const NotificationsPage: FC = () => {
displaySuccess("Notification has been disabled");
})
.catch(() => {
displayError(
"An error occurred when attempting to disable the requested notification",
);
displayError("Error disabling notification");
});
}, [searchParams.delete, disabledId, disableMutation]);

Expand Down
Loading