feat: add support for sns sms provider #306
Conversation
WalkthroughThe recent updates enhance the notification system by integrating SMS capabilities through AWS SNS. Key changes include the introduction of new API requests, data validation, a dedicated service for SMS functionalities, and comprehensive documentation. These modifications improve the robustness and flexibility of the application, allowing it to effectively handle SMS notifications alongside existing channels, thereby broadening communication options for users. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueueService
participant SmsSnsNotificationConsumer
participant SmsSnsService
User->>QueueService: Send SMS Notification
QueueService->>SmsSnsNotificationConsumer: Process Notification
SmsSnsNotificationConsumer->>SmsSnsService: Send SMS Message
SmsSnsService->>SmsSnsService: Assign SNS Config
SmsSnsService->>SmsSnsService: Publish SMS
SmsSnsService-->>SmsSnsNotificationConsumer: Return Result
SmsSnsNotificationConsumer-->>QueueService: Acknowledge Processing
QueueService-->>User: Notify User of Status
Poem
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Harish-osmosys
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (4)
apps/api/docs/channels/sms-sns.md (4)
3-5: Clarify the documentation reference.Consider adding a direct link to the SNS documentation for easy access.
- Refer to the SNS documentation to dive deeper into the features AWS SNS offers and understand how it all works. + Refer to the [AWS SNS documentation](https://docs.aws.amazon.com/sns/latest/dg/welcome.html) to dive deeper into the features AWS SNS offers and understand how it all works.
11-14: Clarify database field descriptions.The instructions for setting fields in the
notify_providerstable could be more explicit about what each field represents.- Create a new entry in table `notify_providers` and set the fields - `name`, `application_id`, `user_id` + Create a new entry in the `notify_providers` table and set the following fields: + - `name`: The name of the provider (e.g., "AWS SNS") + - `application_id`: The ID of the application using this provider + - `user_id`: The ID of the user configuring this provider
18-23: Ensure security of AWS credentials.Highlight the importance of securing AWS credentials and consider recommending environment variables for storing sensitive information.
- | AWS_ACCESS_KEY_ID | Access key for AWS instance | - | AWS_SECRET_ACCESS_KEY | Secret access key for AWS | - | AWS_REGION | Region of the AWS instance | + | AWS_ACCESS_KEY_ID | Access key for AWS instance (store securely) | + | AWS_SECRET_ACCESS_KEY | Secret access key for AWS (store securely) | + | AWS_REGION | Region of the AWS instance | + Note: It is recommended to store AWS credentials in environment variables for enhanced security.
35-45: Correct JSON formatting.Remove the trailing comma in the JSON example to ensure valid JSON syntax.
- "providerId": 11, - "data": { - "phoneNumber": "+1234567890", - "message": "This is a test notification", - } + "providerId": 11, + "data": { + "phoneNumber": "+1234567890", + "message": "This is a test notification" + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- apps/api/OsmoX.postman_collection.json (3 hunks)
- apps/api/docs/channels/sms-sns.md (1 hunks)
- apps/api/src/common/constants/notifications.ts (2 hunks)
- apps/api/src/common/decorators/is-data-valid.decorator.ts (2 hunks)
- apps/api/src/database/migrations/1723113193873-migration.ts (1 hunks)
- apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (1 hunks)
- apps/api/src/modules/notifications/dtos/providers/smsSns-data.dto.ts (1 hunks)
- apps/api/src/modules/notifications/notifications.module.ts (3 hunks)
- apps/api/src/modules/notifications/queues/queue.service.ts (3 hunks)
- apps/api/src/modules/providers/sms-sns/sms-sns.module.ts (1 hunks)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.spec.ts (1 hunks)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.spec.ts
Additional context used
Learnings (1)
apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (1)
Learnt from: LakshayaT PR: OsmosysSoftware/osmo-x#263 File: apps/api/src/jobs/consumers/notifications/waTwilio-notifications.job.consumer.ts:0-0 Timestamp: 2024-06-28T05:14:27.772Z Learning: Error handling for `processWaTwilioNotificationQueue` is implemented in the superclass method, as pointed out by the user LakshayaT.
Additional comments not posted (21)
apps/api/src/modules/notifications/dtos/providers/smsSns-data.dto.ts (1)
1-11: DTO Implementation Looks Good!The use of
class-validatordecorators to ensurephoneNumberandmessageare non-empty strings is appropriate for this DTO.apps/api/src/modules/providers/sms-sns/sms-sns.module.ts (1)
1-12: Module Setup Looks Good!The
SmsSnsModulecorrectly imports necessary modules and provides theSmsSnsServicealong with other services. It also exportsSmsSnsServicefor use in other parts of the application.apps/api/src/database/migrations/1723113193873-migration.ts (1)
1-20: Migration Script Looks Good!The migration script correctly adds and removes the SMS SNS provider entry in the
notify_master_providerstable. The configuration includes validation patterns for AWS credentials, which is a good practice.Ensure that this migration runs successfully in your development and staging environments before deploying to production.
apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (3)
1-2: Consider security implications of AWS credentials.Ensure that AWS credentials are securely managed and not hard-coded. Consider using environment variables or a secure vault.
10-11: Service class looks good.The
@Injectable()decorator is appropriately used for the service class.
14-17: Ensure Logger is properly configured.Verify that the
Loggerinstance is correctly configured and used consistently across the application.apps/api/src/common/constants/notifications.ts (2)
21-21: Addition ofSMS_SNSchannel type looks good.The new channel type
SMS_SNSis correctly added to theChannelTypeobject.
69-69: Update toSkipProviderConfirmationChannelsis appropriate.Including
ChannelType.SMS_SNSinSkipProviderConfirmationChannelsaligns with the intended functionality.apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (2)
11-12: Consumer class looks good.The
@Injectable()decorator is appropriately used for the consumer class.
25-33: Ensure consistent error handling.Ensure that error handling is consistent with other consumer implementations, particularly in the
processSmsSnsNotificationQueuemethod.apps/api/src/modules/notifications/notifications.module.ts (3)
59-59: LGTM! Integration ofSmsSnsModuleis correct.The
SmsSnsModulehas been correctly added to theproviderModulesarray.
73-73: LGTM! Integration ofSmsSnsNotificationConsumeris correct.The
SmsSnsNotificationConsumerhas been correctly added to theconsumersarray.
41-42: Ensure consistency in import paths.Verify that the import paths for
SmsSnsNotificationConsumerandSmsSnsModuleare correct and consistent with the project's structure.Verification successful
Import paths are consistent and correct.
The import paths for
SmsSnsNotificationConsumerandSmsSnsModuleare correctly aligned with the project's structure. No changes are needed.
SmsSnsNotificationConsumeris located atapps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts.SmsSnsModuleis located atapps/api/src/modules/providers/sms-sns/sms-sns.module.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the imported modules to ensure correct paths. # Test: Check for the existence of the SmsSnsNotificationConsumer and SmsSnsModule files. fd 'smsSns-notifications.job.consumer.ts' src/jobs/consumers/notifications fd 'sms-sns.module.ts' src/modules/providers/sms-snsLength of output: 364
Script:
#!/bin/bash # Search for the correct file paths of SmsSnsNotificationConsumer and SmsSnsModule in the repository. # Find the SmsSnsNotificationConsumer file fd 'smsSns-notifications.job.consumer.ts' # Find the SmsSnsModule file fd 'sms-sns.module.ts'Length of output: 200
apps/api/src/common/decorators/is-data-valid.decorator.ts (1)
131-136: LGTM! Validation logic forChannelType.SMS_SNSis correctly implemented.The validation logic for
ChannelType.SMS_SNSis consistent with other channel types and correctly usesSmsSnsDataDto.apps/api/src/modules/notifications/queues/queue.service.ts (3)
10-10: Import statement is correct.The import for
SmsSnsNotificationConsumeris correctly added and follows the existing pattern.
40-40: Constructor parameter addition is correct.The
smsSnsNotificationConsumerparameter is added consistently with the existing pattern for other consumers.
173-176: Switch case addition is correct.The new case for
SMS_SNScorrectly integrates thesmsSnsNotificationConsumerinto the notification processing workflow.apps/api/OsmoX.postman_collection.json (4)
3-7: Update of_postman_idand_exporter_idis appropriate.These changes ensure the uniqueness and traceability of the Postman collection.
4210-4214: Addition of global variablex-api-keyis beneficial.This change enhances the flexibility and maintainability of the collection.
2603-2841: SNS SMS Notification requests are well-structured.The requests include comprehensive test scripts for validating responses and handling error scenarios.
Ensure that the test scripts accurately reflect the expected API behavior.
Verification successful
Test script for "Send SNS SMS Notification - Success" is correctly implemented.
The test script effectively validates JSON structure and key properties, ensuring robust checks for successful notification creation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of test scripts for SNS SMS Notification requests. # Test: Search for SNS SMS Notification test scripts. Expect: Correct validation logic. rg --type json -A 5 $'"name": "Send SNS SMS Notification - Success"'Length of output: 441
Script:
#!/bin/bash # Extract the complete "exec" array for the "Send SNS SMS Notification - Success" test script rg --type json -A 50 $'"name": "Send SNS SMS Notification - Success"' apps/api/OsmoX.postman_collection.jsonLength of output: 2272
2363-2602: AWS SES Notification requests are well-structured.The requests include comprehensive test scripts for validating responses and handling error scenarios.
Ensure that the test scripts accurately reflect the expected API behavior.
Verification successful
Test scripts for AWS SES Notification are correctly implemented.
The test scripts for the "Send AWS SES Notification - Success" request validate the response structure and properties accurately, ensuring the API behaves as expected.
- The scripts check for JSON validity, correct status, and the presence of necessary fields like 'notification', 'channelType', and 'deliveryStatus'.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of test scripts for AWS SES Notification requests. # Test: Search for AWS SES Notification test scripts. Expect: Correct validation logic. rg --type json -A 5 $'"name": "Send AWS SES Notification - Success"'Length of output: 441
Script:
#!/bin/bash # Extract the complete test script for "Send AWS SES Notification - Success" to verify validation logic. rg --type json -A 20 '"name": "Send AWS SES Notification - Success"' apps/api/OsmoX.postman_collection.jsonLength of output: 1149
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apps/api/docs/channels/sms-sns.md (1)
1-59: Excellent documentation for AWS SNS integration! Just a couple of suggestions to further enhance it.The documentation provides a clear and comprehensive guide for integrating AWS SNS to send SMS notifications. The inclusion of configuration details, a sample request body, and relevant links is highly beneficial.
Here are a couple of suggestions for minor improvements:
Consider formatting the table in the "Values to Update in Database" section to improve readability. For example, you can use the following format:
Key Description AWS_ACCESS_KEY_ID Access key for AWS instance AWS_SECRET_ACCESS_KEY Secret access key for AWS AWS_REGION Region of the AWS instance In the "Dependencies" section, consider adding a brief explanation of why the
@aws-sdk/client-snspackage is required. For example:The
@aws-sdk/client-snspackage is required to interact with AWS SNS using the AWS SDK for JavaScript. It provides the necessary methods to send SMS messages via SNS.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- apps/api/OsmoX.postman_collection.json (36 hunks)
- apps/api/docs/channels/sms-sns.md (1 hunks)
- apps/api/src/common/constants/notifications.ts (2 hunks)
- apps/api/src/modules/notifications/dtos/providers/smsSns-data.dto.ts (1 hunks)
- apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (1 hunks)
Additional comments not posted (27)
apps/api/src/modules/notifications/dtos/providers/smsSns-data.dto.ts (1)
1-11: LGTM!The
SmsSnsDataDtoclass is well-defined and follows best practices:
- The class name follows the naming convention and includes the provider name for clarity.
- The
toandmessageproperties are appropriately named and typed.- The validation decorators ensure that the
toproperty is not empty and has a maximum length of 16 characters, which is a reasonable limit for phone numbers.- The validation decorators ensure that the
messageproperty is a non-empty string.The code changes are approved.
apps/api/src/common/constants/notifications.ts (2)
21-21: LGTM!The code changes are approved.
69-69: LGTM!The code changes are approved.
apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (5)
1-8: LGTM!The imports and interface declaration are appropriate.
10-17: LGTM!The
SmsSnsServiceclass and its constructor are properly implemented.
19-35: LGTM!The
assignSnsConfigmethod is implemented correctly and follows best practices for error handling.
37-53: LGTM!The
sendMessagemethod is implemented correctly and follows best practices for error handling.
1-54: Overall, theSmsSnsServiceclass is well-implemented and follows best practices.The file contains all the necessary elements, and the code is properly structured with appropriate error handling.
apps/api/OsmoX.postman_collection.json (19)
3-3: Skipping comment.The change to
_postman_idis non-impactful and can be skipped for review.
7-7: Skipping comment.The change to
_exporter_idis non-impactful and can be skipped for review.
1277-1277: LGTM!The updated description correctly reflects the Plivo SMS notification request.
1338-1338: LGTM!The updated description correctly reflects the Plivo SMS notification request.
1393-1393: LGTM!The updated description correctly reflects the Plivo SMS notification request.
1447-1447: LGTM!The updated description correctly reflects the Plivo SMS notification request.
1519-1519: LGTM!The updated description correctly specifies the Twilio WhatsApp Business API.
1580-1580: LGTM!The updated description correctly specifies the Twilio WhatsApp Business API.
1633-1633: LGTM!The updated description correctly specifies the Twilio WhatsApp Business API.
1687-1687: LGTM!The updated description correctly specifies the Twilio WhatsApp Business API.
1759-1759: LGTM!The updated description correctly reflects the KAPS SMS notification request.
1820-1820: LGTM!The updated description correctly reflects the KAPS SMS notification request.
1873-1873: LGTM!The updated description correctly reflects the KAPS SMS notification request.
1927-1927: LGTM!The updated description correctly reflects the KAPS SMS notification request.
1998-1999: LGTM!The updated description correctly reflects the AWS SES push notification request for Android.
2064-2065: LGTM!The updated description correctly reflects the AWS SES push notification request for iOS.
2137-2137: LGTM!The updated description correctly reflects the Twilio Voice Call notification request.
2198-2198: LGTM!The updated description correctly reflects the Twilio Voice Call notification request.
2251-2251: LGTM!The updated description correctly reflects the Twilio Voice Call notification request.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/api/src/common/constants/notifications.ts (2 hunks)
- apps/api/src/modules/notifications/queues/queue.service.ts (3 hunks)
Additional context used
Learnings (1)
apps/api/src/modules/notifications/queues/queue.service.ts (1)
Learnt from: LakshayaT PR: OsmosysSoftware/osmo-x#262 File: apps/api/src/modules/notifications/queues/queue.service.ts:87-87 Timestamp: 2024-07-03T08:38:36.915Z Learning: The `processNotificationQueue` method in the `NotificationConsumer` class includes error handling, which covers the `processMailgunNotificationQueue` method in the `MailgunNotificationConsumer` class.
Biome
apps/api/src/modules/notifications/queues/queue.service.ts
[error] 176-177: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
Additional comments not posted (4)
apps/api/src/common/constants/notifications.ts (2)
21-21: LGTM!The code changes are approved.
70-70: LGTM!The code changes are approved.
apps/api/src/modules/notifications/queues/queue.service.ts (2)
10-10: LGTM!The import statement is approved.
41-41: LGTM!The new private member is approved.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (1)
34-42: Consider declaringnotificationSendResponseasconst.Based on the learning from PR #262, consider declaring the
notificationSendResponsevariable asconstinstead ofletin theprocessSmsSnsNotificationQueuemethod, as it is never reassigned.async processSmsSnsNotificationQueue(id: number): Promise<void> { return super.processNotificationQueue(id, async () => { const notification = (await this.notificationsService.getNotificationById(id))[0]; - let notificationSendResponse = await this.smsSnsService.sendMessage( + const notificationSendResponse = await this.smsSnsService.sendMessage( notification.data as unknown as SmsSnsData, notification.providerId, ); return notificationSendResponse; }); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (1 hunks)
- apps/api/src/modules/notifications/queues/queue.service.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/notifications/queues/queue.service.ts
Additional context used
Learnings (1)
apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (2)
Learnt from: LakshayaT PR: OsmosysSoftware/osmo-x#263 File: apps/api/src/jobs/consumers/notifications/waTwilio-notifications.job.consumer.ts:0-0 Timestamp: 2024-06-28T05:14:27.772Z Learning: Error handling for `processWaTwilioNotificationQueue` is implemented in the superclass method, as pointed out by the user LakshayaT.Learnt from: ayushnvs PR: OsmosysSoftware/osmo-x#262 File: apps/api/src/jobs/consumers/notifications/mailgun-notifications.job.consumer.ts:0-0 Timestamp: 2024-07-03T13:27:46.550Z Learning: The `notificationSendResponse` variable in the `processMailgunNotificationConfirmationQueue` method of the `MailgunNotificationConsumer` class was changed from `let` to `const` to reflect that it is never reassigned.
Additional comments not posted (1)
apps/api/src/jobs/consumers/notifications/smsSns-notifications.job.consumer.ts (1)
1-43: LGTM!The code changes are approved. The
SmsSnsNotificationConsumerclass follows the same pattern as other notification consumers and theprocessSmsSnsNotificationQueuemethod is implemented correctly.
API PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Description:
This PR add support for sending sms using the sns
Related changes:
Screenshots:
NA
Query request and response:
Documentation changes:
NA
Test suite output:
NA
Pending actions:
NA
Additional notes:
NA
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes