chore(j-s): Indictment Case Arrignment Date #16156
Conversation
WalkthroughThe changes involve modifications to the 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (2)
Line range hint
129-133: Implement error handling improvement instead of TODOThe TODO comment highlights an important improvement for error handling. However, it's better to implement the suggestion rather than leaving it as a TODO in the code.
Consider implementing the error handling improvement as follows:
const [affectedRows] = await this.subpoenaModel.update( { subpoenaId: createdSubpoena.subpoenaId }, { where: { id: subpoena.id } }, ); if (affectedRows === 0) { this.logger.error('Failed to update subpoena with police subpoenaId'); // Consider throwing an error or handling this case appropriately return { delivered: false }; }If you prefer to address this in a separate PR, I can create a GitHub issue to track this improvement. Would you like me to do that?
Line range hint
1-145: Enhance overall robustness and consistency of SubpoenaServiceWhile the
SubpoenaServiceclass is well-structured, consider the following improvements to enhance its robustness and consistency:
Implement consistent error handling across all methods. For example, the
updatemethod could benefit from more detailed error handling similar tofindBySubpoenaId.Use transactions for operations that involve multiple database updates, such as in the
updatemethod when both subpoena and defendant are updated.Enhance logging throughout the class. Add more detailed log messages for important operations and their outcomes.
Here's an example of how you could improve the
updatemethod:async update( subpoena: Subpoena, update: UpdateSubpoenaDto, transaction?: Transaction, ): Promise<Subpoena> { const t = transaction || await this.sequelize.transaction(); try { const [subpoenaAffectedRows] = await this.subpoenaModel.update(update, { where: { subpoenaId: subpoena.subpoenaId }, returning: true, transaction: t, }); if (subpoenaAffectedRows === 0) { throw new Error(`Failed to update subpoena with id ${subpoena.subpoenaId}`); } if (update.defenderChoice || update.defenderNationalId) { const defendantUpdate: Partial<Defendant> = { defenderChoice: update.defenderChoice, defenderNationalId: update.defenderNationalId, defenderName: update.defenderName, defenderEmail: update.defenderEmail, defenderPhoneNumber: update.defenderPhoneNumber, }; const [defenderAffectedRows] = await this.defendantModel.update( defendantUpdate, { where: { id: subpoena.defendantId }, transaction: t, }, ); if (defenderAffectedRows === 0) { throw new Error(`Failed to update defendant with id ${subpoena.defendantId}`); } } if (!transaction) await t.commit(); const updatedSubpoena = await this.findBySubpoenaId(subpoena.subpoenaId); this.logger.info(`Successfully updated subpoena with id ${subpoena.subpoenaId}`); return updatedSubpoena; } catch (error) { if (!transaction) await t.rollback(); this.logger.error(`Error updating subpoena: ${error.message}`, error); throw error; } }This refactored version includes better error handling, use of transactions, and enhanced logging.
apps/judicial-system/web/src/routes/Court/Indictments/Subpoena/Subpoena.tsx (4)
42-42: LGTM: Improved arraignment status checkThe change from
isArraignmentDonetoisArraignmentScheduledbetter aligns with the PR objectives. It correctly focuses on whether an arraignment date is set rather than if it's completed.Consider using nullish coalescing for a more explicit check:
const isArraignmentScheduled = workingCase.arraignmentDate != nullThis makes the intention clearer and handles both
nullandundefinedexplicitly.
Line range hint
46-78: LGTM: Updated navigation logic and removed frontend notificationsThe changes in the
handleNavigationTofunction align well with the PR objectives:
- The condition now correctly checks if an arraignment is scheduled rather than completed.
- The removal of frontend notification logic supports the shift to backend-handled notifications.
Consider adding a comment explaining why the navigation logic differs based on
isArraignmentScheduled. This would improve code readability and maintainability.// If arraignment is already scheduled, we can navigate directly. // Otherwise, we need to send the court date and update defendants before navigation. if (isArraignmentScheduled) { // ... existing code ... }
150-155: LGTM: Updated navigation logic in FormFooterThe changes in the
onNextButtonClickprop correctly useisArraignmentScheduledto determine the navigation behavior. This is consistent with the earlier changes and PR objectives.To improve code clarity, consider extracting the navigation path to a constant:
const NEXT_ROUTE = `${constants.INDICTMENTS_DEFENDER_ROUTE}/${workingCase.id}`; onNextButtonClick={() => { if (isArraignmentScheduled) { router.push(NEXT_ROUTE); } else { setNavigateTo(constants.INDICTMENTS_DEFENDER_ROUTE); } }}This reduces repetition and makes the code more maintainable.
157-160: LGTM: Updated next button text logicThe change to use
isArraignmentScheduledfor determining thenextButtonTextis consistent with the earlier modifications and PR objectives. Usingundefinedwhen the arraignment is scheduled is a good practice, allowing the component to use its default text.To improve readability, consider using an early return:
nextButtonText={ isArraignmentScheduled ? undefined : formatMessage(strings.nextButtonText) }This makes the conditional logic more explicit and easier to understand at a glance.
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
1342-1346: Simplify code by removing unnecessaryBoolean()wrapperSince
arraignmentDateChangedis now a boolean after fixing the date comparisons, wrapping it withBoolean()is redundant.Apply this diff:
await this.addMessagesForNewCourtDateToQueue( updatedCase, user, - Boolean(arraignmentDateChanged), + arraignmentDateChanged, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/notification/notification.service.ts (0 hunks)
- apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (1 hunks)
- apps/judicial-system/web/src/routes/Court/Indictments/Subpoena/Subpoena.tsx (5 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/notification/notification.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
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/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/web/src/routes/Court/Indictments/Subpoena/Subpoena.tsx (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 not posted (4)
apps/judicial-system/web/src/routes/Court/Indictments/Subpoena/Subpoena.tsx (2)
24-24: LGTM: New import for useDefendants hookThe addition of the
useDefendantshook import is appropriate and aligns with the PR objectives. It follows NextJS best practices for importing custom hooks.
115-116: LGTM: Consistent disabling of court arrangementsThe update to use
isArraignmentScheduledfor bothdateTimeDisabledandcourtRoomDisabledprops is consistent with the overall changes and PR objectives. This ensures that court arrangements cannot be modified once an arraignment is scheduled, maintaining data integrity.apps/judicial-system/backend/src/app/modules/case/case.service.ts (2)
1146-1168: Changes toaddMessagesForNewCourtDateToQueuelook goodThe addition of the
arraignmentDateChangedparameter and the conditional logic for sending additional messages are correctly implemented.
1146-1146: Ensure all calls toaddMessagesForNewCourtDateToQueueinclude the new parameterVerify that all calls to
addMessagesForNewCourtDateToQueuehave been updated to include the newarraignmentDateChangedparameter to prevent runtime errors.Run the following script to check for any outdated method calls:
apps/judicial-system/backend/src/app/modules/case/case.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16156 +/- ##
==========================================
+ Coverage 36.69% 36.71% +0.01%
==========================================
Files 6776 6774 -2
Lines 139590 139547 -43
Branches 39678 39662 -16
==========================================
+ Hits 51226 51237 +11
+ Misses 88364 88310 -54
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 15 Total Test Services: 0 Failed, 15 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
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/case/test/caseController/update.spec.ts (1)
906-907: Ensure consistent use ofcaseIdin message objectsIn the
sendMessagesToQueuecalls, you're usingcaseId: theCase.idfor some messages andcaseIdfor others. SincecaseIdandtheCase.idare the same, for consistency and clarity, consider usingcaseIddirectly in all message objects.Apply this diff to make the change:
{ type: MessageType.DELIVERY_TO_POLICE_SUBPOENA, user, - caseId: theCase.id, + caseId, elementId: defendantId1, }, { type: MessageType.DELIVERY_TO_POLICE_SUBPOENA, user, - caseId: theCase.id, + caseId, elementId: defendantId2, },Also applies to: 912-913
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/case/test/caseController/update.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/test/caseController/update.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/caseController/update.spec.ts
Show resolved
Hide resolved
…ment-case-arraignment-date
…ment-case-arraignment-date
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/case.service.ts (1)
1149-1171: LGTM! Consider adding a comment for clarity.The changes to
addMessagesForNewCourtDateToQueuealign well with the PR objectives. The new parameterarraignmentDateChangedallows for more specific handling of arraignment date changes, improving the notification process.Consider adding a brief comment explaining the purpose of the new conditional block:
if (arraignmentDateChanged) { + // Send subpoena messages for each defendant when the arraignment date changes theCase.defendants?.forEach((defendant) => { messages.push({ type: MessageType.DELIVERY_TO_POLICE_SUBPOENA, user, caseId: theCase.id, elementId: defendant.id, }) }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
1330-1352: Great job addressing the date comparison issue!The changes to
addMessagesForUpdatedCaseToQueuesignificantly improve the accuracy of date change detection for indictment cases. The use ofgetTime()for date comparisons resolves the issue mentioned in the past review comments. The addition of thearraignmentDateChangedparameter toaddMessagesForNewCourtDateToQueueallows for more specific handling of arraignment date changes.These changes align well with the PR objectives and enhance the notification system for indictment case arraignment dates.
* Locks subpoena fields when arraignment date has been set * Move arraignment date message handling to the server side * Updates tests and fixes date comparison --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Indictment Case Arrignment Date
Höndla tilkynningar og skil á fyrirkalli í bakenda
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
CaseControllerto include updates for both arraignment and court dates under the indictment case type.