chore(j-s): add slack lock for daily court hearing arrangements #17468
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. WalkthroughThis pull request introduces new functionality for handling court hearing arrangements in the judicial system backend. A new method Changes
Possibly related PRs
Suggested labels
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
|
thorhildurt
changed the title
…github.com/island-is/island.is into j-s/add-slack-log-for-hearing-arrangements # Conflicts: # apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
![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
🧹 Nitpick comments (2)
apps/judicial-system/scheduler/src/app/app.service.ts (1)
105-113: Consider parallel execution for independent tasksThe tasks appear to be independent and could be executed in parallel using
Promise.all()for better performance.Apply this diff:
async run() { this.logger.info('Scheduler starting') - await this.addMessagesForIndictmentsWaitingForConfirmationToQueue() - await this.archiveCases() - await this.postDailyHearingArrangementSummary() + await Promise.all([ + this.addMessagesForIndictmentsWaitingForConfirmationToQueue(), + this.archiveCases(), + this.postDailyHearingArrangementSummary(), + ]) this.logger.info('Scheduler done') }apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
523-544: Add input validation and consider performance optimizationThe method needs input validation and could benefit from performance optimization.
- Add input validation:
async getCaseHearingArrangements(date: Date): Promise<Case[]> { + if (!(date instanceof Date) || isNaN(date.getTime())) { + throw new BadRequestException('Invalid date provided') + }
- Consider adding an index on the
date_typeanddatecolumns for better query performance:CREATE INDEX idx_date_logs_type_date ON date_logs (date_type, date);
- Consider adding a limit to prevent excessive memory usage:
return this.caseModel.findAll({ include: [...], order: [...], + limit: 1000, // Adjust based on requirements })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts(0 hunks)apps/judicial-system/backend/src/app/modules/event/event.service.ts(1 hunks)apps/judicial-system/scheduler/src/app/app.service.ts(4 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/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/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/scheduler/src/app/app.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."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (3)
apps/judicial-system/scheduler/src/app/app.service.ts (2)
Line range hint
27-41: Well-structured message queue handling!Good job extracting this functionality into a separate method with proper error handling and logging.
Line range hint
43-79: Well-implemented archiving logic!Good implementation of retry mechanism with proper time limit and error handling.
apps/judicial-system/backend/src/app/modules/event/event.service.ts (1)
176-219: Add JSDoc documentation for better maintainabilityThe method would benefit from JSDoc documentation explaining its purpose, parameters, and return type.
Add documentation:
+ /** + * Posts a summary of daily hearing arrangements to Slack + * @param date The date for which to post hearing arrangements + * @param cases Array of cases with hearing arrangements + * @returns Promise<void> + */ async postDailyHearingArrangementEvents(date: Date, cases: Case[]) {Also, consider adding unit tests to verify the formatting logic and error handling.
✅ Verification successful
Add JSDoc documentation and unit tests for better maintainability
The original suggestion for adding JSDoc documentation is valid. Additionally, the verification confirms that there are no direct unit tests for the
EventService, despite its important role in posting hearing arrangements. The service is only referenced in test setup modules.
- Add the suggested JSDoc documentation
- Create a dedicated test file (
event.service.spec.ts) to cover:
- Message formatting logic
- Error handling
- Empty case list handling
- Date formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test coverage fd -e spec.ts -e test.ts | grep -l "event.service"Length of output: 50
Script:
#!/bin/bash # Try multiple search strategies to find test files echo "=== Searching for EventService tests ===" rg -l "EventService" --type ts echo -e "\n=== Searching for event.service tests ===" rg -l "event.service" --type ts echo -e "\n=== Looking for spec files in event module ===" fd -e ts . src/app/modules/eventLength of output: 3898
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17468 +/- ##
==========================================
+ Coverage 35.61% 35.65% +0.04%
==========================================
Files 7015 6976 -39
Lines 150294 149937 -357
Branches 42969 42907 -62
==========================================
- Hits 53529 53464 -65
+ Misses 96765 96473 -292
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 97 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 6 Total Test Services: 0 Failed, 6 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/judicial-system/scheduler/src/app/test/run.spec.ts (1)
Line range hint
79-91: Improve test clarity for the new fetch call.The test now expects 4 fetch calls instead of 3, presumably due to the new hearing arrangements functionality. However, the test description and mock responses don't clearly indicate the purpose of this additional call.
Consider:
- Adding a descriptive comment explaining what each fetch call is for
- Including mock responses for hearing arrangement data
- Adding assertions to verify the hearing arrangement API call
beforeEach(async () => { mockFetch + // Mock response for hearing arrangements + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ hearingArrangements: [] }), + }) + // Mock responses for case archiving .mockResolvedValue({ ok: true, json: () => Promise.resolve({ caseArchived: false }), }) .mockResolvedValueOnce({ ok: true, json: () => Promise.resolve({ caseArchived: true }), }) .mockResolvedValueOnce({ ok: true, json: () => Promise.resolve({ caseArchived: true }), }) await givenWhenThen() }) - it('should continue until done', () => { + it('should fetch hearing arrangements and continue archiving until done', () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/scheduler/src/app/test/run.spec.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/scheduler/src/app/test/run.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."
🔇 Additional comments (1)
apps/judicial-system/scheduler/src/app/test/run.spec.ts (1)
31-31: LGTM!The added empty line improves code readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/judicial-system/scheduler/src/app/app.service.ts (4)
Line range hint
27-41: Consider adding retry logic for better resilienceWhile the error handling logs failures, adding retry logic would improve reliability for transient failures.
Consider implementing exponential backoff:
private addMessagesForIndictmentsWaitingForConfirmationToQueue() { + const maxRetries = 3; + const retry = async (attempt = 0): Promise<void> => { + try { return this.messageService .sendMessagesToQueue([ { type: MessageType.NOTIFICATION_DISPATCH, body: { type: NotificationDispatchType.INDICTMENTS_WAITING_FOR_CONFIRMATION, }, }, ]) - .catch((reason) => - // Tolerate failure, but log - this.logger.error('Failed to dispatch notifications', { reason }), - ) + } catch (error) { + if (attempt < maxRetries) { + const delay = Math.pow(2, attempt) * 1000; + this.logger.warn(`Retry attempt ${attempt + 1} after ${delay}ms`, { error }); + await new Promise(resolve => setTimeout(resolve, delay)); + return retry(attempt + 1); + } + this.logger.error('Failed to dispatch notifications after retries', { error }); + } + }; + return retry(); }
Line range hint
43-79: Well-implemented retry mechanism and error handlingThe implementation shows good practices with:
- Time-bounded retries
- Proper error handling
- Secure authorization header usage
Consider extracting the content-type header to a constant:
+const JSON_CONTENT_TYPE = 'application/json'; + private async archiveCases() { // ... headers: { - 'Content-Type': 'application/json', + 'Content-Type': JSON_CONTENT_TYPE, authorization: `Bearer ${this.config.backendAccessToken}`, },
81-103: Improve error handling and URL constructionWhile the timezone-insensitive date handling is intentional (as per previous discussion), there are some improvements possible:
private async postDailyHearingArrangementSummary() { const today = now() + const url = new URL(`api/internal/cases/postHearingArrangements/${today}`, this.config.backendUrl); try { const res = await fetch( - `${this.config.backendUrl}/api/internal/cases/postHearingArrangements/${today}`, + url.toString(), { method: 'POST', headers: { 'Content-Type': 'application/json', authorization: `Bearer ${this.config.backendAccessToken}`, }, }, ) if (!res.ok) { + const errorBody = await res.text(); throw new BadGatewayException( - 'Unexpected error occurred while fetching cases', + `Unexpected error occurred while fetching cases: ${errorBody}`, ) } } catch (error) { + if (error instanceof BadGatewayException) { + throw error; + } throw new BadGatewayException(`Failed to fetch cases: ${error.message}`) } }
105-111: Consider parallel execution for independent operationsThe sequential execution is clear, but independent operations could be executed in parallel for better performance.
async run() { this.logger.info('Scheduler starting') - await this.addMessagesForIndictmentsWaitingForConfirmationToQueue() - await this.archiveCases() - await this.postDailyHearingArrangementSummary() + await Promise.all([ + this.addMessagesForIndictmentsWaitingForConfirmationToQueue(), + this.archiveCases(), + this.postDailyHearingArrangementSummary(), + ]) this.logger.info('Scheduler done') }Note: Only implement this if these operations are truly independent and can be executed concurrently without side effects.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/scheduler/src/app/app.service.ts(4 hunks)apps/judicial-system/scheduler/src/app/test/run.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/scheduler/src/app/test/run.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/scheduler/src/app/app.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/scheduler/src/app/app.service.ts (1)
Learnt from: thorhildurt
PR: island-is/island.is#17468
File: apps/judicial-system/scheduler/src/app/app.service.ts:81-103
Timestamp: 2025-01-10T11:23:01.559Z
Learning: In the judicial system backend, court hearing arrangements and their date handling (including in `postDailyHearingArrangementSummary`) are designed to be timezone-insensitive, using native Date objects without specific timezone handling.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (1)
apps/judicial-system/scheduler/src/app/app.service.ts (1)
3-3: LGTM: Import follows NestJS best practicesThe BadGatewayException import is appropriately added and used for error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/judicial-system/scheduler/src/app/app.service.ts (3)
Line range hint
27-41: Consider adding retry logic for resilienceWhile the error handling is appropriate for a scheduler task, consider implementing retry logic to improve reliability, similar to the archiveCases method.
private addMessagesForIndictmentsWaitingForConfirmationToQueue() { + const startTime = now() + const retry = async () => { return this.messageService .sendMessagesToQueue([ { type: MessageType.NOTIFICATION_DISPATCH, body: { type: NotificationDispatchType.INDICTMENTS_WAITING_FOR_CONFIRMATION, }, }, ]) .catch((reason) => { - // Tolerate failure, but log - this.logger.error('Failed to dispatch notifications', { reason }), + this.logger.error('Failed to dispatch notifications', { reason }) + if (minutesBetween(startTime, now()) < this.config.timeToLiveMinutes) { + return retry() + } }) + } + return retry() }
81-103: Consider enhancing error handling and retry logicWhile the error handling is good, consider these improvements:
- Add retry logic similar to archiveCases for consistency
- Include more specific error details in the BadGatewayException message
private async postDailyHearingArrangementSummary() { const today = now() + const startTime = now() + + const attemptPost = async () => { try { const res = await fetch( `${this.config.backendUrl}/api/internal/cases/postHearingArrangements/${today}`, { method: 'POST', headers: { 'Content-Type': 'application/json', authorization: `Bearer ${this.config.backendAccessToken}`, }, }, ) if (!res.ok) { + const errorBody = await res.text() throw new BadGatewayException( - 'Unexpected error occurred while fetching cases', + `Unexpected error occurred while fetching cases: ${res.status} - ${errorBody}`, ) } } catch (error) { - throw new BadGatewayException(`Failed to fetch cases: ${error.message}`) + if (minutesBetween(startTime, now()) < this.config.timeToLiveMinutes) { + return attemptPost() + } + throw new BadGatewayException( + `Failed to fetch cases after retries: ${error.message}` + ) } + } + return attemptPost() }
105-112: Consider parallel execution for independent tasksThe run method executes tasks sequentially, which is safe but might not be optimal. Consider running independent tasks in parallel using Promise.all() if they don't have dependencies.
async run() { this.logger.info('Scheduler starting') - await this.addMessagesForIndictmentsWaitingForConfirmationToQueue() - await this.archiveCases() - await this.postDailyHearingArrangementSummary() + await Promise.all([ + this.addMessagesForIndictmentsWaitingForConfirmationToQueue(), + this.archiveCases(), + this.postDailyHearingArrangementSummary(), + ]) this.logger.info('Scheduler done') }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/scheduler/src/app/app.service.ts(4 hunks)apps/judicial-system/scheduler/src/app/test/run.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/scheduler/src/app/test/run.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/scheduler/src/app/app.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/scheduler/src/app/app.service.ts (1)
Learnt from: thorhildurt
PR: island-is/island.is#17468
File: apps/judicial-system/scheduler/src/app/app.service.ts:81-103
Timestamp: 2025-01-10T11:23:01.559Z
Learning: In the judicial system backend, court hearing arrangements and their date handling (including in `postDailyHearingArrangementSummary`) are designed to be timezone-insensitive, using native Date objects without specific timezone handling.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (2)
apps/judicial-system/scheduler/src/app/app.service.ts (2)
3-3: LGTM: Import addition is well-placedThe BadGatewayException import is appropriately grouped with other NestJS imports and is used correctly in the error handling.
Line range hint
43-79: LGTM: Well-implemented retry mechanismThe archiveCases method implements a robust retry mechanism with proper time limiting and error handling.
…github.com/island-is/island.is into j-s/add-slack-log-for-hearing-arrangements # Conflicts: # apps/judicial-system/scheduler/src/app/test/run.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/judicial-system/scheduler/src/app/app.service.ts (1)
Line range hint
43-79: Enhance error logging contextThe implementation is robust with proper retry mechanism and timeout. Consider enhancing the error logging by including more context about the failure.
- this.logger.error('Failed to archive cases', { reason }) + this.logger.error('Failed to archive cases', { + reason, + attemptNumber: minutesBetween(startTime, now()), + maxAttempts: this.config.timeToLiveMinutes + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts(1 hunks)apps/judicial-system/scheduler/src/app/app.service.ts(4 hunks)apps/judicial-system/scheduler/src/app/test/run.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/scheduler/src/app/test/run.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/scheduler/src/app/app.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/scheduler/src/app/app.service.ts (1)
Learnt from: thorhildurt
PR: island-is/island.is#17468
File: apps/judicial-system/scheduler/src/app/app.service.ts:81-103
Timestamp: 2025-01-10T11:23:01.559Z
Learning: In the judicial system backend, court hearing arrangements and their date handling (including in `postDailyHearingArrangementSummary`) are designed to be timezone-insensitive, using native Date objects without specific timezone handling.
🔇 Additional comments (3)
apps/judicial-system/scheduler/src/app/app.service.ts (2)
3-3: LGTM!The addition of BadGatewayException import is appropriate for handling gateway errors in the new functionality.
Line range hint
27-41: LGTM! Well-structured queue message handlingThe implementation follows good practices with proper error handling and logging. The method gracefully handles failures without breaking the execution flow.
apps/judicial-system/backend/src/app/modules/case/internalCase.controller.ts (1)
80-92: 🛠️ Refactor suggestionAdd API documentation, input validation, and error handling
The endpoint implementation needs several improvements:
- Missing API documentation
- No input validation for the date parameter
- Basic error handling
- Unsafe date conversion
+ @ApiOkResponse({ + description: 'Successfully posted hearing arrangements summary', + }) @Post('cases/postHearingArrangements') async postHearingArrangements( - @Body() { date }: { date: Date }, + @Body(new ValidationPipe({ + transform: true, + whitelist: true, + })) + body: { date: string }, ): Promise<void> { this.logger.debug( - `Post internal summary of all cases that have court hearing arrangement at ${date}`, + 'Posting internal summary of court hearing arrangements', + { date: body.date } ) - const cases = await this.internalCaseService.getCaseHearingArrangements( - new Date(date), - ) - await this.eventService.postDailyHearingArrangementEvents(date, cases) + try { + const parsedDate = new Date(body.date) + if (isNaN(parsedDate.getTime())) { + throw new BadRequestException('Invalid date format') + } + + const cases = await this.internalCaseService.getCaseHearingArrangements(parsedDate) + await this.eventService.postDailyHearingArrangementEvents(parsedDate, cases) + } catch (error) { + this.logger.error('Failed to post hearing arrangements', { error, date: body.date }) + if (error instanceof BadRequestException) { + throw error + } + throw new InternalServerErrorException('Failed to process hearing arrangements') + } }Likely invalid or redundant comment.
…s/add-slack-log-for-hearing-arrangements
thorhildurt
added
the
deprecated:automerge
Betri yfirsýn fyrir fyrirtökur dagsins
What
Why
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Maintenance