feat(j-s): send criminal records to public prosecutor static email #17406
Conversation
…public prosecutor
…s/criminal-records-notification
…l assignment logic
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces a new feature for handling notifications related to criminal record files in the judicial system backend. The changes involve adding a new secret for the public prosecutor's criminal records email, updating notification configurations, and implementing a new notification type for criminal record file uploads. The modifications span multiple files across infrastructure, backend services, and configuration, focusing on enhancing the email notification system for indictment cases. Changes
Sequence DiagramsequenceDiagram
participant Backend as Judicial System Backend
participant NotificationService as Notification Dispatch Service
participant EmailService as Email Service
participant CriminalRecords as Public Prosecutor Criminal Records
Backend->>NotificationService: Dispatch Indictment Notification
NotificationService->>NotificationService: Check Case File Categories
alt Criminal Record Files Uploaded
NotificationService->>EmailService: Construct Criminal Record Files Notification
EmailService->>CriminalRecords: Send Notification Email
end
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
|
…m/island-is/island.is into j-s/criminal-records-notification
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)
21-24: Ensure default value for new configuration parameter is correct and documented.Having the default value set to an empty string might lead to unintentional behavior if the environment variable is missing. Consider throwing an error if the environment variable is absent, or provide a descriptive default value in production-like environments. Also, document this new configuration requirement clearly so that maintainers know to set the relevant secret.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/indictmentCaseNotification/sendIndictmentCriminalRecordFilesUploadedNotification.spec.ts (3)
14-17: Check for strong typing in the Then interface.Your
Theninterface only capturesresultanderroras broad types. If theDeliverResponseobject can contain more specific fields or if theerrorcan be typed (e.g.,SomeErrorType), consider specifying them for improved clarity and type safety.
19-23: Clarify naming convention of "GivenWhenThen".The custom testing pattern is understandable, but if it’s widely reused, consider a more descriptive or standard naming convention for easier readability and consistency across the codebase.
60-61: Address possible confusion from assignments in expression statements.Static analysis tools warn about the use of assignments (
then.result = result/then.error = error) within.then(...)and.catch(...)callbacks, as it can be less clear than returning or assigning within a clearer code block. Consider rewriting to maintain clarity.- .then((result) => (then.result = result)) - .catch((error) => (then.error = error)) + .then((result) => { + then.result = result + }) + .catch((error) => { + then.error = error + })🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/eventNotificationDispatch/eventNotificationDispatch.spec.ts (3)
5-5: Keep the import order consistent.To improve readability and follow typical NestJS / TypeScript guidelines, consider grouping local imports separately from third-party ones, or follow your team’s established convention.
13-13: Document the structure of CaseFile usage.The added
CaseFileimport indicates expanded logic around attachments or file categories. Provide a brief code comment or docstring explaining howCaseFileCategory.CRIMINAL_RECORD_UPDATEis used in this test scenario and potentially in production features.
65-70: Ensure scenario naming is descriptive.Consider adding more descriptive scenario descriptions for each item in
notificationScenariosso future maintainers can easily spot differences in test coverage when new events are introduced.apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts (1)
165-165: Private vs. public method.Retain consistency in method visibility. If this method is only called internally, maintaining a private or protected modifier is fine. If needed externally, consider making it public.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/judicial-system/backend/infra/judicial-system-backend.ts(1 hunks)apps/judicial-system/backend/src/app/messages/notifications.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/notification.config.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/notificationDispatch.service.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.strings.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/eventNotificationDispatch/eventNotificationDispatch.spec.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/indictmentCaseNotification/sendIndictmentCriminalRecordFilesUploadedNotification.spec.ts(1 hunks)charts/judicial-system/values.dev.yaml(1 hunks)charts/judicial-system/values.prod.yaml(1 hunks)charts/judicial-system/values.staging.yaml(1 hunks)charts/services/judicial-system-backend/values.dev.yaml(1 hunks)charts/services/judicial-system-backend/values.prod.yaml(1 hunks)charts/services/judicial-system-backend/values.staging.yaml(1 hunks)libs/judicial-system/types/src/lib/notification.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
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/infra/judicial-system-backend.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.config.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/services/indictmentCaseNotification/indictmentCaseNotification.strings.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/notificationDispatch.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."
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."
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/eventNotificationDispatch/eventNotificationDispatch.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/internalNotificationController/indictmentCaseNotification/sendIndictmentCriminalRecordFilesUploadedNotification.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/services/indictmentCaseNotification/indictmentCaseNotification.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."
📓 Learnings (3)
apps/judicial-system/backend/src/app/modules/notification/notificationDispatch.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
libs/judicial-system/types/src/lib/notification.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16556
File: apps/judicial-system/backend/src/app/modules/notification/guards/rolesRules.ts:13-16
Timestamp: 2024-11-12T15:15:26.274Z
Learning: In the `CaseNotificationType` enum in `libs/judicial-system/types/src/lib/notification.ts`, `APPEAL_CASE_FILES_UPDATED` and `CASE_FILES_UPDATED` are distinct notification types that need to remain separate, as they serve different purposes in the notification system.
apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/indictmentCaseNotification/sendIndictmentCriminalRecordFilesUploadedNotification.spec.ts
[error] 60-60: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (22)
apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.strings.ts (1)
19-33: Review subject and message for clarity and i18n consistency.The strings for the
criminalRecordFilesUploadedEmailare self-explanatory, but make sure the language is consistent with other notifications sent in this module. Consider verifying capitalization, special characters, and translations for non-Icelandic texts if applicable.apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/indictmentCaseNotification/sendIndictmentCriminalRecordFilesUploadedNotification.spec.ts (2)
39-65: Consider verifying environment variables during tests.While the test sets
PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILexplicitly, ensure that in a continuous integration environment, missing or invalid environment variables won’t break the test. Validate existence or correctness of required environment variables with a setup step or configuration check to avoid silent failures.🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
67-92: Good comprehensive test coverage for criminal record files email dispatch.The test ensures the correct parameters are passed to the email service and verifies a successful delivery response. This coverage is essential to prevent regressions for newly introduced flow. Great job!
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/eventNotificationDispatch/eventNotificationDispatch.spec.ts (3)
17-24: Validate presence of other case file categories.While
CRIMINAL_RECORD_UPDATEis an important category, consider verifying other categories to ensure the test scenario is adequately covering or ignoring them as intended. This will prevent breakage if a future developer adds more file categories without adjusting this test.
45-60: Comprehensive approach to testing multiple messages.Allowing multiple expected messages in a single scenario is a powerful approach, ensuring any newly introduced notifications are also captured. Continue this pattern to maintain robust coverage as more notifications are added.
78-80: Keep parallel structure when asserting calls.Your
expect(mockMessageService.sendMessagesToQueue).toHaveBeenCalledWith(...)is consistent. Maintain similar patterns across other tests for clarity, or use variables if the repeated code becomes substantial.apps/judicial-system/backend/src/app/modules/notification/notificationDispatch.service.ts (2)
11-11: Imports look good.You’ve correctly imported the
CaseFileCategoryenum, which is necessary for checking if any files fall into theCRIMINAL_RECORD_UPDATEcategory.
74-95: Good addition for handling multiple messages.Using a dedicated array of messages before calling
sendMessagesToQueue()makes the code more extensible. The check forCRIMINAL_RECORD_UPDATEfiles and then pushing theCRIMINAL_RECORD_FILES_UPLOADEDnotification is straightforward and clear.apps/judicial-system/backend/infra/judicial-system-backend.ts (1)
77-78: Verify the usage of new secret.The new secret
PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILmust be configured in all relevant environments (dev, staging, prod) to avoid runtime issues.libs/judicial-system/types/src/lib/notification.ts (1)
27-27: New enum value addresses expanded use case.Adding
CRIMINAL_RECORD_FILES_UPLOADEDtoIndictmentCaseNotificationTypeis aligned with the need to detect criminal record file uploads. Update any relevant test coverage to ensure it’s handled properly throughout the system.apps/judicial-system/backend/src/app/modules/notification/services/indictmentCaseNotification/indictmentCaseNotification.service.ts (5)
1-2: Check the benefit of usingapplyCase.Ensure
applyCasefrombeygla/strictis actually needed. IfapplyCaseis unused or only partially used, consider removing it to avoid dead imports.
15-15: Route constant usage.Using
ROUTE_HANDLER_ROUTEcentralizes routing logic and avoids hardcoding. Great approach for maintainability.
21-21: Notifications import is consistent.Importing from a common
messagesorstringsmodule fosters reusability. Keep references consistent for all notifications.
130-164: Well-structured email notification logic.The dedicated
sendCriminalRecordFilesUploadedNotificationmethod clarifies the email’s subject, body, and recipients. The usage offormatMessageensures translation readiness.
173-174: Proper routing of notification types.The switch statement explicitly handles
CRIMINAL_RECORD_FILES_UPLOADED, reinforcing clarity. Good usage of typed enums to avoid string-literal errors.apps/judicial-system/backend/src/app/messages/notifications.ts (1)
784-788: New localized message looks consistent with existing patterns.
This addition aligns with the established structure for notification strings and follows best practices for localized messages.charts/services/judicial-system-backend/values.dev.yaml (1)
145-145: Added secret appears correct and consistent.
The new secret aligns with existing naming conventions. Ensure that it is properly referenced by the application code and that access is controlled to avoid accidental leakage.charts/services/judicial-system-backend/values.prod.yaml (1)
145-145: New secret addition matches environment-specific settings.
Make sure the secret is securely managed and kept in sync with the usage in the application’s production environment.charts/services/judicial-system-backend/values.staging.yaml (1)
145-145: Secret key tracking is consistent across environments.
The new secret is set up uniformly, which helps maintain consistency across staging and other deployment tiers.charts/judicial-system/values.dev.yaml (1)
232-232: LGTM! The secret configuration follows security best practices.The addition of
PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILas a Kubernetes secret:
- Follows the established pattern for email configurations
- Is consistently implemented across all environments (dev/staging/prod)
- Uses the secure
/k8s/judicial-system/path prefix- Aligns with the PR objective of implementing criminal records notifications
Run the following script to verify the secret is properly configured in all environments:
✅ Verification successful
Secret configuration verified: Consistent implementation across all environments
The verification confirms that the
PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILsecret:
- Uses identical path
/k8s/judicial-system/PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILin all environments (dev/staging/prod)- Maintains consistent positioning in the configuration files
- Follows the same pattern as other sensitive configurations (e.g.,
XROAD_CLIENT_CERT)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the secret configuration across environments # Test: Check if the secret path exists in all environment files # Expected: The secret should be present with identical paths echo "Checking secret configuration consistency across environments..." for env in dev staging prod; do rg -A 1 "PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAIL" "charts/judicial-system/values.${env}.yaml" doneLength of output: 1025
charts/judicial-system/values.staging.yaml (1)
232-232: Duplicate configuration - LGTM!This is identical to the configuration in values.dev.yaml.
charts/judicial-system/values.prod.yaml (1)
232-232: Duplicate configuration - LGTM!This is identical to the configuration in values.dev.yaml.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17406 +/- ##
=======================================
Coverage 35.68% 35.69%
=======================================
Files 6931 6931
Lines 148792 148807 +15
Branches 42511 42515 +4
=======================================
+ Hits 53098 53111 +13
- Misses 95694 95696 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 9 Total Test Services: 0 Failed, 9 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
| @@ -1,3 +1,5 @@ | |||
| import { applyCase } from 'beygla/strict' | |||
There was a problem hiding this comment.
Please remove unused import
| import { applyCase } from 'beygla/strict' |
What
PUBLIC_PROSECUTOR_CRIMINAL_RECORDS_EMAILin all env. Values are already set in 1Password but we need to share with devops for update.Why
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
Configuration
Notifications