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(ojoi): PDF generation #16698

Merged
merged 12 commits into from
Nov 4, 2024
Merged

feat(ojoi): PDF generation #16698

merged 12 commits into from
Nov 4, 2024

Conversation

jonbjarnio
Copy link
Member

@jonbjarnio jonbjarnio commented Nov 1, 2024

What

Downloading pdf received from external api

Why

So users can download a pdf of their advert.

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

  • New Features

    • Introduced a new GraphQL query GET_PDF_QUERY to fetch PDF documents.
    • Added a getPdf method for retrieving PDFs based on input IDs.
    • Implemented a custom React hook usePdf for managing PDF fetching.
  • Enhancements

    • Improved error handling for PDF downloads with specific error messages.
    • Streamlined PDF fetching process in the Preview component.
    • Enhanced validation logic for email formats in the advert schema.
  • API Changes

    • Updated OpenAPI specifications for PDF response handling and added new schema definitions.
    • Enhanced the ApplicationAdvert schema with additional properties.

@jonbjarnio jonbjarnio requested a review from a team as a code owner November 1, 2024 13:55
Copy link
Contributor

coderabbitai bot commented Nov 1, 2024

Walkthrough

This pull request introduces a new GraphQL query method getPdf in the OfficialJournalOfIcelandApplicationResolver class and a corresponding service method in OfficialJournalOfIcelandApplicationService. A new response type GetPdfResponse is defined, and a custom React hook usePdf is created to facilitate PDF fetching. The Preview component is updated to utilize this hook, enhancing loading and error handling. Additionally, modifications are made to the OpenAPI specification and client service to reflect these changes, ensuring consistency across the application.

Changes

File Change Summary
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts Added method getPdf with @Query decorator, returning GetPdfResponse.
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts Added method getPdf to retrieve PDF based on input ID and user. Added imports for GetPdfResponse and OJOIAIdInput.
libs/api/domains/official-journal-of-iceland-application/src/models/getPdf.response.ts Introduced new file defining GetPdfResponse with pdf field.
libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx Modified Preview component to use usePdf hook, added error handling and PDF fetching logic.
libs/application/templates/official-journal-of-iceland/src/graphql/queries.ts Added new GraphQL query GET_PDF_QUERY to retrieve PDF document.
libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.ts Introduced custom hook usePdf for fetching PDF data using GET_PDF_QUERY.
libs/application/templates/official-journal-of-iceland/src/lib/messages/preview.ts Added new error messages for PDF fetching errors.
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts Added function base64ToBlob for converting base64 string to Blob.
libs/clients/official-journal-of-iceland/application/src/clientConfig.json Updated OpenAPI spec to rename schema references and added new CaseStatus schema.
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts Modified getPdf method to return Promise<GetPdfRespone> instead of Promise<Buffer>.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • albinagu
  • thorkellmani
  • eirikurn

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
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: 10

🧹 Outside diff range and nitpick comments (5)
libs/api/domains/official-journal-of-iceland-application/src/models/getPdf.response.ts (1)

1-7: LGTM! Consider enhancing documentation and type safety.

The implementation is clean and follows GraphQL/NestJS best practices. The explicit naming with 'OJOIA' prefix helps prevent naming collisions.

Consider these enhancements for better documentation and type safety:

 @ObjectType('OJOIAGetPdfResponse')
 export class GetPdfResponse {
-  @Field()
+  @Field(() => String, {
+    description: 'Base64 encoded PDF content for the official journal advertisement'
+  })
   pdf!: string
 }
libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (1)

101-111: Consider adding response validation

The implementation looks good and follows the service's error handling pattern. However, consider adding response validation to ensure the PDF data is present before returning.

Consider adding validation:

 async getPdf(
   params: GetPdfByApplicationIdRequest,
   auth: Auth,
 ): Promise<GetPdfRespone> {
   try {
-    return this.ojoiApplicationApiWithAuth(auth).getPdfByApplicationId(params)
+    const response = await this.ojoiApplicationApiWithAuth(auth).getPdfByApplicationId(params)
+    if (!response?.pdf) {
+      throw new Error('PDF data is missing in the response')
+    }
+    return response
   } catch (error) {
     this.logger.warn('Failed to get pdf', {
       applicationId: params.id,
       error,
       category: LOG_CATEGORY,
     })
     throw error
   }
 }
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (1)

75-80: Add tests for the new getPdf query.

As mentioned in the PR objectives, tests are missing. Please add unit tests to cover:

  • Successful PDF retrieval
  • Error scenarios
  • Authentication/authorization checks

Would you like me to help create a test suite for this new functionality?

libs/application/templates/official-journal-of-iceland/src/graphql/queries.ts (1)

319-326: Consider adding TypeScript type definitions and documentation.

The query implementation looks good and aligns with the existing patterns. However, to improve maintainability and type safety:

  1. Consider adding TypeScript type definitions for the query response:
export interface GetPdfResponse {
  OJOIAGetPdf: {
    pdf: string;
  }
}
  1. Add JSDoc comments to document the query's purpose and expected input/output:
/**
 * Fetches a PDF document for an Official Journal of Iceland application
 * @param input - The application ID input
 * @returns The PDF content as a base64 string
 */
libs/clients/official-journal-of-iceland/application/src/clientConfig.json (1)

1231-1231: Add format validation for base64 encoded PDF content

The content field should specify the format as base64 to ensure proper validation.

Apply this diff to add format validation:

-          "content": { "type": "string", "description": "Base64 encoded PDF" }
+          "content": { 
+            "type": "string", 
+            "format": "base64",
+            "description": "Base64 encoded PDF",
+            "contentMediaType": "application/pdf"
+          }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dc89848 and 07528fa.

📒 Files selected for processing (10)
  • libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (2 hunks)
  • libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (2 hunks)
  • libs/api/domains/official-journal-of-iceland-application/src/models/getPdf.response.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (6 hunks)
  • libs/application/templates/official-journal-of-iceland/src/graphql/queries.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/preview.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (1 hunks)
  • libs/clients/official-journal-of-iceland/application/src/clientConfig.json (7 hunks)
  • libs/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.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/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.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/api/domains/official-journal-of-iceland-application/src/models/getPdf.response.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/application/templates/official-journal-of-iceland/src/fields/Preview.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/application/templates/official-journal-of-iceland/src/graphql/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."
libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.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/application/templates/official-journal-of-iceland/src/lib/messages/preview.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/application/templates/official-journal-of-iceland/src/lib/utils.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/clients/official-journal-of-iceland/application/src/clientConfig.json (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/clients/official-journal-of-iceland/application/src/lib/ojoiApplicationClient.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."
🔇 Additional comments (14)
libs/api/domains/official-journal-of-iceland-application/src/models/getPdf.response.ts (1)

6-6: Verify PDF content handling.

The pdf field is marked as a required string, but we should ensure proper handling of the PDF content format.

Let's check how the PDF content is handled across the codebase:

✅ Verification successful

Let me gather more specific information about how the PDF content is handled in this context.


Let me check one more aspect to see how the PDF is being processed in the service layer.


PDF content handling is properly implemented

The implementation shows that the PDF content is handled appropriately:

  • The service layer maps the response from ojoiApplicationService.getPdf() to the pdf field using data.content
  • The PDF handling is part of a protected GraphQL endpoint with proper user authentication and scope checks
  • The implementation follows the standard pattern used across the codebase for handling document content
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for PDF-related content handling
rg -l "pdf.*base64" || rg -l "application/pdf"

Length of output: 1313


Script:

#!/bin/bash
# Search for references to the GetPdfResponse or related PDF handling in the official-journal application
rg -A 5 "GetPdfResponse|getPdf" libs/api/domains/official-journal-of-iceland-application/

# Check the implementation details around this PDF field
ast-grep --pattern 'interface GetPdfResponse {
  $$$
}'

Length of output: 6587


Script:

#!/bin/bash
# Look for the implementation of the getPdf method in the service
rg -A 10 "async getPdf" libs/api/domains/official-journal-of-iceland-application/

# Check for any PDF-related type definitions or utilities
rg -A 3 "pdf.*string" libs/api/domains/official-journal-of-iceland-application/

Length of output: 2823

libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.ts (3)

1-3: LGTM! Clean and focused imports.

The imports are well-structured and follow good tree-shaking practices by importing specific entities.


1-28: LGTM! Meets libs directory guidelines.

The implementation successfully meets all requirements for libs directory:

  • ✅ Reusable hook that can be used across different NextJS apps
  • ✅ Proper TypeScript usage for all definitions
  • ✅ Effective tree-shaking with specific imports

10-28: 🛠️ Refactor suggestion

Consider enhancing error handling and performance.

While the hook implementation is functional, there are several improvements we could make:

  1. Add error type definition and enhance error handling:
+type PdfError = {
+  message: string
+  code?: string
+}
+
 export const usePdf = ({ applicationId, onComplete }: Props) => {
   const [fetchPdf, { data, loading, error }] = useLazyQuery<{
     OJOIAGetPdf: OjoiaGetPdfResponse
-  }>(GET_PDF_QUERY, {
+  }, {
+    message: PdfError
+  }>(GET_PDF_QUERY, {
     variables: {
       input: {
         id: applicationId,
       },
     },
     onCompleted: onComplete,
+    onError: (error) => {
+      console.error('PDF fetch failed:', error.message)
+    }
   })

+  const formattedError = React.useMemo(() => {
+    if (!error) return null
+    return {
+      message: error.message,
+      code: error.graphQLErrors[0]?.extensions?.code
+    }
+  }, [error])

+  const handleFetchPdf = React.useCallback(() => {
+    if (!applicationId) {
+      console.error('Application ID is required')
+      return
+    }
+    fetchPdf()
+  }, [applicationId, fetchPdf])

   return {
-    fetchPdf,
+    fetchPdf: handleFetchPdf,
     pdf: data?.OJOIAGetPdf?.pdf,
     loading,
-    error,
+    error: formattedError,
   }
 }

These changes will:

  • Add proper error typing
  • Memoize error formatting
  • Add input validation
  • Memoize the fetch function
  • Improve error handling with logging

Let's verify the GraphQL error handling in other similar hooks:

libs/application/templates/official-journal-of-iceland/src/lib/messages/preview.ts (1)

33-42: LGTM! Well-structured error messages.

The new error messages follow the established pattern and provide clear feedback for PDF-related errors, with both a concise notification and a more detailed message including a retry suggestion.

libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.resolver.ts (3)

30-30: LGTM! Import follows best practices.

The import statement correctly uses TypeScript types and follows tree-shaking practices by importing only the required type.


75-80: LGTM! Implementation follows resolver patterns.

The query method is well-structured with proper TypeScript types, authentication, and authorization.


75-80: Verify error handling in the service layer.

Since the resolver delegates to ojoiApplicationService.getPdf(), ensure proper error handling for scenarios like:

  • PDF generation failures
  • Invalid input IDs
  • Authorization errors
libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (2)

16-16: LGTM: Clean import organization and proper hook usage.

The new imports are well-organized and follow React best practices for custom hooks and utilities.

Also applies to: 30-30


109-119: LGTM: Well-implemented download button.

The button implementation properly handles loading states and follows the island-ui design system guidelines.

libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (1)

27-28: LGTM! Clean type imports.

The new type imports follow TypeScript best practices and maintain consistency with the existing codebase.

libs/clients/official-journal-of-iceland/application/src/clientConfig.json (3)

476-480: LGTM: Well-documented typeName field addition

The new typeName field is properly documented with example and description, and correctly added to the required fields list.

Also applies to: 514-514


1092-1120: LGTM: Well-structured CaseStatus schema

The new CaseStatus schema is well-defined with:

  • Clear property definitions
  • Comprehensive enum values for status titles
  • Required field specifications

1201-1204: LGTM: Improved type safety for ApplicationCase status

Good refactoring to use the CaseStatus schema reference instead of an inline object type, improving type safety and reusability.

@datadog-island-is
Copy link

datadog-island-is bot commented Nov 1, 2024

Datadog Report

All test runs 41790af 🔗

66 Total Test Services: 0 Failed, 63 Passed
🔻 Test Sessions change in coverage: 3 decreased, 3 increased, 194 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
air-discount-scheme-backend 0 0 0 82 0 26.95s N/A Link
air-discount-scheme-web 0 0 0 2 0 9.1s 1 no change Link
api 0 0 0 4 0 3s N/A Link
api-domains-air-discount-scheme 0 0 0 6 0 20.42s 1 no change Link
api-domains-assets 0 0 0 3 0 12.67s 1 no change Link
api-domains-auth-admin 0 0 0 18 0 15.05s N/A Link
api-domains-communications 0 0 0 5 0 34.79s 1 no change Link
api-domains-criminal-record 0 0 0 5 0 10.69s 1 no change Link
api-domains-driving-license 0 0 0 23 0 30.65s 1 no change Link
api-domains-education 0 0 0 8 0 20.5s 1 no change Link

🔻 Code Coverage Decreases vs Default Branch (3)

  • services-auth-delegation-api - jest 51.36% (-0.18%) - Details
  • services-user-profile - jest 52.52% (-0.04%) - Details
  • services-university-gateway - jest 45.08% (-0.01%) - Details

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 (2)
libs/application/templates/official-journal-of-iceland/src/lib/messages/preview.ts (1)

33-52: Consider grouping PDF-related messages for better maintainability.

The error messages are well-structured and provide good user feedback. However, as we add more PDF-related functionality, consider grouping these messages in a nested structure.

Here's a suggested restructuring:

export const preview = {
  errors: defineMessages({
    noContent: {
      // ... existing messages
    },
    pdf: {
      error: {
        id: 'ojoi.application:preview.errors.pdf.error',
        defaultMessage: 'Villa kom upp við að sækja skjal',
        description: 'Error message when pdf download fails',
      },
      errorMessage: {
        id: 'ojoi.application:preview.errors.pdf.errorMessage',
        defaultMessage: 'Ekki tókst að sækja skjal, reyndu aftur',
        description: 'Error message when pdf download fails',
      },
      invalid: {
        id: 'ojoi.application:preview.errors.pdf.invalid',
        defaultMessage: 'Ógilt skjal',
        description: 'Error message when pdf is invalid',
      },
      invalidMessage: {
        id: 'ojoi.application:preview.errors.pdf.invalidMessage',
        defaultMessage: 'Skjalið er ógilt eða á vitlausu formi, reyndu aftur',
        description: 'Error message when pdf is invalid',
      },
    },
  }),
}
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (1)

346-353: Consider adding JSDoc documentation for better reusability.

Since this utility is in the libs directory and meant to be reusable, adding proper documentation would improve its usability across different parts of the application.

Add JSDoc documentation:

+/**
+ * Converts a base64 string to a Blob object.
+ * @param base64 - The base64 encoded string to convert
+ * @param mimeType - The MIME type of the resulting Blob (default: 'application/pdf')
+ * @throws {Error} When the input string is empty or invalid
+ * @returns A Blob object containing the decoded data
+ */
 export const base64ToBlob = (base64: string, mimeType = 'application/pdf'): Blob => {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 07528fa and 26bee33.

📒 Files selected for processing (5)
  • libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (2 hunks)
  • libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (5 hunks)
  • libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/preview.ts (1 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libs/application/templates/official-journal-of-iceland/src/hooks/usePdf.ts
🧰 Additional context used
📓 Path-based instructions (4)
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.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/application/templates/official-journal-of-iceland/src/fields/Preview.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/application/templates/official-journal-of-iceland/src/lib/messages/preview.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/application/templates/official-journal-of-iceland/src/lib/utils.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 (4)
libs/api/domains/official-journal-of-iceland-application/src/lib/ojoiApplication.service.ts (1)

27-28: LGTM: Imports follow TypeScript best practices.

The new imports maintain consistency with the existing import patterns and ensure proper type safety.

libs/application/templates/official-journal-of-iceland/src/fields/Preview.tsx (3)

16-16: Well-structured imports following best practices!

The modular approach with separate utilities and hooks promotes reusability across different NextJS apps and enables effective tree-shaking.

Also applies to: 30-31


120-155: Well-structured error handling implementation!

The error handling is comprehensive with:

  • Clear separation of different error states (PDF error vs invalid PDF)
  • User-friendly error messages
  • Proper TypeScript usage for conditional rendering

33-89: Excellent TypeScript implementation!

The component demonstrates strong TypeScript practices:

  • Proper typing of props and hooks
  • Type-safe error handling
  • Well-defined interfaces for external data

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: 2

🧹 Outside diff range and nitpick comments (2)
libs/application/templates/official-journal-of-iceland/src/lib/messages/error.ts (2)

215-219: LGTM with minor suggestions for improvement.

The new error message follows the established pattern and integrates well with existing messages. Consider making the message more specific about email requirements.

   invalidChannel: {
     id: 'ojoi.application:error.invalidChannel',
-    defaultMessage: 'Samskiptaleið þarf að vera með gilt netfang',
+    defaultMessage: 'Samskiptaleið þarf að vera með gilt netfang (t.d. nafn@fyrirtaeki.is)',
     description: 'Error message when channel is invalid',
   },

Line range hint 1-219: Consider reorganizing email-related messages for better maintainability.

Currently, there are multiple email-related error messages scattered throughout the file (email and invalidChannel). Consider grouping them together for better maintainability.

   email: {
     id: 'ojoi.application:error.email',
     defaultMessage: 'Athugaðu hvort netfang sé rétt slegið inn',
     description: 'Error message when email is invalid or not present',
   },
+  invalidChannel: {
+    id: 'ojoi.application:error.invalidChannel',
+    defaultMessage: 'Samskiptaleið þarf að vera með gilt netfang',
+    description: 'Error message when channel is invalid',
+  },
   phone: {
     id: 'ojoi.application:error.phone',
     defaultMessage: 'Athugaðu hvort símanúmer sé rétt slegið inn',
     description: 'Error message when phone is invalid or not present',
   },
-  invalidChannel: {
-    id: 'ojoi.application:error.invalidChannel',
-    defaultMessage: 'Samskiptaleið þarf að vera með gilt netfang',
-    description: 'Error message when channel is invalid',
-  },
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 26bee33 and da775cf.

📒 Files selected for processing (2)
  • libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (3 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/error.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.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/application/templates/official-journal-of-iceland/src/lib/messages/error.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/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (2)

123-134: LGTM! Robust channel validation implementation.

The validation logic for channels is well-implemented:

  • Makes the field optional but ensures at least one valid channel when present
  • Uses the validateChannel function for individual channel validation
  • Provides appropriate error feedback

Line range hint 6-361: Verify the impact of new validation rules.

Let's verify the usage of these new validation rules across the codebase to ensure compatibility.

✅ Verification successful

Let me analyze the results and generate a final response since we have comprehensive information about email validation patterns and channel schema usage across the codebase.

Email validation and channel schema changes are safe to proceed.

  • The new email validation regex in the official journal template matches the pattern used consistently across other templates in the codebase
  • The channel schema and validation is only used within the official journal template (libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts) and doesn't affect other parts of the codebase
  • The email validation function follows the same pattern (optional check with regex test) as used in other templates
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential impacts of the new validation rules

# Test 1: Find all files that might be affected by the new email validation
echo "Files potentially affected by email validation:"
rg -l "email.*validation|validateEmail|isValidEmail" 

# Test 2: Find all usages of the channel schema
echo "\nFiles using channel schema or validation:"
rg -l "channelSchema|validateChannel"

# Test 3: Check for any existing email regex patterns that might conflict
echo "\nExisting email validation patterns:"
rg -B 2 -A 2 'email.*regex|emailRegex'

Length of output: 17091

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 29.41176% with 12 lines in your changes missing coverage. Please review.

Project coverage is 36.55%. Comparing base (252920b) to head (a114802).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...and-application/src/lib/ojoiApplication.service.ts 0.00% 6 Missing ⚠️
...plication/src/lib/ojoiApplicationClient.service.ts 0.00% 4 Missing ⚠️
...nd-application/src/lib/ojoiApplication.resolver.ts 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #16698   +/-   ##
=======================================
  Coverage   36.55%   36.55%           
=======================================
  Files        6882     6883    +1     
  Lines      143427   143432    +5     
  Branches    40882    40880    -2     
=======================================
+ Hits        52431    52435    +4     
- Misses      90996    90997    +1     
Flag Coverage Δ
air-discount-scheme-backend 54.03% <ø> (ø)
air-discount-scheme-web 0.00% <ø> (ø)
api 3.37% <ø> (ø)
api-domains-air-discount-scheme 36.96% <ø> (ø)
api-domains-assets 26.71% <ø> (ø)
api-domains-auth-admin 48.48% <ø> (ø)
api-domains-communications 39.80% <ø> (ø)
api-domains-criminal-record 47.40% <ø> (ø)
api-domains-driving-license 44.44% <ø> (ø)
api-domains-education 31.20% <ø> (ø)
api-domains-health-insurance 34.23% <ø> (ø)
api-domains-mortgage-certificate 34.63% <ø> (ø)
api-domains-payment-schedule 41.19% <ø> (ø)
application-api-files 56.49% <ø> (ø)
application-core 71.93% <ø> (ø)
application-system-api 41.30% <29.41%> (+<0.01%) ⬆️
application-template-api-modules 27.78% <ø> (-0.01%) ⬇️
application-templates-accident-notification 29.21% <ø> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 26.26% <ø> (ø)
application-templates-driving-license 18.40% <ø> (ø)
application-templates-estate 12.18% <ø> (ø)
application-templates-example-payment 25.17% <ø> (ø)
application-templates-financial-aid 15.56% <ø> (ø)
application-templates-general-petition 23.40% <ø> (ø)
application-templates-inheritance-report 6.52% <ø> (ø)
application-templates-marriage-conditions 15.20% <ø> (ø)
application-templates-mortgage-certificate 43.52% <ø> (ø)
application-templates-parental-leave 29.93% <ø> (ø)
application-types 6.62% <ø> (ø)
application-ui-components 1.27% <ø> (ø)
application-ui-shell 20.86% <ø> (ø)
clients-charge-fjs-v2 24.11% <ø> (ø)
clients-driving-license 40.12% <ø> (ø)
clients-driving-license-book 43.38% <ø> (ø)
clients-financial-statements-inao 48.91% <ø> (ø)
clients-license-client 1.83% <ø> (ø)
clients-regulations 42.18% <ø> (ø)
clients-rsk-company-registry 29.76% <ø> (ø)
clients-rsk-personal-tax-return 38.00% <ø> (ø)
clients-smartsolutions 12.77% <ø> (ø)
clients-syslumenn 49.15% <ø> (ø)
cms 0.42% <ø> (ø)
cms-translations 38.92% <ø> (ø)
content-search-toolkit 8.14% <ø> (ø)
contentful-apps 4.69% <ø> (ø)
dokobit-signing 62.40% <ø> (ø)
download-service 44.23% <ø> (ø)
email-service 60.24% <ø> (ø)
feature-flags 90.32% <ø> (ø)
file-storage 52.41% <ø> (ø)
financial-aid-backend 51.24% <ø> (ø)
judicial-system-api 19.57% <ø> (ø)
judicial-system-audit-trail 68.53% <ø> (ø)
judicial-system-backend 55.22% <ø> (ø)
judicial-system-message 66.66% <ø> (ø)
judicial-system-message-handler 47.53% <ø> (ø)
judicial-system-scheduler 70.35% <ø> (ø)
license-api 42.54% <ø> (-0.08%) ⬇️
nest-config 77.74% <ø> (ø)
nest-feature-flags 50.83% <ø> (ø)
nova-sms 61.71% <ø> (ø)
portals-admin-regulations-admin 1.85% <ø> (ø)
portals-core 15.96% <ø> (ø)
services-auth-admin-api 51.87% <ø> (ø)
services-auth-delegation-api 57.52% <ø> (+0.06%) ⬆️
services-auth-ids-api 51.45% <ø> (-0.01%) ⬇️
services-auth-personal-representative 45.08% <ø> (+0.02%) ⬆️
services-auth-personal-representative-public 41.26% <ø> (ø)
services-auth-public-api 48.96% <ø> (ø)
services-endorsements-api 53.58% <ø> (ø)
services-sessions 65.36% <ø> (+0.04%) ⬆️
services-university-gateway 49.31% <ø> (+0.11%) ⬆️
services-user-notification 46.89% <ø> (+<0.01%) ⬆️
services-user-profile 61.82% <ø> (ø)
web 1.80% <ø> (ø)

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

Files with missing lines Coverage Δ
...-iceland-application/src/models/getPdf.response.ts 100.00% <100.00%> (ø)
...nd-application/src/lib/ojoiApplication.resolver.ts 56.25% <50.00%> (-0.33%) ⬇️
...plication/src/lib/ojoiApplicationClient.service.ts 13.11% <0.00%> (+1.52%) ⬆️
...and-application/src/lib/ojoiApplication.service.ts 15.25% <0.00%> (-1.73%) ⬇️

... and 1 file 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 252920b...a114802. Read the comment docs.

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

🧹 Outside diff range and nitpick comments (1)
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (1)

6-9: Specify explicit return types for functions

It's good practice in TypeScript to explicitly specify the return type of functions to enhance type safety and code readability. Consider adding an explicit return type to the isValidEmail function.

-const isValidEmail = (value?: string) =>
+const isValidEmail = (value?: string): boolean =>
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between da775cf and e6c3f85.

📒 Files selected for processing (2)
  • libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (3 hunks)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/error.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libs/application/templates/official-journal-of-iceland/src/lib/messages/error.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.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 (1)
libs/application/templates/official-journal-of-iceland/src/lib/dataSchema.ts (1)

376-380: LGTM!

The validateChannel function correctly validates the channel's email field using the isValidEmail function.

@jonbjarnio jonbjarnio added the automerge Merge this PR as soon as all checks pass label Nov 4, 2024
@kodiakhq kodiakhq bot merged commit 46b1b48 into main Nov 4, 2024
202 checks passed
@kodiakhq kodiakhq bot deleted the feat/ojoi-pdf branch November 4, 2024 21:38
@coderabbitai coderabbitai bot mentioned this pull request Dec 19, 2024
6 tasks
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