fix(j-s): Prison admin indictment notifications #17097
Conversation
Notifications weren't getting sent due to a bug in the defendant model that returned an undefined value for defendant.caseId
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (1)
Line range hint
78-95: Consider splitting notification logicThe method handles both sending and withdrawing notifications based on a boolean flag. Consider splitting this into two separate methods for better clarity:
getMessageForIndictmentSentToPrisonAdmingetMessageForIndictmentWithdrawnFromPrisonAdminAlso, consider using an enum instead of a boolean for
isSentToPrisonAdminto make the state more explicit.Example refactor:
-private getMessagesForIndictmentToPrisonAdminChanges( - defendant: Defendant, - caseId: string, -): Message { - const messageType = - defendant.isSentToPrisonAdmin === true - ? DefendantNotificationType.INDICTMENT_SENT_TO_PRISON_ADMIN - : DefendantNotificationType.INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN - - const message = { - type: MessageType.DEFENDANT_NOTIFICATION, - caseId, - elementId: defendant.id, - body: { - type: messageType, - }, - } - - return message -} +private getMessageForIndictmentSentToPrisonAdmin( + defendant: Defendant, + caseId: string, +): Message { + return { + type: MessageType.DEFENDANT_NOTIFICATION, + caseId, + elementId: defendant.id, + body: { + type: DefendantNotificationType.INDICTMENT_SENT_TO_PRISON_ADMIN, + }, + } +} + +private getMessageForIndictmentWithdrawnFromPrisonAdmin( + defendant: Defendant, + caseId: string, +): Message { + return { + type: MessageType.DEFENDANT_NOTIFICATION, + caseId, + elementId: defendant.id, + body: { + type: DefendantNotificationType.INDICTMENT_WITHDRAWN_FROM_PRISON_ADMIN, + }, + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.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/defendant/defendant.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/defendant/models/defendant.model.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.ts:0-0
Timestamp: 2024-11-19T09:04:44.520Z
Learning: Additional documentation is not required when the code is clear and self-explanatory.
🔇 Additional comments (3)
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (3)
78-80: LGTM! Improved parameter passing
Adding the explicit caseId parameter instead of relying on defendant.caseId makes the code more robust and fixes the notification dispatch issue.
87-89: LGTM! Using explicit caseId
Using the explicitly passed caseId instead of defendant.caseId ensures the message will always have a valid case identifier.
200-203: LGTM! Proper method invocation
The method call is updated to pass theCase.id explicitly, which is guaranteed to be defined at this point since we check for courtCaseNumber earlier in the method.
apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17097 +/- ##
=======================================
Coverage 35.68% 35.68%
=======================================
Files 6946 6946
Lines 147391 147377 -14
Branches 41897 41892 -5
=======================================
Hits 52595 52595
+ Misses 94796 94782 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 21324 Passed, 0 Skipped, 23m 12.86s Total Time |
Notifications weren't getting sent due to a bug in the defendant model that returned an undefined value for defendant.caseId
Senda tilkynningu til FMST - fullnusta
What
Fix indictment sent and withdrawn from prison admin notifications
Why
Because they weren't sending
Checklist:
Summary by CodeRabbit
New Features
caseIdfor message creation.Bug Fixes
caseproperty in theDefendantmodel to ensure consistency with naming conventions.