Integrate email channel into notifications#3658
Integrate email channel into notifications#3658CarsonF wants to merge 2 commits intonotifications/configfrom
Conversation
CarsonF
force-pushed
the
notifications/email
branch
from
1e97033 to
170f306
Compare
CarsonF
force-pushed
the
notifications/config
branch
from
76285c5 to
338ced1
Compare
CarsonF
force-pushed
the
notifications/email
branch
from
170f306 to
d9168d2
Compare
CarsonF
force-pushed
the
notifications/email
branch
from
d9168d2 to
7f4007f
Compare
There was a problem hiding this comment.
Pull request overview
This PR integrates email as a notification delivery channel alongside the existing in-app notification channel. The implementation adds email rendering capabilities to notification strategies, implements email delivery logic with user permission contexts, and updates the notification channel enumeration.
Changes:
- Added
Emailas a new notification channel with configurable availability based on strategy implementation - Implemented email delivery logic that fetches users, renders emails per recipient, and sends via MailerService
- Added UserModule dependency to NotificationModule to support user data fetching for email delivery
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/notifications/notification.strategy.ts | Added EmailMessage type import, Email channel availability logic, and optional renderEmail interface method |
| src/components/notifications/notification.service.ts | Added email delivery method with user fetching, asUser context for rendering, and asyncPool-based sending |
| src/components/notifications/notification.module.ts | Added UserModule import to support user data access in notification service |
| src/components/notifications/dto/notification-channel.enum.ts | Added 'Email' to NotificationChannel enum values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await asyncPool(Infinity, users, async (user) => { | ||
| const email = user.email.value; | ||
| if (!email) return; | ||
| await this.identity.asUser(user.id, async () => { | ||
| const msg: EmailMessage<any> = strategy.renderEmail!(notification); | ||
| await this.mailer.send(msg.withHeaders({ to: email })); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Email sending failures are not caught or logged. If mailer.send() or renderEmail() throws an error, the entire notification creation will fail silently for all users. Consider adding try-catch blocks around the email rendering and sending operations to log errors without failing the entire notification flow. Similar error handling patterns exist in webhook processing (webhook-processor.worker.ts lines 62-78) and other services (e.g., progress-report migration at line 25-36).
| await this.identity.asUser(user.id, async () => { | ||
| const msg: EmailMessage<any> = strategy.renderEmail!(notification); |
There was a problem hiding this comment.
Calling renderEmail() within the asUser context may limit access to notification data based on the recipient's permissions. If the notification contains data that the user might not have direct access to (but should see in the email), this could cause issues. Consider whether renderEmail() needs to be called within the Administrator context (like the user fetch), or if a different permission context is more appropriate. The webhook executor (webhook.executor.ts:63) uses a similar pattern for executing subscriptions.
| await this.identity.asUser(user.id, async () => { | |
| const msg: EmailMessage<any> = strategy.renderEmail!(notification); | |
| const msg: EmailMessage<any> = await this.identity.asRole( | |
| 'Administrator', | |
| async () => strategy.renderEmail!(notification), | |
| ); | |
| await this.identity.asUser(user.id, async () => { |
| hydrateExtraForGel?(): Record<string, any>; | ||
|
|
||
| /** | ||
| * Send an email notification called for each recipient. |
There was a problem hiding this comment.
The comment contains a grammatical error. "Send an email notification called for each recipient" should be "Send an email notification for each recipient" (remove "called").
| * Send an email notification called for each recipient. | |
| * Send an email notification for each recipient. |
| 'Administrator', | ||
| async () => await this.users.readMany(userIds), | ||
| ); | ||
| await asyncPool(Infinity, users, async (user) => { |
There was a problem hiding this comment.
Using asyncPool(Infinity, ...) removes all concurrency limits, which could cause resource exhaustion if many users receive email notifications simultaneously. Other parts of the codebase use limited concurrency: webhook processing uses 3, product sync uses 5, migrations use 1-2. Consider setting a reasonable concurrency limit (e.g., 5-10) to prevent overwhelming the mail service and to handle errors more gracefully.
No description provided.