chore(j-s): Send signed court record to police for investigation cases #17196
Conversation
|
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. WalkthroughThe changes in this pull request introduce new methods in 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: 2
🧹 Outside diff range and nitpick comments (5)
libs/judicial-system/message/src/lib/message.service.ts (2)
201-203: Consider adding debug logging for empty message arrays.To help with monitoring and debugging, consider logging when an empty message array is received:
if (messages.length === 0) { + this.logger.debug('Received empty message array, skipping queue operation') return // No messages to send }
Line range hint
1-203: Excellent message queue implementation for handling court records.The MessageService implementation is robust and well-suited for handling sensitive court records:
- Proper AWS SQS integration with batch processing
- Built-in retry mechanisms and error handling
- Configurable queue parameters
- Type-safe implementation
The service is well-positioned to handle the delivery of signed court records to police reliably and securely.
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPoliceGuards.spec.ts (1)
14-15: Consider using a more specific type annotation instead of any[]Replace the eslint-disable comment with a proper type definition for better type safety.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let guards: any[] + let guards: Array<new () => CanActivate>apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPolice.spec.ts (2)
44-48: Enhance error handling setup with specific error casesThe current error setup uses a generic 'Some error' message. Consider using specific error types and messages that match real scenarios.
- mockGetGeneratedObject.mockRejectedValue(new Error('Some error')) + mockGetGeneratedObject.mockRejectedValue(new Error('Failed to retrieve court record from S3')) - mockUpdatePoliceCase.mockRejectedValue(new Error('Some error')) + mockUpdatePoliceCase.mockRejectedValue(new Error('Failed to update police case: Invalid document format'))
55-56: Refactor promise handling to avoid assignments in expressionsThe current implementation assigns values in then/catch expressions, which is flagged by static analysis.
- .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] 55-55: 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] 56-56: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/judicial-system/backend/src/app/modules/case/case.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPolice.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPoliceGuards.spec.ts(1 hunks)apps/judicial-system/web/messages/Core/errors.ts(1 hunks)libs/judicial-system/message/src/lib/message.service.ts(1 hunks)libs/judicial-system/message/src/lib/message.ts(2 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- apps/judicial-system/web/messages/Core/errors.ts
- apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts
- apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
🧰 Additional context used
📓 Path-based instructions (8)
libs/judicial-system/message/src/lib/message.service.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/message/src/lib/message.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/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/internalCaseController/deliverSignedCourtRecordToPoliceGuards.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.controller.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/deliverSignedCourtRecordToPolice.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/messages/Core/errors.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 (2)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPoliceGuards.spec.ts (4)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getIndictmentPdfGuards.spec.ts:24-25
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In certain scenarios within the judicial-system backend, the `RolesGuard` may intentionally follow the `CaseExistsGuard` when specific roles rules require the guard order to be reversed, as seen in tests like `getIndictmentPdfGuards.spec.ts`.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
Learnt from: gudjong
PR: island-is/island.is#15241
File: apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedRulingToPolice.spec.ts:41-42
Timestamp: 2024-11-12T15:15:26.274Z
Learning: User gudjong prefers not to make changes to error handling messages in test setups at this time.
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.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.
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPolice.spec.ts
[error] 55-55: 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] 56-56: 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 (4)
libs/judicial-system/message/src/lib/message.service.ts (1)
201-203: LGTM! Good defensive programming practice.
The early return for empty message arrays is a good optimization that prevents unnecessary AWS SQS API calls.
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPoliceGuards.spec.ts (1)
49-51: Verify the case types allowed for police delivery
The test confirms that both restriction and investigation cases are allowed. This aligns with the PR objectives mentioning investigation cases, but we should verify if restriction cases should also be included.
✅ Verification successful
Allowing both restriction and investigation cases for police delivery is correct
Based on the codebase analysis, the inclusion of both restriction cases (ADMISSION_TO_FACILITY, CUSTODY, TRAVEL_BAN) and investigation cases (AUTOPSY, BANKING_SECRECY_WAIVER, etc.) for police delivery is intentional and correct. This is consistently implemented across multiple endpoints in the codebase, particularly in file and case controllers where police-related operations are involved. The test is correctly verifying this established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if restriction cases are handled similarly in other police delivery endpoints
ast-grep --pattern 'CaseTypeGuard.*\[...restrictionCases, ...investigationCases\]'
Length of output: 84
Script:
#!/bin/bash
# Let's search for case type guards in a broader context
ast-grep --pattern 'CaseTypeGuard'
# Also search for files containing both restrictionCases and investigationCases
rg -l "restrictionCases.*investigationCases|investigationCases.*restrictionCases"
# Let's check the actual implementation of the endpoint
rg -A 5 "deliverSignedCourtRecordToPolice"
Length of output: 35828
Script:
#!/bin/bash
# Let's check the implementation of CaseTypeGuard and case types
rg -A 10 "export const restrictionCases|export const investigationCases"
# Also check the implementation of the endpoint in the service
rg -A 10 "class InternalCaseService"
Length of output: 2844
libs/judicial-system/message/src/lib/message.ts (1)
26-26: LGTM! Message type and endpoint mapping follow established patterns
The new message type DELIVERY_TO_POLICE_SIGNED_COURT_RECORD and its endpoint mapping are consistent with existing police delivery message types.
Also applies to: 66-66
✅ Verification successful
Let me verify the consistency of police delivery message types and their usage with another search.
New message type follows consistent naming and implementation patterns
The verification confirms that:
- The new message type follows the established
DELIVERY_TO_POLICE_*naming pattern - Its endpoint mapping
deliverSignedCourtRecordToPolicefollows the camelCase pattern used by other police delivery endpoints - The message type is properly used in both the controller and service layers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the new endpoint is used consistently
rg "DELIVERY_TO_POLICE_SIGNED_COURT_RECORD" -A 1
Length of output: 1000
Script:
#!/bin/bash
# Check all police delivery related message types and their patterns
rg "DELIVERY_TO_POLICE" -A 1 libs/judicial-system/message/src/lib/message.ts
# Check the corresponding endpoint implementations
ast-grep --pattern 'deliverSignedCourtRecordToPolice($$$) {
$$$
}'
Length of output: 1289
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
1924-1925: LGTM!
The addition of addMessagesForSignedCourtRecordToQueue is well-placed after successful upload and update operations, maintaining consistency with similar patterns in the codebase.
...nd/src/app/modules/case/test/internalCaseController/deliverSignedCourtRecordToPolice.spec.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/case/case.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 98 Total Test Services: 0 Failed, 90 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts (1)
579-583: Consider adding rate limiting for this endpoint.Since this endpoint handles sensitive documents (signed court records), consider adding rate limiting to prevent potential abuse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.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/case/internalCase.controller.ts (2)
574-601: LGTM! The implementation follows established patterns and meets security requirements.
The new endpoint for delivering signed court records to police is well-implemented with:
- Appropriate guards ensuring case existence, type restrictions, and completion status
- Proper logging for audit trails
- Consistent error handling through the service layer
- Type-safe parameter handling with decorators
576-576: Verify case type guard configuration.
The guard allows both restriction and investigation cases, but PR objectives specifically mention investigation cases. Please confirm if restriction cases should also be included.
✅ Verification successful
The case type guard configuration is correctly implemented
The consistent pattern of using both restriction and investigation cases across multiple endpoints in the codebase, particularly for court record and ruling-related operations, indicates this is an intentional design. The guard is used in endpoints handling:
- Court record delivery
- Signed rulings
- Case conclusions
- Appeals processes
This implementation ensures both restriction and investigation cases have the necessary access controls for these judicial operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if restriction cases should have access to signed court records
# by looking for related business logic or documentation
# Check for any documentation about restriction cases and signed court records
rg -i "restriction.*signed.*court.*record" --type md
# Look for existing usage patterns of signed court records with case types
ast-grep --pattern 'new CaseTypeGuard([...restrictionCases, ...investigationCases])'
Length of output: 4313
Script:
#!/bin/bash
# Let's check the case type guard usage context and the endpoint definition
# Check the endpoint and its surrounding context
rg -B 5 -A 5 "new CaseTypeGuard\(\[\.\.\.restrictionCases, \.\.\.investigationCases\]\)" apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts
# Look for case type definitions and constants
rg "restrictionCases|investigationCases" apps/judicial-system/backend/src/app/shared/constants.ts
# Check for any documentation about case types
fd README.md | xargs rg -i "case.*type|restriction.*case|investigation.*case"
Length of output: 3302
Script:
#!/bin/bash
# Let's check the actual constants file location and its content
# Find the constants file that defines case types
fd -e ts constants
# Check the message types being used in these endpoints
rg "MessageType\." apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts -A 1
Length of output: 12337
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Show resolved
Hide resolved
Við erum að senda úrskurði og þingbækur of snemma í LÖKE
What
Send signed court record to police for investigation cases
Why
Because prosecutors sometimes need to use it when continuing the investigation
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests