chore(j-s): Logging more info for system status #16808
Conversation
WalkthroughThe pull request introduces several changes across multiple service files in the judicial system application. Key modifications include the removal of logging related to Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable 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
|
![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 (3)
apps/judicial-system/xrd-api/src/app/app.controller.ts (1)
74-76: Consider enhancing logging with more context and result trackingWhile the current logging is good, consider adding more structured context and result tracking for better observability:
- this.logger.info(`Updating subpoena ${subpoenaId}`) + this.logger.info('Updating subpoena', { + subpoenaId, + updateType: updateSubpoena.type, + operation: 'start' + }) - return this.appService.updateSubpoena(subpoenaId, updateSubpoena) + const result = await this.appService.updateSubpoena(subpoenaId, updateSubpoena) + this.logger.info('Subpoena updated successfully', { + subpoenaId, + operation: 'complete' + }) + return resultapps/judicial-system/backend/src/app/modules/event/event.service.ts (1)
Line range hint
1-251: Consider splitting event notification and logging responsibilitiesThe
EventServicecurrently handles both Slack notifications and system logging, which violates the Single Responsibility Principle. Consider splitting these into separate services:
EventNotificationService: Handle Slack notificationsEventLoggingService: Handle system loggingThis would:
- Improve maintainability
- Make the code easier to test
- Allow independent scaling of notification and logging systems
- Make it easier to add new notification channels or logging backends
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)
334-337: Enhance the log message with additional context.The addition of logging for successful subpoena delivery is good. However, consider enriching the log message with more context for better traceability.
- this.logger.info( - `Subpoena ${createdSubpoena.subpoenaId} delivered to police`, - ) + this.logger.info( + `Subpoena ${createdSubpoena.subpoenaId} delivered to police for case ${theCase.id}`, + { + caseId: theCase.id, + defendantId: defendant.id, + subpoenaId: createdSubpoena.subpoenaId + } + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/judicial-system/backend/src/app/modules/case/case.service.ts(0 hunks)apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts(0 hunks)apps/judicial-system/backend/src/app/modules/event/event.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts(1 hunks)apps/judicial-system/xrd-api/src/app/app.controller.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts
- apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/backend/src/app/modules/event/event.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/subpoena/subpoena.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/xrd-api/src/app/app.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/xrd-api/src/app/app.controller.ts (1)
74-74: LGTM! Appropriate logging level change
The change from debug to info level is appropriate as subpoena updates represent significant business events that should be visible in standard logging output.
apps/judicial-system/backend/src/app/modules/event/event.service.ts (1)
81-90: LGTM! Well-chosen events for logging
The selection of events to log covers critical system state transitions and important operational events, which aligns well with the PR objective of improving system status visibility.
apps/judicial-system/backend/src/app/modules/event/event.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/event/event.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/xrd-api/src/app/app.controller.ts (2)
71-77: Consider enhancing error handling and logging context.While the logging change is good, consider these improvements:
- Add try-catch block to handle service errors explicitly
- Include update details in the log message for better traceability
Consider applying this enhancement:
@Patch('subpoena/:subpoenaId') @ApiResponse({ status: 500, description: 'Failed to update subpoena' }) async updateSubpoena( @Param('subpoenaId', new ParseUUIDPipe()) subpoenaId: string, @Body() updateSubpoena: UpdateSubpoenaDto, ): Promise<SubpoenaResponse> { - this.logger.info(`Updating subpoena ${subpoenaId}`) - return this.appService.updateSubpoena(subpoenaId, updateSubpoena) + try { + this.logger.info(`Updating subpoena ${subpoenaId}`, { + operation: 'updateSubpoena', + updates: Object.keys(updateSubpoena), + }) + return await this.appService.updateSubpoena(subpoenaId, updateSubpoena) + } catch (error) { + this.logger.error(`Failed to update subpoena ${subpoenaId}`, error) + throw new InternalServerErrorException('Failed to update subpoena') + } }
71-71: Enhance Swagger documentation for better API clarity.The current API response documentation could be more descriptive.
Consider enhancing the Swagger decoration:
- @ApiResponse({ status: 500, description: 'Failed to update subpoena' }) + @ApiResponse({ type: SubpoenaResponse, description: 'Updates an existing subpoena with the provided data' }) + @ApiResponse({ status: 400, description: 'Invalid subpoena ID or update data format' }) + @ApiResponse({ status: 500, description: 'Internal server error while updating subpoena' })apps/judicial-system/backend/src/app/modules/event/event.service.ts (1)
81-90: LGTM! Consider adding documentation.The selection of events to log is well-thought-out and covers critical system state transitions. Consider adding a brief comment explaining the criteria for including events in this list.
+// Events that require additional logging for system status monitoring const caseEventsToLog = [apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1)
334-337: Enhance the logging context for better traceability.The addition of success logging is valuable for tracking subpoena delivery status. However, consider enriching the log message with more context for better debugging and monitoring.
Consider applying this improvement:
- this.logger.info( - `Subpoena ${createdSubpoena.subpoenaId} delivered to police`, - ) + this.logger.info( + `Subpoena ${createdSubpoena.subpoenaId} delivered to police`, + { + caseId: theCase.id, + defendantId: defendant.id, + subpoenaId: createdSubpoena.subpoenaId, + userId: user.id, + } + )This structured logging approach will:
- Make logs more queryable in log aggregation systems
- Provide complete context for troubleshooting
- Enable better metrics and analytics
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
1226-1231: LGTM! Consider extracting the allowed states into a constant.The filtering logic correctly prevents exposure of deleted or inaccessible cases. This is a good security practice.
Consider extracting the allowed states into a constant for better maintainability:
+const ACCESSIBLE_INDICTMENT_STATES = [ + CaseState.RECEIVED, + CaseState.COMPLETED, + CaseState.WAITING_FOR_CANCELLATION, +] as const; async getIndictmentCases(nationalId: string): Promise<Case[]> { return this.caseModel.findAll({ // ... other options ... where: { type: CaseType.INDICTMENT, // Make sure we don't send cases that are in deleted or other inaccessible states - state: [ - CaseState.RECEIVED, - CaseState.COMPLETED, - CaseState.WAITING_FOR_CANCELLATION, - ], + state: ACCESSIBLE_INDICTMENT_STATES, '$defendants.national_id$': normalizeAndFormatNationalId(nationalId), }, }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/judicial-system/backend/src/app/modules/case/case.service.ts(0 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts(2 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts(0 hunks)apps/judicial-system/backend/src/app/modules/event/event.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts(1 hunks)apps/judicial-system/xrd-api/src/app/app.controller.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts
- apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/event/event.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/subpoena/subpoena.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/xrd-api/src/app/app.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 (1)
apps/judicial-system/xrd-api/src/app/app.controller.ts (1)
74-74: LGTM! Logging level change aligns with PR objectives.
The change from debug to info level is appropriate for tracking system status updates.
apps/judicial-system/backend/src/app/modules/event/event.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/event/event.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16808 +/- ##
==========================================
+ Coverage 36.45% 36.46% +0.01%
==========================================
Files 6851 6852 +1
Lines 143558 143594 +36
Branches 40977 41040 +63
==========================================
+ Hits 52330 52360 +30
- Misses 91228 91234 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
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/event/event.service.ts (1)
226-250: Consider adding XRD-specific logging informationTo better understand XRD-API operations, consider adding more context when logging CREATE_XRD events.
switch (event) { + case 'CREATE_XRD': + extraInfo = `source: xrd-api, prosecutorsOffice: ${theCase.prosecutorsOffice?.name}` + break case 'SCHEDULE_COURT_DATE': // existing code...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/event/event.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/event/event.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."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/event/event.service.ts (2)
81-90: LGTM! Well-defined event filtering.
The constant effectively defines a focused set of important events to log, which aligns well with the PR objective of improving system status tracking.
166-166: Refer to previous review comment about moving logInfo call
The previous review comment about moving the logInfo call inside the try block is still valid and should be addressed.
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/event/event.service.ts (1)
81-90: LGTM! Consider documenting selection criteria.The event filtering list is well-structured and includes key system events. Consider adding a comment explaining why these specific events were chosen for logging.
+// Events that represent significant state changes or user actions that require monitoring const caseEventsToLog = [ 'CREATE', 'CREATE_XRD',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/event/event.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/event/event.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."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/event/event.service.ts (1)
166-166: Placement of logInfo call already addressed.
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Show resolved
Hide resolved
* chore(j-s): Logging more info for system status * fix(j-s): Filtering case list to remove deleted indictments for digital mailbox * Update event.service.ts * Update event.service.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Logga betur successful köll í xrd-api þegar birting uppfærist.
What
Added logs to get a better feel for what´s going on in the system
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor