fix(service-portal): petitions - export list #16850
Conversation
WalkthroughThe changes in this pull request 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: 3
🧹 Outside diff range and nitpick comments (4)
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx (4)
Line range hint
16-31: Unusedpetitionprop inDropdownExportcomponentThe
petitionprop is defined in thePropsinterface but is not utilized within theDropdownExportcomponent. Including unused props can lead to confusion and unnecessarily complicates the code.Consider removing the
petitionprop from thePropsinterface to simplify the component and improve code clarity.
38-38: Direct use ofwindowanddocumentmay affect component reusabilityThe component directly accesses
document.location.originandwindow.open, which can cause issues in environments wherewindowanddocumentare not available, such as server-side rendering (SSR) or during static site generation (SSG).To enhance reusability across different NextJS apps and prevent potential runtime errors, consider:
- Checking if
windowanddocumentare defined before use.- Moving the code that uses
windowanddocumentintouseEffecthooks to ensure they run only on the client side.- Abstracting these calls into utility functions that handle environment checks.
Also applies to: 49-49, 61-61
41-51: Specify TypeScript types for mutation resultsThe
useMutationhooks forexportPdfandexportCsvdo not explicitly specify TypeScript types for the mutation results and variables. Explicit types enhance type safety, improve code readability, and help prevent runtime errors.Consider defining the types for the mutation results and variables:
const [exportPdf, { loading: loadingPdf }] = useMutation<ExportListResponse, ExportListVariables>(ExportList, { ... })Replace
ExportListResponseandExportListVariableswith the appropriate types from your GraphQL schema.Also applies to: 53-63
15-15: Consider using absolute imports for better maintainability and reusabilityImporting modules using relative paths like
'../../queries'can make the codebase harder to maintain and may hinder reusability across different projects or apps.Consider configuring the project to use absolute imports or path aliases. This can be set up in your
tsconfig.json:{ "compilerOptions": { "baseUrl": "src", "paths": { "@your-alias/*": ["*"] } } }Then, you can import using:
import { ExportList } from '@your-alias/queries'This approach improves code readability and makes module paths more manageable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx(3 hunks)libs/service-portal/petitions/src/screens/PetitionsTable/styles.css.ts(1 hunks)libs/service-portal/petitions/src/screens/queries.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/service-portal/petitions/src/screens/PetitionsTable/styles.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/service-portal/petitions/src/screens/queries.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/service-portal/petitions/src/screens/PetitionsTable/styles.css.ts (1)
6-6: LGTM! Well-structured style definition.
The addition of paddingBottom creates symmetric vertical spacing for menu items. The code demonstrates good practices:
- Uses theme tokens for consistent styling
- Properly exports typed styles for reuse
- Follows tree-shakeable pattern with individual exports
libs/service-portal/petitions/src/screens/queries.ts (1)
174-177: Verify the removal of list management mutations
The AI summary indicates that LockList, UnlockList, and UpdateList mutations were removed. Let's verify if these operations are now handled differently or if they're still needed.
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx
Outdated
Show resolved
Hide resolved
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx
Outdated
Show resolved
Hide resolved
…s/island.is into sp_petitions_exportlist
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx (4)
Line range hint
17-30: Consider extracting shared types for better reusability.Since this component is in the
libsdirectory, consider extracting the dropdown item type into a separate types file for reuse across different components.+// types.ts +export interface DropdownItem { + href?: string + onClick?: () => void + title: string + render?: ( + element: ReactElement, + index: number, + className: string, + ) => ReactElement +} -interface Props { +type Props = { petition?: EndorsementList petitionId: string - dropdownItems?: { - href?: string - onClick?: () => void - title: string - render?: ( - element: ReactElement, - index: number, - className: string, - ) => ReactElement - }[] + dropdownItems?: DropdownItem[] }
41-63: Refactor mutation setup to reduce duplication.The PDF and CSV export mutations share similar configuration. Consider creating a custom hook to encapsulate the export logic:
const useExportMutation = (fileType: 'pdf' | 'csv') => { return useMutation(ExportList, { variables: { input: { listId: petitionId, fileType, }, }, onCompleted: (data) => { const url = data.endorsementSystemExportList.url; fileType === 'pdf' ? window.open(url, '_blank') : window.location.href = url; }, onError: (error) => { toast.error(formatMessage(m.exportError)); } }); }; // Usage: const [exportPdf, { loading: loadingPdf }] = useExportMutation('pdf'); const [exportCsv, { loading: loadingCsv }] = useExportMutation('csv');
Line range hint
67-81: Extract copy link functionality to reduce code duplication.The copy link logic is duplicated between desktop and mobile views. Consider extracting it into a reusable handler:
const handleCopyLink = () => { const copied = copyToClipboard(baseUrl + petitionId); if (!copied) { return toast.error(formatMessage(m.copyLinkError.defaultMessage)); } toast.success(formatMessage(m.copyLinkSuccess.defaultMessage)); }; // Usage in both desktop and mobile buttons: onClick={handleCopyLink}Also applies to: 90-107
Line range hint
31-150: Consider adding ARIA attributes for better accessibility.The component could benefit from improved accessibility:
- Add
aria-labelto buttons- Include
role="menu"androle="menuitem"attributes- Add keyboard navigation support
Example:
<Button + aria-label={formatMessage(m.copyLinkToList)} onClick={handleCopyLink} variant="utility" icon="link" > {formatMessage(m.copyLinkToList)} </Button>apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (4)
362-386: Refactor Suggestion: Reduce code duplication in PDF text renderingThe PDF generation code contains repetitive patterns for setting fonts and adding text. Consider creating helper functions to streamline the code and enhance maintainability.
For example, you could define a helper function:
function addText(doc, font, fontSize, text, x, y, options = {}) { doc.font(font).fontSize(fontSize).text(text, x, y, options) }And refactor the code as:
addText(doc, 'Bold', 12, 'Þetta skjal var framkallað sjálfvirkt þann: ', 60, currentYPosition, { align: 'left' })
422-424: Typographical Correction: Update label to reflect correct language genderThe label
'Opinn til: 'should be gender-neutral or match the context appropriately. In Icelandic, consider using'Opin til: 'if referring to a feminine noun.Apply this diff to correct the label:
-.text('Opinn til: ', 60, currentYPosition, { align: 'left' }) +.text('Opin til: ', 60, currentYPosition, { align: 'left' })
Line range hint
691-742: Security Issue: Sanitize 'listId' when generating filenamesThe
listIdis directly used in the filename, which could pose a security risk if it contains special characters. Ensure thatlistIdis sanitized to prevent potential directory traversal or injection attacks.Apply this diff to sanitize
listId:-const filename = `undirskriftalisti-${listId}-${new Date() +const safeListId = listId.replace(/[^a-zA-Z0-9_-]/g, '') +const filename = `undirskriftalisti-${safeListId}-${new Date() .toISOString() .replace(/[:.]/g, '-')}.${fileType}`
Line range hint
777-785: Error Handling Improvement: Provide specific error messagesIn the
createPdfBuffermethod, errors are caught and rethrown with a generic message. Consider providing more specific error messages to aid in debugging.Apply this diff to include the original error message:
-throw new Error( - \`Error generating PDF for endorsement list \${endorsementList.id}\`, -) +throw new Error( + \`Error generating PDF for endorsement list \${endorsementList.id}: \${error.message}\`, +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts(10 hunks)libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/DownloadPdf/index.tsx(0 hunks)libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/downloadCSV.ts(0 hunks)libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/DownloadPdf/index.tsx
- libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/downloadCSV.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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."
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (12)
libs/service-portal/petitions/src/screens/PetitionsTable/ExportPetition/index.tsx (2)
1-16: LGTM! Well-structured imports following TypeScript guidelines.
The imports are properly organized and typed, adhering to the library code guidelines for TypeScript usage.
82-148: LGTM! Well-implemented dropdown menu with proper loading states.
The dropdown menu implementation is clean with:
- Consistent onClick handling
- Clear loading indicators
- Good user feedback
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (10)
32-33: Approved: New imports are correctly added and used
The new imports for kennitala and date-fns/format are appropriately added and utilized in the code.
351-353: Approved: Header image placement and initial Y-coordinate calculation
The header image is correctly positioned, and currentYPosition is accurately calculated to ensure proper spacing for subsequent content.
395-395: Approved: Display of endorsement list title is correct
The endorsement list title is correctly displayed using the appropriate font settings.
410-410: Approved: Display of endorsement list description is correct
The description is rendered properly, maintaining consistency in styling.
441-441: Approved: Correct display of endorsement count
The endorsement count is displayed accurately with proper formatting.
455-455: Approved: Owner's name displayed correctly
The owner's name is rendered properly in the document.
532-532: Approved: Footer image is added correctly
The footer image is correctly placed at the bottom of the page.
Line range hint 760-766: Security Advisory: Ensure CSV fields are properly escaped
When generating CSV files, verify that all fields are properly escaped to prevent CSV injection attacks. Confirm that the csvStringify library securely handles data that may contain malicious content.
To ensure csvStringify is used securely, review its documentation and update to the latest version if necessary.
Line range hint 805-820: Approved: Proper content type set when uploading files to S3
The content type is correctly set based on the file type during the S3 upload.
469-476: Verify: Ensure 'endorsementList.owner' contains a valid national ID
When formatting the owner's national ID using nationalId.format, confirm that endorsementList.owner is a valid national ID to prevent any formatting errors.
To verify that all endorsement lists have valid owner national IDs, run the following script:
Datadog ReportAll test runs ✅ 7 Total Test Services: 0 Failed, 7 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
364-383: Improve code maintainability with constantsThe positioning calculations use magic numbers. Consider extracting these values into named constants for better maintainability.
+ private readonly PDF_MARGINS = { + top: 40, + left: 60, + right: 60, + bottom: 60 + }; + private readonly SPACING = { + paragraph: 15, + line: 5 + }; doc .font('Bold') .fontSize(12) - .text('Þetta skjal var framkallað sjálfvirkt þann: ', 60, currentYPosition) + .text('Þetta skjal var framkallað sjálfvirkt þann: ', + this.PDF_MARGINS.left, + currentYPosition + )
Line range hint
766-803: Consider implementing file management strategyThe S3 upload implementation is good, but consider adding:
- File size limits to prevent excessive resource usage
- A cleanup strategy for exported files (e.g., TTL policy)
- Rate limiting for export requests
private async uploadFileToS3( fileBuffer: Buffer, filename: string, fileType: 'pdf' | 'csv', ): Promise<void> { + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB + if (fileBuffer.length > MAX_FILE_SIZE) { + throw new BadRequestException('File size exceeds maximum limit'); + } try { await this.s3Service.uploadFile( fileBuffer, { bucket: environment.exportsBucketName, key: filename }, { ContentType: fileType === 'pdf' ? 'application/pdf' : 'text/csv', + Metadata: { + 'expiry-date': new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() // 24 hours + } }, ) } catch (error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (1)
32-33: LGTM: Appropriate imports added
The new imports for national ID formatting and date formatting are well-chosen for the enhanced PDF generation functionality.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16850 +/- ##
==========================================
- Coverage 36.45% 36.43% -0.02%
==========================================
Files 6852 6852
Lines 143561 143522 -39
Branches 40973 40977 +4
==========================================
- Hits 52328 52295 -33
+ Misses 91233 91227 -6
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.
|
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…_petitions_exportlist
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (1)
Line range hint
815-835: Consider implementing rate limiting for export operationsThe
fetchEndorsementListmethod has good permission checks, but consider adding rate limiting to prevent potential abuse of the export functionality, especially since it involves resource-intensive operations like PDF generation and S3 uploads.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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 (4)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (4)
33-34: LGTM! Good choice of libraries
The addition of kennitala for national ID formatting and date-fns for date formatting improves code maintainability and standardization.
359-359: Consider environment-independent asset loading
The hard-coded paths to assets (images) might cause issues in different deployment environments. Consider implementing proper error handling for missing assets.
Also applies to: 537-538
478-480: LGTM! Proper formatting of national ID
The use of nationalId.format ensures consistent formatting of national IDs in the PDF output.
372-391: LGTM! Clear and consistent date formatting
The use of format from date-fns ensures consistent date formatting throughout the document. The positioning and styling are well-organized.
* fix(service-portal): petitions - export list * chore: nx format:write update dirty files * tweaks * final tweak * aaand clean * tiny * one more clean up * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Access Control