chore(j-s): add police case numbers to revoke indictment notification email #17385
Conversation
…s/show-police-case-numbers-in-email
WalkthroughThe pull request updates the notification system for judicial cases, specifically focusing on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
apps/judicial-system/backend/src/app/modules/case/case.service.ts
Outdated
Show resolved
Hide resolved
thorhildurt
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: 0
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCancellationNoticeToCourt.spec.ts (1)
112-112: Consider enumerating all police case numbers
Right now, the test code references hardcodedpoliceCase1andpoliceCase2within the string. If you add more police case numbers later, you'll need to update this manually. You could dynamically join the array within the string for scalability.- `${prosecutorsOffice} hefur afturkallað ákæru í máli ${policeCase1}, ${policeCase2}...` + const joinedCaseNumbers = policeCaseNumbers.join(', ') + `${prosecutorsOffice} hefur afturkallað ákæru í máli ${joinedCaseNumbers}...`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/judicial-system/backend/src/app/messages/notifications.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCancellationNoticeToCourt.spec.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCancellationNoticeToCourt.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/case/internalCase.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/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/services/caseNotification/caseNotification.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/case/case.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/case/internalCase.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#15393
File: apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts:28-28
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The `DeliverCancellationNoticeDto` import in `apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts` is used in the `deliverIndictmentCancellationNoticeToCourt` method.
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.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.
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
Learnt from: gudjong
PR: island-is/island.is#15393
File: apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts:691-711
Timestamp: 2024-11-12T15:15:20.158Z
Learning: The `deliverIndictmentCancellationNoticeToCourt` method in `InternalCaseService` returns `{ delivered: false }` in case of an exception, as error handling is managed by a message queue handler.
🔇 Additional comments (16)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCancellationNoticeToCourt.spec.ts (3)
28-30: Use descriptive variable naming or a shared fixture for test data
It's good to see police case numbers explicitly declared, but consider adding a comment or using shared fixtures to clarify the context of these test constants.
39-39: Nice addition ofpoliceCaseNumbersto the case object
This clearly associates the new data with the test scenario.
96-96: Good descriptive test name
Renaming the test to focus on police case numbers makes the purpose clearer.apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (6)
Line range hint
28-30: Clarify intent of hardcoded test data
Same comment as in your spec file–the test constants are clear, but might be more maintainable if grouped in common test data utilities.
Line range hint
31-31: Explicitly grouping police case numbers
Bundling the two variables into one array is a good way to pass them around as a collection.
Line range hint
39-39: AddingpoliceCaseNumbers
Looks solid. Ensure all references handle an empty array scenario correctly.
675-678: Verify empty or large arrays
Joining the police case numbers with “, ” works fine when there's at least one item. Ensure it behaves as expected if the array is empty or has more than two items.
686-686: PassingcourtCaseNumber
This code paths into the format function seamlessly. No issues found.
693-694: Inserting placeholders
The placeholders forcourtCaseNumberandpoliceCaseNumbersare consistent with the new email template.apps/judicial-system/backend/src/app/messages/notifications.ts (1)
842-842: Graceful fallback topoliceCaseNumbers
The updated select statement elegantly handles the scenario where no court case number is provided. Great use of fallback placeholders.apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
863-863: Confirm correct usage ofwithCourtCaseNumber: false
When the state isWAITING_FOR_CANCELLATION, we explicitly disable the court case number usage. Verify that no scenario requires the actualcourtCaseNumberin the subsequent logic.apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (5)
1442-1444: Consider handling empty arrays gracefully.
Assigning a default'NONE'tocourtCaseNumberis fine. However, forpoliceCaseNumbers.join(', '), ensure there's no unexpected behavior ifpoliceCaseNumbersis empty. Either handle an empty array scenario or confirm such a case cannot occur.
1448-1448: No issues found.
This reference tocourtCaseNumberaligns with the variable defined above.
1455-1456: Check for user clarity with appended police case numbers.
IncludingcourtCaseNumberandpoliceCaseNumbersin the email template is a good addition. Consider verifying that these values render properly, especially whencourtCaseNumberis'NONE'.
1463-1463: Good fix of the method name typo.
RenamingsendRevodeNotificationsForIndictmentCasetosendRevokeNotificationsForIndictmentCaseimproves code readability and flow.
1541-1541: Method call consistency.
This change to callsendRevokeNotificationsForIndictmentCaseappropriately reflects the corrected method name, ensuring consistency.
Datadog ReportBranch report: ✅ 0 Failed, 21055 Passed, 0 Skipped, 18m 55.47s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17385 +/- ##
=======================================
Coverage 35.65% 35.65%
=======================================
Files 6939 6939
Lines 149151 149153 +2
Branches 42652 42653 +1
=======================================
+ Hits 53180 53182 +2
Misses 95971 95971
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts (1)
38-39: Add tests for multiple police case numbers.Currently, the array only contains a single police case number. Consider adding covering test scenarios for multiple police case numbers or an empty array scenario to ensure broader coverage and robustness.
apps/judicial-system/backend/src/app/messages/notifications.ts (1)
840-842: Manage multiple police case numbers gracefully.When
courtCaseNumberisNONE, the template displays{policeCaseNumbers}. For multiple case numbers in an array, consider a format that clearly separates them (e.g., comma-separated). This will enrich the clarity of the notification and match user expectations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/messages/notifications.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.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/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 (1)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts (1)
54-54: Confirm consistent parameter usage.This addition to
theCasealigns well with the rest of the code. Ensure that all references in other modules (e.g., services, controllers) expect or handle this newpoliceCaseNumbersfield appropriately.
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/judicial-system/backend/src/app/messages/notifications.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts
- apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/services/caseNotification/caseNotification.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 (1)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/notification/services/caseNotification/caseNotification.service.ts (2)
1446-1460: LGTM! Police case numbers are now included in the notification.The implementation correctly:
- Joins multiple police case numbers with commas
- Passes the police case numbers to the email notification
1467-1467: LGTM! Fixed typo in method name.Changed from
sendRevodeNotificationsForIndictmentCasetosendRevokeNotificationsForIndictmentCaseto fix the spelling.Also applies to: 1545-1545
|
Addressed feedback and tested again both states when (1) court emails are sent, and (2) robot email are sent and both consistently contain the police case numbers in the body 👍 |
…b.com/island-is/island.is into j-s/show-police-case-numbers-in-email
thorhildurt
added
the
deprecated:automerge
What
policeCaseNumbersas stated in the task descriptionWhy
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements