fix(j-s): LÖKE File Upload #16732
fix(j-s): LÖKE File Upload #16732
Conversation
WalkthroughThe changes in this pull request primarily focus on 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: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (2)
1008-1010: Consider enhancing error handling for individual file retrievals.While there is error handling at the function level, individual file retrievals could benefit from more specific error handling to ensure proper logging and error recovery.
- const file = await this.fileService.getCaseFileFromS3( - theCase, - caseFile, - ) + try { + const file = await this.fileService.getCaseFileFromS3( + theCase, + caseFile, + ) + return { + type: caseFile.category === CaseFileCategory.COURT_RECORD + ? PoliceDocumentType.RVTB + : PoliceDocumentType.RVDO, + courtDocument: Base64.btoa(file.toString('binary')), + } + } catch (error) { + this.logger.warn( + `Failed to retrieve file ${caseFile.id} for case ${theCase.id}`, + { error } + ) + return null + }
1157-1159: Apply consistent error handling pattern for file retrievals.Similar to the previous suggestion, implement granular error handling for individual file retrievals in the appeal delivery process.
- const file = await this.fileService.getCaseFileFromS3( - theCase, - caseFile, - ) + try { + const file = await this.fileService.getCaseFileFromS3( + theCase, + caseFile, + ) + return { + type: PoliceDocumentType.RVUL, + courtDocument: Base64.btoa(file.toString('binary')), + } + } catch (error) { + this.logger.warn( + `Failed to retrieve appeal file ${caseFile.id} for case ${theCase.id}`, + { error } + ) + return null + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
1008-1010: LGTM: File retrieval changes align with PR objectives.
The switch to using fileService.getCaseFileFromS3 provides better control over when and how files are retrieved, addressing the core issue of ensuring confirmed rulings and court records are sent at the appropriate time.
Also applies to: 1157-1159
Datadog ReportBranch report: ✅ 0 Failed, 21217 Passed, 0 Skipped, 16m 1.93s Total Time |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverAppealToPolice.spec.ts (2)
70-70: Consider adding test cases for unconfirmed documentsWhile the test correctly handles the confirmed APPEAL_RULING case, we should also verify that unconfirmed documents are not sent to LÖKE.
Consider adding test cases for:
- Documents with different categories
- Cases with multiple files
- Cases with unconfirmed rulings
Also applies to: 82-82
88-100: LGTM: Consider adding response validationThe service interaction testing is properly implemented. However, consider adding assertions to validate the structure and content of the response object beyond just the
deliveredflag.Add assertions for:
expect(then.result).toEqual({ delivered: true, // Add other expected properties });apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCaseToPolice.spec.ts (2)
35-35: Consider using a more specific error message in the mockThe mock setup is well-structured, but the error message 'Some error' could be more specific to aid in debugging.
- mockGetCaseFileFromS3.mockRejectedValue(new Error('Some error')) + mockGetCaseFileFromS3.mockRejectedValue(new Error('Failed to retrieve case file from S3'))Also applies to: 43-43, 48-49
74-83: LGTM: Well-structured case file objectsThe case file objects are well-structured and properly typed. Consider adding a brief comment explaining what each case file represents in the test context.
+ // Test case files representing court record and ruling documents const caseFile1 = { id: uuid(), key: uuid(), category: CaseFileCategory.COURT_RECORD, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverAppealToPolice.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCaseToPolice.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverAppealToPolice.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/internalCaseController/deliverIndictmentCaseToPolice.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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverAppealToPolice.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-10-17T11:30:12.482Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
🔇 Additional comments (4)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverAppealToPolice.spec.ts (2)
16-16: LGTM: Service transition implemented correctly
The switch from AwsS3Service to FileService is properly implemented in the test setup, maintaining consistency with the service layer changes.
Also applies to: 32-32, 37-42
43-44: Verify error handling coverage
The error handling setup looks good, but let's verify that we're testing all potential error scenarios from FileService.
✅ Verification successful
Error handling coverage is appropriate
The error handling in the test is sufficient as it covers the main error path from getCaseFileFromS3. Looking at the implementation:
-
The test mocks
getCaseFileFromS3to reject with a generic error, which is appropriate since this method can throw errors from:- AWS S3 service calls
- File verification checks
- Document confirmation process
-
The error propagation is handled at the controller level, which is the correct architectural approach.
-
Other test files (deliverIndictmentToPolice.spec.ts, deliverIndictmentCaseToPolice.spec.ts) follow the same error handling pattern, showing consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error handling scenarios in FileService
ast-grep --pattern 'getCaseFileFromS3($_) {
$$$
throw new $_($_)
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Let's first find the FileService implementation
fd "FileService" --type f
# Then examine its content and error handling
rg -A 5 "getCaseFileFromS3"
Length of output: 12948
Script:
#!/bin/bash
# Let's examine the implementation of FileService and its error handling
cat apps/judicial-system/backend/src/app/modules/file/file.service.ts
Length of output: 17143
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/deliverIndictmentCaseToPolice.spec.ts (2)
16-16: LGTM: FileService import added correctly
The import statement for FileService is properly added, aligning with the service replacement changes.
92-92: Verify test coverage for error scenarios
The happy path testing is well implemented. However, we should verify that error scenarios are properly covered, especially since this PR fixes a bug related to document transmission timing.
Let's check if there are other test files covering error scenarios:
Also applies to: 98-101, 110-116
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16732 +/- ##
==========================================
- Coverage 36.55% 36.55% -0.01%
==========================================
Files 6892 6892
Lines 143767 143630 -137
Branches 40964 40919 -45
==========================================
- Hits 52557 52504 -53
+ Misses 91210 91126 -84
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 53 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
oddsson
added
the
deprecated:automerge
* Fix loke file upload * Updates unit tests --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Ívar Oddsson <[email protected]>
LÖKE File Upload
Seinka hvenær þingbok og dómur er sent í LÖKE- þarf að vera með staðfestingu
What
Why
Checklist:
Summary by CodeRabbit