feat(endorsement-system): Improved error logs #16835
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes in this pull request primarily focus on enhancing error handling and logging within the Changes
Possibly related PRs
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
|
rafnarnason
changed the title
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 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(9 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."
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (1)
603-607: Ensure consistent error handling across methods.In the
emailPDFmethod, when an error occurs, the method logs the error and returns{ success: false }. However, in similar scenarios, other methods likeemailLockthrow anInternalServerErrorException. For consistency and better error propagation, consider throwing an exception inemailPDFwhen email sending fails, aligning it with the error handling strategy used elsewhere.
📜 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(9 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)
6-6: Import statement addition is appropriate.
The inclusion of InternalServerErrorException in the import statements enables proper handling of internal server errors within the service.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 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(9 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 (8)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (8)
6-6: LGTM!
The addition of InternalServerErrorException aligns with the improved error handling strategy.
198-200: Previously reviewed: Environment check issue
603-607: LGTM!
The error logging follows best practices by including relevant context (error message, listId, recipientEmail).
687-689: Previously reviewed: Error message exposure
802-809: LGTM!
The validation error messages are clear and user-friendly.
887-893: LGTM!
Using InternalServerErrorException is appropriate as an invalid buffer is a server-side issue.
900-901: Previously reviewed: Error message exposure
912-914: LGTM!
Using InternalServerErrorException is appropriate as missing S3 bucket configuration is a server-side issue.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
…/island-is/island.is into endorsement-system/s3-bucket-fix
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(9 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 (8)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (8)
6-6: LGTM: Added InternalServerErrorException import
The addition of InternalServerErrorException is appropriate for handling internal server errors.
603-607: LGTM: Improved error logging in emailPDF
The error logging has been enhanced with additional context information.
790-796: LGTM: Improved error handling in emailCreated
The error handling has been enhanced with proper logging and graceful failure handling.
891-897: LGTM: Added PDF buffer validation
The addition of buffer validation before processing is a good security measure.
311-316:
Improve error handling in getOwnerInfo method
Returning an empty string on error could lead to silent failures downstream. Consider throwing an exception to handle the error explicitly.
Apply this diff:
} catch (error) {
this.logger.error('Failed to fetch owner name from NationalRegistry', {
error: error.message,
listId,
})
- return ''
+ throw new InternalServerErrorException('Failed to fetch owner information.')
}Likely invalid or redundant comment.
835-850:
Improve error handling in exportList
The implementation properly logs errors and re-throws validation errors, but still exposes internal error details in the exception message.
Apply this diff:
if (
error instanceof BadRequestException ||
error instanceof NotFoundException
) {
throw error // Re-throw validation errors
}
throw new InternalServerErrorException(
- `Failed to export list: ${error.message}`,
+ 'Failed to export list.'
)Likely invalid or redundant comment.
196-209: 🛠️ Refactor suggestion
Improve error handling in lock method
The current implementation exposes internal error details in the exception message. Additionally, the environment check is hardcoded within the business logic.
Apply this diff to fix these issues:
try {
this.logger.info(`Locking endorsement list: ${endorsementList.id}`)
- if (process.env.NODE_ENV === 'production') {
+ if (environment.production) {
await this.emailLock(endorsementList)
}
return await endorsementList.update({ adminLock: true })
} catch (error) {
this.logger.error('Failed to lock endorsement list', {
error: error.message,
listId: endorsementList.id,
})
throw new InternalServerErrorException(
- `Failed to lock endorsement list: ${error.message}`,
+ 'Failed to lock endorsement list.'
)
}Likely invalid or redundant comment.
916-933:
Improve error handling in uploadFileToS3
The implementation properly validates the S3 bucket name but exposes internal error details in the exception message.
Apply this diff:
if (!environment.exportsBucketName) {
throw new InternalServerErrorException('S3 bucket name is undefined')
}
await this.s3Service.uploadFile(
fileBuffer,
{ bucket: environment.exportsBucketName, key: filename },
{ ContentType: fileType === 'pdf' ? 'application/pdf' : 'text/csv' },
)
} catch (error) {
this.logger.error('Failed to upload file to S3', {
error: error.message,
filename,
bucketName: environment.exportsBucketName,
})
throw new InternalServerErrorException(
- `Failed to upload file to S3: ${error.message}`,
+ 'Failed to upload file to S3.'
)
}Likely invalid or redundant comment.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai resolve |
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)
198-200: Consider using configuration service for environment checksHardcoding environment checks with
process.env.NODE_ENVcouples the business logic with environment configuration. This makes testing and maintenance more difficult.Consider injecting a configuration service:
- if (process.env.NODE_ENV === 'production') { + if (this.configService.isProduction()) {
📜 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 (7)
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (7)
6-6: LGTM: Import of InternalServerErrorException
The addition of InternalServerErrorException is appropriate for the improved error handling implementation.
293-293: LGTM: Improved type safety
The addition of the explicit return type Promise<string> enhances type safety and code clarity.
603-607: LGTM: Well-structured error logging
The error logging includes all relevant context (error message, listId, recipientEmail) and appropriately returns a failure status.
790-793: LGTM: Consistent error handling
The error handling follows the same pattern as emailPDF with appropriate logging and failure status return.
840-845: LGTM: Proper validation error handling
The code correctly preserves and re-throws validation errors (BadRequestException, NotFoundException) while wrapping other errors appropriately.
891-895: LGTM: Added PDF buffer validation
The validation check ensures the PDF generation was successful before proceeding, preventing potential runtime errors.
915-917: LGTM: Added S3 bucket validation
The validation check for S3 bucket name prevents potential runtime errors and provides a clear error message.
✅ Actions performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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."
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts
Show resolved
Hide resolved
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
…/island-is/island.is into endorsement-system/s3-bucket-fix
…/island-is/island.is into endorsement-system/s3-bucket-fix
* better error log * chore: nx format:write update dirty files * better error logs * better error logs * better error logs * chore: nx format:write update dirty files * better error logs * chore: nx format:write update dirty files * better error logs * chore: nx format:write update dirty files * dev bucket fix * cleanup * 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>
adding consistency to include error.message in logs and error responses
fixing autoload envs for local development
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation