fix(j-s): Handle and log updated accused postponed appeal date #16555
Conversation
WalkthroughThe changes involve enhancements to the appeal information handling within the judicial system's case management module. A new test case has been added to verify the behavior of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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/api/src/app/modules/case/interceptors/case.transformer.ts (1)
129-134: Add type safety and documentation to appeal date transformation.While the logic is correct, the code could benefit from improved type safety and documentation.
Consider these improvements:
+ // Only preserve postponed appeal dates if an appeal has been made + // to prevent showing stale dates for non-appealed cases accusedPostponedAppealDate: appealInfo.hasBeenAppealed - ? theCase.accusedPostponedAppealDate + ? theCase.accusedPostponedAppealDate ?? null : undefined, prosecutorPostponedAppealDate: appealInfo.hasBeenAppealed - ? theCase.prosecutorPostponedAppealDate + ? theCase.prosecutorPostponedAppealDate ?? null : undefined,apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (1)
Line range hint
333-342: Improve error handling consistencyWhile the error handling covers the edge cases, it could be more consistent and actionable:
- The error for multiple rows affected should be treated as seriously as zero rows
- Error messages could provide more context for debugging
Consider this improved implementation:
const [numberOfAffectedRows] = await this.caseModel.update( { ...update }, { where: { id: theCase.id } }, ) -if (numberOfAffectedRows > 1) { - // Tolerate failure, but log error - this.logger.error( - `Unexpected number of rows (${numberOfAffectedRows}) affected when updating case ${theCase.id}`, - ) -} else if (numberOfAffectedRows < 1) { +if (numberOfAffectedRows !== 1) { + const errorMessage = numberOfAffectedRows > 1 + ? `Multiple rows (${numberOfAffectedRows}) affected when updating case` + : 'No rows affected during update' + throw new InternalServerErrorException( - `Could not update case ${theCase.id}`, + `Update failed for case ${theCase.id}: ${errorMessage}`, + { + cause: { + caseId: theCase.id, + affectedRows: numberOfAffectedRows, + update, + }, + } ) }apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (2)
543-566: Suggest improving test name for clarity.The current test name "should return not return appealedDate if case has not been appealed" has a grammatical error and could be more descriptive. Consider renaming to better reflect the edge case being tested.
- it('should return not return appealedDate if case has not been appealed', () => { + it('should not return appealedDate when appeal is postponed but not yet submitted', () => {
543-566: Add documentation for edge case context.Consider adding a comment to explain the significance of this test case, as it verifies an important edge case where postponed appeal dates are set but the appeal hasn't been formally submitted.
+ // This test verifies that even when postponed appeal dates are set, + // the appealedDate should not be returned until the appeal is formally + // submitted (appealState is set) const rulingDate = new Date().toISOString()apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
1672-1687: LGTM! Consider adding type safety for the logged object.The logging implementation follows good practices with structured JSON format and captures relevant appeal information without exposing sensitive data.
Consider defining an interface for the logged object to ensure type safety:
interface AppealDateUpdateInfo { appealState: CaseAppealState; accusedAppealDecision: CaseAppealDecision; accusedPostponedAppealDate: Date | null; prosecutorAppealDecision: CaseAppealDecision; prosecutorPostponedAppealDate: Date | null; update: Partial<Pick<UpdateCase, 'accusedPostponedAppealDate'>>; }Then use it in the logging:
- const relevantInfo = { + const relevantInfo: AppealDateUpdateInfo = { appealState: theCase.appealState, accusedAppealDecision: theCase.accusedAppealDecision, accusedPostponedAppealDate: theCase.accusedPostponedAppealDate, prosecutorAppealDecision: theCase.prosecutorAppealDecision, prosecutorPostponedAppealDate: theCase.prosecutorPostponedAppealDate, update: update, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (1 hunks)
- apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (2 hunks)
- apps/judicial-system/backend/src/app/modules/case/case.service.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.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/api/src/app/modules/case/interceptors/case.transformer.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/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/limitedAccessCase.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/api/src/app/modules/case/interceptors/case.transformer.ts (1)
65-78: Verify logging implementation for appeal date changes.The PR objectives mention logging of appeal date changes, but no logging is implemented in this transformer. Ensure that logging is implemented in the appropriate service layer.
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (1)
543-566: LGTM! Test implementation is thorough and well-structured.The test case:
- Properly validates the edge case of postponed but not submitted appeals
- Has comprehensive assertions covering all relevant appeal info properties
- Follows the established testing patterns in the codebase
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16555 +/- ##
==========================================
- Coverage 36.80% 36.77% -0.03%
==========================================
Files 6854 6852 -2
Lines 142215 142166 -49
Branches 40549 40544 -5
==========================================
- Hits 52336 52286 -50
- Misses 89879 89880 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 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
|
https://app.asana.com/0/home/1203322535524092/1203701116749449
What
Handle and log cases where accused postponed appeal date is filled out before appeal is sent
Why
For better handling of appeals
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes