Issue #243540 feat: Email notification using SenGrid#33
Issue #243540 feat: Email notification using SenGrid#33vaivk369 wants to merge 9 commits intotekdi:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis update introduces two new GitHub Actions workflows for deployment automation and upgrades several dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Remote Server
participant Deploy Script
GitHub Actions->>Remote Server: SSH (using secrets)
Remote Server->>Deploy Script: cd /target/dir && ./deploy.sh [branch]
Deploy Script-->>Remote Server: Executes deployment steps
Remote Server-->>GitHub Actions: Deployment logs/status
sequenceDiagram
participant NotificationService
participant EmailAdapter
participant SendGrid
participant SMTP
participant NotificationLog
NotificationService->>EmailAdapter: send(notificationData)
alt EMAIL_PROVIDER is SendGrid
EmailAdapter->>SendGrid: send(message)
SendGrid-->>EmailAdapter: response
else EMAIL_PROVIDER is SMTP
EmailAdapter->>SMTP: send(message via NotifmeSdk)
SMTP-->>EmailAdapter: response
end
EmailAdapter->>NotificationLog: create and save log
EmailAdapter-->>NotificationService: result
sequenceDiagram
participant NotificationService
participant PushAdapter
participant Firebase
NotificationService->>PushAdapter: sendNotification(dataArray)
alt PushAdapter is initialized
PushAdapter->>Firebase: send(notification)
Firebase-->>PushAdapter: response
PushAdapter-->>NotificationService: result
else PushAdapter not initialized
PushAdapter-->>NotificationService: throw error (not initialized)
end
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (3)
package.json (1)
73-73: Upgrade TypeScript to ^4.8.0 for NestJS 10 compatibilityTo ensure full support for NestJS 10 and its CLI plugins (e.g., @nestjs/swagger, @nestjs/graphql), bump your TypeScript version to at least 4.8.0:
• File: package.json
• Section: devDependencies
• Current (around line 73):
json "typescript": "^4.3.5"
• Update to:
diff - "typescript": "^4.3.5" + "typescript": "^4.8.0"Using TypeScript 4.8+ guarantees compatibility with all NestJS 10 features and breaking changes in its CLI ecosystem.
src/modules/notification/adapters/pushService.adapter.ts (1)
130-137: Add initialization check to getAccessToken methodThe
getAccessTokenmethod attempts to create credentials even when the adapter is not initialized, which could throw an error.Add initialization check:
async getAccessToken() { + if (!this.isInitialized) { + throw new Error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED); + } const token = await admin.credential.cert({src/modules/notification/adapters/emailService.adapter.ts (1)
277-327: sendRawEmail method doesn't support SendGrid providerThe
sendRawEmailmethod only uses SMTP provider regardless of the configured provider. This is inconsistent with thesendmethod which supports both SMTP and SendGrid.Add SendGrid support to maintain consistency:
async sendRawEmail(emailData: RawEmailData) { const notificationLogs = this.createRawNotificationLog( emailData.subject, emailData.body, emailData.to ); + let result; try { - const emailConfig = this.getEmailConfig('raw-email'); - const notifmeSdk = new NotifmeSdk(emailConfig); - - // Determine content type (HTML or plain text) - const emailPayload: any = { - from: emailData.from || emailConfig.email.from, - to: emailData.to, - subject: emailData.subject, - }; - - // Set content as HTML or text based on isHtml flag - if (emailData.isHtml !== false) { - emailPayload.html = emailData.body; - } else { - emailPayload.text = emailData.body; - } - - const result = await notifmeSdk.send({ - email: emailPayload, - }); + if (this.provider === 'smtp') { + const emailConfig = this.getEmailConfig('raw-email'); + const notifmeSdk = new NotifmeSdk(emailConfig); + + const emailPayload: any = { + from: emailData.from || emailConfig.email.from, + to: emailData.to, + subject: emailData.subject, + }; + + if (emailData.isHtml !== false) { + emailPayload.html = emailData.body; + } else { + emailPayload.text = emailData.body; + } + + result = await notifmeSdk.send({ + email: emailPayload, + }); + } else if (this.provider === 'sendgrid') { + const msg: any = { + to: emailData.to, + from: emailData.from || this.configService.get('EMAIL_FROM'), + subject: emailData.subject, + }; + + if (emailData.isHtml !== false) { + msg.html = emailData.body; + } else { + msg.text = emailData.body; + } + + const sgResponse = await sgMail.send(msg); + result = { + status: sgResponse[0]?.statusCode === 202 ? 'success' : 'error', + id: sgResponse[0]?.headers['x-message-id'] || null, + errors: sgResponse[0]?.statusCode !== 202 ? sgResponse[0]?.body : undefined, + }; + }
🧹 Nitpick comments (3)
.github/workflows/aspire-leader-dev.yaml (1)
11-11: Fix trailing spaces formatting issue.There are trailing spaces on line 11 that should be removed.
- - name: Deploy Stack + - name: Deploy Stack.github/workflows/dev-PYM.yaml (2)
1-5: Branch naming inconsistencyThe workflow name suggests "DEVPYM" deployment, but it triggers on the
east-africa-ymbranch. Consider aligning the workflow name with the branch name for clarity.
21-28: Security consideration for environment variablesWriting secrets directly to a file poses security risks. Ensure that:
- The
.envfile has proper permissions (e.g., 600)- The target directory is secure
- Consider using a secrets management solution for production deployments
Add file permission settings:
echo '${{ secrets.DEVPYM_ENV }}' > .env +chmod 600 .env ls -ltra
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (9)
.github/workflows/aspire-leader-dev.yaml(1 hunks).github/workflows/dev-PYM.yaml(1 hunks)package.json(3 hunks)src/common/utils/constant.util.ts(1 hunks)src/modules/notification/adapters/emailService.adapter.ts(4 hunks)src/modules/notification/adapters/pushService.adapter.ts(1 hunks)src/modules/notification/notification.service.ts(1 hunks)src/modules/notification_events/entity/notificationActionTemplates.entity.ts(1 hunks)src/modules/rabbitmq/rabbitmq.module.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/modules/notification/adapters/pushService.adapter.ts (2)
src/common/logger/LoggerUtil.ts (1)
LoggerUtil(3-78)src/common/utils/constant.util.ts (1)
ERROR_MESSAGES(26-61)
src/modules/notification/adapters/emailService.adapter.ts (2)
src/common/logger/LoggerUtil.ts (1)
LoggerUtil(3-78)src/common/utils/constant.util.ts (1)
ERROR_MESSAGES(26-61)
🪛 YAMLlint (1.37.1)
.github/workflows/aspire-leader-dev.yaml
[error] 11-11: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
src/common/utils/constant.util.ts (1)
58-60: LGTM! Well-structured error constants with clear messaging.The new error constants follow the established pattern and provide clear, actionable guidance for developers when adapters are not properly configured.
src/modules/rabbitmq/rabbitmq.module.ts (1)
2-2: All RabbitMQ imports successfully migrated
- No active
@nestjs-plus/rabbitmqimports remain in the codebase.- Verified that both
src/modules/rabbitmq/rabbitmq.module.tsandsrc/modules/notification/notification.service.tsnow import from@golevelup/nestjs-rabbitmq.- The only leftover reference is a commented-out import in
src/modules/notification-queue/notificationQueue.module.ts—you may remove this stale comment for clarity.src/modules/notification/notification.service.ts (1)
17-17: LGTM! Consistent with the package migration.The RabbitMQ import update is consistent with the package migration from
@nestjs-plus/rabbitmqto@golevelup/nestjs-rabbitmq.
| createdBy: string; | ||
|
|
||
| @Column() | ||
| @Column({ nullable: true }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Schema changes add appropriate flexibility.
Making the image and link columns nullable is a sensible change since not all notification templates require these fields.
Ensure that a database migration is created for this schema change to avoid runtime issues:
Also applies to: 34-34
🏁 Script executed:
#!/bin/bash
# Description: Check if database migration files exist for this schema change
# Expected: Migration files should exist for nullable column changes
echo "Checking for recent migration files..."
fd -t f -e ts -e js migration | head -10
echo "Checking for nullable column changes in migrations..."
rg -A 3 -B 3 "nullable.*true" --type tsLength of output: 7478
Add missing database migration for nullable columns
The schema changes to make image and link nullable in NotificationActionTemplates aren’t reflected in any migration file. Without a migration, deployments will fail when syncing the database.
Action items:
- Create and commit a migration (e.g. with TypeORM CLI) that alters the
notification_action_templatestable:- ALTER the
imagecolumn to allow NULL - ALTER the
linkcolumn to allow NULL
- ALTER the
- Verify the generated migration includes both
upanddownmethods for these changes.
Files needing attention:
- src/modules/notification_events/entity/notificationActionTemplates.entity.ts (columns at lines 31 & 34)
- New migration file under your project’s migrations directory
🤖 Prompt for AI Agents
In src/modules/notification_events/entity/notificationActionTemplates.entity.ts
at lines 31 and 34, the columns `image` and `link` were updated to be nullable
but no migration was created. Use the TypeORM CLI to generate a new migration
that alters the `notification_action_templates` table to make these two columns
nullable. Ensure the migration file includes both `up` and `down` methods to
apply and revert these changes, then commit the migration file along with the
updated entity.
| uses: appleboy/ssh-action@master | ||
| with: | ||
| host: ${{ secrets.HOST_AL }} | ||
| username: ${{ secrets.USERNAME_AL }} | ||
| key: ${{ secrets.SSH_PRIVATE_KEY_AL }} | ||
| port: ${{ secrets.PORT_AL }} | ||
| script: | | ||
| cd /home/ubuntu/Aspire-leader/MICROSERVICES/NOTIFICATION | ||
| ./deploy.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance deployment workflow security and reliability.
The deployment workflow could benefit from improved error handling and verification:
- name: Deploy Stack
uses: appleboy/ssh-action@master
with:
host: ${{ secrets.HOST_AL }}
username: ${{ secrets.USERNAME_AL }}
key: ${{ secrets.SSH_PRIVATE_KEY_AL }}
port: ${{ secrets.PORT_AL }}
+ script_stop: true
script: |
cd /home/ubuntu/Aspire-leader/MICROSERVICES/NOTIFICATION
+ if [ ! -f "./deploy.sh" ]; then
+ echo "Deploy script not found!"
+ exit 1
+ fi
./deploy.sh
+ if [ $? -ne 0 ]; then
+ echo "Deployment failed!"
+ exit 1
+ fi
+ echo "Deployment completed successfully"Consider also:
- Using a specific version instead of
@masterfor the SSH action - Adding deployment verification steps
- Using environment-specific configuration
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: appleboy/ssh-action@master | |
| with: | |
| host: ${{ secrets.HOST_AL }} | |
| username: ${{ secrets.USERNAME_AL }} | |
| key: ${{ secrets.SSH_PRIVATE_KEY_AL }} | |
| port: ${{ secrets.PORT_AL }} | |
| script: | | |
| cd /home/ubuntu/Aspire-leader/MICROSERVICES/NOTIFICATION | |
| ./deploy.sh | |
| - name: Deploy Stack | |
| uses: appleboy/ssh-action@master | |
| with: | |
| host: ${{ secrets.HOST_AL }} | |
| username: ${{ secrets.USERNAME_AL }} | |
| key: ${{ secrets.SSH_PRIVATE_KEY_AL }} | |
| port: ${{ secrets.PORT_AL }} | |
| script_stop: true | |
| script: | | |
| cd /home/ubuntu/Aspire-leader/MICROSERVICES/NOTIFICATION | |
| if [ ! -f "./deploy.sh" ]; then | |
| echo "Deploy script not found!" | |
| exit 1 | |
| fi | |
| ./deploy.sh | |
| if [ $? -ne 0 ]; then | |
| echo "Deployment failed!" | |
| exit 1 | |
| fi | |
| echo "Deployment completed successfully" |
🤖 Prompt for AI Agents
In .github/workflows/aspire-leader-dev.yaml around lines 12 to 20, improve the
deployment workflow by replacing the SSH action version from '@master' to a
specific stable release tag to ensure consistent behavior. Add error handling by
checking the exit status of the deployment script and fail the workflow if
deployment fails. Include verification steps after deployment, such as running a
health check or status command to confirm successful deployment. Use
environment-specific configuration variables instead of hardcoded paths or
values to enhance flexibility and security.
.github/workflows/dev-PYM.yaml
Outdated
| if [ -f .env ]; then | ||
| rm .env | ||
| fi | ||
| echo '${{ secrets.DEVPYM_ENV }}"' > .env |
There was a problem hiding this comment.
Fix syntax error in echo command
There's an extra double quote at the end of the echo command that will cause the script to fail.
Apply this fix:
-echo '${{ secrets.DEVPYM_ENV }}"' > .env
+echo '${{ secrets.DEVPYM_ENV }}' > .env📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo '${{ secrets.DEVPYM_ENV }}"' > .env | |
| echo '${{ secrets.DEVPYM_ENV }}' > .env |
🤖 Prompt for AI Agents
In .github/workflows/dev-PYM.yaml at line 26, the echo command has an extra
double quote at the end which causes a syntax error. Remove the trailing double
quote so the line reads: echo '${{ secrets.DEVPYM_ENV }}' > .env to fix the
syntax and ensure the environment variable is correctly written to the .env
file.
| "@nestjs/cli": "^8.0.0", | ||
| "@nestjs/schematics": "^8.0.0", | ||
| "@nestjs/testing": "^8.0.0", | ||
| "@nestjs/testing": "^10.0.0", |
There was a problem hiding this comment.
Version mismatch between runtime and dev dependencies
NestJS runtime packages are upgraded to v10, but dev dependencies (@nestjs/cli, @nestjs/schematics) remain at v8. This version mismatch could cause compatibility issues during development and testing.
Update dev dependencies to match:
-"@nestjs/cli": "^8.0.0",
-"@nestjs/schematics": "^8.0.0",
+"@nestjs/cli": "^10.0.0",
+"@nestjs/schematics": "^10.0.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@nestjs/cli": "^8.0.0", | |
| "@nestjs/schematics": "^8.0.0", | |
| "@nestjs/testing": "^8.0.0", | |
| "@nestjs/testing": "^10.0.0", | |
| "@nestjs/cli": "^10.0.0", | |
| "@nestjs/schematics": "^10.0.0", | |
| "@nestjs/testing": "^10.0.0", |
🤖 Prompt for AI Agents
In package.json around lines 52 to 54, the dev dependencies @nestjs/cli and
@nestjs/schematics are at version 8 while runtime packages are at version 10,
causing a version mismatch. Update the versions of @nestjs/cli and
@nestjs/schematics to "^10.0.0" to align with the runtime dependencies and
ensure compatibility.
| console.warn('PushAdapter not initialized: Missing Firebase config.'); | ||
| LoggerUtil.error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent logging approach
Using both console.warn and LoggerUtil.error for the same issue. Stick to one logging approach for consistency.
-console.warn('PushAdapter not initialized: Missing Firebase config.');
-LoggerUtil.error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED);
+LoggerUtil.warn('PushAdapter not initialized: Missing Firebase config.');
+LoggerUtil.error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.warn('PushAdapter not initialized: Missing Firebase config.'); | |
| LoggerUtil.error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED); | |
| LoggerUtil.warn('PushAdapter not initialized: Missing Firebase config.'); | |
| LoggerUtil.error(ERROR_MESSAGES.PUSH_ADAPTER_NOT_INITIALIZED); |
🤖 Prompt for AI Agents
In src/modules/notification/adapters/pushService.adapter.ts around lines 34 to
35, the code uses both console.warn and LoggerUtil.error to log the same issue,
which is inconsistent. Choose one logging method, preferably LoggerUtil.error
for error-level logs, and remove the other to maintain consistent logging
throughout the file.
| import { ERROR_MESSAGES, SUCCESS_MESSAGES } from "src/common/utils/constant.util"; | ||
| import { LoggerUtil } from "src/common/logger/LoggerUtil"; | ||
| @Injectable() | ||
| export class PushAdapter implements NotificationServiceInterface { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing @Injectable() decorator
The @Injectable() decorator was removed from the PushAdapter class. If this class is still being injected via NestJS DI container, it needs the decorator.
🏁 Script executed:
#!/bin/bash
# Check if PushAdapter is being injected anywhere in the codebase
ast-grep --pattern 'PushAdapter' | head -20
rg -A 5 "PushAdapter" --type tsLength of output: 6813
Add @Injectable() to PushAdapter for NestJS DI
NestJS requires classes registered in module providers to be decorated with @Injectable() so the DI container can resolve and instantiate them. PushAdapter is listed in both NotificationModule and NotificationQueueModule but is missing this decorator.
• File: src/modules/notification/adapters/pushService.adapter.ts
import { ConfigService } from '@nestjs/config';
+import { Injectable, Inject, forwardRef } from '@nestjs/common';
+@Injectable()
export class PushAdapter implements NotificationServiceInterface {
private readonly fcmurl: string;
private isInitialized = false;
constructor(
@Inject(forwardRef(() => NotificationService)) private readonly notificationServices: NotificationService,
private readonly configService: ConfigService
) { … }Ensure you import and apply @Injectable() above the class declaration so NestJS DI can wire it up correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class PushAdapter implements NotificationServiceInterface { | |
| import { ConfigService } from '@nestjs/config'; | |
| import { Injectable, Inject, forwardRef } from '@nestjs/common'; | |
| @Injectable() | |
| export class PushAdapter implements NotificationServiceInterface { | |
| private readonly fcmurl: string; | |
| private isInitialized = false; | |
| constructor( | |
| @Inject(forwardRef(() => NotificationService)) | |
| private readonly notificationServices: NotificationService, | |
| private readonly configService: ConfigService | |
| ) { … } | |
| } |
🤖 Prompt for AI Agents
In src/modules/notification/adapters/pushService.adapter.ts at line 10, the
PushAdapter class lacks the @Injectable() decorator required by NestJS for
dependency injection. Fix this by importing Injectable from '@nestjs/common' and
adding the @Injectable() decorator immediately above the PushAdapter class
declaration.
| console.warn('EmailAdapter not configured: Missing required settings.'); | ||
| LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add guard check for adapter configuration
The adapter continues to work even when not configured properly. Consider adding a guard check in the send methods.
Would you like me to generate code that adds initialization checks to all public methods to prevent operations when the adapter is not properly configured?
🤖 Prompt for AI Agents
In src/modules/notification/adapters/emailService.adapter.ts around lines 46 to
49, the adapter lacks a guard check to prevent operations when it is not
properly configured. Add a guard clause at the start of all public send methods
that checks if the adapter is configured; if not, log an error or warning and
return early to avoid proceeding with sending emails when settings are missing.
| console.warn('EmailAdapter not configured: Missing required settings.'); | ||
| LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent logging approach
Using both console.warn and LoggerUtil.error. Maintain consistency with the logging approach used throughout the application.
-console.warn('EmailAdapter not configured: Missing required settings.');
-LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED);
+LoggerUtil.warn('EmailAdapter not configured: Missing required settings.');
+LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.warn('EmailAdapter not configured: Missing required settings.'); | |
| LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED); | |
| LoggerUtil.warn('EmailAdapter not configured: Missing required settings.'); | |
| LoggerUtil.error(ERROR_MESSAGES.EMAIL_ADAPTER_NOT_CONFIGURED); |
🤖 Prompt for AI Agents
In src/modules/notification/adapters/emailService.adapter.ts at lines 46 to 47,
the code uses both console.warn and LoggerUtil.error for logging, which is
inconsistent with the application's logging approach. Replace the console.warn
call with an appropriate LoggerUtil method (such as LoggerUtil.warn) to maintain
consistent logging practices throughout the codebase.
|
|
Why do we need provider specific API implementations ? Why can't we just stick to using SMTP which all email services will provide? |
4 participants
No description provided.