feat(j-s): Zip all files for defenders in indictment cases #17716
Conversation
WalkthroughThis pull request introduces modifications to the judicial system's backend and frontend, specifically enhancing file access and download capabilities for various case types. Key changes include the addition of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
🧹 Nitpick comments (4)
apps/judicial-system/web/src/components/ZipButton/ZipButton.tsx (2)
22-22: Improve download filename handling.The filename should handle cases where
courtCaseNumberis null and include proper file extension.Apply this diff to improve the filename handling:
- download={`mal_${courtCaseNumber}`} + download={`mal_${courtCaseNumber || 'unknown'}.zip`}
25-27: Add loading state to prevent multiple clicks.The button should show a loading state while the download is in progress to prevent multiple clicks and provide better user feedback.
Apply this diff to add loading state:
+import { useState } from 'react' + const ZipButton: FC<Props> = (props) => { const { formatMessage } = useIntl() const { caseId, courtCaseNumber } = props + const [isLoading, setIsLoading] = useState(false) + + const handleClick = () => { + setIsLoading(true) + } return ( <a href={`${api.apiUrl}/api/case/${encodeURIComponent(caseId)}/limitedAccess/allFiles`} download={`mal_${courtCaseNumber || 'unknown'}.zip`} className={styles.downloadAllButton} + onClick={handleClick} > - <Button variant="ghost" size="small" icon="download" iconType="outline"> + <Button + variant="ghost" + size="small" + icon="download" + iconType="outline" + loading={isLoading} + > {formatMessage(strings.getAllDocuments)} </Button> </a>apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts (1)
Line range hint
33-41: Consider using array spread for better readability.While
concatworks correctly, using array spread operator would improve readability.Apply this diff to use array spread:
-export const defenderCaseFileCategoriesForIndictmentCases = - defenderDefaultCaseFileCategoriesForIndictmentCases.concat( - CaseFileCategory.CRIMINAL_RECORD, - CaseFileCategory.COST_BREAKDOWN, - CaseFileCategory.CASE_FILE, - CaseFileCategory.PROSECUTOR_CASE_FILE, - CaseFileCategory.DEFENDANT_CASE_FILE, - CaseFileCategory.CIVIL_CLAIM, - ) +export const defenderCaseFileCategoriesForIndictmentCases = [ + ...defenderDefaultCaseFileCategoriesForIndictmentCases, + CaseFileCategory.CRIMINAL_RECORD, + CaseFileCategory.COST_BREAKDOWN, + CaseFileCategory.CASE_FILE, + CaseFileCategory.PROSECUTOR_CASE_FILE, + CaseFileCategory.DEFENDANT_CASE_FILE, + CaseFileCategory.CIVIL_CLAIM, +]apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (1)
571-574: Use optional chaining for safer property access.The nested property access can be simplified using optional chaining.
- const defendant = theCase.defendants && theCase.defendants[0] - const subpoena = - defendant && defendant.subpoenas && defendant.subpoenas[0] + const defendant = theCase.defendants?.[0] + const subpoena = defendant?.subpoenas?.[0]🧰 Tools
🪛 Biome (1.9.4)
[error] 571-572: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts(4 hunks)apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts(1 hunks)apps/judicial-system/backend/src/app/modules/file/index.ts(1 hunks)apps/judicial-system/web/src/components/ZipButton/ZipButton.strings.ts(1 hunks)apps/judicial-system/web/src/components/ZipButton/ZipButton.tsx(1 hunks)apps/judicial-system/web/src/components/index.ts(1 hunks)apps/judicial-system/web/src/routes/Defender/CaseOverview.strings.ts(0 hunks)apps/judicial-system/web/src/routes/Defender/CaseOverview.tsx(5 hunks)apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/web/src/routes/Defender/CaseOverview.strings.ts
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/web/src/components/ZipButton/ZipButton.strings.ts
🧰 Additional context used
📓 Path-based instructions (8)
apps/judicial-system/web/src/components/index.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/file/index.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/components/ZipButton/ZipButton.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."
apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.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/Shared/IndictmentOverview/IndictmentOverview.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."
apps/judicial-system/web/src/routes/Defender/CaseOverview.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."
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.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/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."
📓 Learnings (2)
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getIndictmentPdfGuards.spec.ts:24-25
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In certain scenarios within the judicial-system backend, the `RolesGuard` may intentionally follow the `CaseExistsGuard` when specific roles rules require the guard order to be reversed, as seen in tests like `getIndictmentPdfGuards.spec.ts`.
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
[error] 571-572: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 573-575: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
apps/judicial-system/backend/src/app/modules/file/index.ts (1)
5-7: LGTM! The exports align with the PR objectives.The new exports enable the implementation of ZIP functionality for indictment cases, maintaining consistent code organization.
apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts (1)
28-31: LGTM! Well-defined default categories.The default categories for indictment cases are correctly defined and align with the PR objectives.
apps/judicial-system/web/src/components/index.ts (1)
116-116: LGTM! Export follows consistent pattern.The ZipButton export maintains the established pattern of component exports in the file.
apps/judicial-system/web/src/routes/Defender/CaseOverview.tsx (2)
226-226: Consistent prop value format.The change from
pdfType={'value'}topdfType="value"improves code consistency and readability.Also applies to: 234-234, 247-247
259-262: LGTM! ZipButton integration for completed cases.The ZipButton is correctly placed after other document buttons and properly guarded by the completed case check.
apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx (2)
186-225: Improved layout structure with semantic HTML.The restructuring enhances readability by:
- Using semantic section elements
- Properly organizing conditional rendering
- Maintaining clear component hierarchy
305-311: LGTM! Proper access control for ZipButton.The ZipButton is correctly restricted to:
- Defender users only
- Completed cases only
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (1)
399-403: LGTM! Extended case type support.The CaseTypeGuard is correctly updated to include indictment cases, maintaining proper guard ordering as per established patterns.
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (2)
Line range hint
538-551: Performance improvement using Promise.all.Good optimization by fetching files concurrently instead of sequentially.
🧰 Tools
🪛 Biome (1.9.4)
[error] 571-572: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 573-575: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
605-615: LGTM! Well-structured file category logic.The getFileCategories method:
- Cleanly separates category logic
- Handles both request and indictment cases
- Considers file sharing status
| @@ -519,18 +526,17 @@ export class LimitedAccessCaseService { | |||
|
|
|||
| async getAllFilesZip(theCase: Case, user: TUser): Promise<Buffer> { | |||
There was a problem hiding this comment.
Question to @gudjong
This PR introduces quite a bit of complexity to this function. This function now decides what case files should be included in the ZIP file based on the type of the case and whether or not the the case files are shared with the defender or not.
I'm a bit afraid that a regression will lead to files being included in the ZIP file that should not be included and users will be able to see files that they are not supposed to.
Would you / how would you add tests for this function? Would you write test for this function in the service or would you test the controller? Can we maybe pair on this?
There was a problem hiding this comment.
Great questions. I am not sure what would be the best way to test but lean towards testing the controller. I´ll take a look at the code and pairing the tests sounds good.
There was a problem hiding this comment.
This looks really good. I have one change request regarding multiple case files records and one suggestion to support multiple defendants right away so we do not need to revisit this code when prosecutors start sending us indictments with multiple defendants.
I am also wondering if we shouldn't also include files from merged cases. The defenders can access from the client and including them in the ZIP file sounds prudent.
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 7e809e8.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (1)
Line range hint
530-552: Consider implementing error recovery for failed file fetches.While the current implementation logs warnings for failed file fetches, it might be worth considering whether to:
- Notify the user about failed files
- Implement retry logic for transient failures
- Add file fetch timeout handling
const fileFetchPromises = caseFilesByCategory.map((file) => { + const timeout = 30000; // 30 seconds + const fetchWithTimeout = Promise.race([ this.awsS3Service .getObject(theCase.type, file.key) - .then((content) => filesToZip.push({ data: content, name: file.name })) + .then((content) => { + filesToZip.push({ data: content, name: file.name }); + return { success: true, file: file.name }; + }) .catch((reason) => { // Tolerate failure, but log what happened this.logger.warn( `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, { reason }, - ), + ); + return { success: false, file: file.name, error: reason }; + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`Timeout fetching ${file.name}`)), timeout) + ), + ]); + return fetchWithTimeout; })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts(4 hunks)apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/file/guards/caseFileCategory.ts
🧰 Additional context used
📓 Path-based instructions (1)
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."
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
[error] 572-573: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 574-576: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests (judicial-system-backend)
🔇 Additional comments (3)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts (3)
28-29: LGTM! Clean import organization.The new imports are well-organized and clearly indicate the added functionality for handling different case types.
Also applies to: 44-49
582-586: 🛠️ Refactor suggestionHandle all police case numbers as noted in past reviews.
As mentioned in past reviews, we need to handle all police case numbers since there's a separate case files record for each one.
-data: await this.pdfService.getCaseFilesRecordPdf( - theCase, - theCase.policeCaseNumbers[0], -), -name: `Skjalaskrá ${theCase.courtCaseNumber}.pdf`, +...(await Promise.all(theCase.policeCaseNumbers.map(async (policeCaseNumber) => ({ + data: await this.pdfService.getCaseFilesRecordPdf( + theCase, + policeCaseNumber, + ), + name: `Skjalaskrá ${policeCaseNumber}.pdf`, +})))),Likely invalid or redundant comment.
606-616: LGTM! Clean separation of file category logic.The
getFileCategoriesmethod effectively encapsulates the logic for determining which file categories should be included based on case type and sharing status.However, consider adding unit tests as suggested in past reviews to ensure the security of file access controls.
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/judicial-system/web/src/utils/utils.ts (1)
148-170: Consider breaking down complex boolean logic into smaller functions.The function combines multiple complex conditions. Consider extracting the defendant and civil claimant checks into separate helper functions for better maintainability and testability.
+const isEligibleDefender = (defendant: Defendant, userNationalId?: string) => + defendant.isDefenderChoiceConfirmed && + defendant.caseFilesSharedWithDefender && + defendant.defenderNationalId && + normalizeAndFormatNationalId(userNationalId).includes( + defendant.defenderNationalId, + ) +const isEligibleCivilClaimant = (civilClaimant: CivilClaimant, userNationalId?: string) => + civilClaimant.hasSpokesperson && + civilClaimant.isSpokespersonConfirmed && + civilClaimant.caseFilesSharedWithSpokesperson && + civilClaimant.spokespersonNationalId && + normalizeAndFormatNationalId(userNationalId).includes( + civilClaimant.spokespersonNationalId, + ) export const shouldDisplayGeneratedPdfFiles = (theCase: Case, user?: User) => Boolean( isProsecutionUser(user) || - theCase.defendants?.some( - (defendant) => - defendant.isDefenderChoiceConfirmed && - defendant.caseFilesSharedWithDefender && - defendant.defenderNationalId && - normalizeAndFormatNationalId(user?.nationalId).includes( - defendant.defenderNationalId, - ), - ) || - theCase.civilClaimants?.some( - (civilClaimant) => - civilClaimant.hasSpokesperson && - civilClaimant.isSpokespersonConfirmed && - civilClaimant.caseFilesSharedWithSpokesperson && - civilClaimant.spokespersonNationalId && - normalizeAndFormatNationalId(user?.nationalId).includes( - civilClaimant.spokespersonNationalId, - ), - ), + theCase.defendants?.some((defendant) => + isEligibleDefender(defendant, user?.nationalId) + ) || + theCase.civilClaimants?.some((civilClaimant) => + isEligibleCivilClaimant(civilClaimant, user?.nationalId) + ), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts(3 hunks)apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx(4 hunks)apps/judicial-system/web/src/utils/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx
🧰 Additional context used
📓 Path-based instructions (2)
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."
apps/judicial-system/web/src/utils/utils.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/limitedAccessCase.service.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-11-12T15:15:20.157Z
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`
| private shouldDisplayGeneratedPdfFiles = (theCase: Case, user?: TUser) => | ||
| Boolean( | ||
| isProsecutionUser(user) || | ||
| theCase.defendants?.some( | ||
| (defendant) => | ||
| defendant.isDefenderChoiceConfirmed && | ||
| defendant.caseFilesSharedWithDefender && | ||
| defendant.defenderNationalId && | ||
| normalizeAndFormatNationalId(user?.nationalId).includes( | ||
| defendant.defenderNationalId, | ||
| ), | ||
| ) || | ||
| theCase.civilClaimants?.some( | ||
| (civilClaimant) => | ||
| civilClaimant.hasSpokesperson && | ||
| civilClaimant.isSpokespersonConfirmed && | ||
| civilClaimant.caseFilesSharedWithSpokesperson && | ||
| civilClaimant.spokespersonNationalId && | ||
| normalizeAndFormatNationalId(user?.nationalId).includes( | ||
| civilClaimant.spokespersonNationalId, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicated code by importing from utils.ts.
This function is identical to the one in utils.ts. To avoid code duplication and maintain consistency, consider importing it from utils.ts.
+import { shouldDisplayGeneratedPdfFiles as utilsDisplayPdfFiles } from '@island.is/judicial-system/utils'
export class LimitedAccessCaseService {
// ...
- private shouldDisplayGeneratedPdfFiles = (theCase: Case, user?: TUser) =>
- Boolean(
- isProsecutionUser(user) ||
- theCase.defendants?.some(
- (defendant) =>
- defendant.isDefenderChoiceConfirmed &&
- defendant.caseFilesSharedWithDefender &&
- defendant.defenderNationalId &&
- normalizeAndFormatNationalId(user?.nationalId).includes(
- defendant.defenderNationalId,
- ),
- ) ||
- theCase.civilClaimants?.some(
- (civilClaimant) =>
- civilClaimant.hasSpokesperson &&
- civilClaimant.isSpokespersonConfirmed &&
- civilClaimant.caseFilesSharedWithSpokesperson &&
- civilClaimant.spokespersonNationalId &&
- normalizeAndFormatNationalId(user?.nationalId).includes(
- civilClaimant.spokespersonNationalId,
- ),
- ),
- )
+ private shouldDisplayGeneratedPdfFiles = utilsDisplayPdfFilesCommittable suggestion skipped: line range outside the PR's diff.
| private getFileCategories = (theCase: Case) => { | ||
| if (isRequestCase(theCase.type)) { | ||
| return defenderCaseFileCategoriesForRequestCases | ||
| } | ||
|
|
||
| if (theCase.defendants?.[0].caseFilesSharedWithDefender) { | ||
| return defenderCaseFileCategoriesForIndictmentCases | ||
| } | ||
|
|
||
| return defenderDefaultCaseFileCategoriesForIndictmentCases | ||
| } |
There was a problem hiding this comment.
Consider all defendants when determining file categories.
The function only checks defendants[0].caseFilesSharedWithDefender, which might not be accurate when there are multiple defendants with different sharing statuses.
private getFileCategories = (theCase: Case) => {
if (isRequestCase(theCase.type)) {
return defenderCaseFileCategoriesForRequestCases
}
- if (theCase.defendants?.[0].caseFilesSharedWithDefender) {
+ if (theCase.defendants?.some(defendant => defendant.caseFilesSharedWithDefender)) {
return defenderCaseFileCategoriesForIndictmentCases
}
return defenderDefaultCaseFileCategoriesForIndictmentCases
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private getFileCategories = (theCase: Case) => { | |
| if (isRequestCase(theCase.type)) { | |
| return defenderCaseFileCategoriesForRequestCases | |
| } | |
| if (theCase.defendants?.[0].caseFilesSharedWithDefender) { | |
| return defenderCaseFileCategoriesForIndictmentCases | |
| } | |
| return defenderDefaultCaseFileCategoriesForIndictmentCases | |
| } | |
| private getFileCategories = (theCase: Case) => { | |
| if (isRequestCase(theCase.type)) { | |
| return defenderCaseFileCategoriesForRequestCases | |
| } | |
| if (theCase.defendants?.some(defendant => defendant.caseFilesSharedWithDefender)) { | |
| return defenderCaseFileCategoriesForIndictmentCases | |
| } | |
| return defenderDefaultCaseFileCategoriesForIndictmentCases | |
| } |
| .then((content) => filesToZip.push({ data: content, name: file.name })) | ||
| .catch((reason) => | ||
| // Tolerate failure, but log what happened | ||
| this.logger.warn( | ||
| `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, | ||
| { reason }, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| filesToZip.push( | ||
| { | ||
| data: await this.pdfService.getRequestPdf(theCase), | ||
| name: 'krafa.pdf', | ||
| }, | ||
| { | ||
| data: await this.pdfService.getCourtRecordPdf(theCase, user), | ||
| name: 'þingbok.pdf', | ||
| }, | ||
| { | ||
| data: await this.pdfService.getRulingPdf(theCase), | ||
| name: 'urskurður.pdf', | ||
| const caseFilesByCategoryPromises = caseFilesByCategory.map( | ||
| async (file) => { | ||
| return this.awsS3Service | ||
| .getObject(theCase.type, file.key) | ||
| .then((data) => filesToZip.push({ data, name: file.name })) | ||
| .catch((reason) => | ||
| // Tolerate failure, but log what happened | ||
| this.logger.warn( | ||
| `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, | ||
| { reason }, | ||
| ), | ||
| ) | ||
| }, | ||
| ) | ||
|
|
||
| await Promise.all(caseFilesByCategoryPromises) | ||
|
|
||
| if (isRequestCase(theCase.type)) { | ||
| filesToZip.push( | ||
| { | ||
| data: await this.pdfService.getRequestPdf(theCase), | ||
| name: 'Krafa.pdf', | ||
| }, | ||
| { | ||
| data: await this.pdfService.getCourtRecordPdf(theCase, user), | ||
| name: 'Þingbók.pdf', | ||
| }, | ||
| { | ||
| data: await this.pdfService.getRulingPdf(theCase), | ||
| name: 'Úrskurður.pdf', | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| if ( | ||
| isIndictmentCase(theCase.type) && | ||
| this.shouldDisplayGeneratedPdfFiles(theCase, user) | ||
| ) { | ||
| filesToZip.push({ | ||
| data: await this.pdfService.getIndictmentPdf(theCase), | ||
| name: 'Ákæra.pdf', | ||
| }) | ||
|
|
||
| const caseFilesRecordPromises = theCase.policeCaseNumbers.map( | ||
| async (policeCaseNumber) => { | ||
| return this.pdfService | ||
| .getCaseFilesRecordPdf(theCase, policeCaseNumber) | ||
| .then((data) => | ||
| filesToZip.push({ | ||
| data, | ||
| name: `Skjalaskrá-${policeCaseNumber}.pdf`, | ||
| }), | ||
| ) | ||
| }, | ||
| ) | ||
|
|
||
| await Promise.all(caseFilesRecordPromises) | ||
|
|
||
| const subpoenaPromises: Promise<number>[] = [] | ||
|
|
||
| theCase.defendants?.map((defendant) => | ||
| defendant.subpoenas?.map((subpoena) => | ||
| subpoenaPromises.push( | ||
| this.pdfService | ||
| .getSubpoenaPdf(theCase, defendant, subpoena) | ||
| .then((data) => | ||
| filesToZip.push({ | ||
| data, | ||
| name: `Fyrirkall-${defendant.name}.pdf`, | ||
| }), | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
|
|
||
| await Promise.all(subpoenaPromises) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and promise chain management.
The function has several areas where error handling could be enhanced:
- The catch block for S3 files only logs warnings
- No error handling for PDF generation
- Nested promises in subpoena handling could be simplified
async getAllFilesZip(theCase: Case, user: TUser): Promise<Buffer> {
const filesToZip: { data: Buffer; name: string }[] = []
const caseFileCategories = this.getFileCategories(theCase)
const caseFilesByCategory =
theCase.caseFiles?.filter(
(file) =>
file.key &&
file.category &&
caseFileCategories.includes(file.category),
) ?? []
try {
const caseFilesByCategoryPromises = caseFilesByCategory.map(
async (file) => {
return this.awsS3Service
.getObject(theCase.type, file.key)
.then((data) => filesToZip.push({ data, name: file.name }))
.catch((error) => {
- this.logger.warn(
+ this.logger.error(
`Could not get file ${file.id} of case ${file.caseId} from AWS S3`,
- { reason },
+ { error },
)
+ throw error
})
},
)
await Promise.all(caseFilesByCategoryPromises)
if (isRequestCase(theCase.type)) {
const requestFiles = await Promise.all([
this.pdfService.getRequestPdf(theCase).then(data => ({
data,
name: 'Krafa.pdf'
})),
this.pdfService.getCourtRecordPdf(theCase, user).then(data => ({
data,
name: 'Þingbók.pdf'
})),
this.pdfService.getRulingPdf(theCase).then(data => ({
data,
name: 'Úrskurður.pdf'
}))
])
filesToZip.push(...requestFiles)
}
if (
isIndictmentCase(theCase.type) &&
this.shouldDisplayGeneratedPdfFiles(theCase, user)
) {
- filesToZip.push({
- data: await this.pdfService.getIndictmentPdf(theCase),
- name: 'Ákæra.pdf',
- })
+ const [indictmentPdf] = await Promise.all([
+ this.pdfService.getIndictmentPdf(theCase).then(data => ({
+ data,
+ name: 'Ákæra.pdf'
+ }))
+ ])
+ filesToZip.push(indictmentPdf)
const caseFilesRecordPromises = theCase.policeCaseNumbers.map(
async (policeCaseNumber) => {
return this.pdfService
.getCaseFilesRecordPdf(theCase, policeCaseNumber)
.then((data) => ({
data,
name: `Skjalaskrá-${policeCaseNumber}.pdf`,
}))
},
)
const caseFilesRecords = await Promise.all(caseFilesRecordPromises)
filesToZip.push(...caseFilesRecords)
- const subpoenaPromises: Promise<number>[] = []
- theCase.defendants?.map((defendant) =>
- defendant.subpoenas?.map((subpoena) =>
- subpoenaPromises.push(
- this.pdfService
- .getSubpoenaPdf(theCase, defendant, subpoena)
- .then((data) =>
- filesToZip.push({
- data,
- name: `Fyrirkall-${defendant.name}.pdf`,
- }),
- ),
- ),
- ),
- )
+ const subpoenaPromises = theCase.defendants?.flatMap((defendant) =>
+ defendant.subpoenas?.map((subpoena) =>
+ this.pdfService
+ .getSubpoenaPdf(theCase, defendant, subpoena)
+ .then((data) => ({
+ data,
+ name: `Fyrirkall-${defendant.name}.pdf`,
+ }))
+ ) ?? []
+ ) ?? []
- await Promise.all(subpoenaPromises)
+ const subpoenas = await Promise.all(subpoenaPromises)
+ filesToZip.push(...subpoenas)
}
return this.zipFiles(filesToZip)
+ } catch (error) {
+ this.logger.error('Failed to generate ZIP file', { error })
+ throw new InternalServerErrorException('Failed to generate ZIP file')
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const caseFileCategories = this.getFileCategories(theCase) | |
| const caseFilesByCategory = | |
| theCase.caseFiles?.filter( | |
| (file) => | |
| file.key && | |
| file.category && | |
| defenderCaseFileCategoriesForRequestCases.includes(file.category), | |
| caseFileCategories.includes(file.category), | |
| ) ?? [] | |
| // TODO: speed this up by fetching all files in parallel | |
| for (const file of caseFilesByCategory) { | |
| await this.awsS3Service | |
| .getObject(theCase.type, file.key) | |
| .then((content) => filesToZip.push({ data: content, name: file.name })) | |
| .catch((reason) => | |
| // Tolerate failure, but log what happened | |
| this.logger.warn( | |
| `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, | |
| { reason }, | |
| ), | |
| ) | |
| } | |
| filesToZip.push( | |
| { | |
| data: await this.pdfService.getRequestPdf(theCase), | |
| name: 'krafa.pdf', | |
| }, | |
| { | |
| data: await this.pdfService.getCourtRecordPdf(theCase, user), | |
| name: 'þingbok.pdf', | |
| }, | |
| { | |
| data: await this.pdfService.getRulingPdf(theCase), | |
| name: 'urskurður.pdf', | |
| const caseFilesByCategoryPromises = caseFilesByCategory.map( | |
| async (file) => { | |
| return this.awsS3Service | |
| .getObject(theCase.type, file.key) | |
| .then((data) => filesToZip.push({ data, name: file.name })) | |
| .catch((reason) => | |
| // Tolerate failure, but log what happened | |
| this.logger.warn( | |
| `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, | |
| { reason }, | |
| ), | |
| ) | |
| }, | |
| ) | |
| await Promise.all(caseFilesByCategoryPromises) | |
| if (isRequestCase(theCase.type)) { | |
| filesToZip.push( | |
| { | |
| data: await this.pdfService.getRequestPdf(theCase), | |
| name: 'Krafa.pdf', | |
| }, | |
| { | |
| data: await this.pdfService.getCourtRecordPdf(theCase, user), | |
| name: 'Þingbók.pdf', | |
| }, | |
| { | |
| data: await this.pdfService.getRulingPdf(theCase), | |
| name: 'Úrskurður.pdf', | |
| }, | |
| ) | |
| } | |
| if ( | |
| isIndictmentCase(theCase.type) && | |
| this.shouldDisplayGeneratedPdfFiles(theCase, user) | |
| ) { | |
| filesToZip.push({ | |
| data: await this.pdfService.getIndictmentPdf(theCase), | |
| name: 'Ákæra.pdf', | |
| }) | |
| const caseFilesRecordPromises = theCase.policeCaseNumbers.map( | |
| async (policeCaseNumber) => { | |
| return this.pdfService | |
| .getCaseFilesRecordPdf(theCase, policeCaseNumber) | |
| .then((data) => | |
| filesToZip.push({ | |
| data, | |
| name: `Skjalaskrá-${policeCaseNumber}.pdf`, | |
| }), | |
| ) | |
| }, | |
| ) | |
| await Promise.all(caseFilesRecordPromises) | |
| const subpoenaPromises: Promise<number>[] = [] | |
| theCase.defendants?.map((defendant) => | |
| defendant.subpoenas?.map((subpoena) => | |
| subpoenaPromises.push( | |
| this.pdfService | |
| .getSubpoenaPdf(theCase, defendant, subpoena) | |
| .then((data) => | |
| filesToZip.push({ | |
| data, | |
| name: `Fyrirkall-${defendant.name}.pdf`, | |
| }), | |
| ), | |
| ), | |
| ), | |
| ) | |
| await Promise.all(subpoenaPromises) | |
| } | |
| const caseFileCategories = this.getFileCategories(theCase) | |
| const caseFilesByCategory = | |
| theCase.caseFiles?.filter( | |
| (file) => | |
| file.key && | |
| file.category && | |
| caseFileCategories.includes(file.category), | |
| ) ?? [] | |
| try { | |
| const caseFilesByCategoryPromises = caseFilesByCategory.map( | |
| async (file) => { | |
| return this.awsS3Service | |
| .getObject(theCase.type, file.key) | |
| .then((data) => filesToZip.push({ data, name: file.name })) | |
| .catch((error) => { | |
| this.logger.error( | |
| `Could not get file ${file.id} of case ${file.caseId} from AWS S3`, | |
| { error }, | |
| ) | |
| throw error | |
| }) | |
| }, | |
| ) | |
| await Promise.all(caseFilesByCategoryPromises) | |
| if (isRequestCase(theCase.type)) { | |
| const requestFiles = await Promise.all([ | |
| this.pdfService.getRequestPdf(theCase).then(data => ({ | |
| data, | |
| name: 'Krafa.pdf' | |
| })), | |
| this.pdfService.getCourtRecordPdf(theCase, user).then(data => ({ | |
| data, | |
| name: 'Þingbók.pdf' | |
| })), | |
| this.pdfService.getRulingPdf(theCase).then(data => ({ | |
| data, | |
| name: 'Úrskurður.pdf' | |
| })) | |
| ]) | |
| filesToZip.push(...requestFiles) | |
| } | |
| if ( | |
| isIndictmentCase(theCase.type) && | |
| this.shouldDisplayGeneratedPdfFiles(theCase, user) | |
| ) { | |
| const [indictmentPdf] = await Promise.all([ | |
| this.pdfService.getIndictmentPdf(theCase).then(data => ({ | |
| data, | |
| name: 'Ákæra.pdf' | |
| })) | |
| ]) | |
| filesToZip.push(indictmentPdf) | |
| const caseFilesRecordPromises = theCase.policeCaseNumbers.map( | |
| async (policeCaseNumber) => { | |
| return this.pdfService | |
| .getCaseFilesRecordPdf(theCase, policeCaseNumber) | |
| .then((data) => ({ | |
| data, | |
| name: `Skjalaskrá-${policeCaseNumber}.pdf`, | |
| })) | |
| }, | |
| ) | |
| const caseFilesRecords = await Promise.all(caseFilesRecordPromises) | |
| filesToZip.push(...caseFilesRecords) | |
| const subpoenaPromises = theCase.defendants?.flatMap((defendant) => | |
| defendant.subpoenas?.map((subpoena) => | |
| this.pdfService | |
| .getSubpoenaPdf(theCase, defendant, subpoena) | |
| .then((data) => ({ | |
| data, | |
| name: `Fyrirkall-${defendant.name}.pdf`, | |
| })) | |
| ) ?? [] | |
| ) ?? [] | |
| const subpoenas = await Promise.all(subpoenaPromises) | |
| filesToZip.push(...subpoenas) | |
| } | |
| return this.zipFiles(filesToZip) | |
| } catch (error) { | |
| this.logger.error('Failed to generate ZIP file', { error }) | |
| throw new InternalServerErrorException('Failed to generate ZIP file') | |
| } |
Zip all files for defenders in indictment cases
Asana
Asana
What
In R-cases, we offer defenders to download all case files as a ZIP file. This PR enables that for S-cases as well. If the case files are not shared with the defender, only the ruling and court record will be included. If they are, all case files visible to the defender will be included.
This PR also implements a minor change in the names of the files in the ZIP file. They now are capitalized and include Icelandic letters.
Why
So that defender users don't have to open each individual file to print out all the case files.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
ZipButtoncomponent for downloading all case documents.Improvements
User Experience