fix(j-s): Court Receival Date #16903
Conversation
WalkthroughThe changes in this pull request primarily enhance the functionality of the case management system within the judicial application. Modifications include the addition of a 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16903 +/- ##
==========================================
+ Coverage 36.44% 36.59% +0.15%
==========================================
Files 6904 6865 -39
Lines 144729 143711 -1018
Branches 41350 40958 -392
==========================================
- Hits 52749 52596 -153
+ Misses 91980 91115 -865
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 98 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![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
🧹 Outside diff range and nitpick comments (9)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentInfoToCourt.spec.ts (1)
Line range hint
1-113: Consider adding more test casesWhile the current test verifies the happy path, consider adding test cases for:
- Error scenarios when court service fails
- Cases with missing indictment confirmation date
- Cases with multiple defendants
- Edge cases with different event log combinations
This will ensure the robustness of the date handling logic.
apps/judicial-system/backend/src/app/modules/court/test/createCourtCase.spec.ts (3)
37-37: Consider using a more descriptive variable name for the test date.While
receivalDateis functional, consider using a more descriptive name likemockReceivalDateortestReceivalDateto clearly indicate its role in the test context.Also applies to: 52-52
133-134: Consider adding test cases for invalid indictment subtypes.While the happy path is well tested, consider adding test cases for:
- Invalid indictment subtypes
- Empty indictment subtypes map
- Multiple subtypes for the same case
Also applies to: 147-147
252-254: Consider testing edge cases for receivalDate.While basic success and failure scenarios are covered, consider adding test cases for:
- Invalid date formats
- Future dates
- Very old dates
Also applies to: 289-291
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (1)
Line range hint
1-300: Consider enhancing email content validation.While the test coverage is comprehensive, the email content validation could be more robust. Consider these improvements:
it('should send correct email', () => { expect(mockEmailService.sendEmail).toHaveBeenCalledTimes(1) expect(mockEmailService.sendEmail).toHaveBeenCalledWith({ from: { name: mockConfig.email.fromName, address: mockConfig.email.fromEmail, }, to: [ { name: civilClaimant.spokespersonName, address: civilClaimant.spokespersonEmail, }, ], replyTo: { name: mockConfig.email.replyToName, address: mockConfig.email.replyToEmail, }, attachments: undefined, subject: `Skráning í máli ${theCase.courtCaseNumber}`, - text: expect.anything(), // same as html but stripped html tags + text: expect.stringContaining(theCase.courtCaseNumber), - html: `Héraðsdómur Reykjavíkur hefur skráð þig lögmann einkaréttarkröfuhafa í máli ${theCase.courtCaseNumber}.<br /><br />Sjá nánar á <a href="${mockConfig.clientUrl}${DEFENDER_INDICTMENT_ROUTE}/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt</a>.`, + html: expect.stringMatching(new RegExp([ + `Héraðsdómur Reykjavíkur`, + theCase.courtCaseNumber, + mockConfig.clientUrl, + DEFENDER_INDICTMENT_ROUTE, + caseId, + 'Réttarvörslugátt' + ].join('.*'))) }) })This change would:
- Validate that the text version contains the case number
- Use regex to verify all required elements in the HTML content
- Make the tests more resilient to minor text changes
apps/judicial-system/backend/src/app/modules/case/test/caseController/createCourtCase.spec.ts (2)
90-146: LGTM: Test improvements enhance clarity and determinismGood changes:
- More specific test description
- Fixed case type instead of random
- Proper date handling in test cases
Consider adding a brief comment explaining why
CaseType.CUSTODYwas chosen as the fixed type for better test documentation.describe('request court case created', () => { + // Using CUSTODY type as a representative case for request court cases const date = randomDate() const caseId = uuid() const type = CaseType.CUSTODY
163-168: Consider adding test cases for multiple confirmation eventsThe current test assumes a single confirmation event. Consider adding test cases to verify behavior when multiple
INDICTMENT_CONFIRMEDevents exist, ensuring we use the most recent confirmation date.// Example additional test case: it('should use the most recent confirmation date when multiple exist', () => { const oldDate = randomDate() const newDate = new Date(oldDate.getTime() + 86400000) // next day const caseWithMultipleConfirmations = { ...theCase, eventLogs: [ { eventType: EventType.INDICTMENT_CONFIRMED, created: oldDate, }, { eventType: EventType.INDICTMENT_CONFIRMED, created: newDate, }, ], } // Test implementation })apps/judicial-system/backend/src/app/modules/court/court.service.ts (1)
Line range hint
41-77: Consider enhancing PII protection in court subtypes mappingWhile the code handles PII well in general, consider moving the court subtypes mapping to a configuration file. This would:
- Make it easier to update without code changes
- Allow for different mappings in different environments
- Reduce the risk of exposing internal court type mappings
-export const courtSubtypes: CourtSubtypes = { - ALCOHOL_LAWS: 'Áfengislagabrot', - // ... other mappings -} +// Move to a configuration file (e.g., court-subtypes.config.ts) +export const getCourtSubtypes = (): CourtSubtypes => { + return config.getCourtSubtypes(); +}apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
583-595: LGTM! Consider adding a descriptive comment.The implementation correctly uses the indictment confirmation date as the court receival date. The code is efficient and handles undefined values properly.
Add a comment to explain why we're using the same date for both parameters:
+ // Use the indictment confirmation date as the court receival date const indictmentIssuedByProsecutorAndReceivedByCourt = theCase.eventLogs?.find( (eventLog) => eventLog.eventType === EventType.INDICTMENT_CONFIRMED, )?.created
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
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/caseController/createCourtCase.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentInfoToCourt.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/court/court.service.ts(2 hunks)apps/judicial-system/backend/src/app/modules/court/test/createCourtCase.spec.ts(13 hunks)apps/judicial-system/backend/src/app/modules/file/file.controller.ts(0 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaToPolice.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/file/file.controller.ts
🧰 Additional context used
📓 Path-based instructions (8)
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."
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/modules/case/test/caseController/createCourtCase.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/test/internalCaseController/deliverIndictmentInfoToCourt.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/court/court.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/court/test/createCourtCase.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/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/subpoena/test/internalSubpoenaController/deliverSubpoenaToPolice.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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (2)
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:11.835Z
Learning: The `deliverIndictmentCancellationNoticeToCourt` method in `InternalCaseService` returns `{ delivered: false }` in case of an exception, as error handling is managed by a message queue handler.
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 (15)
apps/judicial-system/backend/src/app/modules/subpoena/test/internalSubpoenaController/deliverSubpoenaToPolice.spec.ts (2)
7-7: LGTM!
The import statement is properly organized with other related imports.
26-26: Great improvement in type safety!
Replacing any type with DeliverDto enhances type checking and makes the test more maintainable. This change ensures that the mock user object strictly follows the expected structure defined in the DTO.
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentInfoToCourt.spec.ts (2)
Line range hint 42-58: LGTM: Well-structured test data setup
The test data setup is comprehensive and properly models the case structure with:
- Relevant event logs including both court receival and indictment confirmation dates
- Multiple police case numbers with associated subtypes
- Defendant and prosecutor information
99-100: Verify the duplicate indictmentDate parameters
The mock expectation shows indictmentDate being passed twice consecutively. Please verify if this is intentional and if these parameters serve different purposes in the court API.
Let's check the court service implementation:
✅ Verification successful
The duplicate indictmentDate parameters are correct and intentional
Looking at the updateIndictmentCaseWithIndictmentInfo method in the court service, the method signature shows two distinct date parameters:
receivedByCourtDate: When the court received the caseindictmentDate: When the indictment was confirmed
In the test, passing indictmentDate twice is correct because it's being used for both date parameters, indicating that both the court receival date and indictment confirmation date are the same in this test scenario.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the updateIndictmentCaseWithIndictmentInfo method definition
ast-grep --pattern 'updateIndictmentCaseWithIndictmentInfo($$$) {
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Let's try to find the court service interface or implementation
rg -l "updateIndictmentCaseWithIndictmentInfo" --type ts
# Then let's look at the actual test file to understand the context better
cat "apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentInfoToCourt.spec.ts"
Length of output: 3543
Script:
#!/bin/bash
# Let's check the court service implementation to understand the parameters
cat "apps/judicial-system/backend/src/app/modules/court/court.service.ts"
Length of output: 27629
apps/judicial-system/backend/src/app/modules/court/test/createCourtCase.spec.ts (4)
30-30: LGTM: Type definition updated correctly.
The GivenWhenThen type has been properly updated to include the new required receivalDate parameter.
173-173: Test coverage looks comprehensive for extendable and extended cases.
The test cases properly handle both initial and extension scenarios with appropriate date formatting.
Also applies to: 184-184, 208-208, 219-219
Line range hint 1-301: Verify alignment with PR objectives.
The test changes align well with the PR objective of updating court receival dates for indictments. However, let's verify if all relevant test scenarios are covered.
#!/bin/bash
# Search for other test files that might need similar updates
rg -t ts "createCourtCase.*test" -l
# Search for actual implementation to ensure test coverage matches
ast-grep --pattern 'createCourtCase($$$) {
$$$
}'93-93: Verify date formatting consistency across test cases.
The test correctly passes the receivalDate parameter and validates its formatting in the court case creation. However, let's verify the date formatting consistency across the codebase.
Also applies to: 106-106
✅ Verification successful
Based on the search results, I can see that date formatting is consistently handled across the codebase using the date-fns library. The specific line in question is using receivalDate which is properly formatted in the court case creation process. The date formatting is consistent with the rest of the codebase where:
- For ISO date formatting,
formatISOfrom date-fns is used - For display formatting,
formatfrom date-fns is used with appropriate locale settings - The
receivalDateis specifically formatted usingformatISOwithrepresentation: 'date'parameter when creating court cases
The date formatting is consistent with the codebase standards and no changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all date formatting patterns to ensure consistency
rg "formatISO.*representation.*date" --type ts
Length of output: 91
Script:
#!/bin/bash
# Search for date formatting patterns in test files
rg "date.*format|format.*date" -A 2 -B 2 --type ts
# Search specifically for receivalDate usage
rg "receivalDate" -A 2 -B 2 --type ts
# Look for date-fns imports and usage
rg "from 'date-fns'" --type ts
Length of output: 461439
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (1)
47-48: LGTM! Clean test setup refactoring.
The simplified destructuring improves code clarity while maintaining all necessary test dependencies.
Let's verify that all test scenarios are still covered after removing the notification model:
✅ Verification successful
The shell script results show that the notification system is comprehensive and well-tested. Let me verify the test coverage for advocate assigned notifications specifically:
Verified: Test coverage remains comprehensive after refactoring
The test file maintains complete coverage for advocate assigned notifications across all scenarios:
- Civil claims with lawyer advocate
- Civil claims with legal rights protector
- Civil claims without representation
- Restriction cases with defender (with and without national ID)
- Investigation cases
The simplified destructuring in the test setup is safe as all required dependencies (emailService, notificationConfig, internalNotificationController) are properly initialized and used throughout the test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for notification scenarios
# Expected: All notification scenarios should have corresponding test cases
# Check for all notification type definitions
rg "CaseNotificationType\." -A 2
# Verify test coverage for each notification scenario
ast-grep --pattern 'describe("when $scenario", () => {
$$$
it($$$) {
$$$
}
})'
Length of output: 60334
Script:
# Check test coverage for advocate assigned notifications
ast-grep --pattern 'describe("when $scenario", () => {
$$$
it("should send notification to $recipient", () => {
$$$
})
})' apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
Length of output: 270
Script:
# Let's try a broader pattern to catch the test cases
rg -A 10 "describe\\(.*when" apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
# Also check the test setup to verify notification dependencies
rg -A 5 "createTestingNotificationModule" apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
Length of output: 3238
apps/judicial-system/backend/src/app/modules/case/test/caseController/createCourtCase.spec.ts (2)
10-11: LGTM: Import changes align with PR objectives
The addition of EventType import and factory mocking supports the new functionality for handling indictment confirmation dates.
Also applies to: 19-19, 25-26
Line range hint 148-168: LGTM: Core functionality for indictment court case creation
The test correctly implements the PR objective by:
- Using indictment confirmation date from event logs
- Including indictment subtypes
- Properly structuring the test data
apps/judicial-system/backend/src/app/modules/court/court.service.ts (2)
Line range hint 349-367: LGTM! Robust error handling implementation
The error handling follows best practices:
- Graceful degradation with fallback values for service unavailability
- Structured error logging with sensitive data masking
- Consistent error event pattern across the service
Line range hint 326-345: LGTM! Verify usage of the new receivalDate parameter
The addition of the receivalDate parameter and its implementation in the createCourtCase method looks correct. The date is properly formatted using formatISO with date-only representation.
✅ Verification successful
Let me gather more information about the call chain and parameter usage.
The receivalDate parameter is correctly implemented and used throughout the call chain
The verification shows that:
- In
case.service.ts, thereceivalDateis properly determined based on case type:- For indictment cases: uses the timestamp from
INDICTMENT_CONFIRMEDevent - For other cases: uses current timestamp
- For indictment cases: uses the timestamp from
- This date is then passed to
court.service.tswhich formats it correctly for the court client
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all calls to createCourtCase include the receivalDate parameter
rg -A 3 "createCourtCase\("
Length of output: 4817
Script:
#!/bin/bash
# Check the implementation of case.service.ts createCourtCase to see how receivalDate is determined
rg -A 10 "async createCourtCase\(theCase: Case, user: TUser\)" apps/judicial-system/backend/src/app/modules/case/case.service.ts
# Check the court.service.ts implementation to verify parameter usage
rg -A 15 "async createCourtCase\(" apps/judicial-system/backend/src/app/modules/court/court.service.ts
Length of output: 996
apps/judicial-system/backend/src/app/modules/case/case.service.ts (2)
2037-2046: LGTM! The receival date logic aligns with requirements.
The implementation correctly uses the indictment confirmation date for indictment cases and current date for others, with proper null handling.
2053-2053: LGTM! The receival date is correctly passed to the court service.
The implementation properly integrates the new receivalDate parameter into the createCourtCase method call.
Datadog ReportBranch report: ✅ 0 Failed, 21243 Passed, 0 Skipped, 17m 11.56s Total Time |
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Court Receival Date
Upplýsingapóstur - leiðrétting á Móttökudegi
What
Why
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Documentation