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
addressed the comments
  • Loading branch information
@joobisb
joobisb committed Sep 3, 2024
commit 254f4dce98fde018dbff5a7a53f40a571e9d5a00
Binary file added coderd/notifications/Archive.zip
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion coderd/notifications/dispatch/smtp/html.gotmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
<div style="border-top: 1px solid #e2e8f0; color: #475569; font-size: 12px; margin-top: 64px; padding-top: 24px; line-height: 1.6;">
<p>&copy;&nbsp;{{ current_year }}&nbsp;Coder. All rights reserved&nbsp;-&nbsp;<a href="{{ base_url }}" style="color: #2563eb; text-decoration: none;">{{ base_url }}</a></p>
<p><a href="{{ base_url }}/settings/notifications" style="color: #2563eb; text-decoration: none;">Click here to manage your notification settings</a></p>
<p><a href="{{ base_url }}/settings/notifications?unsubscribe={{ .NotificationTemplateID }}" style="color: #2563eb; text-decoration: none;">Stop receiving emails like this</a></p>
<p><a href="{{ base_url }}/settings/notifications?disabled={{ .NotificationTemplateID }}" style="color: #2563eb; text-decoration: none;">Stop receiving emails like this</a></p>
</div>
</div>
</body>
Expand Down
2 changes: 1 addition & 1 deletion coderd/notifications/enqueuer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func (s *StoreEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUI
// actions which can be taken by the recipient.
func (s *StoreEnqueuer) buildPayload(metadata database.FetchNewMessageMetadataRow, labels map[string]string) (*types.MessagePayload, error) {
payload := types.MessagePayload{
Version: "1.0",
Version: "1.1",

NotificationName: metadata.NotificationName,
NotificationTemplateID: metadata.NotificationTemplateID.String(),
Expand Down
18 changes: 10 additions & 8 deletions coderd/notifications/types/payload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ package types
type MessagePayload struct {
Version string `json:"_version"`

NotificationName string `json:"notification_name"`
NotificationTemplateID string `json:"notification_template_id"`
UserID string `json:"user_id"`
UserEmail string `json:"user_email"`
UserName string `json:"user_name"`
UserUsername string `json:"user_username"`
Actions []TemplateAction `json:"actions"`
Labels map[string]string `json:"labels"`
NotificationName string `json:"notification_name"`
NotificationTemplateID string `json:"notification_template_id"`

UserID string `json:"user_id"`
UserEmail string `json:"user_email"`
UserName string `json:"user_name"`
UserUsername string `json:"user_username"`

Actions []TemplateAction `json:"actions"`
Labels map[string]string `json:"labels"`
}
44 changes: 29 additions & 15 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 @@ -18,7 +18,7 @@ import type {
NotificationPreference,
NotificationTemplate,
} from "api/typesGenerated";
import { displaySuccess } from "components/GlobalSnackbar/utils";
import { displayError, displaySuccess } from "components/GlobalSnackbar/utils";
import { Loader } from "components/Loader/Loader";
import { Stack } from "components/Stack/Stack";
import { useAuthenticated } from "contexts/auth/RequireAuth";
Expand Down Expand Up @@ -63,24 +63,38 @@ export const NotificationsPage: FC = () => {
updateUserNotificationPreferences(user.id, queryClient),
);
const [searchParams] = useSearchParams();
const unsubscribeTemplateId = searchParams.get("unsubscribe");
const templateId = searchParams.get("disabled");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rename this to something like 'disabledId' to make it clearer about its usage.


useEffect(() => {
if (unsubscribeTemplateId) {
handleUnsubscribe(unsubscribeTemplateId);
if (templateId && templatesByGroup.isSuccess && templatesByGroup.data) {
disableTemplate(templateId);
}
}, [unsubscribeTemplateId]);
}, [templateId, templatesByGroup.isSuccess, templatesByGroup.data]);

const handleUnsubscribe = async (templateId: string) => {
await updatePreferences.mutateAsync({
template_disabled_map: {
[templateId]: true,
},
});
displaySuccess("Notification preferences updated");
queryClient.invalidateQueries(
userNotificationPreferences(user.id).queryKey,
);
const disableTemplate = async (templateId: string) => {
try {
await updatePreferences.mutateAsync({
template_disabled_map: {
[templateId]: true,
},
});

const allTemplates = Object.values(templatesByGroup.data ?? {}).flat();
const template = allTemplates.find((t) => t.id === templateId);

if (!template) {
throw new Error(`Template with ID ${templateId} not found`);
}

displaySuccess(`${template.name} notification has been disabled`);

queryClient.invalidateQueries(
userNotificationPreferences(user.id).queryKey,
);
} catch (error) {
console.error(error);
displayError("Error on disabling notification");
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that moving the disableTemplate mutation inside the queries/notifications would improve the readability of the view. This approach aligns with the pattern we've been using in the UI, where all "server data" is handled in the queries module. Eg.:

const queryClient = useQueryClient()
const disableMutation = useMutation(disableNotification(userId, queryClient))

useEffect(() => {
  try {
    disableMutation
      .mutateAsync()
      .then(() => {
        displaySuccess("...")
      }).catch(() => {
        displayError("...")
      })
  }
}, [...])

}
};

const ready =
Expand Down
Loading