Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(j-s): Service certificate pdf #16463

Merged
merged 32 commits into from
Oct 21, 2024
Merged

feat(j-s): Service certificate pdf #16463

merged 32 commits into from
Oct 21, 2024

Conversation

oddsson
Copy link
Member

@oddsson oddsson commented Oct 18, 2024

Service certificate pdf

Asana

What

Implement a service certificate pdf.

Why

This document is needed to be able to tell that a service was done correctly after the case has been archived from RVG

Screenshots / Gifs

Screenshot 2024-10-17 at 13 42 50 Screenshot 2024-10-17 at 13 44 47 Screenshot 2024-10-17 at 13 46 49

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new endpoint to retrieve service certificate PDFs for defendants.
    • Added functionality for generating service certificate PDFs with enhanced formatting options.
    • New button added for downloading service certificates in the user interface.
  • Bug Fixes

    • Corrected terminology in localized strings for better clarity.
  • Enhancements

    • Improved error handling for missing parameters in service certificate requests.
    • Expanded PDF generation capabilities to include service certificates.
    • Enhanced conditional rendering in the UI for managing service certificates.
    • Updated user role access checks for improved security.

@oddsson oddsson requested a review from a team as a code owner October 18, 2024 11:56
Copy link
Contributor

coderabbitai bot commented Oct 18, 2024

Walkthrough

The changes in this pull request introduce new functionality for generating service certificate PDFs within the judicial system application. A new method getServiceCertificatePdf is added to the FileController, SubpoenaController, and LimitedAccessFileController, along with corresponding methods in the PdfService. Additionally, new utility functions and exports related to service certificates are introduced across various modules. The updates enhance PDF generation capabilities and improve localization support, ensuring that the existing functionalities remain intact.

Changes

File Path Change Summary
apps/judicial-system/api/src/app/modules/file/file.controller.ts Added method getServiceCertificatePdf with new route and parameters; updated getSubpoenaPdf.
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.controller.ts Added method getServiceCertificatePdf for limited access.
apps/judicial-system/backend/src/app/formatters/index.ts Added export for createServiceCertificate function.
apps/judicial-system/backend/src/app/formatters/pdfHelpers.ts Added method addMediumCenteredText for formatting PDFs.
apps/judicial-system/backend/src/app/formatters/serviceCertificatePdf.ts Introduced createServiceCertificate function for generating service certificate PDFs.
apps/judicial-system/backend/src/app/messages/index.ts Added export for serviceCertificate from pdfServiceCertificate.
apps/judicial-system/backend/src/app/messages/pdfServiceCertificate.ts Introduced serviceCertificate constant for internationalization support.
apps/judicial-system/backend/src/app/modules/case/pdf.service.ts Added method getServiceCertificatePdf for generating service certificate PDFs.
apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts Added method getServiceCertificatePdf with error handling for missing parameters.
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts Modified subpoenaButtonText description; added serviceCertificateButtonText.
apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx Added conditional rendering for service certificate button based on service status.
apps/judicial-system/web/src/components/PdfButton/PdfButton.tsx Updated pdfType property to include 'serviceCertificate'.
libs/judicial-system/audit-trail/src/lib/auditTrail.service.ts Added GET_SERVICE_CERTIFICATE_PDF to AuditedAction enum.
libs/judicial-system/types/src/lib/defendant.ts Updated isSuccessfulServiceStatus parameter type to accept null.
apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts Added method getServiceCertificatePdf for limited access with error handling.
libs/judicial-system/auth/src/lib/guards/roles.guard.ts Updated logic in canActivate method for role checks.
apps/judicial-system/backend/src/app/modules/case/guards/rolesRules.ts Corrected a typo in comment, no functional changes.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • unakb

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 23.07692% with 90 lines in your changes missing coverage. Please review.

Project coverage is 36.79%. Comparing base (6cc38e6) to head (799179f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ackend/src/app/formatters/serviceCertificatePdf.ts 10.60% 59 Missing ⚠️
...system/api/src/app/modules/file/file.controller.ts 0.00% 7 Missing ⚠️
...c/app/modules/file/limitedAccessFile.controller.ts 0.00% 7 Missing ⚠️
...nd/src/app/modules/subpoena/subpoena.controller.ts 54.54% 5 Missing ⚠️
...system/backend/src/app/modules/case/pdf.service.ts 0.00% 4 Missing ⚠️
...dules/subpoena/limitedAccessSubpoena.controller.ts 70.00% 3 Missing ⚠️
...al-system/backend/src/app/formatters/pdfHelpers.ts 33.33% 2 Missing ⚠️
...ndictmentCaseFilesList/IndictmentCaseFilesList.tsx 0.00% 2 Missing ⚠️
libs/judicial-system/types/src/lib/defendant.ts 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16463      +/-   ##
==========================================
+ Coverage   36.78%   36.79%   +0.01%     
==========================================
  Files        6845     6844       -1     
  Lines      141749   141599     -150     
  Branches    40381    40335      -46     
==========================================
- Hits        52140    52106      -34     
+ Misses      89609    89493     -116     
Flag Coverage Δ
judicial-system-api 18.40% <6.25%> (-0.09%) ⬇️
judicial-system-audit-trail 69.43% <100.00%> (+0.07%) ⬆️
judicial-system-backend 55.03% <26.00%> (-0.16%) ⬇️
judicial-system-formatters 79.19% <50.00%> (-0.09%) ⬇️
judicial-system-message 67.31% <ø> (ø)
judicial-system-message-handler 48.44% <ø> (ø)
judicial-system-scheduler 69.58% <50.00%> (-0.07%) ⬇️
judicial-system-types 46.57% <0.00%> (-0.08%) ⬇️
judicial-system-web 27.92% <25.00%> (-0.01%) ⬇️
nest-core ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...udicial-system/backend/src/app/formatters/index.ts 100.00% <100.00%> (ø)
.../judicial-system/backend/src/app/messages/index.ts 100.00% <100.00%> (ø)
.../backend/src/app/messages/pdfServiceCertificate.ts 100.00% <100.00%> (ø)
.../backend/src/app/modules/case/guards/rolesRules.ts 40.84% <ø> (ø)
...ntCaseFilesList/IndictmentCaseFilesList.strings.ts 100.00% <ø> (ø)
...-system/web/src/components/PdfButton/PdfButton.tsx 28.57% <ø> (ø)
...l-system/audit-trail/src/lib/auditTrail.service.ts 96.87% <100.00%> (+0.03%) ⬆️
...judicial-system/auth/src/lib/guards/roles.guard.ts 12.50% <ø> (ø)
libs/judicial-system/types/src/lib/defendant.ts 90.62% <50.00%> (-2.93%) ⬇️
...al-system/backend/src/app/formatters/pdfHelpers.ts 39.90% <33.33%> (-0.10%) ⬇️
... and 7 more

... and 31 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d49b96...799179f. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Oct 18, 2024

Datadog Report

All test runs b0dbd6b 🔗

10 Total Test Services: 1 Failed, 9 Passed
🔻 Test Sessions change in coverage: 2 decreased, 2 increased, 10 no change

Test Services
This report shows up to 10 services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
judicial-system-backend 1 0 0 21202 0 20m 52.18s 1 decreased (-0.14%) Link
judicial-system-api 0 0 0 57 0 7.18s 1 decreased (-0.05%) Link
judicial-system-audit-trail 0 0 0 7 0 11.12s 1 increased (+0.08%) Link
judicial-system-formatters 0 0 0 38 0 5.99s 1 no change Link
judicial-system-message 0 0 0 33 0 12.79s 1 no change Link
judicial-system-message-handler 0 0 0 4 0 3.36s 1 no change Link
judicial-system-scheduler 0 0 0 4 0 3.94s 1 no change Link
judicial-system-types 0 0 0 23 0 6.81s 1 no change Link
judicial-system-web 0 0 0 338 0 1m 4.58s 1 no change Link
judicial-system-xrd-api 0 0 0 6 0 4.82s 1 increased (+0.02%) Link

❌ Failed Tests (1)

  • LimitedAccessSubpoenaController - guards should have the right guard configuration - apps/judicial-system/backend/src/app/modules/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.spec.ts - Details

    Expand for error
     expect(received).toHaveLength(expected)
     
     Expected length: 7
     Received length: 6
     Received array:  [[Function JwtAuthGuard], [Function CaseExistsGuard], [Function RolesGuard], {"allowedCaseTypes": ["INDICTMENT"]}, [Function CaseReadGuard], [Function DefendantExistsGuard]]
    

🔻 Code Coverage Decreases vs Default Branch (2)

  • judicial-system-backend - jest 58.5% (-0.14%) - Details
  • judicial-system-api - jest 19.66% (-0.05%) - Details

Copy link
Member

@gudjong gudjong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except defenders will see a button to open the service certificate, but will not be able to open it. Do give them access, code needs to be added to limited access controllers in the api and backend.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (10)
apps/judicial-system/backend/src/app/messages/pdfServiceCertificate.ts (1)

3-9: LGTM: serviceCertificate object is well-structured, but consider expanding it.

The serviceCertificate object is correctly defined and exported. The title property uses defineMessage appropriately, with a well-structured id, defaultMessage, and description. This setup supports internationalization effectively.

However, a service certificate typically requires more than just a title. Consider expanding this object to include other necessary fields for a complete service certificate, such as date, recipient, or any other relevant information.

Example of how you might expand the object:

export const serviceCertificate = {
  title: defineMessage({
    id: 'judicial.system.backend:pdf.service_certificate.title',
    defaultMessage: 'Birtingarvottorð',
    description: 'Notaður sem titill á birtingarvottorði.',
  }),
  date: defineMessage({
    id: 'judicial.system.backend:pdf.service_certificate.date',
    defaultMessage: 'Dagsetning',
    description: 'Label for the date on the service certificate.',
  }),
  // Add more fields as necessary
}
apps/judicial-system/backend/src/app/messages/index.ts (1)

11-11: LGTM! Consider grouping related exports.

The addition of the serviceCertificate export is consistent with the existing pattern and supports the new functionality for generating service certificate PDFs. It adheres to NextJS best practices and TypeScript conventions.

For improved readability and maintenance, consider grouping related exports together. For example, you could group all PDF-related exports:

// PDF-related exports
export { core } from './pdfCore'
export { request } from './pdfRequest'
export { ruling } from './pdfRuling'
export { courtRecord } from './pdfCourtRecord'
export { custodyNotice } from './pdfCustodyNotice'
export { caseFilesRecord } from './pdfCaseFilesRecord'
export { indictment } from './pdfIndictment'
export { subpoena } from './pdfSubpoena'
export { serviceCertificate } from './pdfServiceCertificate'

// Other exports
export { notifications } from './notifications'
export { courtUpload } from './courtUpload'

This organization can make it easier to understand the purpose of each export at a glance.

libs/judicial-system/types/src/lib/defendant.ts (1)

45-47: Approved: Type safety improvement

The change to accept null as a valid input improves type safety and aligns with TypeScript best practices. The function remains reusable and doesn't affect tree-shaking or bundling.

For improved clarity, consider using a non-null assertion in the function body:

export const isSuccessfulServiceStatus = (
  status?: ServiceStatus | null,
): boolean => {
  return status != null && successfulServiceStatus.includes(status);
}

This makes the null check explicit and may be easier to understand at a glance.

apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts (2)

48-52: LGTM: New message for service certificate button

The addition of serviceCertificateButtonText aligns with the PR objectives and follows the existing structure and naming conventions. The message is well-formed and includes appropriate placeholders.

Consider adding a {date} placeholder to the defaultMessage, similar to the subpoenaButtonText, if the date is relevant for the service certificate filename:

-    defaultMessage: 'Birtingarvottorð {name}.pdf',
+    defaultMessage: 'Birtingarvottorð {name} {date}.pdf',

This would maintain consistency with the subpoenaButtonText format and potentially provide more information in the filename.


Line range hint 1-52: Overall assessment: Changes enhance localization and add service certificate support

The modifications to this file are well-structured and align with the PR objectives. They improve localization support by correcting terminology and add necessary functionality for service certificates. The changes maintain consistency with existing patterns and adhere to NextJS and React-Intl best practices.

To further improve maintainability, consider grouping related messages (e.g., subpoena-related, service certificate-related) using nested objects within the strings constant. This can help organize the growing number of localized strings as new features are added.

apps/judicial-system/web/src/components/PdfButton/PdfButton.tsx (1)

21-21: LGTM. Consider adding documentation for the new option.

The addition of 'serviceCertificate' to the pdfType options is correct and maintains type safety. It aligns with the PR objective of implementing a service certificate in PDF format.

Consider adding a brief comment explaining the purpose of the 'serviceCertificate' option for better code maintainability.

apps/judicial-system/api/src/app/modules/file/file.controller.ts (1)

180-203: LGTM! Consider adding a query parameter for flexibility.

The new getServiceCertificatePdf method is well-implemented and consistent with the existing code structure. It correctly handles the retrieval of service certificate PDFs and uses appropriate auditing.

For consistency with the getSubpoenaPdf method, consider adding optional query parameters (e.g., for date or type) if they might be needed in the future. This would provide more flexibility without requiring changes to the method signature later.

apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (1)

250-260: LGTM: Service certificate rendering.

The conditional rendering for service certificates is well-implemented and integrates seamlessly with the existing code. It follows React best practices and properly handles localization.

Consider extracting the PdfButton for service certificates into a separate component or function to improve readability and maintainability, especially if this pattern is used elsewhere in the codebase.

apps/judicial-system/backend/src/app/modules/case/pdf.service.ts (1)

360-375: LGTM: Method implementation is correct, but consider additional enhancements.

The getServiceCertificatePdf method is implemented correctly and follows the existing patterns in the class. However, consider the following suggestions for improvement:

  1. Error handling: Add try-catch block to handle potential errors from createServiceCertificate.
  2. Caching: Consider implementing S3 caching similar to other methods in the class to improve performance for repeated requests.
  3. Throttling: Evaluate if this method should use the throttle mechanism like getCaseFilesRecordPdf.

Would you like me to provide an example implementation incorporating these suggestions?

apps/judicial-system/backend/src/app/formatters/pdfHelpers.ts (1)

473-479: LGTM! Consider adding JSDoc for consistency.

The new addMediumCenteredText function is well-implemented and follows the existing patterns in the file. It enhances the PDF generation capabilities by providing a specific utility for centered medium text.

For consistency with other functions in this file, consider adding a JSDoc comment to describe the function's purpose and parameters. Here's a suggested addition:

/**
 * Adds medium-sized centered text to the PDF document.
 * @param doc - The PDF document to add text to.
 * @param text - The text to be added.
 * @param font - Optional font to be used for the text.
 */
export const addMediumCenteredText = (
  doc: PDFKit.PDFDocument,
  text: string,
  font?: string,
) => {
  addAlignedText(doc, mediumFontSize, text, 'center', font)
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 40811a5 and 225844d.

📒 Files selected for processing (13)
  • apps/judicial-system/api/src/app/modules/file/file.controller.ts (1 hunks)
  • apps/judicial-system/backend/src/app/formatters/index.ts (1 hunks)
  • apps/judicial-system/backend/src/app/formatters/pdfHelpers.ts (1 hunks)
  • apps/judicial-system/backend/src/app/formatters/serviceCertificatePdf.ts (1 hunks)
  • apps/judicial-system/backend/src/app/messages/index.ts (1 hunks)
  • apps/judicial-system/backend/src/app/messages/pdfServiceCertificate.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/case/pdf.service.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts (2 hunks)
  • apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts (1 hunks)
  • apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (2 hunks)
  • apps/judicial-system/web/src/components/PdfButton/PdfButton.tsx (1 hunks)
  • libs/judicial-system/audit-trail/src/lib/auditTrail.service.ts (1 hunks)
  • libs/judicial-system/types/src/lib/defendant.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
apps/judicial-system/api/src/app/modules/file/file.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/formatters/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/formatters/pdfHelpers.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/formatters/serviceCertificatePdf.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/messages/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/messages/pdfServiceCertificate.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/pdf.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.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/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.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/IndictmentCaseFilesList/IndictmentCaseFilesList.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/components/PdfButton/PdfButton.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."
libs/judicial-system/audit-trail/src/lib/auditTrail.service.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/judicial-system/types/src/lib/defendant.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 (13)
apps/judicial-system/backend/src/app/messages/pdfServiceCertificate.ts (1)

1-1: LGTM: Import statement is correct and follows best practices.

The import statement correctly imports only the necessary defineMessage function from the @formatjs/intl library, which is good for tree-shaking and overall bundle size optimization.

apps/judicial-system/backend/src/app/formatters/index.ts (1)

39-39: LGTM! Verify usage of the new export.

The addition of the createServiceCertificate export is consistent with the existing code structure and aligns with the PR objectives. It follows NextJS best practices for modular exports and maintains type safety.

Let's verify the usage of this new export in the codebase:

✅ Verification successful

Verified!

The createServiceCertificate export is correctly imported and utilized in pdf.service.ts, ensuring that the new functionality is properly integrated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the newly exported createServiceCertificate function

# Test: Search for imports of createServiceCertificate
rg -A 5 "import.*createServiceCertificate.*from.*formatters"

# Test: Search for direct usage of createServiceCertificate
rg -A 5 "createServiceCertificate\("

Length of output: 628

apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.strings.ts (1)

46-46: LGTM: Typo correction in description

The correction from "firyrkall" to "fyrirkall" in the description improves the accuracy of the documentation. This change enhances the clarity of the code without affecting functionality.

apps/judicial-system/backend/src/app/modules/subpoena/subpoena.controller.ts (1)

111-151: LGTM! Well-structured implementation of the new getServiceCertificatePdf method.

The new method follows the existing pattern in the controller, maintains consistency, and adheres to NextJS best practices. Good job on adding the check for the subpoena parameter, which improves error handling.

Consider enhancing error handling for missing parameters.

While you've added a check for the subpoena parameter, it might be beneficial to add similar checks for other required parameters like theCase and defendant. This would provide more comprehensive error handling.

Here's a suggestion for additional checks:

if (!theCase || !defendant || !subpoena) {
  throw new InternalServerErrorException('Missing required parameters');
}

Add unit tests for the new method.

To ensure the reliability and correctness of the new getServiceCertificatePdf method, consider adding unit tests. This will help verify the method's behavior under different scenarios, including error cases.

Would you like assistance in generating unit test code for this method?

libs/judicial-system/audit-trail/src/lib/auditTrail.service.ts (1)

41-41: LGTM! Consider verifying the enum value.

The addition of GET_SERVICE_CERTIFICATE_PDF to the AuditedAction enum is consistent with the existing pattern and adheres to TypeScript usage. However, there's a slight difference between the enum key (GET_SERVICE_CERTIFICATE_PDF) and its value ('GET_SERVICE_CERTIFICATE'). While this might be intentional, it's worth verifying to ensure it aligns with the intended behavior and to prevent potential confusion.

apps/judicial-system/api/src/app/modules/file/file.controller.ts (1)

Line range hint 205-240: LGTM! Good addition of flexibility.

The update to the getSubpoenaPdf method, adding an optional subpoenaId parameter, is a good improvement. It adds flexibility to the method without breaking existing functionality. The change is consistent with the newly added getServiceCertificatePdf method and maintains the overall structure and best practices of the controller.

apps/judicial-system/web/src/components/IndictmentCaseFilesList/IndictmentCaseFilesList.tsx (2)

14-14: LGTM: New utility function import.

The import of isSuccessfulServiceStatus is correctly placed and follows the established naming convention.


Line range hint 1-286: Overall assessment: Well-implemented feature addition.

The changes to add service certificate functionality are well-implemented and integrate seamlessly with the existing component. The code follows NextJS and React best practices, makes effective use of TypeScript for type safety, and maintains consistency with the existing codebase structure and style.

The new feature aligns with the PR objectives to implement a service certificate in PDF format. The conditional rendering ensures that the service certificate is only displayed when appropriate, based on the subpoena's service status.

No significant issues were identified during the review. The code is ready for merging, pending any minor refactoring suggestions and thorough testing.

apps/judicial-system/backend/src/app/modules/case/pdf.service.ts (2)

29-29: LGTM: Import statement is correct and consistent.

The new import for createServiceCertificate is correctly placed and follows the existing import style. It's also necessary for the new functionality.


Line range hint 1-375: Overall assessment: Changes are well-implemented and align with project objectives.

The new getServiceCertificatePdf method and its associated import enhance the PDF generation capabilities of the PdfService class, aligning with the PR objective of implementing a service certificate in PDF format. The changes adhere to NextJS best practices, maintain consistency with existing code patterns, and make effective use of TypeScript for type safety.

While the implementation is solid, consider the suggestions in the previous comment to further improve error handling, performance, and consistency with other methods in the class.

apps/judicial-system/backend/src/app/formatters/serviceCertificatePdf.ts (3)

115-122: Validate defendant national ID before formatting

When using formatDOB, ensure that defendant.nationalId or defendant.noNationalId is provided to prevent errors.

Identify defendants missing national ID information:

#!/bin/bash
# Description: Ensure all defendants have national ID information.

# Test: Search for defendants without nationalId and noNationalId flag.
rg -A5 'defendant' | rg 'nationalId:\s*(null|undefined)' | rg 'noNationalId:\s*(false|null|undefined)'

96-103: Handle missing defender names gracefully

In cases where subpoena.serviceStatus === ServiceStatus.DEFENDER, you're adding the defender's name if available. Ensure that defendant.defenderName is not undefined to avoid displaying undefined in the PDF output.

Check for defendants with DEFENDER service status but missing defender names:

#!/bin/bash
# Description: Find defendants with DEFENDER service status lacking defender names.

# Test: Locate such instances in the codebase.
rg -A5 'serviceStatus:\s*ServiceStatus.DEFENDER' | rg -B5 'defenderName:\s*(null|undefined)'

76-77: Ensure service dates are valid before formatting

When formatting subpoena.serviceDate, ensure that the date is valid to prevent runtime errors. An invalid or null date could cause issues when using formatDate.

Run the following script to check for any subpoenas with invalid or null service dates:

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (1)

87-119: LGTM! New method implementation looks good.

The new getServiceCertificatePdf method is well-structured and consistent with the existing getSubpoenaPdf method. It correctly handles the new endpoint for service certificates and integrates well with the PdfService.

Consider enhancing error handling and logging.

While the new error handling for missing subpoena is good, consider adding more comprehensive error handling and logging:

  1. Add error handling for the pdfService.getServiceCertificatePdf call.
  2. Include more detailed logging, especially for error cases.

Example improvement:

try {
  const pdf = await this.pdfService.getServiceCertificatePdf(
    theCase,
    defendant,
    subpoena,
  )
  res.end(pdf)
} catch (error) {
  this.logger.error(`Failed to generate service certificate PDF: ${error.message}`, error)
  throw new InternalServerErrorException('Failed to generate service certificate PDF')
}

Add unit test for the new method.

To ensure the reliability of the new functionality, consider adding a unit test for the getServiceCertificatePdf method. This test should cover both successful PDF generation and error handling scenarios.

Would you like me to provide a sample unit test for this method?

apps/judicial-system/api/src/app/modules/file/limitedAccessFile.controller.ts (1)

203-226: LGTM! Consider handling the optional subpoenaId.

The new getServiceCertificatePdf method is well-implemented and consistent with the existing codebase. It follows TypeScript best practices and maintains the established pattern for file retrieval endpoints.

One minor suggestion:

Consider handling the case where subpoenaId is undefined. You might want to adjust the file path construction in the tryGetFile call to account for this:

`limitedAccess/defendant/${defendantId}/subpoena/${subpoenaId ?? 'latest'}/serviceCertificate`

This ensures a valid path even when subpoenaId is not provided, assuming 'latest' is a valid fallback. Adjust the fallback value as appropriate for your use case.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 225844d and 34e6fdb.

📒 Files selected for processing (4)
  • apps/judicial-system/api/src/app/modules/file/limitedAccessFile.controller.ts (2 hunks)
  • apps/judicial-system/backend/src/app/modules/case/guards/rolesRules.ts (1 hunks)
  • apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (2 hunks)
  • libs/judicial-system/auth/src/lib/guards/roles.guard.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • libs/judicial-system/auth/src/lib/guards/roles.guard.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/judicial-system/backend/src/app/modules/case/guards/rolesRules.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/api/src/app/modules/file/limitedAccessFile.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/subpoena/limitedAccessSubpoena.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."

Copy link
Member

@gudjong gudjong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One suggestion.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (1)

89-118: LGTM: New method for service certificate PDF is well-implemented.

The getServiceCertificatePdf method is correctly implemented, following NestJS best practices and maintaining consistency with similar methods in other controllers. It uses appropriate guards and decorators, and the logging is helpful for debugging.

Consider adding error handling to improve robustness:

async getServiceCertificatePdf(
  // ... existing parameters
): Promise<void> {
  try {
    this.logger.debug(
      `Getting service certificate for defendant ${defendantId} in subpoena ${subpoenaId} of case ${caseId} as a pdf document`,
    )

    const pdf = await this.pdfService.getServiceCertificatePdf(
      theCase,
      defendant,
      subpoena,
    )

    res.end(pdf)
  } catch (error) {
    this.logger.error('Error generating service certificate PDF', error)
    res.status(500).json({ message: 'Error generating service certificate PDF' })
  }
}

This change would ensure that any errors during PDF generation are properly logged and communicated to the client.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c99f8c9 and c7824ed.

📒 Files selected for processing (1)
  • apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.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."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/subpoena/limitedAccessSubpoena.controller.ts (2)

35-38: LGTM: Import statements are correctly updated.

The new imports for SubpoenaExistsGuard and SubpoenaExistsOptionalGuard are appropriately added to support the changes in the controller methods.


59-59: LGTM: Guard updated as per previous review suggestion.

The SubpoenaExistsOptionalGuard is now correctly applied to the getSubpoenaPdf method, addressing the previous review comment. This change allows for more flexible handling of subpoena existence.

Copy link
Member

@gudjong gudjong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Some failing tests.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c7824ed and 799179f.

📒 Files selected for processing (1)
  • apps/judicial-system/backend/src/app/modules/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.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/subpoena/test/limitedAccessSubpoenaController/limitedAccessSubpoenaControllerGuards.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-10-17T11:53:19.983Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.

@oddsson oddsson added the automerge Merge this PR as soon as all checks pass label Oct 21, 2024
@kodiakhq kodiakhq bot merged commit e384cdc into main Oct 21, 2024
46 checks passed
@kodiakhq kodiakhq bot deleted the j-s/service-certificate branch October 21, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants