chore(j-s): Update subpoena status from digital-mailbox #16645
Conversation
WalkthroughA new controller, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts (3)
13-16: Consider adding rate limiting for the internal API.
While the endpoint is properly secured with TokenGuard and CaseExistsGuard, consider adding rate limiting to prevent potential abuse of the LRH status check endpoint.
22-26: Enhance API documentation for better clarity.
The current API documentation could be more descriptive. Consider adding:
- Expected error responses
- Format of the subpoena status
- Rate limiting details if implemented
Example enhancement:
@ApiOkResponse({
type: Subpoena,
- description: 'Gets subpoena status',
+ description: 'Retrieves the current status of a subpoena from LRH. Returns cached status if available, otherwise fetches fresh status.',
})
+@ApiResponse({ status: 403, description: 'Forbidden - Invalid or missing token' })
+@ApiResponse({ status: 404, description: 'Case or subpoena not found' })1-36: Add unit tests for the new controller.
As mentioned in the PR objectives, tests are missing. Please add unit tests to cover:
- Successful status retrieval
- Error handling scenarios
- Guard behavior
Would you like me to help generate the unit test suite for this controller?
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts (1)
241-286: Consider architectural improvements for status updates.
The current implementation fetches status updates synchronously during case retrieval, which could impact response times. Consider these architectural improvements:
- Implement caching for status responses with a reasonable TTL
- Consider implementing a background job for status updates
- Add retry mechanism with exponential backoff for failed status fetches
This would improve performance and reliability while reducing load on the LRH service.
Would you like assistance in implementing any of these improvements?
apps/judicial-system/backend/src/app/modules/police/police.service.ts (1)
Line range hint 339-424: Consider enhancing error logging for digital-mailbox.
When the user is undefined (digital-mailbox case), we might want to include additional context in the error event to help with debugging and monitoring.
Consider adding more context to the error event:
this.eventService.postErrorEvent(
'Failed to get subpoena',
{
subpoenaId,
actor: user?.name || 'Digital-mailbox',
institution: user?.institution?.name,
+ source: user ? 'user' : 'digital-mailbox',
+ timestamp: new Date().toISOString(),
},
reason,
)📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/police/police.module.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/police/police.service.ts (2 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.controller.ts (1 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/backend/src/app/modules/police/internalPolice.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/police/police.module.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/police/police.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/digital-mailbox-api/src/app/modules/cases/case.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/digital-mailbox-api/src/app/modules/cases/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 (1)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts:164-169
Timestamp: 2024-10-14T21:34:16.234Z
Learning: Ensure that suggested code changes in `subpoena.response.ts` are accurate and necessary, avoiding redundant suggestions.
🪛 Biome
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
apps/judicial-system/backend/src/app/modules/police/police.module.ts (2)
17-17: LGTM! Controllers array updated correctly.
The InternalPoliceController is properly integrated into the module. Since this module uses circular dependencies with forwardRef, ensure the new controller doesn't introduce additional circular dependencies.
#!/bin/bash
# Check for potential circular dependencies in the new controller
rg -A 5 "import.*from.*police.module" --type ts4-4: Verify the controller import path
The import path looks correct, but let's verify the file exists and follows the module structure.
✅ Verification successful
The results show that the internalPolice.controller.ts file exists in the correct location and the import path is correctly specified. The controller is properly imported and used in the police.module.ts. Let's verify the controller's implementation and its dependencies to ensure everything is properly connected.
Import path and module structure are correctly implemented
The verification confirms:
- The
internalPolice.controller.tsfile exists in the correct location - The controller is properly imported and registered in the module
- Circular dependencies are properly handled with
forwardRef - The module structure follows NestJS best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and location of the controller file
fd "internalPolice.controller.ts" --type f
# Check for any other internal police related files that might need updates
rg "InternalPoliceController" --type ts
Length of output: 1433
Script:
#!/bin/bash
# Check the implementation of InternalPoliceController
ast-grep --pattern 'export class InternalPoliceController {
$$$
}'
# Check for any circular dependencies
rg -B 2 -A 2 "forwardRef" apps/judicial-system/backend/src/app/modules/police/police.module.ts
Length of output: 501
apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts (2)
1-11: LGTM! Dependencies are properly organized.
The imports are well-structured and include all necessary decorators, types, and services.
27-35:
Add error handling for LRH communication failures.
The current implementation doesn't handle potential failures when communicating with LRH. Consider adding try-catch block and proper error responses.
Example implementation:
getSubpoenaStatus(
@Param('subpoenaId') subpoenaId: string,
@CurrentCase() theCase: Case,
@CurrentHttpUser() user: User,
): Promise<Subpoena> {
this.logger.debug(`Gets subpoena status in case ${theCase.id}`)
-
- return this.policeService.getSubpoenaStatus(subpoenaId, user)
+ try {
+ return this.policeService.getSubpoenaStatus(subpoenaId, user)
+ } catch (error) {
+ this.logger.error(
+ `Failed to fetch subpoena status for case ${theCase.id}`,
+ error,
+ )
+ throw new ServiceUnavailableException('Unable to fetch subpoena status from LRH')
+ }
}Let's verify if error handling is implemented in the PoliceService:
apps/judicial-system/backend/src/app/modules/police/police.service.ts (2)
410-411: LGTM! The changes align with the PR objectives.
The modifications to make the user parameter optional and provide a default actor for error logging are well-implemented. This enables the digital-mailbox to check subpoena status while maintaining existing functionality.
Line range hint 339-424: Verify error handling in the digital-mailbox integration.
The method now supports being called without a user, which implies it's being integrated with the digital-mailbox system. Let's verify this integration.
✅ Verification successful
Let me gather more information about the digital-mailbox integration by checking the actual implementation and calls.
Let me check one more aspect to see how the digital-mailbox API actually calls the police service.
Error handling in digital-mailbox integration is properly implemented
The verification shows that:
- The digital-mailbox API has proper error handling with specific exception types (NotFoundException, BadGatewayException)
- Error messages are appropriately propagated and transformed
- The police service's getSubpoenaStatus method correctly handles the optional user parameter, logging errors with 'Digital-mailbox' as the actor when no user is provided
- The integration is properly set up with dedicated endpoints and error handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for digital-mailbox integration points and error handling
# Test: Look for digital-mailbox related code and error handling
rg -l "digital-mailbox|Digital-mailbox" --type ts
# Test: Check for calls to getSubpoenaStatus without user parameter
ast-grep --pattern 'getSubpoenaStatus($subpoenaId)'
Length of output: 597
Script:
#!/bin/bash
# Check digital-mailbox API implementation and how it interacts with police service
rg -A 10 "getSubpoenaStatus" apps/judicial-system/digital-mailbox-api/
# Check for any error handling patterns in digital-mailbox API
rg -A 5 "catch|throw|error" apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
# Look for the actual integration point between digital-mailbox and police service
ast-grep --pattern 'import { PoliceService } from'
Length of output: 2511
Script:
#!/bin/bash
# Check for the actual service status check implementation in digital-mailbox API
rg -A 15 "serviceStatus|subpoena.*status" apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
# Look for the controller endpoints that might be calling getSubpoenaStatus
rg -A 10 "@Get.*subpoena|@Post.*subpoena" apps/judicial-system/digital-mailbox-api/src/
Length of output: 2215
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.controller.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16645 +/- ##
==========================================
- Coverage 36.53% 36.53% -0.01%
==========================================
Files 6888 6889 +1
Lines 143680 143563 -117
Branches 40942 40900 -42
==========================================
- Hits 52497 52447 -50
+ Misses 91183 91116 -67
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 21193 Passed, 0 Skipped, 16m 10.59s Total Time 🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts (1 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/police/internalPolice.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/digital-mailbox-api/src/app/modules/cases/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."
🪛 Biome
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts (2)
1-10: LGTM! Dependencies are well organized.
The imports follow NestJS best practices and include all necessary decorators, guards, and types.
12-19: Verify internal API access controls.
While the endpoint is properly guarded with TokenGuard, being an internal API endpoint requires additional verification of access controls.
Let's verify the access control implementation:
✅ Verification successful
Let me generate one more verification script to check the CaseExistsGuard implementation:
Internal API endpoints are properly secured
The security implementation for internal API endpoints is robust and appropriate:
- TokenGuard validates requests using a secret token in the Authorization header
- CaseExistsGuard ensures valid case IDs and prevents unauthorized access
- All internal controllers consistently use these guards
- The pattern is consistently applied across other internal endpoints (notifications, subpoenas, files, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper access control implementation in the auth module
# Expected: Find TokenGuard implementation to ensure it properly restricts access to internal endpoints
# Check TokenGuard implementation
echo "Checking TokenGuard implementation..."
rg -A 10 "export class TokenGuard"
# Check for other internal endpoints and their guards
echo "Checking other internal endpoint guards..."
rg -g "*.controller.ts" "api/internal" -A 5
Length of output: 5630
Script:
#!/bin/bash
# Check CaseExistsGuard implementation
echo "Checking CaseExistsGuard implementation..."
rg -A 10 "export class CaseExistsGuard"
# Check if there are any additional security checks in the module configuration
echo "Checking module configuration..."
rg -g "*.module.ts" "TokenGuard|CaseExistsGuard" -A 5
Length of output: 1475
apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/police/internalPolice.controller.ts
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM but would be great if Una would review the digital mailbox code.
Please consider adding unit test for the internal police controller to keep good test coverage of the backend code. Also please consider adding and/or updating unit tests for the digital mail box service.
…-db-digital-mailbox
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/case.service.ts
Outdated
Show resolved
Hide resolved
oddsson
added
the
deprecated:automerge
* Checkpoint * Cleanup * Remove user from getSubpoenaStatus * Uncomment scopes * Remove unused console..og * Refactor * Refactor * Refactor * Only update service status if there is no service status set --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Update subpoena status from digital-mailbox
Asana
What
When a case is fetched on minar-sidur, we now check if we've received a subpoena status from LRH and if not, we try to fetch it from them.
Why
LRH sends the subpoena status when it changes on their end. If something happens and they are unable to do so, then this is a failsafe.
Checklist:
Summary by CodeRabbit
New Features
InternalPoliceController, allowing users to retrieve the status of subpoenas via a dedicated GET endpoint.Improvements
getSubpoenaStatusmethod to allow optional user parameters, improving flexibility in request handling.CaseService, ensuring better reliability and user feedback.These updates enhance the functionality and user experience within the judicial system application.