chore(j-s): Notifications to advocates in civil claims cases #16264
Conversation
…-is/island.is into j-s/civil-claimant-courts
WalkthroughThis pull request introduces multiple changes across the judicial system application, primarily focusing on updating the notification system to accommodate a new role designation for advocates. Key modifications include a migration script for updating the database enum values, adjustments to notification types and related logic in various services and components, and the introduction of a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
📓 Learnings (1)
🔇 Additional comments (3)
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
|
…nd.is into j-s/advocate-notifications
Datadog ReportAll test runs ❌ 24 Total Test Services: 1 Failed, 23 Passed Test ServicesThis report shows up to 10 services
❌ Failed Tests (2)
🔻 Code Coverage Decreases vs Default Branch (1)
|
oddsson
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: 7
🧹 Outside diff range and nitpick comments (16)
libs/judicial-system/types/src/lib/advocate.ts (1)
45-49: LGTM! Consider adding JSDoc comments for better documentation.The
AdvocateTypeenum is a good addition that aligns with the PR objectives and provides type safety for different advocate roles. The naming convention and export are correct, allowing for reuse across different NextJS apps.Consider adding JSDoc comments to describe the purpose of this enum and each of its values. This would enhance the self-documentation of the code. For example:
/** * Represents the different types of advocates in the judicial system. */ export enum AdvocateType { /** A defender appointed to represent the accused. */ DEFENDER = 'DEFENDER', /** A lawyer representing a party in a civil case. */ LAWYER = 'LAWYER', /** A legal professional protecting the rights of involved parties. */ LEGAL_RIGHTS_PROTECTOR = 'LEGAL_RIGHTS_PROTECTOR', }apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendDefenderAssignedNotifications.spec.ts (1)
42-42: LGTM! Consider updating the test description for clarity.The change from
DEFENDER_ASSIGNEDtoADVOCATE_ASSIGNEDaligns with the PR objective of implementing notifications for advocates in civil claims cases. Good job on maintaining type safety with TypeScript.Consider updating the test description to reflect the new notification type:
-describe('NotificationController - Send defender assigned notification', () => { +describe('NotificationController - Send advocate assigned notification', () => {This will make the test description more accurate and consistent with the new functionality.
apps/judicial-system/backend/migrations/20241003213354-update-notification.js (3)
1-3: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in ES6 modules as they are automatically in strict mode.
Apply this diff to remove the redundant directive:
-'use strict' const replaceEnum = require('sequelize-replace-enum-postgres').default🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
5-33: Approve changes with a minor suggestionThe
upmethod correctly implements the new 'ADVOCATE_ASSIGNED' enum value, aligning with the PR objective. The enum values are comprehensive and include all necessary notification types.Consider enhancing the comment about transaction support for better clarity:
- // replaceEnum does not support transactions + // Note: replaceEnum does not support transactions. Ensure the migration is run in isolation.
36-65: Approve rollback logic with a minor suggestionThe
downmethod correctly reverts the changes, replacing 'ADVOCATE_ASSIGNED' with 'DEFENDER_ASSIGNED'. This ensures proper rollback functionality and maintains backwards compatibility.As suggested for the
upmethod, consider enhancing the comment about transaction support:- // replaceEnum does not support transactions + // Note: replaceEnum does not support transactions. Ensure the migration is run in isolation.apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendCourtDateNotification.spec.ts (1)
85-85: LGTM! Consider adding a test for the originalDEFENDER_ASSIGNEDtype.The change from
DEFENDER_ASSIGNEDtoADVOCATE_ASSIGNEDaligns well with the PR objectives of implementing notifications for advocates in civil claims cases. This update ensures that the test suite covers the new notification type.To maintain comprehensive test coverage, consider adding a separate test case for the
DEFENDER_ASSIGNEDnotification type if it's still in use elsewhere in the system. This would ensure both notification types are properly tested.apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts (1)
53-53: LGTM: Consistent update across all district court rolesThe addition of
NotificationType.ADVOCATE_ASSIGNEDfor the District Court Assistant role completes the consistent update across all three district court roles (Judge, Registrar, and Assistant). This change ensures that all relevant roles can send advocate assignment notifications, fully aligning with the PR objective.Consider adding a unit test to verify that all district court roles (Judge, Registrar, and Assistant) have the same notification types in their
dtoFieldValuesarrays. This will help catch any future inconsistencies if one role is updated but others are missed.apps/judicial-system/backend/src/app/modules/notification/notification.service.ts (3)
Line range hint
65-69: LGTM! Consider adding a comment for clarity.The change from
NotificationType.DEFENDER_ASSIGNEDtoNotificationType.ADVOCATE_ASSIGNEDis correct and aligns with the PR objectives. This ensures that advocates receive notifications when assigned to a case, similar to defenders.Consider adding a brief comment explaining why we're sending an
ADVOCATE_ASSIGNEDnotification in theCOURT_DATEcase wheneventOnlyis true. This would improve code readability and maintainability.if (eventOnly) { this.eventService.postEvent('SCHEDULE_COURT_DATE', theCase, true) + // Send an advocate assignment notification even when not sending a calendar invitation messages = [ this.getNotificationMessage( NotificationType.ADVOCATE_ASSIGNED, user, theCase, ), ] } else {
75-78: LGTM! Consider grouping related notification types.The addition of
NotificationType.ADVOCATE_ASSIGNEDto this case block is correct and aligns with the PR objectives. This ensures that advocate assignment notifications are processed consistently with other notification types.For better code organization and readability, consider grouping related notification types together. For example:
-case NotificationType.HEADS_UP: -case NotificationType.ADVOCATE_ASSIGNED: -case NotificationType.APPEAL_JUDGES_ASSIGNED: -case NotificationType.APPEAL_CASE_FILES_UPDATED: +// Assignment notifications +case NotificationType.ADVOCATE_ASSIGNED: +case NotificationType.APPEAL_JUDGES_ASSIGNED: +// Update notifications +case NotificationType.HEADS_UP: +case NotificationType.APPEAL_CASE_FILES_UPDATED:This grouping can make it easier to understand the different categories of notifications at a glance.
Line range hint
1-92: Overall, the changes look good and align with the PR objectives.The modifications to the
NotificationServiceclass successfully implement notifications for advocates in civil claims cases. The changes are consistent with the existing code structure and maintain proper error handling.To further improve the code, consider the following suggestion:
- Extract the notification type mapping logic into a separate method or configuration object. This would make it easier to add or modify notification types in the future without changing the main
switchstatement. For example:private getNotificationConfig(type: NotificationType): { eventOnly: boolean, messages: Message[] } { // Define configuration for each notification type // Return the appropriate configuration } // In addNotificationMessagesToQueue const config = this.getNotificationConfig(type); if (config.eventOnly) { this.eventService.postEvent('SCHEDULE_COURT_DATE', theCase, true); } messages = config.messages;This refactoring would improve the maintainability and scalability of the notification system.
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/Advocates.tsx (1)
36-36: Approved: Notification type updated correctly.The change from
NotificationType.DEFENDER_ASSIGNEDtoNotificationType.ADVOCATE_ASSIGNEDaligns with the PR objective of implementing notifications for advocates in civil claims cases. This update ensures that the correct notification type is sent when an advocate is assigned.To improve clarity, consider adding a comment explaining the reason for this change:
// Use ADVOCATE_ASSIGNED notification type to cover both defenders and civil claimant advocates await sendNotification(workingCase.id, NotificationType.ADVOCATE_ASSIGNED)This comment would help future developers understand the broader context of this notification type.
apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (2)
42-42: LGTM: Prop type updated to use AdvocateType enumThe change to use the
AdvocateTypeenum for theadvocateTypeprop improves type safety and clarity. This aligns well with TypeScript best practices.Consider adding a default value for the
advocateTypeprop in the component's declaration to improve its usability:const InputAdvocate: FC<Props> = ({ // ... other props advocateType = AdvocateType.DEFENDER, // ... rest of the component }) => { // ... }
Line range hint
1-497: Summary: InputAdvocate component successfully updated to use AdvocateType enumThe changes to the
InputAdvocatecomponent have successfully integrated the newAdvocateTypeenum, improving type safety and code clarity. These updates include:
- Importing the
AdvocateTypeenum- Updating the
advocateTypeprop type- Modifying label rendering logic to use the enum
These changes align well with NextJS and TypeScript best practices, and indirectly support the PR objective of updating notifications for advocates in civil claims cases by improving the robustness of the advocate input component.
Consider reviewing other components that may interact with this one to ensure consistent use of the
AdvocateTypeenum throughout the application. This will help maintain the improved type safety across the entire codebase.apps/judicial-system/backend/src/app/formatters/formatters.ts (1)
Line range hint
674-683: Approve changes with a minor suggestionThe addition of the
advocateTypeparameter to theformatDefenderAssignedEmailNotificationfunction allows for more specific email notifications based on the type of advocate. This change aligns well with the PR objectives of implementing notifications for lawyers and legal rights protectors in civil claim cases.However, I suggest adding a type annotation for the
advocateTypeparameter to improve type safety and code clarity.Consider adding a type annotation to the
advocateTypeparameter:export const formatDefenderAssignedEmailNotification = ( formatMessage: FormatMessage, theCase: Case, - advocateType: AdvocateType, + advocateType: AdvocateType, overviewUrl?: string, ): SubjectAndBody => {apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (1)
Line range hint
301-312: Ensuredefendantis defined before useIn the
it('should record notification', () => { ... })block, ensure that thedefendantvariable is defined in the test scope before being used in the expectations to prevent reference errors.If
defendantis defined in an outer scope or a beforeEach block, consider declaring it within the test scope or ensuring it's accessible where needed.apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts (1)
Line range hint
1547-1552: Incorrect Email Parameter in Notification CheckIn the
shouldSendAdvocateAssignedNotificationmethod, when checking if the advocate has already been notified, the code usestheCase.defenderEmailinstead of theadvocateEmailparameter. This could lead to incorrect behavior if the advocate's email differs from the defender's email. To ensure the correct advocate is checked, you should useadvocateEmailin thehasReceivedNotificationcall.Apply this diff to fix the issue:
const hasDefenderBeenNotified = this.hasReceivedNotification( [ NotificationType.READY_FOR_COURT, NotificationType.COURT_DATE, NotificationType.ADVOCATE_ASSIGNED, ], - theCase.defenderEmail, + advocateEmail, theCase.notifications, ) if (hasDefenderBeenNotified) { return false }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
- apps/judicial-system/backend/migrations/20241003213354-update-notification.js (1 hunks)
- apps/judicial-system/backend/src/app/formatters/formatters.ts (3 hunks)
- apps/judicial-system/backend/src/app/messages/notifications.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts (3 hunks)
- apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts (8 hunks)
- apps/judicial-system/backend/src/app/modules/notification/notification.service.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (11 hunks)
- apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendCourtDateNotification.spec.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendDefenderAssignedNotifications.spec.ts (2 hunks)
- apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (5 hunks)
- apps/judicial-system/web/src/routes/Court/Indictments/Advocates/Advocates.tsx (1 hunks)
- libs/judicial-system/types/src/index.ts (1 hunks)
- libs/judicial-system/types/src/lib/advocate.ts (1 hunks)
- libs/judicial-system/types/src/lib/notification.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
apps/judicial-system/backend/migrations/20241003213354-update-notification.js (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."
apps/judicial-system/backend/src/app/formatters/formatters.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."
apps/judicial-system/backend/src/app/messages/notifications.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."
apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.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."
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.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."
apps/judicial-system/backend/src/app/modules/notification/notification.service.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."
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.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."
apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendCourtDateNotification.spec.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."
apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendDefenderAssignedNotifications.spec.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."
apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (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."
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/Advocates.tsx (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/judicial-system/types/src/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/judicial-system/types/src/lib/advocate.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/judicial-system/types/src/lib/notification.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."
🪛 Biome
apps/judicial-system/backend/migrations/20241003213354-update-notification.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (14)
libs/judicial-system/types/src/lib/advocate.ts (1)
Line range hint
1-64: Great job on maintaining code quality and following guidelines!The file structure and content adhere well to the coding guidelines for the
libsdirectory:
- The types and utility function are reusable across different NextJS apps.
- TypeScript is effectively used for defining props and exporting types.
- The exported entities support effective tree-shaking and bundling.
The addition of the
AdvocateTypeenum enhances the type system without breaking existing code, and the overall file structure remains clean and logical.apps/judicial-system/backend/src/app/modules/notification/test/notificationController/sendDefenderAssignedNotifications.spec.ts (1)
64-64: LGTM! Verify consistency across the codebase.The change from
DEFENDER_ASSIGNEDtoADVOCATE_ASSIGNEDin the message body is consistent with the previous modification and aligns with the PR objective.To ensure consistency across the codebase, let's verify that all occurrences of
DEFENDER_ASSIGNEDhave been updated toADVOCATE_ASSIGNEDwhere appropriate:This will help identify any places where the notification type might have been missed during the update.
✅ Verification successful
Verified! All instances of
DEFENDER_ASSIGNEDhave been successfully updated toADVOCATE_ASSIGNED.The shell script results show that there are no remaining occurrences of
DEFENDER_ASSIGNEDand thatADVOCATE_ASSIGNEDis consistently used across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of DEFENDER_ASSIGNED that might need updating # Test: Search for DEFENDER_ASSIGNED in TypeScript files rg --type typescript 'DEFENDER_ASSIGNED' # Test: Search for ADVOCATE_ASSIGNED to confirm the changes rg --type typescript 'ADVOCATE_ASSIGNED'Length of output: 151
Script:
#!/bin/bash # Description: Check for any remaining instances of DEFENDER_ASSIGNED that might need updating # Test: Search for DEFENDER_ASSIGNED in TypeScript files rg 'DEFENDER_ASSIGNED' -g '*.ts' -g '*.tsx' # Test: Search for ADVOCATE_ASSIGNED to confirm the changes rg 'ADVOCATE_ASSIGNED' -g '*.ts' -g '*.tsx'Length of output: 4580
apps/judicial-system/backend/migrations/20241003213354-update-notification.js (1)
1-66: Overall assessment: Well-implemented migration with minor suggestionsThis migration script effectively implements the required changes for the new advocate notification feature. It correctly updates the enum values in the
notificationtable, adding 'ADVOCATE_ASSIGNED' and providing a proper rollback mechanism.Key points:
- The changes align well with the PR objectives.
- Both
upanddownmethods are correctly implemented.- The use of
sequelize-replace-enum-postgresis appropriate for this task.Minor suggestions have been made to improve code quality and documentation. Once these are addressed, the migration script will be ready for implementation.
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
libs/judicial-system/types/src/index.ts (1)
100-100: LGTM: Export changes align with PR objectives and coding guidelines.The modification to export
Lawyer,mapToLawyer, and the newAdvocateTypefrom./lib/advocateinstead of./lib/defenderaligns well with the PR objectives of implementing notifications for advocates in civil claims cases. This change enhances the specificity of the types and functions related to advocates.The update adheres to the coding guidelines for
libs/**/*files:
- Reusability: By exporting these types and functions from a central location, it promotes reusability across different NextJS apps in the judicial system.
- TypeScript usage: The export statement correctly uses TypeScript syntax for type exports.
- Tree-shaking and bundling: The specific export of named entities (
Lawyer,mapToLawyer,AdvocateType) allows for effective tree-shaking in the bundling process.To ensure the
Lawyertype is no longer exported from the defender module, run the following script:apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts (1)
42-42: LGTM: Consistent update for District Court RegistrarThe addition of
NotificationType.ADVOCATE_ASSIGNEDfor the District Court Registrar role is consistent with the change made for the District Court Judge. This update maintains uniformity across roles and aligns with the PR objective.apps/judicial-system/web/src/routes/Court/Indictments/Advocates/Advocates.tsx (1)
Line range hint
1-103: Overall assessment: Well-structured component with appropriate change.The
Advocatescomponent is well-structured and follows NextJS and TypeScript best practices. The use of hooks, context, and TypeScript typing is appropriate throughout the file. The single change made to update the notification type is correct and aligns with the PR objectives.Key observations:
- Proper use of NextJS routing with
useRouter.- Effective use of React hooks and context.
- Type-safe usage of enums and props.
- Clear separation of concerns within the component.
The change doesn't introduce any new warnings or errors and maintains the overall code quality of the file.
apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (3)
15-15: LGTM: Import statement updated correctlyThe import of
AdvocateTypefrom the types package is appropriate and aligns with NextJS best practices for type imports.
Line range hint
322-327: LGTM: Label rendering logic updated correctlyThe condition for rendering the label has been properly updated to use the
AdvocateType.LEGAL_RIGHTS_PROTECTORenum value. This change maintains the existing logic while improving type safety and readability.
Line range hint
370-375: LGTM: Email and phone number label rendering logic updated correctlyThe conditions for rendering the email and phone number labels have been properly updated to use the
AdvocateType.LEGAL_RIGHTS_PROTECTORenum value. These changes maintain the existing logic while improving type safety and readability, and are consistent with the previous label rendering logic update.Also applies to: 484-489
apps/judicial-system/backend/src/app/formatters/formatters.ts (1)
17-21: LGTM: Import statement updated correctlyThe addition of
AdvocateTypeto the import statement is consistent with the changes made to theformatDefenderAssignedEmailNotificationfunction. The reorganization of the import statement improves code readability by grouping related types together.apps/judicial-system/backend/src/app/messages/notifications.ts (1)
618-620: LGTM! Improved notification specificity for different advocate types.The changes to the
defenderAssignedEmail.bodymessage enhance the notification system by distinguishing between different types of advocates (LAWYER, LEGAL_RIGHTS_PROTECTOR, and others). This improvement aligns well with the PR objectives to implement notifications for lawyers and legal rights protectors in civil claims cases.The update provides more specific and relevant information to the recipients, which should improve clarity and reduce potential confusion about their role in the case.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (3)
83-83: Change NotificationType to ADVOCATE_ASSIGNED is appropriateThe update from
NotificationType.DEFENDER_ASSIGNEDtoNotificationType.ADVOCATE_ASSIGNEDcorrectly reflects the new notification logic for advocate assignments.
Line range hint
570-588: Confirm that no email is sent for investigation casesThe test correctly checks that no email is sent when handling investigation cases with
NotificationType.ADVOCATE_ASSIGNED. This behavior aligns with the expected logic.
128-176: Ensure consistency in email content and variablesThe new test case for when the case has civil claims and the advocate is a lawyer is well-structured. However, consider verifying that all variable names and email content accurately reflect the role of an advocate instead of a defender to maintain consistency and avoid confusion.
Run the following script to check for instances of 'defender' in the advocate test cases:
apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts
Show resolved
Hide resolved
...s/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
Outdated
Show resolved
Hide resolved
...s/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts
Outdated
Show resolved
Hide resolved
…and.is into j-s/advocate-notifications
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts (2)
Line range hint
1523-1566: Approved with a minor suggestion for improvementThe
shouldSendAdvocateAssignedNotificationfunction looks well-implemented and covers different case types appropriately. It correctly checks for previous notifications to avoid duplicates.To improve readability, consider extracting the session arrangement check into a separate constant:
+ const isDefenderIncludedInSessionArrangements = [ + SessionArrangements.ALL_PRESENT, + SessionArrangements.ALL_PRESENT_SPOKESPERSON, + ].includes(theCase.sessionArrangements); - if (!isDefenderIncludedInSessionArrangements) return false + if (!isDefenderIncludedInSessionArrangements) { + return false; + }This change would make the condition more self-explanatory and consistent with the coding style used elsewhere in the file.
Line range hint
1593-1679: Approved with suggestions for improvementThe
sendAdvocateAssignedNotificationsfunction is well-implemented and correctly handles different case types. However, there are a few suggestions to improve its structure and efficiency:
Consider extracting the logic for sending notifications to defendants and civil claimants into separate functions to reduce nesting and improve readability.
The civil claimant notification logic can be optimized. Instead of checking
shouldSendinside the loop, you can filter the civil claimants first:const eligibleCivilClaimants = theCase.civilClaimants?.filter( (claimant) => this.shouldSendAdvocateAssignedNotification(theCase, claimant.spokespersonEmail) && claimant.hasSpokesperson ) ?? []; eligibleCivilClaimants.forEach((claimant) => { promises.push( this.sendAdvocateAssignedNotification( theCase, claimant.spokespersonIsLawyer ? AdvocateType.LAWYER : AdvocateType.LEGAL_RIGHTS_PROTECTOR, claimant.spokespersonNationalId, claimant.spokespersonName, claimant.spokespersonEmail ) ); });
- Consider using
Promise.allfor defendants as well, similar to how it's used for civil claimants, to process all notifications in parallel.These changes would make the function more efficient and easier to read without changing its core functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts (8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.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."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts (2)
1568-1590: LGTM! Well-implemented functionThe
sendAdvocateAssignedNotificationfunction is implemented correctly and follows the established pattern for notification functions in this file. It properly uses the formatter function to generate email content and handles the case where an advocate might not have a national ID. The function is concise and clear in its purpose.
2568-2569: LGTM! Proper integration of new notification typeThe addition of the
NotificationType.ADVOCATE_ASSIGNEDcase in thesendCaseNotificationfunction is implemented correctly. It follows the existing pattern in the switch statement and properly calls thesendAdvocateAssignedNotificationsfunction for this new notification type. This change seamlessly integrates the new advocate assigned notifications into the existing notification system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/messages/notifications.ts (1)
610-634: LGTM! Consider adding comments for clarity.The restructuring of the
advocateAssignedEmailobject enhances the flexibility of the notification system. The new fields allow for more specific messages based on the advocate type and their access to case files, which is a good improvement.Consider adding brief comments above each field to explain when each message is used. This would improve code readability and maintainability. For example:
advocateAssignedEmail: defineMessages({ // Used when the advocate has access to case files subjectAccessToCaseFiles: { // ... existing code ... }, // Used when notifying about general case access subjectAccess: { // ... existing code ... }, // ... add comments for other fields ... }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/judicial-system/backend/src/app/formatters/formatters.ts (2 hunks)
- apps/judicial-system/backend/src/app/messages/notifications.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/formatters/formatters.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."
apps/judicial-system/backend/src/app/messages/notifications.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."
🔇 Additional comments (4)
apps/judicial-system/backend/src/app/formatters/formatters.ts (4)
17-21: Import statement updated to includeAdvocateTypeThe import statement has been updated to include
AdvocateTypefrom the@island.is/judicial-system/typesmodule. This change is necessary to support the new parameter in theformatAdvocateAssignedEmailNotificationfunction.
671-709: Summary of changes to advocate notification systemThe modifications to the
formatAdvocateAssignedEmailNotificationfunction significantly enhance the flexibility and specificity of the notification system for different advocate types in civil claims cases. By introducing theAdvocateTypeparameter and implementing conditional logic, the function now generates tailored notifications for defenders and other advocate types.These changes align well with the PR objectives of implementing notifications for lawyers and legal rights protectors in civil claim cases. The implementation is clean, follows TypeScript best practices, and improves the overall modularity of the notification system.
677-709: Conditional logic added to handle different advocate typesThe function now uses conditional statements to format the subject and body of the email based on the
advocateType. This allows for more specific and relevant notifications for different types of advocates.Key changes:
- Different subject lines for defenders and other advocate types.
- Different body content for defenders and other advocate types.
- Utilization of the
advocateTypeparameter in the formatting of the email body.These changes improve the specificity and relevance of the notifications sent to different types of advocates.
To ensure that all possible
AdvocateTypevalues are handled correctly, let's verify the implementation:#!/bin/bash # Search for the definition of AdvocateType enum rg "enum AdvocateType" --type typescript # Search for usage of AdvocateType in the codebase rg "AdvocateType\." --type typescript
671-676: Function signature updated to support multiple advocate typesThe function has been renamed from
formatDefenderAssignedEmailNotificationtoformatAdvocateAssignedEmailNotificationand now includes anadvocateTypeparameter of typeAdvocateType. This change allows the function to handle notifications for different types of advocates, improving its flexibility and reusability.To ensure this change is properly implemented across the codebase, let's verify the usage of this function:
✅ Verification successful
Function signature update verified
The
formatAdvocateAssignedEmailNotificationfunction is correctly updated and used in the following locations:
apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.tsapps/judicial-system/backend/src/app/formatters/index.tsapps/judicial-system/backend/src/app/formatters/formatters.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new function name rg "formatAdvocateAssignedEmailNotification" --type typescriptLength of output: 152
Script:
#!/bin/bash # Search for usage of the new function name in TypeScript files rg "formatAdvocateAssignedEmailNotification" --type ts rg "formatAdvocateAssignedEmailNotification" --type tsxLength of output: 651
Script:
#!/bin/bash # Search for usage of the new function name in TypeScript and TSX files rg "formatAdvocateAssignedEmailNotification" --glob "*.ts" --glob "*.tsx"Length of output: 586
…and.is into j-s/advocate-notifications
…and.is into j-s/advocate-notifications
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (1)
91-92: LGTM: Improved type safety with enum usageThe replacement of string literals with
AdvocateTypeenum values (AdvocateType.LAWYERandAdvocateType.LEGAL_RIGHTS_PROTECTOR) is a good improvement. It enhances type safety and makes the code more maintainable.Consider using a ternary operator for a more concise expression:
advocateType={civilClaimant.spokespersonIsLawyer ? AdvocateType.LAWYER : AdvocateType.LEGAL_RIGHTS_PROTECTOR}This change would slightly improve readability while maintaining the same functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (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."
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (1)
12-12: LGTM: New import enhances type safetyThe addition of the
AdvocateTypeimport from@island.is/judicial-system/typesis a good practice. It enhances type safety by introducing an enum for advocate types, which aligns well with TypeScript best practices.
Notifications to advocates in civil claims cases
Asana
What
Send notifications to lawyers and legal rights protectors in civil claim cases. The notifications are the same ones we send to defenders when they are assigned
Why
These advocates need these notifications.
Checklist:
Summary by CodeRabbit
Release Notes
New Features
AdvocateTypeenum for better type safety in components.Bug Fixes
Tests