feat(application-system): built in notifications #16352
Conversation
norda-gunni
force-pushed
the
as-notifications
branch
from
d57a7a5 to
fcd6c0c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16352 +/- ##
==========================================
- Coverage 36.77% 36.70% -0.07%
==========================================
Files 6838 6816 -22
Lines 141603 141433 -170
Branches 40338 40243 -95
==========================================
- Hits 52069 51909 -160
+ Misses 89534 89524 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 240 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
WalkthroughThe changes in this pull request involve the integration of 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: 11
🧹 Outside diff range and nitpick comments (8)
libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.module.ts (1)
10-10: LGTM: Addition of ApplicationsNotificationsModule to importsThe addition of
ApplicationsNotificationsModuleto the imports array is consistent with the new import statement and allows theReferenceTemplateModuleto use functionalities from the notifications module. This change adheres to the NestJS module structure.For improved readability, consider formatting the imports array vertically when it contains multiple items:
imports: [ SharedTemplateAPIModule, ApplicationsNotificationsModule, ],libs/clients/user-notification/src/lib/apiConfiguration.ts (1)
11-27: LGTM:exportedApisarray updated correctly with a minor suggestionThe addition of
NotificationsApito theexportedApisarray expands the functionality of the user notification client while maintaining reusability across different NextJS apps. The mapping function ensures both APIs use the same configuration and factory function, which is efficient and consistent.The change adheres to the coding guidelines:
- It maintains reusability of components across different NextJS apps.
- It uses TypeScript for defining types.
- It continues to use
createEnhancedFetch, which is good for effective bundling.For improved readability, consider extracting the configuration object into a separate constant:
const apiConfig = (config: ConfigType<typeof UserNotificationClientConfig>) => ({ fetchApi: createEnhancedFetch({ name: 'clients-user-notification', organizationSlug: 'stafraent-island', }), basePath: `${config.basePath}`, }); export const exportedApis = [UserNotificationApi, NotificationsApi].map( (Api) => ({ provide: Api, scope: LazyDuringDevScope, useFactory: (config: ConfigType<typeof UserNotificationClientConfig>) => new Api(new Configuration(apiConfig(config))), inject: [UserNotificationClientConfig.KEY], }), );This refactoring would make the code more modular and easier to maintain.
libs/application/templates/reference-template/src/dataProviders/index.ts (3)
3-6: Remove unused importUserProfileApiThe
UserProfileApiis imported but not used in this file. To maintain clean and efficient code, it's recommended to remove unused imports.Apply this diff to remove the unused import:
import { NationalRegistryUserApi, - UserProfileApi, } from '@island.is/application/types'
40-42: LGTM: NewNationalRegistryApiconfigurationThe addition of
NationalRegistryApiis consistent with the existing pattern and enhances the API definitions. Theorderproperty ensures it runs beforeReferenceDataApi.Consider adding a comment to explain the purpose of this API and why it needs to run first.
You could add a comment like this:
export const NationalRegistryApi = NationalRegistryUserApi.configure({ order: 1, // Runs first to fetch user data from the National Registry })
Line range hint
45-47: Remove duplicateMyParameterTypeinterfaceThe
MyParameterTypeinterface is duplicated at the end of the file. To maintain clean and maintainable code, it's recommended to remove this duplicate definition.Apply this diff to remove the duplicate interface:
-export interface MyParameterType { - id: number -}libs/application/template-api-modules/src/lib/notification/notificationsTemplates.ts (1)
12-45: LGTM: Well-structured notification configuration with type safetyThe
NotificationConfigconstant is well-implemented:
- It provides a clear mapping between notification types and their configurations.
- The use of type assertions for the
keysproperty ensures type safety.- The structure supports reusability across different NextJS apps.
Consider using a more robust type definition for the
keysproperty. Instead of using type assertions, you could define an interface for each notification type's keys. This would provide better type checking and autocompletion. For example:interface SystemNotificationKeys { documentId: string; } interface ChildrenResidenceChangeKeys { applicantName: string; applicationId: string; } // ... define interfaces for other notification types export const NotificationConfig: Record<NotificationType, { templateId: string; keys: SystemNotificationKeys | ChildrenResidenceChangeKeys | ... // union of all key types }> = { [NotificationType.System]: { templateId: 'HNIPP.TEST.INBOX.TEMPLATE', keys: {} as SystemNotificationKeys, }, // ... other configurations };This approach would provide stronger type checking and better IDE support.
libs/application/templates/reference-template/src/forms/Prerequisites.ts (1)
14-19: LGTM with a minor suggestion for import organization.The changes to the import statements look good and align with the coding guidelines. The separate import for
UserProfileApiand the replacement ofNationalRegistryUserApiwithNationalRegistryApiimprove code organization and potentially enhance reusability.Consider grouping related imports together for better readability. For example:
import { UserProfileApi } from '@island.is/application/types' import { ReferenceDataApi, NationalRegistryApi, MyMockProvider, } from '../dataProviders'This groups external and internal imports separately, which is a common practice in TypeScript projects.
libs/application/templates/reference-template/src/lib/ReferenceApplicationTemplate.ts (1)
Line range hint
120-124: Approve API configuration change with minor suggestionThe update to use
NationalRegistryApiis consistent with the import changes and maintains the existing functionality. The TypeScript usage for defining props is correctly maintained.Consider extracting the
ageToValidatevalue to a constant at the top of the file for better maintainability:const AGE_TO_VALIDATE = 18; // Then use it in the configuration: NationalRegistryApi.configure({ params: { ageToValidate: AGE_TO_VALIDATE, }, }),This change would improve readability and make it easier to update the age requirement if needed in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- apps/application-system/api/src/app/app.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts (1 hunks)
- libs/application/template-api-modules/src/lib/notification/notificationTypes.ts (1 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 (1 hunks)
- libs/application/template-api-modules/src/lib/notification/notificationsTemplates.ts (1 hunks)
- libs/application/templates/reference-template/src/dataProviders/index.ts (2 hunks)
- libs/application/templates/reference-template/src/forms/Prerequisites.ts (2 hunks)
- libs/application/templates/reference-template/src/lib/ReferenceApplicationTemplate.ts (2 hunks)
- libs/clients/user-notification/src/lib/apiConfiguration.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
apps/application-system/api/src/app/app.module.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.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/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/modules/templates/reference-template/reference-template.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/modules/templates/reference-template/reference-template.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/notificationTypes.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."
libs/application/template-api-modules/src/lib/notification/notificationsTemplates.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/templates/reference-template/src/dataProviders/index.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/templates/reference-template/src/forms/Prerequisites.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/templates/reference-template/src/lib/ReferenceApplicationTemplate.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/clients/user-notification/src/lib/apiConfiguration.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 (27)
libs/application/template-api-modules/src/lib/notification/notificationTypes.ts (4)
3-3: LGTM: Well-defined type for NotificationConfigThe
NotificationConfigTypeis correctly defined using thetypeofoperator, which ensures type safety and easy maintenance as it will automatically update ifNotificationConfigchanges.
4-4: LGTM: Effective type definition for notification keysThe
NotificationTypeKeytype is well-defined usingkeyof typeof, providing a type-safe way to reference keys inNotificationConfig. This enhances code quality by enabling autocomplete and type checking for notification types.
5-6: Excellent use of advanced TypeScript featuresThe
NotificationArgs<T>generic type is a great example of leveraging TypeScript's advanced features. It provides a type-safe way to access notification arguments specific to each notification type, enhancing code quality and developer experience.This implementation promotes:
- Type safety when working with notification arguments
- Code reusability across different parts of the application
- Better IDE support with autocomplete for specific notification types
1-6: Excellent implementation of notification typesThis file demonstrates high-quality TypeScript code that adheres to the coding guidelines for the
libsdirectory:
- It promotes reusability by exporting well-defined types.
- It effectively uses TypeScript for defining and exporting types.
- The lightweight type definitions should not interfere with tree-shaking and bundling.
The three type definitions work together to create a comprehensive and type-safe system for handling notifications. The use of advanced TypeScript features like
typeof,keyof, and generics showcases a deep understanding of the language and promotes maintainable, self-documenting code.libs/application/template-api-modules/src/lib/notification/notifications.module.ts (3)
1-4: Imports look good and align with coding guidelines.The imports are correctly using TypeScript, which is in line with the coding guidelines for files in the
libsdirectory. The use of@island.is/clients/user-notificationsuggests good modularity and potential for reusability across different NextJS apps.
6-11: Module structure adheres to best practices and coding guidelines.The
ApplicationsNotificationsModuleis well-structured and follows NestJS best practices. It correctly uses the@Moduledecorator and properly imports, provides, and exports the necessary components. This structure promotes reusability across different NextJS apps, which aligns with the coding guidelines for thelibsdirectory.The module also seems to follow effective tree-shaking practices by only importing and exporting what's necessary, which is crucial for optimal bundling.
1-11: Overall, excellent implementation of the ApplicationsNotificationsModule.This new module is well-structured, follows NestJS best practices, and adheres to the coding guidelines for the
libsdirectory. It promotes reusability and effective tree-shaking, which are crucial for optimal performance across different NextJS apps. The introduction of this module seems to be a valuable addition to the application system, potentially enhancing the notification handling capabilities.libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.module.ts (2)
8-8: LGTM: Import statement for ApplicationsNotificationsModuleThe import statement is correctly formatted and follows TypeScript conventions. It also aligns with the guideline for reusability across different NextJS apps by importing from within the same library.
Line range hint
1-14: Adherence to coding guidelines for libs//* files**The changes in this file adhere to the specified coding guidelines:
- Reusability: The module structure allows for reuse across different NextJS apps.
- TypeScript usage: The file correctly uses TypeScript for module definition.
- Tree-shaking: The specific imports and exports support effective tree-shaking and bundling practices.
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.module.ts (3)
7-7: LGTM: Import statement for notifications module.The import of
ApplicationsNotificationsModuleis correctly formatted and aligns with the PR objective of integrating built-in notifications.
15-15: LGTM: Integration of ApplicationsNotificationsModule.The addition of
ApplicationsNotificationsModuleto the imports array correctly integrates the notifications functionality into theChildrenResidenceChangeModuleV2. This change is consistent with the new import statement and follows NestJS module structure.
Line range hint
1-21: Verification: Compliance with coding guidelines for libs//* files.**The changes in this file adhere to the coding guidelines:
- The module maintains its reusability across different NextJS apps.
- TypeScript is used appropriately for defining the module structure.
- The changes do not negatively impact tree-shaking or bundling practices.
However, to ensure full compliance, please verify that the imported
ApplicationsNotificationsModuleis also designed with reusability in mind.To confirm the reusability of the
ApplicationsNotificationsModule, please run the following script:libs/clients/user-notification/src/lib/apiConfiguration.ts (1)
4-8: LGTM: Import statement updated correctlyThe addition of
NotificationsApito the import statement is consistent with its usage in theexportedApisarray. This change follows TypeScript best practices and adheres to the coding guidelines for TypeScript usage in library components.libs/application/templates/reference-template/src/dataProviders/index.ts (2)
13-13: LGTM: Addition oforderpropertyThe addition of the
orderproperty toReferenceDataApiis consistent with the existing pattern and helps in defining the execution order of API calls.
Line range hint
1-47: Overall review summaryThe changes in this file enhance the API definitions by introducing new configurations and properties. The code adheres to the coding guidelines for reusability and TypeScript usage. Minor improvements have been suggested to remove an unused import and a duplicate interface definition. After addressing these minor issues, the code will be in excellent shape.
libs/application/template-api-modules/src/lib/notification/notificationsTemplates.ts (2)
1-10: LGTM: Well-structured and type-safe enum implementationThe
NotificationTypeenum is well-implemented, following TypeScript best practices:
- It uses string literal types for improved type safety.
- The naming convention is consistent and descriptive.
- The enum is exported, allowing reuse across different modules.
This implementation supports effective tree-shaking and bundling practices.
1-45: Overall: Excellent adherence to coding guidelines and best practicesThis file demonstrates excellent adherence to the coding guidelines for
libs/**/*:
- The code is structured to support reusability across different NextJS apps.
- TypeScript is used effectively for defining props (in this case, notification configurations) and exporting types.
- The implementation supports effective tree-shaking and bundling practices.
The clear separation of concerns between the
NotificationTypeenum and theNotificationConfigobject enhances maintainability and scalability. The use of TypeScript's type system ensures type safety throughout the notification system.Great job on creating a well-structured and type-safe foundation for the notification system!
libs/application/templates/reference-template/src/forms/Prerequisites.ts (1)
Line range hint
1-124: Overall changes adhere to coding guidelines and improve code organization.The modifications in this file are focused on import statements and data provider configuration, while maintaining the overall structure of the form. These changes align well with the coding guidelines for files in the
libsdirectory:
- Reusability: The updated import structure and data provider configuration potentially enhance the reusability of components across different NextJS apps.
- TypeScript usage: The changes maintain proper TypeScript usage for defining types and exports.
- Effective tree-shaking: The more specific imports (e.g., separate import for
UserProfileApi) can contribute to better tree-shaking and bundling practices.The changes improve code organization without altering the core functionality of the form, which is a positive approach to code maintenance and evolution.
apps/application-system/api/src/app/app.module.ts (3)
63-63: LGTM: Import statement for UserNotificationClientConfigThe import statement for
UserNotificationClientConfigis correctly added and follows the established pattern for importing client configurations in this module.
Line range hint
1-128: Summary: Integration of UserNotificationClientConfigThe changes in this file successfully integrate the
UserNotificationClientConfiginto theAppModule. This addition aligns with the PR objective of building in notifications to the application system. The modifications are minimal, focused, and adhere to the existing code structure and TypeScript best practices.Key points:
- The import statement is correctly added.
- The configuration is properly integrated into the
ConfigModule.forRootmethod.- The changes should enable user notification functionality without disrupting existing features.
These modifications lay the groundwork for implementing user notifications in the application system. Ensure that any related components or services that will use this configuration are also updated accordingly.
123-123: LGTM: Integration of UserNotificationClientConfigThe
UserNotificationClientConfigis correctly added to theloadarray in theConfigModule.forRootmethod, following the established pattern for configuration integration.To ensure proper setup, please verify that any necessary environment variables or additional configurations for the UserNotificationClientConfig are in place. Run the following script to check for any related environment variables:
✅ Verification successful
Verified: UserNotificationClientConfig Integration Confirmed
The
UserNotificationClientConfigintegration in theConfigModule.forRootmethod has been successfully verified. Configuration is handled within the codebase without reliance on external environment variables.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for UserNotificationClientConfig related environment variables # Test: Search for environment variables related to UserNotificationClientConfig rg --type dotenv 'USER_NOTIFICATION|NOTIFICATION' .env*Length of output: 101
Script:
#!/bin/bash # Description: Check for UserNotificationClientConfig related environment variables # Test: Search for environment variables related to UserNotificationClientConfig rg 'USER_NOTIFICATION|NOTIFICATION' .env*Length of output: 56
Script:
#!/bin/bash # Description: Search the codebase for all occurrences of UserNotificationClientConfig # Search for the keyword in all relevant files rg 'UserNotificationClientConfig' .Length of output: 1098
libs/application/templates/reference-template/src/lib/ReferenceApplicationTemplate.ts (1)
Line range hint
1-364: Summary: Changes adhere to coding guidelinesThe modifications in this file, specifically the import changes and API configuration update, adhere to the coding guidelines for the
libs/**/*pattern:
- Reusability: The change to use a local import for
NationalRegistryApiimproves reusability across different NextJS apps.- TypeScript usage: The code maintains proper TypeScript usage for defining props and types.
- Tree-shaking and bundling: The changes don't negatively impact effective tree-shaking and bundling practices.
Overall, the changes are minimal, focused, and align well with the project's coding standards.
libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts (5)
13-14: Imports added correctly for notification functionalityThe import statements for
NotificationsServiceandNotificationTypeare correctly added and necessary for the notification functionality.
21-21: Dependency injection of NotificationsService addedThe
NotificationsServiceis correctly injected into the constructor, enabling notification capabilities within the service.
26-26: 'auth' parameter added to method signatureThe
authparameter is appropriately added to thegetReferenceDatamethod, allowing access to authentication information needed for sending notifications.
29-32: Retrieval of applicant's full nameThe
applicantNameis correctly retrieved fromapplication.externalData, ensuring the accurate name is used in notifications.
55-55: Updated return value includes 'applicantName'The return data now correctly includes
applicantName, ensuring consistent usage of the variable.
libs/application/templates/reference-template/src/forms/Prerequisites.ts
Show resolved
Hide resolved
libs/application/templates/reference-template/src/lib/ReferenceApplicationTemplate.ts
Show resolved
Hide resolved
...plate-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts
Outdated
Show resolved
Hide resolved
...plate-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts
Show resolved
Hide resolved
.../src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
.../src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
.../src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/notification/notifications.service.ts
Outdated
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/notification/notifications.service.ts
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/notification/notifications.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (5)
Line range hint
1-43: LGTM! Consider using type inference for ConfigService.The changes to imports and constructor align well with the transition to a unified notification system. The use of TypeScript for imports and type definitions is good for maintainability.
Consider using type inference for ConfigService instead of explicitly specifying SharedModuleConfig:
- private readonly configService: ConfigService<SharedModuleConfig>, + private readonly configService: ConfigService,This change allows for better flexibility if the config structure changes in the future.
169-195: LGTM! Consider extracting the notification logic.The changes in the
sendNotificationToCounterPartymethod effectively address the previous issue of potential undefined access and align well with the new unified notification system.Consider extracting the notification logic into a separate private method for better readability and reusability:
private sendCounterPartyNotification(applicant: any, otherParent: any, contractLink: string) { this.notificationsService.sendNotification({ type: NotificationType.AssignCounterParty, messageParties: { recipient: otherParent.nationalId, sender: applicant.nationalId, }, args: { applicantName: applicant.fullName, contractLink, }, }); }Then call this method in
sendNotificationToCounterParty:if (otherParent) { this.sendCounterPartyNotification(applicant, otherParent, contractLink); }This refactoring improves the method's readability and makes the notification logic more reusable.
201-242: LGTM! Consider consolidating duplicate notification logic.The changes in the
approvedByOrganizationmethod effectively address the previous issue of potential undefined access and align well with the new unified notification system.Consider consolidating the duplicate notification logic into a single method call:
private sendApprovalNotification(recipientId: string, applicationLink: string, caseNumber: string) { this.notificationsService.sendNotification({ type: NotificationType.ChildrenResidenceChangeApprovedByOrg, messageParties: { recipient: recipientId, }, args: { applicationLink, caseNumber: caseNumber || '', }, }); }Then in the
approvedByOrganizationmethod:this.sendApprovalNotification(applicant.nationalId, applicationLink, caseNumber || ''); this.sendApprovalNotification(otherParent.nationalId, applicationLink, caseNumber || '');This refactoring reduces code duplication and improves maintainability.
270-313: LGTM! Consider extracting common logic for notifications.The changes in the
rejectedByOrganizationmethod effectively address the previous issue of potential undefined access and align well with the new unified notification system. The addition of error handling for missing custody information and other parent data is a good improvement.Consider extracting the common notification logic into a separate method to reduce duplication:
private sendRejectionNotification(recipientId: string, orgName: string) { this.notificationsService.sendNotification({ type: NotificationType.RejectedByOrganization, messageParties: { recipient: recipientId, }, args: { orgName, }, }); }Then in the
rejectedByOrganizationmethod:this.sendRejectionNotification(applicant.nationalId, syslumennName); this.sendRejectionNotification(otherParent.nationalId, syslumennName);This refactoring improves code maintainability and reduces the risk of inconsistencies between notifications.
315-323: LGTM! Consider using template literals for URL construction.The new
getApplicationLinkmethod is a good addition that centralizes the link generation logic, improving code reusability. The use of the injected ConfigService aligns well with dependency injection principles.Consider using template literals for cleaner URL construction:
private async getApplicationLink(application: CRCApplication) { const clientLocationOrigin = getConfigValue( this.configService, 'clientLocationOrigin', ) as string const slug = getSlugFromType(application.typeId) as string return `${clientLocationOrigin}/${slug}/${application.id}` as string }This change makes the URL construction more readable and less prone to errors from string concatenation.
📜 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 (4 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts (1 hunks)
- libs/application/template-api-modules/src/lib/notification/notifications.service.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/template-api-modules/src/lib/notification/notifications.service.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/modules/templates/reference-template/reference-template.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 (7)
libs/application/template-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts (5)
13-14: LGTM: Imports for notification functionality added correctlyThe new imports for
NotificationsServiceandNotificationTypeare correctly added and follow TypeScript conventions. These specific imports will aid in effective tree-shaking.
21-21: LGTM: NotificationsService correctly injectedThe
NotificationsServiceis properly injected as a dependency in the constructor. This follows dependency injection principles and Angular/NestJS conventions, supporting the new notification functionality.
26-26: LGTM: Method signature updated correctlyThe
getReferenceDatamethod signature has been appropriately updated to include theauthparameter fromTemplateApiModuleActionProps. This change enables access to authentication information needed for the new notification functionality.
53-53: LGTM: Return object updated correctlyThe return object has been correctly updated to use the
applicantNamevariable, which is consistent with the changes made earlier in the method.
Line range hint
1-124: LGTM: Changes adhere to coding guidelinesThe modifications to this file in the
libsdirectory adhere to the specified coding guidelines:
- TypeScript is used effectively for defining types and props.
- The changes enhance the reusability of the
ReferenceTemplateServiceacross different NextJS apps by adding notification functionality.- The code structure supports effective tree-shaking and bundling practices.
These changes maintain code quality and improve the overall functionality of the service.
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change-v2/children-residence-change.service.ts (2)
246-266: LGTM! Improved error handling and notification system integration.The changes in the
rejectedByCounterPartymethod effectively address the previous issue of potential undefined access and align well with the new unified notification system. The addition of error handling for missing custody information is a good improvement.The use of the
NotificationsServicefor sending notifications is consistent with the overall changes in the class, promoting a more unified approach to user communication.
Line range hint
1-324: Great job on implementing the unified notification system!The changes in this file significantly improve the notification handling capabilities of the
ChildrenResidenceChangeServiceV2class. The transition to using theNotificationsServiceacross all methods ensures a consistent approach to user communication. The addition of error handling and null checks enhances the robustness of the code.Key improvements:
- Consistent use of the new
NotificationsService.- Better error handling and null checks throughout the methods.
- Introduction of the
getApplicationLinkmethod for improved code reusability.- Removal of unused services from the constructor.
These changes align well with the PR objectives and maintain good coding practices such as TypeScript usage and code reusability. The overall structure of the application remains intact while significantly enhancing its functionality.
Great work on this implementation! The code is now more maintainable, robust, and aligned with the project's notification system goals.
...plate-api-modules/src/lib/modules/templates/reference-template/reference-template.service.ts
Show resolved
Hide resolved
norda-gunni
added
the
deprecated:automerge
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
NotificationsServicefor improved error handling and validation.ApplicationsNotificationsModuleto various templates for streamlined notification management.Bug Fixes
NotificationsServicefor sending notifications.Documentation
NotificationsApialongside existing APIs.