chore(application-system): Add logging to notification service and make it awaitable #16821
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the functionality and logging capabilities of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
libs/application/template-api-modules/src/lib/notification/notifications.service.ts (1)
27-27: Update examples to use national IDs instead of email addressesThe JSDoc examples use email addresses for recipients, but based on the parameter description and type, it should be using national IDs. This could be misleading for other developers.
Consider updating the examples like this:
- recipient: '[email protected]', + recipient: '1234567890',Also applies to: 38-38, 52-52, 60-68
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (3)
224-247: Consider improving caseNumber fallback handlingWhile the Promise.all implementation for parallel notifications is excellent, the caseNumber fallback to an empty string might not be the best approach. Consider using a more descriptive placeholder or logging when caseNumber is missing.
-caseNumber: caseNumber || '', +caseNumber: caseNumber || 'PENDING', +// Add logging when caseNumber is missing +if (!caseNumber) { + this.logger.warn(`No case number available for application ${application.id}`); +}
301-322: Consider extracting syslumennName resolution logicThe Promise.all implementation for parallel notifications is good. However, the syslumennName resolution logic could be extracted to improve readability and reusability.
+private getSyslumennNameFromChildResidenceInfo( + childResidenceInfo: ReturnType<typeof childrenResidenceInfo> +): string { + return syslumennDataFromPostalCode( + childResidenceInfo?.future?.address?.postalCode || '' + ).name; +} async rejectedByOrganization({ application }: Props) { // ... existing code ... - const syslumennName = syslumennDataFromPostalCode( - childResidenceInfo?.future?.address?.postalCode || '', - ).name; + const syslumennName = this.getSyslumennNameFromChildResidenceInfo(childResidenceInfo); // ... rest of the code ...
Line range hint
185-322: Consider implementing a notification helper methodThe notification sending pattern is repeated across multiple methods. Consider implementing a helper method to reduce duplication and ensure consistent handling.
Example implementation:
private async sendApplicationNotification(params: { type: NotificationType; recipient: string; applicationId: string; args: Record<string, string>; }) { return this.notificationsService.sendNotification({ type: params.type, messageParties: { recipient: params.recipient, }, applicationId: params.applicationId, args: params.args, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts(5 hunks)libs/application/template-api-modules/src/lib/notification/notifications.module.ts(1 hunks)libs/application/template-api-modules/src/lib/notification/notifications.service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/notification/notifications.module.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/notification/notifications.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
libs/application/template-api-modules/src/lib/notification/notifications.module.ts (2)
5-5: LGTM: Clean import of LoggingModule
The import statement follows the established pattern and uses the proper scoped package.
8-8: LGTM: Module configuration is correct
The LoggingModule is properly added to the imports array while maintaining the existing configuration. This change appropriately supports the enhanced logging capabilities being added to the notification service.
libs/application/template-api-modules/src/lib/notification/notifications.service.ts (2)
1-7: LGTM: Import statements are well-structured
The new imports for logging functionality are properly organized and follow the project's import structure.
13-18: LGTM: Logger injection and configuration are well implemented
The constructor properly injects and configures the logger with a specific context, following dependency injection best practices.
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (2)
263-268: LGTM! Consistent implementation
The changes maintain consistency with the notification service updates, properly including the applicationId while preserving existing error handling.
185-191: LGTM! Verify notification handling
The changes correctly implement awaitable notifications with application ID tracking. The error handling for missing custody information remains robust.
Let's verify the notification handling:
✅ Verification successful
NotificationType.AssignCounterParty is properly defined and used
The enum member AssignCounterParty is correctly defined in notificationsTemplates.ts and used in children-residence-change.service.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NotificationType.AssignCounterParty is properly defined
ast-grep --pattern 'enum NotificationType {
$$$
AssignCounterParty = $_
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Retrieve the full definition of the NotificationType enum
ast-grep --pattern 'enum NotificationType {
$$$
}' --file-type typescript
Length of output: 273
Script:
#!/bin/bash
# Search for the definition of AssignCounterParty in the codebase
rg 'AssignCounterParty' --glob '*.ts'
Length of output: 488
libs/application/template-api-modules/src/lib/notification/notifications.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16821 +/- ##
=======================================
Coverage 36.43% 36.44%
=======================================
Files 6852 6853 +1
Lines 143537 143469 -68
Branches 40966 40940 -26
=======================================
- Hits 52303 52287 -16
+ Misses 91234 91182 -52
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 3 Passed Test Services
|
norda-gunni
added
automerge
…ke it awaitable (#16821) * chore(application-system): Add logging to notification service and make it awaitable * Add templateId for clarity --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Logging applicationIds alongside returned messageIds
Why
For better traceability
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
applicationIdparameter in notifications.Bug Fixes