From ad42fb4721ff7f731168306c648bcb357ae26ba8 Mon Sep 17 00:00:00 2001 From: x032205 Date: Fri, 5 Sep 2025 22:03:57 -0400 Subject: [PATCH] Review fixes --- .../access-approval-request-service.ts | 71 +++++++---- backend/src/queue/queue-service.ts | 2 +- .../server/routes/v1/notification-router.ts | 21 +++- .../notification/notification-queue.ts | 27 ++-- .../notification/notification-service.ts | 25 ++-- .../notification/notification-types.ts | 3 +- .../notification/user-notification-dal.ts | 2 +- .../src/hooks/api/notifications/mutations.tsx | 14 ++- .../src/hooks/api/notifications/queries.tsx | 2 +- .../components/NavBar/Navbar.tsx | 104 +--------------- .../components/NavBar/Notification.tsx | 8 +- .../NavBar/NotificationDropdown.tsx | 116 ++++++++++++++++++ 12 files changed, 226 insertions(+), 169 deletions(-) create mode 100644 frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts index 49ddf28ec..f2b9f4e8d 100644 --- a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts +++ b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts @@ -69,7 +69,7 @@ type TSecretApprovalRequestServiceFactoryDep = { projectSlackConfigDAL: Pick; microsoftTeamsService: Pick; projectMicrosoftTeamsConfigDAL: Pick; - notificationService: Pick; + notificationService: Pick; }; export const accessApprovalRequestServiceFactory = ({ @@ -279,15 +279,15 @@ export const accessApprovalRequestServiceFactory = ({ } }); - for await (const approver of approverUsers) { - await notificationService.createUserNotification({ + await notificationService.createUserNotifications( + approverUsers.map((approver) => ({ userId: approver.id, type: NotificationType.ACCESS_APPROVAL_REQUEST, title: "Access Approval Request", body: `**${requesterFullName}** (${requestedByUser.email}) has requested ${isTemporary ? "temporary" : "permanent"} access to **${secretPath}** in the **${envSlug}** environment for project **${project.name}**.`, link: approvalPath - }); - } + })) + ); await smtpService.sendMail({ recipients: approverUsers.filter((approver) => approver.email).map((approver) => approver.email!), @@ -406,7 +406,8 @@ export const accessApprovalRequestServiceFactory = ({ const requesterFullName = `${requestedByUser.firstName} ${requestedByUser.lastName}`; const editorFullName = `${editedByUser.firstName} ${editedByUser.lastName}`; - const approvalUrl = `${cfg.SITE_URL}/projects/secret-management/${project.id}/approval`; + const approvalPath = `/projects/secret-management/${project.id}/approval`; + const approvalUrl = `${cfg.SITE_URL}${approvalPath}`; await triggerWorkflowIntegrationNotification({ input: { @@ -437,27 +438,43 @@ export const accessApprovalRequestServiceFactory = ({ } }); - await smtpService.sendMail({ - recipients: policy.approvers - .filter((approver) => Boolean(approver.email) && approver.userId !== editedByUser.id) - .map((approver) => approver.email!), - subjectLine: "Access Approval Request Updated", - substitutions: { - projectName: project.name, - requesterFullName, - requesterEmail: requestedByUser.email, - isTemporary: true, - expiresIn: msFn(ms(temporaryRange || ""), { long: true }), - secretPath, - environment: envSlug, - permissions: accessTypes, - approvalUrl, - editNote, - editorFullName, - editorEmail: editedByUser.email - }, - template: SmtpTemplates.AccessApprovalRequestUpdated - }); + await notificationService.createUserNotifications( + policy.approvers + .filter((approver) => Boolean(approver.userId) && approver.userId !== editedByUser.id) + .map((approver) => ({ + userId: approver.userId!, + type: NotificationType.ACCESS_APPROVAL_REQUEST_UPDATED, + title: "Access Approval Request Updated", + body: `**${editorFullName}** (${editedByUser.email}) has updated the access request submitted by **${requesterFullName}** (${requestedByUser.email}) for **${secretPath}** in the **${envSlug}** environment for project **${project.name}**.`, + link: approvalPath + })) + ); + + const recipients = policy.approvers + .filter((approver) => Boolean(approver.email) && approver.userId !== editedByUser.id) + .map((approver) => approver.email!); + + if (recipients.length > 0) { + await smtpService.sendMail({ + recipients, + subjectLine: "Access Approval Request Updated", + substitutions: { + projectName: project.name, + requesterFullName, + requesterEmail: requestedByUser.email, + isTemporary: true, + expiresIn: msFn(ms(temporaryRange || ""), { long: true }), + secretPath, + environment: envSlug, + permissions: accessTypes, + approvalUrl, + editNote, + editorFullName, + editorEmail: editedByUser.email + }, + template: SmtpTemplates.AccessApprovalRequestUpdated + }); + } return approvalRequest; }); diff --git a/backend/src/queue/queue-service.ts b/backend/src/queue/queue-service.ts index a26cc5398..5a7c92f22 100644 --- a/backend/src/queue/queue-service.ts +++ b/backend/src/queue/queue-service.ts @@ -318,7 +318,7 @@ export type TQueueJobTypes = { }; [QueueName.UserNotification]: { name: QueueJobs.UserNotification; - payload: TCreateUserNotificationDTO; + payload: { notifications: TCreateUserNotificationDTO[] }; }; }; diff --git a/backend/src/server/routes/v1/notification-router.ts b/backend/src/server/routes/v1/notification-router.ts index 5a5dd588e..edf28e799 100644 --- a/backend/src/server/routes/v1/notification-router.ts +++ b/backend/src/server/routes/v1/notification-router.ts @@ -64,15 +64,23 @@ export const registerNotificationRouter = async (server: FastifyZodProvider) => }); server.route({ - url: "/user/:notificationId/mark-as-read", + url: "/user/:notificationId", config: { rateLimit: writeLimit }, - method: "POST", + method: "PATCH", schema: { params: z.object({ notificationId: z.string() - }) + }), + body: z.object({ + isRead: z.boolean() + }), + response: { + 200: z.object({ + notification: UserNotificationsSchema + }) + } }, onRequest: verifyAuth([AuthMode.JWT]), handler: async (req) => { @@ -80,10 +88,13 @@ export const registerNotificationRouter = async (server: FastifyZodProvider) => throw new UnauthorizedError({ message: "This endpoint can only be accessed by users" }); } - await server.services.notification.markUserNotificationAsRead({ + const notification = await server.services.notification.updateUserNotification({ notificationId: req.params.notificationId, - userId: req.auth.userId + userId: req.auth.userId, + ...req.body }); + + return { notification }; } }); diff --git a/backend/src/services/notification/notification-queue.ts b/backend/src/services/notification/notification-queue.ts index 15f4b09ed..e5d89c83b 100644 --- a/backend/src/services/notification/notification-queue.ts +++ b/backend/src/services/notification/notification-queue.ts @@ -4,43 +4,36 @@ import { TCreateUserNotificationDTO } from "./notification-types"; import { TUserNotificationDALFactory } from "./user-notification-dal"; type TNotificationQueueServiceFactoryDep = { - userNotificationDAL: Pick; + userNotificationDAL: Pick; queueService: TQueueServiceFactory; }; export type TNotificationQueueServiceFactory = { - pushUserNotification: (data: TCreateUserNotificationDTO) => Promise; + pushUserNotifications: (data: TCreateUserNotificationDTO[]) => Promise; }; export const notificationQueueServiceFactory = async ({ userNotificationDAL, queueService }: TNotificationQueueServiceFactoryDep): Promise => { - const pushUserNotification = async (data: TCreateUserNotificationDTO) => { - await queueService.queuePg(QueueJobs.UserNotification, data); + const pushUserNotifications = async (data: TCreateUserNotificationDTO[]) => { + await queueService.queuePg(QueueJobs.UserNotification, { notifications: data }); }; await queueService.startPg( QueueJobs.UserNotification, async ([job]) => { - const { userId, type, title, body, link } = job.data as TCreateUserNotificationDTO; - - await userNotificationDAL.create({ - userId, - type, - title, - body, - link - }); + const { notifications } = job.data as { notifications: TCreateUserNotificationDTO[] }; + await userNotificationDAL.batchInsert(notifications); }, { - batchSize: 100, - workerCount: 5, - pollingIntervalSeconds: 2 + batchSize: 1, + workerCount: 2, + pollingIntervalSeconds: 1 } ); return { - pushUserNotification + pushUserNotifications }; }; diff --git a/backend/src/services/notification/notification-service.ts b/backend/src/services/notification/notification-service.ts index 5e0f96add..7ad24d30b 100644 --- a/backend/src/services/notification/notification-service.ts +++ b/backend/src/services/notification/notification-service.ts @@ -29,8 +29,8 @@ export const notificationServiceFactory = ({ return notifications; }; - const createUserNotification = async (data: TCreateUserNotificationDTO) => { - return notificationQueue.pushUserNotification(data); + const createUserNotifications = async (data: TCreateUserNotificationDTO[]) => { + return notificationQueue.pushUserNotifications(data); }; const deleteUserNotification = async ({ userId, notificationId }: { userId: string; notificationId: string }) => { @@ -47,23 +47,34 @@ export const notificationServiceFactory = ({ await userNotificationDAL.markAllNotificationsAsRead(userId); }; - const markUserNotificationAsRead = async ({ userId, notificationId }: { userId: string; notificationId: string }) => { - await userNotificationDAL.update( + const updateUserNotification = async ({ + userId, + notificationId, + isRead + }: { + userId: string; + notificationId: string; + isRead: boolean; + }) => { + const [updatedNotification] = await userNotificationDAL.update( { id: notificationId, userId }, { - isRead: true + isRead } ); + + if (!updatedNotification) throw new NotFoundError({ message: "Notification not found" }); + return updatedNotification; }; return { listUserNotifications, - createUserNotification, + createUserNotifications, deleteUserNotification, markUserNotificationsAsRead, - markUserNotificationAsRead + updateUserNotification }; }; diff --git a/backend/src/services/notification/notification-types.ts b/backend/src/services/notification/notification-types.ts index 2ac65cf38..eb451a761 100644 --- a/backend/src/services/notification/notification-types.ts +++ b/backend/src/services/notification/notification-types.ts @@ -1,5 +1,6 @@ export enum NotificationType { - ACCESS_APPROVAL_REQUEST = "access-approval-request" + ACCESS_APPROVAL_REQUEST = "access-approval-request", + ACCESS_APPROVAL_REQUEST_UPDATED = "access-approval-request-updated" } export interface TCreateUserNotificationDTO { diff --git a/backend/src/services/notification/user-notification-dal.ts b/backend/src/services/notification/user-notification-dal.ts index 7163118f5..efa5e7f0a 100644 --- a/backend/src/services/notification/user-notification-dal.ts +++ b/backend/src/services/notification/user-notification-dal.ts @@ -21,7 +21,7 @@ export const userNotificationDALFactory = (db: TDbClient) => { userId, startDate, endDate, - limit = 10000, + limit = 1000, offset = 0 }: { userId: string; diff --git a/frontend/src/hooks/api/notifications/mutations.tsx b/frontend/src/hooks/api/notifications/mutations.tsx index c61ed5c46..596e54fe6 100644 --- a/frontend/src/hooks/api/notifications/mutations.tsx +++ b/frontend/src/hooks/api/notifications/mutations.tsx @@ -23,17 +23,21 @@ export const useMarkAllNotificationsAsRead = () => { }); }; -export const useMarkNotificationAsRead = () => { +export const useUpdateNotification = () => { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (notificationId: string) => { - await apiRequest.post(`/api/v1/notifications/user/${notificationId}/mark-as-read`); + mutationFn: async ({ notificationId, isRead }: { notificationId: string; isRead: boolean }) => { + const { data } = await apiRequest.patch<{ notification: TUserNotification }>( + `/api/v1/notifications/user/${notificationId}`, + { isRead } + ); + return data.notification; }, - onSuccess: (_, notificationId) => { + onSuccess: (updatedNotification) => { queryClient.setQueryData(notificationKeys.list(), (oldData) => { if (!oldData) return oldData; return oldData.map((notification) => - notification.id === notificationId ? { ...notification, isRead: true } : notification + notification.id === updatedNotification.id ? updatedNotification : notification ); }); } diff --git a/frontend/src/hooks/api/notifications/queries.tsx b/frontend/src/hooks/api/notifications/queries.tsx index 66bbee7a8..1fcde5d3f 100644 --- a/frontend/src/hooks/api/notifications/queries.tsx +++ b/frontend/src/hooks/api/notifications/queries.tsx @@ -20,6 +20,6 @@ export const useGetMyNotifications = () => { ); return notifications; }, - refetchInterval: 10 * 1000 // Poll every 10 seconds + refetchInterval: 30 * 1000 // Poll every 30 seconds }); }; diff --git a/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx b/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx index f72fa7975..63da8b533 100644 --- a/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx +++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx @@ -1,6 +1,6 @@ -import { useMemo, useState } from "react"; +import { useState } from "react"; import { faGithub, faSlack } from "@fortawesome/free-brands-svg-icons"; -import { faBell, faCircleQuestion, faUserCircle } from "@fortawesome/free-regular-svg-icons"; +import { faCircleQuestion, faUserCircle } from "@fortawesome/free-regular-svg-icons"; import { faArrowUpRightFromSquare, faBook, @@ -25,7 +25,6 @@ import SecurityClient from "@app/components/utilities/SecurityClient"; import { BreadcrumbContainer, Button, - ContentLoader, DropdownMenu, DropdownMenuContent, DropdownMenuItem, @@ -43,19 +42,13 @@ import { useToggle } from "@app/hooks"; import { useGetOrganizations, useLogoutUser, workspaceKeys } from "@app/hooks/api"; import { authKeys, selectOrganization } from "@app/hooks/api/auth/queries"; import { MfaMethod } from "@app/hooks/api/auth/types"; -import { - useDeleteNotification, - useMarkAllNotificationsAsRead, - useMarkNotificationAsRead -} from "@app/hooks/api/notifications/mutations"; -import { useGetMyNotifications } from "@app/hooks/api/notifications/queries"; import { getAuthToken } from "@app/hooks/api/reactQuery"; import { SubscriptionPlan } from "@app/hooks/api/types"; import { AuthMethod } from "@app/hooks/api/users/types"; import { navigateUserToOrg } from "@app/pages/auth/LoginPage/Login.utils"; import { ServerAdminsPanel } from "../ServerAdminsPanel/ServerAdminsPanel"; -import { Notification } from "./Notification"; +import { NotificationDropdown } from "./NotificationDropdown"; const getPlan = (subscription: SubscriptionPlan) => { if (subscription.groups) return "Enterprise"; @@ -127,16 +120,6 @@ export const Navbar = () => { const router = useRouter(); const queryClient = useQueryClient(); - const { data: notifications, isLoading } = useGetMyNotifications(); - const { mutate: markAllAsRead } = useMarkAllNotificationsAsRead(); - const { mutate: markNotificationAsRead } = useMarkNotificationAsRead(); - const { mutate: deleteNotification } = useDeleteNotification(); - - const unreadCount = useMemo( - () => notifications?.filter((n) => !n.isRead).length || 0, - [notifications] - ); - const location = useLocation(); const matches = useRouterState({ select: (s) => s.matches.at(-1)?.context }); const breadcrumbs = matches && "breadcrumbs" in matches ? matches.breadcrumbs : undefined; @@ -377,86 +360,7 @@ export const Navbar = () => { )} - - -
- - {unreadCount > 0 && ( - - {unreadCount > 99 ? "99+" : unreadCount} - - )} -
- - -
-
- Notifications - -
-
- {isLoading && ( -
- -
- )} - {!isLoading && notifications?.length === 0 && ( -
- - No new notifications - - We'll let you know when something important happens. - -
- )} - {!isLoading && notifications && notifications.length > 0 && ( -
- {notifications.map((notification) => ( -
{ - if (!notification.isRead) { - markNotificationAsRead(notification.id); - } - if (notification.link) { - router.navigate({ to: notification.link }); - } - }} - onKeyDown={(e) => { - if (e.key !== "Enter") return; - if (!notification.isRead) { - markNotificationAsRead(notification.id); - } - if (notification.link) { - router.navigate({ to: notification.link }); - } - }} - > - -
- ))} -
- )} -
-
- - +
diff --git a/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx b/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx index 38eb8654f..59e6861fc 100644 --- a/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx +++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx @@ -26,9 +26,9 @@ export const Notification = ({ notification, onDelete }: Props) => { {!notification.isRead && ( )} - - - {notification.title} + {notification.title}} delayDuration={300}> + + {notification.title} @@ -36,7 +36,7 @@ export const Notification = ({ notification, onDelete }: Props) => {
{notification.body && ( - + {notification.body} )} diff --git a/frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx b/frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx new file mode 100644 index 000000000..6e2150be9 --- /dev/null +++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx @@ -0,0 +1,116 @@ +import { useMemo } from "react"; +import { faBell } from "@fortawesome/free-solid-svg-icons"; +import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; +import { useRouter } from "@tanstack/react-router"; + +import { + ContentLoader, + DropdownMenu, + DropdownMenuContent, + DropdownMenuTrigger +} from "@app/components/v2"; +import { + useDeleteNotification, + useMarkAllNotificationsAsRead, + useUpdateNotification +} from "@app/hooks/api/notifications/mutations"; +import { useGetMyNotifications } from "@app/hooks/api/notifications/queries"; + +import { Notification } from "./Notification"; + +export const NotificationDropdown = () => { + const router = useRouter(); + + const { data: notifications, isLoading } = useGetMyNotifications(); + const { mutate: markAllAsRead } = useMarkAllNotificationsAsRead(); + const { mutate: updateNotification } = useUpdateNotification(); + const { mutate: deleteNotification } = useDeleteNotification(); + + const unreadCount = useMemo( + () => notifications?.filter((n) => !n.isRead).length || 0, + [notifications] + ); + + return ( + + +
+ + {unreadCount > 0 && ( + + {unreadCount > 99 ? "99+" : unreadCount} + + )} +
+ + +
+
+ Notifications + +
+
+ {isLoading && ( +
+ +
+ )} + {!isLoading && notifications?.length === 0 && ( +
+ + No new notifications + + We'll let you know when something important happens. + +
+ )} + {!isLoading && notifications && notifications.length > 0 && ( +
+ {notifications.map((notification) => ( +
{ + if (!notification.isRead) { + updateNotification({ notificationId: notification.id, isRead: true }); + } + if (notification.link) { + router.navigate({ to: notification.link }); + } + }} + onKeyDown={(e) => { + if (e.key !== "Enter") return; + if (!notification.isRead) { + updateNotification({ notificationId: notification.id, isRead: true }); + } + if (notification.link) { + router.navigate({ to: notification.link }); + } + }} + > + +
+ ))} +
+ )} +
+
+ + + ); +};