chore(application-system): replacing more old s3 implementations with new one #16444
Conversation
|
Affected services are: api,application-system-api,services-endorsements-api, Deployed services: application-system-api,api,endorsement-system-api,application-system-api-worker,consultation-portal,web. |
Datadog ReportAll test runs ✅ 46 Total Test Services: 0 Failed, 44 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
…d-class' of github.com:island-is/island.is into chore/replacing-custom-s3-implementations-with-dedicated-class
…d-class' of github.com:island-is/island.is into chore/replacing-custom-s3-implementations-with-dedicated-class
…d-class' of github.com:island-is/island.is into chore/replacing-custom-s3-implementations-with-dedicated-class
apps/services/endorsements/api/src/app/modules/endorsement/endorsement.service.ts
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…th-dedicated-class
WalkthroughThe changes in this pull request involve a comprehensive refactoring of the codebase, primarily replacing instances of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (16)
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (1)
15-15: LGTM: AwsModule correctly added to imports. Consider sorting imports alphabetically.The addition of
AwsModuleto the imports array is correct and aligns with the PR objective. This change supports the update of S3 implementations in the application system.For improved readability, consider sorting the imports alphabetically:
@Module({ imports: [ - SocialInsuranceAdministrationClientModule, - SharedTemplateAPIModule, ApplicationApiCoreModule, - NationalRegistryClientModule, AwsModule, + NationalRegistryClientModule, + SharedTemplateAPIModule, + SocialInsuranceAdministrationClientModule, ], providers: [SocialInsuranceAdministrationService], exports: [SocialInsuranceAdministrationService], })libs/application/template-api-modules/src/lib/modules/templates/accident-notification/attachments/applicationAttachment.service.ts (1)
65-66: Approved: Improved file content retrieval methodThe update to
getFileContent(url, 'base64')is a good change that makes the encoding explicit and potentially allows for more flexibility in the future. This aligns well with the modernization efforts of the S3 implementation.Consider renaming the
fileContentvariable tofileContentBase64for improved clarity:-const fileContent = - (await this.s3Service.getFileContent(url, 'base64')) ?? '' +const fileContentBase64 = + (await this.s3Service.getFileContent(url, 'base64')) ?? '' -return { key, fileContent, answerKey, fileName: name } +return { key, fileContent: fileContentBase64, answerKey, fileName: name }apps/application-system/api/src/app/modules/application/template-api.service.ts (1)
Line range hint
43-47: Method updated correctly, but there's a typo in the method nameThe
saveAttachmentToApplicatonmethod has been correctly updated to uses3Service.uploadFileinstead ofawsService.uploadFile. This change is consistent with the constructor and import modifications.However, there's a typo in the method name:
Consider fixing the typo in the method name:
-async saveAttachmentToApplicaton( +async saveAttachmentToApplication(libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.ts (1)
34-37: LGTM: S3Service added to test module providers.The addition of
S3Serviceto the test module providers is consistent with the import and prepares the test environment for potential future use.Consider using a mock implementation instead of an empty object for better test coverage:
{ provide: S3Service, useValue: { // Add mock methods as needed // For example: // upload: jest.fn().mockResolvedValue({}), // download: jest.fn().mockResolvedValue({}), }, },libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
76-80: LGTM:getAttachmentsmethod updated to use S3Service.The method has been successfully refactored to use the new
S3Service, simplifying the file retrieval process. This change enhances reusability and maintains consistent error handling.Consider improving the error message to include more details:
catch (error) { this.logger.error('Error fetching attachment', { error, fileName }); throw new Error(`Error occurred while fetching attachment: ${error.message}`); }This would provide more context for debugging while maintaining the existing error handling structure.
libs/nest/aws/src/lib/s3.service.ts (4)
Line range hint
46-80: LGTM with a minor suggestion:uploadFilemethod implementation.The
uploadFilemethod maintains its correct implementation after the class renaming. It effectively uses theUploadclass from '@aws-sdk/lib-storage' for parallel uploads, which is an efficient approach. Error handling and logging are properly implemented.A minor suggestion for improvement:
Consider adding more specific error types in the catch block to handle different scenarios separately. For example:
} catch (error) { if (error instanceof S3ServiceException) { this.logger.error('S3 service error while uploading file', error); } else { this.logger.error('Unexpected error while uploading file to S3', error); } throw error; }This would provide more detailed error logging and potentially allow for more specific error handling in the calling code.
Line range hint
82-101: LGTM with a note:getPresignedUrlmethod implementation.The
getPresignedUrlmethod maintains its correct implementation after the class renaming. It effectively uses thegetSignedUrlfunction from '@aws-sdk/s3-request-presigner' to generate presigned URLs with an optional expiration override.Regarding the TODO comment:
// TODO: Select default length for presigned url's in island.isIt would be beneficial to address this TODO by deciding on a default expiration time for presigned URLs. This could be done by:
- Discussing with the team to determine an appropriate default value.
- Possibly moving this default value to a configuration file for easier management.
- Updating the comment with the reasoning behind the chosen default value once it's decided.
Would you like me to create a GitHub issue to track this TODO?
Line range hint
103-122: LGTM with a suggestion:fileExistsmethod implementation.The
fileExistsmethod maintains its correct implementation after the class renaming. It efficiently uses theHeadObjectCommandto check for file existence in S3.Consider improving the error handling in this method. Currently, it catches all errors and returns
false, which might mask underlying issues. A more robust approach could be:public async fileExists( BucketKeyPairOrFilename: BucketKeyPair | string, ): Promise<boolean> { try { const { bucket, key } = this.getBucketKey(BucketKeyPairOrFilename) const command = new HeadObjectCommand({ Bucket: bucket, Key: key, }) const results = await this.s3Client.send(command) return results.$metadata.httpStatusCode === 200 } catch (error) { if (error instanceof NotFound) { return false } this.logger.error( 'Unexpected error occurred while checking if file exists in S3', error, ) throw error } }This approach would:
- Return
falseonly forNotFounderrors, which genuinely indicate the file doesn't exist.- Log and re-throw unexpected errors, allowing the caller to handle them appropriately.
- Provide more accurate information about the file's existence or any issues encountered.
Line range hint
166-181: LGTM with a suggestion:getFileResponsemethod implementation.The private
getFileResponsemethod maintains its correct implementation after the class renaming. It effectively uses theGetObjectCommandto fetch files from S3.Consider improving the error handling in this method. Currently, it catches all errors, logs them, and returns
undefined, which might mask underlying issues. A more robust approach could be:private async getFileResponse( bucket: string, key: string, ): Promise<GetObjectCommandOutput> { try { return await this.s3Client.send( new GetObjectCommand({ Bucket: bucket, Key: key, }), ) } catch (error) { this.logger.error('Error occurred while fetching file from S3', error) throw error // Re-throw the error after logging } }This approach would:
- Log the error for debugging purposes.
- Re-throw the error, allowing the calling method to handle it appropriately.
- Provide more accurate information about any issues encountered during file retrieval.
libs/nest/aws/src/lib/s3.service.spec.ts (1)
20-20: Consider renaming the service variable for consistency.The variable
awsServiceis of typeS3Service. For better consistency and to avoid confusion, consider renaming this variable tos3Service.- let awsService: S3Service + let s3Service: S3ServiceThis change would align the variable name with the service name, improving code readability.
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (1)
83-87: LGTM: getAttachment method refactored effectively.The
getAttachmentmethod has been successfully refactored to use the newS3Service. This change improves code reusability and abstracts S3 interactions, aligning with the guidelines for thelibsdirectory.Consider enhancing the error handling to provide more specific error messages:
try { const fileContent = await this.s3Service.getFileContent( fileName, 'base64', ) return fileContent || '' } catch (error) { this.logger.error(`Error retrieving attachment ${fileName} from S3`, error) throw new Error(`Failed to retrieve attachment ${fileName} from S3: ${error.message}`) }This change would provide more context in case of errors, aiding in debugging and maintenance.
apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-integration.spec.ts (1)
83-85: LGTM: S3Service initialization and mock setup updated correctlyThe changes correctly update the service initialization and mock setup to use S3Service instead of AwsService. This is consistent with the PR objective and previous modifications.
However, consider adding a type assertion to improve type safety:
s3Service = app.get<S3Service>(S3Service); jest.spyOn(s3Service, 'deleteObject').mockResolvedValue(undefined as any);This ensures that the mock resolves with the correct type, improving type safety in the test.
libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (1)
44-44: LGTM with suggestions: Constructor parameter type updated correctly.The change from
AwsServicetoS3Serviceis consistent with the new import. However, consider the following suggestions:
- Rename the
awsServiceproperty tos3Servicefor clarity.- Review the need for the
s3property instantiation in the constructor, as it might be redundant withS3Service.Would you like me to provide a code snippet for these changes?
libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (1)
Line range hint
272-283: LGTM with a minor suggestion: Improved S3 file content retrieval.The refactoring to use
S3Serviceis well-implemented, simplifying the method and aligning with the new S3 implementation. However, consider the following minor improvement:Instead of returning an empty string when
getFileContentreturns falsy, it might be better to throw an error or return a specific error value. This would make it clearer when a file is not found or empty. Consider updating the code as follows:private async getFileContentBase64(fileName: string): Promise<string> { try { const fileContent = await this.s3Service.getFileContent( fileName, 'base64', ) - return fileContent || '' + if (!fileContent) { + throw new Error(`File ${fileName} not found or empty`) + } + return fileContent } catch (e) { this.logger.warn('[estate]: Failed to get file content - ', e) - return 'err' + throw e } }This change would provide more explicit error handling and avoid silent failures.
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (1)
385-385: LGTM: Updated to use S3Service for file retrieval.The
getPdfmethod has been correctly updated to usethis.s3Service.getFileContentinstead ofthis.awsService.getFileContent, which is consistent with the overall changes in this PR.Consider conducting a performance comparison between the old
AwsServiceand the newS3Serviceimplementations to ensure that this change doesn't negatively impact file retrieval times, especially for larger files.libs/application/api/files/src/lib/file.service.ts (1)
Line range hint
61-66: Correct 'ContentEncoding' usage and ensure correct content uploadThe 'ContentEncoding' header is generally used to indicate any additional content encodings applied to the data, such as 'gzip' or 'compress'. 'base64' is not a standard content encoding and may not be properly handled by S3 or HTTP clients. If the
contentis base64-encoded, it should be decoded before uploading, and the 'ContentEncoding' header can be omitted. Alternatively, ifcontentis already a binary Buffer, you can remove the 'ContentEncoding' header.Apply this diff to fix the issue:
await this.s3Service.uploadFile( - content, + Buffer.from(content, 'base64'), { bucket, key: fileName }, { - ContentEncoding: 'base64', ContentDisposition: 'inline', ContentType: 'application/pdf', }, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (35)
- apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-integration.spec.ts (3 hunks)
- apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-unit.spec.ts (6 hunks)
- apps/application-system/api/src/app/modules/application/template-api.service.ts (3 hunks)
- apps/services/endorsements/api/src/app/modules/endorsement/endorsement.service.ts (0 hunks)
- apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (4 hunks)
- libs/application/api/files/src/lib/file.service.spec.ts (9 hunks)
- libs/application/api/files/src/lib/file.service.ts (9 hunks)
- libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.service.spec.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/attachments/applicationAttachment.service.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/attachments/s3.service.ts (0 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (6 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/estate.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.modules.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.service.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.spec.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (2 hunks)
- libs/nest/aws/src/index.ts (1 hunks)
- libs/nest/aws/src/lib/aws.module.ts (1 hunks)
- libs/nest/aws/src/lib/s3.service.spec.ts (2 hunks)
- libs/nest/aws/src/lib/s3.service.ts (1 hunks)
💤 Files with no reviewable changes (2)
- apps/services/endorsements/api/src/app/modules/endorsement/endorsement.service.ts
- libs/application/template-api-modules/src/lib/modules/templates/accident-notification/attachments/s3.service.ts
🧰 Additional context used
📓 Path-based instructions (33)
apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-integration.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."
apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-unit.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."
apps/application-system/api/src/app/modules/application/template-api.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/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/application/api/files/src/lib/file.service.spec.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/api/files/src/lib/file.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/template-api-modules/src/lib/modules/shared/shared.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/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.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/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.service.spec.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/template-api-modules/src/lib/modules/templates/accident-notification/attachments/applicationAttachment.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/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.module.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/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.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/template-api-modules/src/lib/modules/templates/estate/estate.module.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/template-api-modules/src/lib/modules/templates/estate/estate.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/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.module.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/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.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/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.module.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/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.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/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.modules.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/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.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/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.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/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.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/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.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/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.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/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.spec.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/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.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/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.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/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.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/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.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/nest/aws/src/index.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/nest/aws/src/lib/aws.module.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/nest/aws/src/lib/s3.service.spec.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/nest/aws/src/lib/s3.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."
📓 Learnings (1)
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (1)
Learnt from: HjorturJ PR: island-is/island.is#16281 File: libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts:398-398 Timestamp: 2024-10-09T11:17:59.573Z Learning: In the `ParentalLeaveService`, the `getPdf` method returns a base64 encoded string, and this return type remains unchanged even after updating the implementation to use `awsService.getFileContent`.
🔇 Additional comments (95)
libs/nest/aws/src/index.ts (1)
1-2: LGTM! The changes align with the PR objectives.The replacement of
aws.servicewiths3.servicein the exports aligns well with the PR objective of updating S3 implementations. This change promotes more specific and potentially more efficient use of AWS services in the codebase.This change may improve tree-shaking and bundling by allowing consumers to import only the S3-specific functionality they need. To further optimize, consider splitting other AWS services into separate exports in the future if they're not always used together.
Let's verify the usage of this new export across the codebase:
This script will help us ensure that the changes have been consistently applied across the codebase and identify any areas that might need further updates.
✅ Verification successful
Verified: AwsService imports have been successfully removed and replaced with S3Service imports.
All instances of
AwsServicehave been eliminated, andS3Serviceis correctly imported across the codebase. This confirms that the changes align with the PR objectives and ensure consistent usage of the new S3-specific service.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of AwsService and new imports of S3Service echo "Checking for remaining AwsService imports:" rg --type typescript "import.*AwsService.*from.*['\"]@island.is/nest/aws['\"]" echo "Checking for new S3Service imports:" rg --type typescript "import.*S3Service.*from.*['\"]@island.is/nest/aws['\"]"Length of output: 414
Script:
#!/bin/bash # Description: Check for any remaining imports of AwsService and new imports of S3Service in .ts and .tsx files echo "Checking for remaining AwsService imports:" fd --extension ts --extension tsx --exec rg "import.*AwsService.*from.*['\"]@island\.is/nest/aws['\"]" echo "Checking for new S3Service imports:" fd --extension ts --extension tsx --exec rg "import.*S3Service.*from.*['\"]@island\.is/nest/aws['\"]"Length of output: 1363
libs/nest/aws/src/lib/aws.module.ts (4)
2-2: LGTM: Import statement updated correctlyThe import statement has been updated to use
S3Serviceinstead ofAwsService, which aligns with the refactoring goal. This change supports the reusability of components across different NextJS apps as per our coding guidelines.
8-8: LGTM: Providers array updated correctlyThe providers array has been updated to include
S3Serviceinstead ofAwsService, maintaining consistency with the import changes. This update adheres to TypeScript usage for defining props and supports effective bundling practices as outlined in our coding guidelines.
14-14: LGTM: Exports array updated correctlyThe exports array has been updated to include
S3Serviceinstead ofAwsService, which is consistent with the other changes in the file. This update supports reusability across different NextJS apps and follows TypeScript conventions for exporting types, aligning with our coding guidelines.
Line range hint
1-17: Overall changes look good and align with coding guidelinesThe modifications in this file successfully replace
AwsServicewithS3Servicethroughout theAwsModule. These changes:
- Support reusability across different NextJS apps.
- Maintain proper TypeScript usage for defining props and exporting types.
- Should not negatively impact tree-shaking or bundling practices.
The refactoring is consistent and maintains the module's structure while updating the service implementation. Great job on adhering to our coding guidelines!
libs/application/template-api-modules/src/lib/modules/templates/estate/estate.module.ts (3)
7-7: LGTM: Import statement for AwsModule added correctly.The import statement for
AwsModulefrom@island.is/nest/awsis correctly formatted and aligns with the project's modular structure. This change is consistent with the PR objective of replacing old S3 implementations with new ones.
10-10: LGTM: AwsModule correctly added to module imports.The
AwsModulehas been properly added to theimportsarray in the@Moduledecorator. This change is consistent with the new import statement and aligns with the PR objective of updating S3 implementations. The existing imports are maintained, ensuring that the module's previous functionality is preserved.
Line range hint
1-15: Verify: Adherence to coding guidelines for libs//* files.**The changes adhere to the coding guidelines for files in the
libsdirectory:
- The modular structure supports reusability across different NextJS apps.
- The file uses TypeScript, maintaining type safety.
- The import structure supports effective tree-shaking and bundling.
While the changes don't directly demonstrate all aspects of the guidelines, they maintain the existing adherence to these principles.
Consider the following suggestions for future improvements:
- If
AwsModuleintroduces new types or interfaces, consider exporting them for use in other parts of the application.- Ensure that the
AwsModuleis designed for optimal tree-shaking, allowing unused parts to be excluded from the final bundle.libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.module.ts (3)
5-5: LGTM: Import statement for AwsModule added correctly.The import statement for AwsModule is properly formatted and sourced from an appropriate internal package. This addition aligns with the module's evolving requirements.
Line range hint
1-13: Coding guidelines forlibs/**/*are followed.The module adheres to the coding guidelines for files in the
libsdirectory:
- It's designed for reusability across different NextJS apps.
- TypeScript is used for module definition.
- The module structure allows for effective tree-shaking and bundling.
8-8: LGTM: AwsModule correctly added to imports. Verify AWS service usage.The AwsModule has been properly added to the imports array, maintaining the existing imports. This change aligns with the PR objectives of updating S3 implementations.
To ensure the AwsModule is being utilized correctly, please run the following script to check for AWS service usage in the PSignSubmissionService:
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.module.ts (3)
7-7: LGTM: AWS module import added correctlyThe import statement for
AwsModulefrom the shared package@island.is/nest/awsis correctly implemented. This aligns with the coding guidelines for reusability across different NextJS apps.
15-15: LGTM: AwsModule correctly added to importsThe
AwsModulehas been properly added to the imports array of the@Moduledecorator. This change is consistent with the new import statement and follows the module structure conventions.
Line range hint
1-21: Verify AWS usage and its impact on bundlingThe addition of
AwsModuleto this shared library component is appropriate and maintains reusability. However, it's important to ensure that this addition doesn't negatively impact tree-shaking and bundling for applications that don't use AWS features.Could you please confirm that:
- The AWS functionality is actually needed in this module?
- The AWS features are used in a way that allows effective tree-shaking in consuming applications?
You can use the following script to check for AWS usage in this module:
If there are no matches, consider removing the AwsModule import to keep the module lean.
✅ Verification successful
AWS Usage Verified and Impact Assessed
The
AwsModuleis appropriately imported and utilized within theChildrenResidenceChangeModule, ensuring that AWS functionalities are integrated as needed. It's recommended to verify that AWS integrations are optimized to allow effective tree-shaking, preventing any unnecessary impact on the bundle size for applications that do not leverage AWS features.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for AWS usage in the ChildrenResidenceChangeModule # Test: Search for AWS-related function calls or service usage rg --type typescript 'aws|s3|AwsService' libs/application/template-api-modules/src/lib/modules/templates/children-residence-changeLength of output: 167
Script:
#!/bin/bash # Description: Check for AWS usage in the ChildrenResidenceChangeModule # Test: Search for AWS-related function calls or service usage in TypeScript files rg 'aws|s3|AwsService' --glob '*.ts' libs/application/template-api-modules/src/lib/modules/templates/children-residence-changeLength of output: 1189
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.module.ts (3)
9-9: LGTM: AwsModule import added correctlyThe import statement for AwsModule is correctly formatted and aligns with the PR objective of updating S3 implementations. This change is consistent with the project structure and follows TypeScript conventions.
14-14: LGTM: AwsModule correctly added to importsThe AwsModule has been properly added to the imports array of the @module decorator. This change is consistent with the new import statement and aligns with the PR objective of updating S3 implementations.
9-14: Adherence to coding guidelines confirmedThe changes in this file comply with the coding guidelines for the
libsdirectory:
- They don't affect the reusability of components or hooks across different NextJS apps.
- While not directly related to TypeScript prop definitions or type exports, the changes maintain proper TypeScript usage.
- The addition of AwsModule doesn't appear to negatively impact tree-shaking or bundling practices.
The modifications are minimal and focused on updating the S3 implementation as per the PR objectives.
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.module.ts (3)
9-9: LGTM: Import statement for AwsModule.The import of AwsModule from '@island.is/nest/aws' is correctly added and follows TypeScript conventions. This aligns with the guideline of reusability across different NextJS apps.
Line range hint
1-24: Compliance with coding guidelines forlibs/**/*files.The changes in this file adhere to the coding guidelines:
- Reusability: The use of AwsModule from '@island.is/nest/aws' supports reusability across different NextJS apps.
- TypeScript usage: While the changes don't directly involve prop definitions or type exports, the file maintains TypeScript usage.
- Tree-shaking and bundling: The specific import of AwsModule supports effective tree-shaking.
However, to fully comply with the guidelines, please ensure that any new AWS functionality introduced by this change is implemented in a reusable manner.
14-14: Approved, but clarification needed on AwsModule usage.The addition of AwsModule to the imports array is consistent with the new import statement. However, could you please clarify:
- How does this relate to replacing old S3 implementations as mentioned in the PR objectives?
- What specific AWS functionality is now required in this module?
This information will help ensure that the change aligns with the PR's goals and maintains the module's intended functionality.
To verify the usage of AwsModule in this context, please run the following script:
libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.module.ts (2)
8-8: LGTM: Import statement for AwsModule added correctly.The import of
AwsModulefrom@island.is/nest/awsis properly formatted and aligns with the PR objective of updating S3 implementations.
17-17: LGTM: AwsModule correctly added to imports.The addition of
AwsModuleto the imports array is correct and consistent with the new import statement. This change:
- Enhances reusability by making AWS functionality available within the module.
- Maintains proper TypeScript usage in the module configuration.
- Should have minimal impact on tree-shaking and bundling, as it only imports the necessary module.
These aspects align well with the coding guidelines for
libs/**/*files.libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.modules.ts (3)
9-9: LGTM: AWS module import added correctly.The import of
AwsModulefrom '@island.is/nest/aws' is correctly added and aligns with the PR objective of updating S3 implementations.
14-14: LGTM: AwsModule correctly added to imports.The
AwsModuleis appropriately added to the imports array, making AWS services available to this module. This change aligns with the PR objective of updating S3 implementations.
Line range hint
1-24: Adherence to coding guidelines confirmed.The changes in this file adhere to the coding guidelines for the
libsdirectory:
- The module structure supports reusability across different NextJS apps.
- TypeScript is used appropriately for module definition.
- The import and export statements support effective tree-shaking and bundling practices.
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.module.ts (2)
7-7: LGTM: AwsModule import added correctly.The import of
AwsModulefrom@island.is/nest/awsis properly formatted and aligns with the PR objective of updating S3 implementations. This change supports the reusability of components across different NextJS apps, as per our coding guidelines.
Line range hint
1-20: Adherence to coding guidelines confirmed.The changes in this file adhere to the coding guidelines for
libs/**/*:
- The module structure promotes reusability across different NextJS apps.
- TypeScript is used appropriately for defining the module structure.
- The modular design supports effective tree-shaking and bundling practices at the application level.
libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.module.ts (2)
8-8: LGTM: AwsModule import addedThe addition of the AwsModule import aligns with the PR objective of replacing old S3 implementations with new ones. This change is appropriate and doesn't negatively impact the module's structure or adherence to coding guidelines.
22-22: LGTM: Module decorator updated correctlyThe changes to the module decorator, including the addition of AwsModule to the imports array and the removal of S3-related providers, are in line with the PR objective. These modifications:
- Maintain the module's structure while updating AWS interactions.
- Do not negatively impact the reusability of the module across different NextJS apps.
- Do not affect TypeScript usage for defining props and exporting types.
- Do not seem to impact tree-shaking or bundling practices.
The changes adhere to the coding guidelines for files in the
libs/**/*pattern.libs/application/template-api-modules/src/lib/modules/templates/accident-notification/attachments/applicationAttachment.service.ts (2)
5-5: Excellent update to centralized S3Service importThe change from a local import to
@island.is/nest/awsaligns well with the PR objective of replacing old S3 implementations. This update promotes better reusability across different NextJS apps and indicates a more centralized approach to AWS services, which is a positive architectural decision.
Line range hint
1-70: Overall assessment: Changes align with guidelines and objectivesThe modifications in this file adhere well to the coding guidelines for the
libsdirectory:
- The
ApplicationAttachmentServiceremains reusable across different NextJS apps.- TypeScript is consistently used for defining props and exporting types.
- The changes don't negatively impact tree-shaking or bundling practices.
The updates successfully contribute to the PR objective of replacing old S3 implementations with new ones, while maintaining the overall structure and functionality of the service.
apps/application-system/api/src/app/modules/application/template-api.service.ts (2)
11-11: LGTM: S3Service import added correctlyThe import of
S3Servicefrom '@island.is/nest/aws' is correctly added to replace the previous AwsService. This change aligns with the PR objective of updating S3 implementations.
20-20: LGTM: Constructor updated to use S3ServiceThe constructor parameter has been correctly updated to use
S3Serviceinstead ofAwsService. This change is consistent with the import modification and adheres to TypeScript best practices for dependency injection.libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.ts (2)
Line range hint
1-156: Coding guidelines adherence verified.The changes in this file adhere to the applicable coding guidelines for
libs/**/*files:
- TypeScript is used consistently for imports and type definitions.
- The specific import of
S3Servicesupports effective tree-shaking.Note: Some guidelines (e.g., reusability of components and hooks) are not directly applicable to this test file.
9-9: LGTM: S3Service import added correctly.The import statement for
S3Servicefrom@island.is/nest/awsis correctly added. This aligns with the PR objective of updating S3 implementations.To ensure this import is used correctly throughout the file, run the following script:
✅ Verification successful
Verified: S3Service is correctly imported and provided.
The import statement for
S3Servicefrom@island.is/nest/awsand its provision in the test module are correctly implemented. This aligns with the PR objective of updating S3 implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of S3Service in the file # Test: Check if S3Service is used in the file rg --type typescript 'S3Service' libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.tsLength of output: 211
Script:
#!/bin/bash # Description: Verify the usage of S3Service in the specified TypeScript file # Test: Check if S3Service is used in the file by specifying the file extension directly rg 'S3Service' libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.spec.tsLength of output: 236
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (3)
23-23: LGTM: S3Service import added correctly.The import statement for
S3Serviceis properly added, aligning with the refactoring to use the new S3 implementation.
52-52: LGTM: Constructor updated to use S3Service.The constructor has been correctly updated to inject
S3Service. The parameter is properly marked asprivate readonly, adhering to TypeScript best practices.
Line range hint
1-160: Excellent adherence to library coding guidelines.The changes in this file align well with the coding guidelines for library files:
- Reusability: The switch to
S3Serviceenhances reusability across different NextJS apps.- TypeScript usage: Types and interfaces are properly defined and used throughout the file.
- Bundling and tree-shaking: The changes don't introduce any issues that would negatively impact these practices.
Great job on maintaining code quality while refactoring!
apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-unit.spec.ts (6)
4-4: LGTM: S3Service import added correctlyThe import of S3Service from '@island.is/nest/aws' is correctly added, aligning with the PR objective of replacing old S3 implementations with new ones.
13-13: LGTM: Variable declaration updated to use S3ServiceThe variable declaration is correctly updated from 'awsService' to 's3Service', maintaining consistency with the import change and aligning with the PR objective.
91-91: LGTM: Service initialization updated to use S3ServiceThe service initialization is correctly updated to use S3Service instead of AwsService, maintaining consistency with previous changes and aligning with the PR objective.
100-100: LGTM: Spy creation updated to use s3ServiceThe spy creation is correctly updated to use s3Service instead of awsService, ensuring that the test is spying on the correct service. This change maintains consistency with previous updates.
119-119: LGTM: Second spy creation updated to use s3ServiceThe second spy creation is correctly updated to use s3Service instead of awsService, maintaining consistency with previous changes and ensuring that all tests are spying on the correct service.
Line range hint
1-168: Summary: Successful migration from AwsService to S3ServiceThe changes in this file consistently replace AwsService with S3Service throughout the test suite. This includes updates to imports, variable declarations, service initializations, and spy creations. The overall structure and logic of the tests remain unchanged, ensuring that the functionality is preserved while updating the underlying service.
Key points:
- The changes align with the PR objective of replacing old S3 implementations with new ones.
- The file continues to adhere to NextJS best practices and maintains proper use of TypeScript for type safety.
- All modifications are consistent and do not introduce any apparent issues or inconsistencies.
These changes contribute to the modernization of the application system's S3 implementations as intended by the pull request.
libs/nest/aws/src/lib/s3.service.ts (6)
Line range hint
22-26: Improved class naming and consistent implementation.The renaming of the class from
AwsServicetoS3Serviceis a positive change that better reflects the service's specific purpose of interacting with AWS S3. The constructor remains unchanged and correctly uses dependency injection forS3ClientandLogger. This change enhances code readability and maintains the existing functionality.
Line range hint
28-34: LGTM:getFilemethod implementation.The
getFilemethod maintains its correct implementation after the class renaming. It effectively uses helper methodsgetBucketKeyandgetFileResponse, adhering to the Single Responsibility Principle. The method signature and functionality remain intact, ensuring consistent behavior with the previous implementation.
Line range hint
36-44: LGTM:getFileContentmethod implementation.The
getFileContentmethod maintains its correct implementation after the class renaming. It effectively handles different input types and properly uses the optionalencodingparameter. The method continues to use helper methodsgetBucketKeyandgetFileResponse, ensuring consistent behavior with the previous implementation.
Line range hint
124-147: LGTM:deleteObjectmethod implementation.The
deleteObjectmethod maintains its correct implementation after the class renaming. It effectively uses theDeleteObjectCommandto delete objects from S3. The method properly checks for both 204 and 200 status codes as successful responses, which is a good practice as S3 might return either. Error handling and logging are well-implemented, ensuring that any issues during the deletion process are caught and reported.
Line range hint
149-164: LGTM:getBucketKeymethod implementation.The
getBucketKeymethod maintains its correct implementation after the class renaming. It effectively handles both object (BucketKeyPair) and string (S3 URI) inputs, using theAmazonS3URIfunction to parse string inputs. The method includes proper error handling for invalid S3 URIs, ensuring robust behavior when dealing with different input types.
Line range hint
1-181: Summary: Successful refactoring ofAwsServicetoS3ServiceThe renaming of
AwsServicetoS3Servicehas been successfully implemented throughout the file. This change improves code clarity and better represents the service's specific purpose of interacting with AWS S3. All methods maintain their correct implementation, ensuring that the functionality remains intact after the refactoring.The file adheres to the coding guidelines for the
libsdirectory, providing reusable components for interacting with AWS S3 across different NextJS apps. The use of TypeScript for defining types and interfaces (such asBucketKeyPairandEncodingString) aligns with the guideline of using TypeScript for defining props and exporting types.Some minor suggestions for improvement have been made, primarily focusing on enhancing error handling in certain methods. These suggestions aim to provide more detailed error information and allow for more specific error handling in the calling code.
Overall, this refactoring enhances the codebase by:
- Improving code readability and maintainability.
- Ensuring consistent S3 interactions across the application.
- Providing a clear and focused API for S3 operations.
libs/nest/aws/src/lib/s3.service.spec.ts (3)
12-12: LGTM: Import statement updated correctly.The import statement has been updated to reflect the service rename from
AwsServicetoS3Service. The addition ofEncodingStringimport enhances type safety in the tests. This change adheres to the TypeScript usage guideline for thelibsdirectory.
26-26: LGTM: Test setup correctly updated for S3Service.The changes in the
beforeEachblock are consistent with the service rename:
- The provider has been updated to
S3Service.- The service instance is now retrieved using
S3Service.These changes maintain the integrity of the test setup and ensure that the tests are using the correct service. This update also preserves the reusability of the component across different NextJS apps.
Also applies to: 40-40
Line range hint
1-186: Overall assessment: Changes are consistent and maintain test integrity.The renaming from
AwsServicetoS3Servicehas been consistently applied throughout the file. The test cases themselves remain unchanged, which is appropriate as the underlying functionality being tested hasn't changed. This ensures that the S3Service is still thoroughly tested for all its operations.The changes adhere to the coding guidelines for the
libsdirectory:
- They maintain the reusability of the component across different NextJS apps.
- TypeScript is used effectively for defining types.
- The structure supports effective tree-shaking and bundling practices.
One minor suggestion would be to rename the
awsServicevariable tos3Servicefor complete consistency, but this doesn't affect the functionality or validity of the tests.libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (3)
20-20: LGTM: Import statement for S3Service is correct and follows best practices.The import of
S3Servicefrom@island.is/nest/awsis well-placed and uses named import, which is good for tree-shaking. This aligns with the reusability guideline for thelibsdirectory.
55-55: LGTM: Constructor updated correctly with S3Service.The constructor parameter has been properly updated to use the new
S3Service. The use ofprivate readonlyis a good TypeScript practice for dependency injection, and the naming is consistent.
Line range hint
20-87: Overall changes align well with coding guidelines for libs directory.The modifications in this file adhere to the coding guidelines for the
libsdirectory:
- Reusability: The introduction of
S3Servicefrom a shared package enhances reusability across different NextJS apps.- TypeScript usage: Proper type annotations are maintained throughout the changes.
- Tree-shaking and bundling: The use of named imports and the abstraction of S3 operations into a separate service can potentially improve tree-shaking and bundling.
These changes contribute to better maintainability and consistency across the codebase.
apps/application-system/api/src/app/modules/application/lifecycle/test/application-lifecycle-integration.spec.ts (3)
10-10: LGTM: S3Service import added correctlyThe import statement for S3Service is correctly added to replace the removed AwsService. This change aligns with the PR objective of updating S3 implementations.
21-21: LGTM: Variable declaration updated to use S3ServiceThe variable declaration is correctly updated from
awsService: AwsServicetos3Service: S3Service. This change is consistent with the new import statement and the PR objective.
Line range hint
1-185: Summary: Successfully updated S3 implementation in test fileThe changes in this file successfully replace the usage of
AwsServicewithS3Service. The modifications include:
- Updating the import statement
- Changing the variable declaration
- Adjusting the service initialization and mock setup
These changes align well with the PR objective of updating S3 implementations. The file structure and testing approach remain consistent with NextJS best practices, and TypeScript is used effectively for type safety.
The existing tests should now work correctly with the new S3Service implementation, ensuring that the application lifecycle management functionality remains intact.
libs/application/template-api-modules/src/lib/modules/templates/p-sign-submission/p-sign-submission.service.ts (3)
22-22: LGTM: Import changes align with PR objectivesThe replacement of direct AWS SDK imports with the
S3Servicefrom@island.is/nest/awsaligns well with the PR objectives. This change promotes better encapsulation of AWS services and enhances reusability across different NextJS apps, adhering to the coding guidelines for thelibsdirectory.
64-64: LGTM: Constructor updated with S3Service dependencyThe addition of
S3Serviceas a constructor parameter adheres to dependency injection principles and aligns with the PR objective of replacing old S3 implementations. The use of TypeScript for type definition complies with the coding guidelines for thelibsdirectory.
204-205: LGTM: Simplified file retrieval using S3ServiceThe replacement of direct S3 client usage with
s3Service.getFileContentsimplifies the file retrieval process and aligns with the PR objective. This change enhances maintainability and promotes reusability across different NextJS apps. The error handling is preserved by returning an empty string if the file is not found.libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (1)
28-28: LGTM: Import statement added correctly.The import of
S3Servicefrom@island.is/nest/awsis correctly added and aligns with the constructor parameter change. This import from a shared library promotes reusability across different NextJS apps.libs/application/template-api-modules/src/lib/modules/templates/estate/estate.service.ts (3)
35-35: LGTM: S3Service import added correctly.The import of
S3Servicefrom@island.is/nest/awsis correctly placed and aligns with the refactoring to use the new S3 implementation.
44-44: LGTM: S3Service correctly injected in the constructor.The S3Service is properly injected as a private readonly property in the constructor. This change supports the refactoring to use the new S3 implementation and follows the existing dependency injection pattern.
Line range hint
1-283: Compliance with coding guidelines confirmed.The changes in this file adhere to the coding guidelines for
libs/**/*:
- The S3Service is likely a reusable component across different NextJS apps.
- TypeScript is used effectively, with proper typing for method parameters and return values.
- The specific import of S3Service supports effective tree-shaking and bundling practices.
These changes maintain the quality and consistency of the codebase while improving its structure.
libs/application/template-api-modules/src/lib/modules/templates/accident-notification/accident-notification.service.spec.ts (3)
12-12: LGTM: Improved import for better reusabilityThe change to import
S3Servicefrom@island.is/nest/awsinstead of a local path enhances the reusability of this component across different NextJS apps. This aligns well with our coding guidelines for modularity and maintainability.
253-253: LGTM: Consistent update in test implementationThe change from
getFilecontentAsBase64togetFileContentin the s3Service spy is consistent with the earlier modification in the S3Service mock. The test logic remains intact, ensuring that the functionality is still properly tested.
135-135: Verify consistency with S3Service implementationThe change from
getFilecontentAsBase64togetFileContentin the S3Service mock is noted. This update likely reflects changes in the actual S3Service interface.Please confirm that this change is consistent with the current S3Service implementation. Run the following script to verify:
libs/application/api/files/src/lib/file.service.spec.ts (8)
8-8: LGTM: Import statement updated correctly.The import statement has been properly updated to include
S3Servicefrom@island.is/nest/aws, which aligns with the PR objective of replacing old S3 implementations.
20-20: LGTM: Variable declaration updated correctly.The variable declaration has been properly updated from
awsService: AwsServicetos3Service: S3Service, which is consistent with the import statement change and reflects the transition to the new S3 implementation.
131-131: LGTM: Service initialization updated correctly.The service initialization has been properly updated to use
S3Serviceinstead ofAwsService, which is consistent with the previous changes and correctly initializes the S3Service from the test module.
134-144: LGTM: Mock implementations updated correctly.The mock implementations have been properly updated to use
s3Serviceinstead ofawsService. All relevant methods (getFileContent, fileExists, uploadFile, getPresignedUrl) have been correctly modified while preserving the original test logic.
Line range hint
176-186: LGTM: Test expectations updated correctly.The test expectations have been properly updated to use
s3Serviceinstead ofawsService. The expectations foruploadFileandgetPresignedUrlmethods have been correctly modified while maintaining the original expected parameters and behavior.
201-204: LGTM: Test expectation updated correctly.The test expectation for
getFileContenthas been properly updated to uses3Serviceinstead ofawsService. The expected parameters and behavior remain unchanged, maintaining the integrity of the test case.
Line range hint
227-241: LGTM: Mock implementation and expectation updated correctly.The mock implementation and expectation for
getFileContenthave been properly updated to uses3Serviceinstead ofawsService. The changes are consistent with the service transition, and the expected parameters and behavior remain unchanged, preserving the test case logic.
Line range hint
1-341: Summary: Successful transition from AwsService to S3ServiceThe changes in this file consistently replace
AwsServicewithS3Servicethroughout the test suite. All necessary updates have been made, including imports, variable declarations, service initialization, mock implementations, and test expectations. The modifications align with the PR objective of replacing old S3 implementations while maintaining the integrity and logic of the existing test cases.Key points:
- Import statement updated to include S3Service
- Variable declarations changed from awsService to s3Service
- Service initialization updated to use S3Service
- Mock implementations and expectations consistently updated across all test cases
The changes have been implemented correctly and thoroughly, ensuring that the test suite remains functional and relevant after the transition to the new S3 implementation.
apps/services/endorsements/api/src/app/modules/endorsementList/endorsementList.service.ts (5)
29-29: LGTM: S3Service import added correctlyThe addition of the S3Service import from '@island.is/nest/aws' is correct and aligns with the PR objective of updating S3 implementations.
49-49: LGTM: Constructor updated to use S3ServiceThe constructor has been correctly updated to use S3Service instead of AwsService, maintaining the dependency injection pattern and aligning with the PR objective.
813-816: LGTM: S3Service used for generating presigned URLThe exportList method has been correctly updated to use S3Service for generating the presigned URL, consistent with the PR objective of updating S3 implementations.
Line range hint
879-886: LGTM: S3Service used for file uploadThe uploadFileToS3 method has been correctly updated to use S3Service for uploading files to S3, consistent with the PR objective of updating S3 implementations.
Line range hint
1-892: Overall changes align well with PR objectiveThe modifications in this file consistently replace AwsService with S3Service, aligning perfectly with the PR objective of updating S3 implementations. The changes are minimal, focused, and systematically applied throughout the file, which reduces the risk of introducing unintended side effects.
libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts (3)
2-2: LGTM: Import changes align with PR objectives.The replacement of
AwsServicewithS3Serviceis in line with the PR's goal of updating old S3 implementations. This change should lead to more specific and potentially optimized S3 operations.
2-2: Overall changes align well with PR objectives.The modifications in this file consistently replace
AwsServicewithS3Service, which aligns perfectly with the PR's goal of updating old S3 implementations. The changes are focused and consistent, demonstrating good refactoring practices.To ensure all
AwsServicereferences have been replaced, run the following script:#!/bin/bash # Description: Verify complete replacement of AwsService with S3Service # Test 1: Check for any remaining AwsService imports or usages rg --type typescript 'AwsService|awsService' libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.ts # Test 2: Confirm S3Service is properly imported and used rg --type typescript 'S3Service|s3Service' libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.tsAlso applies to: 114-114, 385-385
114-114: LGTM: Constructor updated to use S3Service.The constructor has been correctly updated to inject
S3Serviceinstead ofAwsService, which is consistent with the import changes.To ensure all
awsServiceusages have been updated, run the following script:✅ Verification successful
Action Required: Re-run verification with corrected search parameters.
Please execute the following script to check for any remaining references to
awsServicein TypeScript files:
Verified: No remaining references to
awsServicefound in the specified file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to awsService in the file # Test: Search for awsService usage. Expect: No results. rg --type typescript 'this\.awsService' libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.tsLength of output: 371
Script:
#!/bin/bash # Description: Check for any remaining references to awsService in the specified TypeScript file # Test: Search for awsService usage. Expect: No results. rg 'this\.awsService' libs/application/template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.tsLength of output: 128
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-individual-election/financial-statement-individual-election.service.ts (1)
15-15: Proper injection ofS3ServiceThe
S3Serviceis correctly imported and injected via the constructor, aligning with dependency injection best practices.Also applies to: 37-37
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (3)
29-29: ImportingS3ServiceCorrectlyThe
S3Serviceis properly imported from@island.is/nest/aws, aligning with the update to use the new S3 implementation.
45-45: InjectingS3Serviceinto the ConstructorThe
s3Serviceis correctly added as a constructor parameter and set as a private readonly member, ensuring dependency injection is properly handled.
55-58: Usings3Service.getFileContentAppropriatelyThe
getFileContentmethod is used correctly to retrieve the file content from S3 using the specified bucket and key.libs/application/api/files/src/lib/file.service.ts (2)
88-90: Verify the encoding when creating a Buffer from 'file'Using
Buffer.from(file, 'binary')may not correctly handle the file data iffileis not in the expected encoding. The 'binary' encoding is an alias for 'latin1' and may not be appropriate here. Iffileis a binary string or base64-encoded data, ensure that you're using the correct encoding to create the Buffer.Consider verifying the encoding of
fileand adjust theBuffer.fromcall accordingly. For example, iffileis base64-encoded:- await this.s3Service.uploadFile(Buffer.from(file, 'binary'), { + await this.s3Service.uploadFile(Buffer.from(file, 'base64'), { bucket, key: s3FileName, })
17-17: Successful replacement of 'AwsService' with 'S3Service'The import and injection of
S3Servicecorrectly replace the previousAwsService. This ensures that all S3 operations are now consolidated throughS3Service, promoting consistency and maintainability.Also applies to: 48-48
libs/application/template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.service.ts (2)
37-37: ImportingS3Servicealigns with refactoring goalsThe addition of
S3Serviceimport follows the PR objective to replace old S3 implementations with the new one.
47-47: InjectingS3Servicevia constructor is appropriateAdding
s3Serviceto the constructor ensures dependency injection is properly utilized.libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (2)
41-41: Import statement is appropriateThe
S3Serviceis correctly imported from@island.is/nest/aws.
52-52: Dependency injection ofS3Serviceis correctly implemented
S3Serviceis properly injected via the constructor, adhering to NestJS dependency injection practices.
libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts
Outdated
Show resolved
Hide resolved
...template-api-modules/src/lib/modules/templates/parental-leave/parental-leave.service.spec.ts
Show resolved
Hide resolved
...s/financial-statement-individual-election/financial-statement-individual-election.service.ts
Show resolved
Hide resolved
...les/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
Show resolved
Hide resolved
...les/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
...les/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
...template-api-modules/src/lib/modules/templates/operating-license/operatingLicense.service.ts
Show resolved
Hide resolved
...modules/templates/social-insurance-administration/social-insurance-administration.service.ts
Show resolved
Hide resolved
…d-class' of github.com:island-is/island.is into chore/replacing-custom-s3-implementations-with-dedicated-class
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/nest/aws/src/lib/s3.service.spec.ts (1)
164-165: LGTM: New getBucketKey method tested.The addition of tests for the
getBucketKeymethod is a good practice, covering both bucket-key pair and file URI inputs. This demonstrates the method's flexibility and ensures its functionality is verified.Consider adding a brief comment explaining the purpose of the
getBucketKeymethod and its different input types, to enhance code readability and documentation.libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (1)
44-44: LGTM: S3Service injected correctly. Consider removing s3 property.The injection of S3Service as a private readonly property is correct. However, the s3 property initialized with
new S3()in the constructor might be unnecessary now. Consider removing it if it's no longer used.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (3 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (6 hunks)
- libs/nest/aws/src/lib/s3.service.spec.ts (12 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/template-api-modules/src/lib/modules/shared/shared.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/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.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/nest/aws/src/lib/s3.service.spec.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."
📓 Learnings (1)
libs/nest/aws/src/lib/s3.service.spec.ts (1)
Learnt from: HjorturJ PR: island-is/island.is#16281 File: libs/nest/aws/src/lib/aws.service.spec.ts:165-172 Timestamp: 2024-10-09T11:20:29.966Z Learning: In our test classes, we typically mock the `awsService` functions instead of mocking the actual S3 client directly.
🔇 Additional comments (11)
libs/nest/aws/src/lib/s3.service.spec.ts (6)
12-12: LGTM: Import statement updated correctly.The import statement has been updated to use
S3Serviceinstead ofAwsService, which aligns with the PR objective of replacing old S3 implementations. The inclusion ofEncodingStringsuggests proper typing is maintained.
19-19: LGTM: Test suite description updated.The describe block has been correctly updated to 'S3Service', maintaining consistency with the renamed service. This change ensures that the test suite accurately describes the component being tested.
26-26: LGTM: Service provider updated in test module.The provider has been correctly updated to use
S3Service, ensuring that the test module is configured to use the new implementation. This change is crucial for proper dependency injection and testing of the refactored service.
40-40: LGTM: Service instantiation updated.The service instantiation has been correctly updated to use
S3Service. This change ensures that the test suite is working with the new implementation, maintaining consistency with the provider configuration.
Line range hint
1-185: Overall, excellent refactoring and adherence to guidelines.The changes in this file successfully transition from
AwsServicetoS3Service, aligning perfectly with the PR objective of replacing old S3 implementations. The test suite maintains comprehensive coverage of S3 operations, promoting reusability across different NextJS apps. The use of TypeScript for type definitions (e.g.,EncodingString) adheres to the coding guidelines for thelibs/**/*pattern.The refactoring is consistent throughout the file, and the test structure remains robust, ensuring that the new
S3Serviceimplementation is thoroughly verified. This change contributes to the goal of simplifying development processes and preparing for the upcoming deprecation of AWS SDK v2.
51-51: LGTM: Method calls updated consistently.All occurrences of
awsServicehave been correctly updated tos3Service. The method names and parameters remain unchanged, indicating that the interface of the service has been preserved. This consistency ensures that the refactoring maintains the existing functionality while transitioning to the new implementation.To ensure that the S3Service interface matches the old AwsService interface, run the following script:
Also applies to: 63-63, 77-79, 93-95, 105-105, 116-116, 125-125, 136-136, 148-148
libs/application/template-api-modules/src/lib/modules/shared/shared.service.ts (2)
28-28: LGTM: S3Service import added correctly.The import of S3Service from '@island.is/nest/aws' aligns with the PR objective of replacing old S3 implementations with a new shared service class.
270-273: LGTM: S3Service used correctly for file upload.The
saveAttachmentToApplicatonmethod now correctly usesthis.s3Service.uploadFile, aligning with the PR objective of replacing old S3 implementations. The overall method logic remains intact, ensuring backwards compatibility.libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (3)
29-29: Integration ofS3Serviceis implemented correctlyThe
S3Servicehas been properly imported and injected into theChildrenResidenceChangeService, replacing the previous AWS S3 client instantiation.Also applies to: 45-45
55-58: Correct usage ofgetFileContentmethodThe
getFileContentmethod is appropriately called with the correct bucket and key parameters, aligning with the newS3Serviceusage.
60-62: Effective error handling for missing file contentThe code correctly checks for undefined
fileContentRawand throws a descriptive error, which will aid in debugging if the file is not found.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (1)
380-390: LGTM: getPdf method updated to use S3ServiceThe getPdf method has been successfully updated to use the injected S3Service for file retrieval. The return type is now correctly specified as Promise, addressing the past review comment. The implementation is simplified and maintains error handling for undefined file content.
Consider enhancing the error message to include the key of the file that couldn't be retrieved:
if (!fileContent) { throw new Error(`File content was undefined for key: ${key}`) }This would provide more context for debugging if an error occurs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/application/api/files/src/lib/file.service.ts (9 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (6 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/api/files/src/lib/file.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/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.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/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.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."
🪛 Biome
libs/application/api/files/src/lib/file.service.ts
[error] 263-263: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (14)
libs/application/template-api-modules/src/lib/modules/templates/social-insurance-administration/social-insurance-administration.service.ts (2)
41-41: LGTM: S3Service import addedThe import of S3Service from '@island.is/nest/aws' is correctly added, aligning with the PR objective of replacing old S3 implementations with a new shared service class.
52-52: LGTM: S3Service injected in constructorThe S3Service is correctly injected as a private readonly property in the constructor. This change is consistent with the PR objective and follows the existing dependency injection pattern in the class.
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (5)
29-29: Correctly ImportingS3ServiceThe import statement for
S3Servicefrom@island.is/nest/awsis appropriate and aligns with the move to replace old S3 implementations with the new shared service.
45-45: Proper Dependency Injection ofS3ServiceInjecting
S3Serviceinto the constructor ensures that the service is available for use within the class, adhering to dependency injection best practices.
55-58: UtilizingS3Serviceto Retrieve File ContentThe use of
this.s3Service.getFileContentto retrieve the file content from S3 with the specified bucket and key is correct and aligns with the updated implementation.
60-62: Improved Error Handling with Detailed MessageIncluding the
s3FileNameand bucket name in the error message enhances debugging by providing specific context when the file content is undefined.
83-83: Using Base64 Content for AttachmentThe attachment uses
fileContentBase64, ensuring that the file content is correctly encoded for transmission.libs/application/api/files/src/lib/file.service.ts (7)
17-17: S3Service Imported CorrectlyThe
S3Serviceis properly imported from'@island.is/nest/aws'.
48-48: Dependency Injection of S3ServiceThe
S3Serviceis correctly injected into theFileServiceconstructor.
58-58: File Existence Check UpdatedThe file existence check now correctly uses
s3Service.fileExistswith the appropriate parameters.
Line range hint
61-66: VerifyContentEncodingHeader UsageThe
ContentEncoding: 'base64'header suggests that thecontentis base64-encoded. Please verify thatcontentis indeed base64-encoded data. If not, this header may cause issues when retrieving the file.
72-72: Presigned URL Retrieval UpdatedThe method
getPresignedUrlons3Serviceis correctly used to obtain the presigned URL.
138-138: Presigned URL Retrieval ConsistentUsing
s3Service.getPresignedUrlto retrieve the presigned URL is consistent with the updated service.
234-234: Consistent Presigned URL GenerationThe presigned URL is correctly generated using
s3Service.getPresignedUrlwith the appropriate bucket and key.
...les/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/api/files/src/lib/file.service.ts (1)
260-264: LGTM: File deletion updated, but increment logic can be improvedThe file deletion has been correctly updated to use
s3Service.deleteObject, maintaining consistency with the new S3 implementation. However, the increment logic forresult.deletedcan be further improved.Consider simplifying the increment logic to avoid assignment within the object literal:
- result = { - ...result, - deleted: (result.deleted += 1), - } + result.deleted += 1;This change improves readability and avoids potential confusion with assignments in expressions.
🧰 Tools
🪛 Biome
[error] 263-263: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/application/api/files/src/lib/file.service.ts (9 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/api/files/src/lib/file.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/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.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."
🪛 Biome
libs/application/api/files/src/lib/file.service.ts
[error] 263-263: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (14)
libs/application/template-api-modules/src/lib/modules/templates/children-residence-change/children-residence-change.service.ts (7)
29-29: LGTM: S3Service import added correctlyThe import of S3Service from '@island.is/nest/aws' is correctly placed and aligns with the PR objective of replacing old S3 implementations with a new shared service class.
45-45: LGTM: S3Service correctly injectedThe S3Service is properly injected as a private readonly parameter in the constructor, following good dependency injection practices.
55-61: LGTM: File content retrieval implemented correctlyThe S3Service is used appropriately to retrieve file content with the correct bucket and key parameters. The use of 'binary' encoding is consistent with the previous implementation.
63-67: LGTM: Improved error handling for undefined file contentThe error handling for undefined file content is well-implemented. The error message now includes both the key and bucket name, which will greatly aid in debugging if this error occurs.
69-71: LGTM: Correct Base64 conversion of binary stringThe Base64 conversion is correctly implemented, creating a buffer from the binary string before converting to Base64. This approach is appropriate given that
fileContentBinaryis a binary string, not a Buffer.
90-90: LGTM: Attachment content updated correctlyThe attachment content is correctly updated to use the Base64 encoded file content, which is consistent with the new file retrieval method.
162-162: LGTM: Email functions updated, but verify binary data handlingThe email sending functions have been correctly updated to use
fileContentBinary. This is consistent with the new file retrieval method.Please verify that the
generateApplicationSubmittedEmailandgenerateSyslumennNotificationEmailfunctions are designed to handle binary data correctly. Run the following script to check their implementations:Also applies to: 172-172, 184-184
libs/application/api/files/src/lib/file.service.ts (7)
17-17: LGTM: S3Service import addedThe import of
S3Servicefrom@island.is/nest/awsis correctly added, aligning with the transition fromAwsServicetoS3Servicefor S3 operations.
48-48: LGTM: Constructor updated to use S3ServiceThe constructor parameter has been correctly updated to use
S3Serviceinstead ofAwsService, maintaining consistency with the transition to the new S3 implementation.
58-59: LGTM: File existence check updatedThe file existence check has been correctly updated to use
s3Service.fileExists, maintaining consistency with the new S3 implementation.
Line range hint
61-69: LGTM: File upload method updatedThe file upload method has been correctly updated to use
s3Service.uploadFile, maintaining consistency with the new S3 implementation. The parameters and options are appropriately set.
72-72: LGTM: Presigned URL generation updatedThe presigned URL generation has been correctly updated to use
s3Service.getPresignedUrl, maintaining consistency with the new S3 implementation.
156-159: LGTM: File content retrieval updatedThe file content retrieval has been correctly updated to use
s3Service.getFileContent, maintaining consistency with the new S3 implementation.
Line range hint
1-300: Overall: Successful transition to S3Service with minor improvement opportunityThe file has been successfully updated to use
S3Serviceinstead ofAwsServicefor S3 operations. All relevant methods have been consistently modified to use the new service, improving code consistency and maintainability. The changes align well with the PR objectives of replacing old S3 implementations with a new shared service class.Key points:
- Imports, constructor, and all S3 operations have been updated correctly.
- The code maintains type safety with TypeScript usage.
- The changes support reusability across different NextJS apps.
One minor improvement opportunity exists in the increment logic for
result.deleted(lines 261-264), which could be simplified for better readability.Overall, these changes contribute positively to the codebase by streamlining S3 operations and preparing for the upcoming deprecation of AWS SDK v2.
…th-dedicated-class
…d-class' of github.com:island-is/island.is into chore/replacing-custom-s3-implementations-with-dedicated-class
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/nest/aws/src/lib/s3.service.spec.ts (1)
110-121: Great addition of error handling test cases.The new test cases significantly improve the error handling coverage of the S3Service. They properly check for correct behavior in various error scenarios and validate the error logging.
Consider adding a test case for when the S3 client throws an error without any metadata. This would further improve the robustness of your error handling tests. For example:
it('should handle errors without metadata', async () => { s3Mock.on(HeadObjectCommand).rejectsOnce(new Error('Unknown error')); const result = await s3Service.fileExists({ bucket: 'x', key: 'y' }); expect(logger.error).toBeCalledTimes(1); expect(logger.error).toBeCalledWith( 'Error occurred while checking if file exists in S3', expect.any(Error) ); expect(result).toEqual(false); });Also applies to: 123-137, 145-150
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/nest/aws/src/lib/s3.service.spec.ts (11 hunks)
- libs/nest/aws/src/lib/s3.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/nest/aws/src/lib/s3.service.spec.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/nest/aws/src/lib/s3.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."
📓 Learnings (1)
libs/nest/aws/src/lib/s3.service.spec.ts (1)
Learnt from: HjorturJ PR: island-is/island.is#16281 File: libs/nest/aws/src/lib/aws.service.spec.ts:165-172 Timestamp: 2024-10-09T11:20:29.966Z Learning: In our test classes, we typically mock the `awsService` functions instead of mocking the actual S3 client directly.
🔇 Additional comments (9)
libs/nest/aws/src/lib/s3.service.ts (2)
108-113: Great improvements to error handling and code simplification!The changes to the
fileExistsmethod are well-implemented:
- The simplified return statement on line 108 improves readability.
- The enhanced error handling (lines 110-113) now correctly handles both 404 Not Found and 403 Forbidden status codes, allowing the method to work without requiring ListBucket permissions.
These changes align with TypeScript best practices and improve the overall robustness of the S3 service.
22-22: LGTM! Verify usage across the codebase.The renaming from
AwsServicetoS3Serviceis a good change that better reflects the specific functionality of this service. This aligns well with the PR objectives of replacing old S3 implementations.To ensure all references have been updated, please run the following script:
libs/nest/aws/src/lib/s3.service.spec.ts (7)
12-12: LGTM: Import statement updated correctly.The import statement has been properly updated to reflect the change from AwsService to S3Service. The addition of EncodingString import is also appropriate.
19-19: LGTM: Test suite description updated.The describe block has been correctly updated to refer to 'S3Service', maintaining consistency with the service renaming.
26-26: LGTM: TestingModule provider updated.The provider in the TestingModule has been correctly updated to S3Service, ensuring proper dependency injection for the tests.
40-40: LGTM: Service instantiation updated.The service instantiation in the beforeEach block has been correctly updated to use S3Service, maintaining consistency with the service renaming.
189-190: LGTM: getBucketKey test updated correctly.The getBucketKey test has been properly updated to use s3Service instead of awsService, maintaining consistency with the service renaming while preserving the original test logic.
Line range hint
1-208: Overall, excellent updates to the S3Service test suite.The changes in this file align well with the PR objectives of replacing old S3 implementations. Key points:
- Consistent renaming from AwsService to S3Service throughout the file.
- Addition of new error handling test cases, improving the robustness of the test suite.
- Preservation of the original test logic while updating service references.
- Adherence to coding guidelines for libs/**/* files, using TypeScript and maintaining reusability.
These changes contribute to the goal of simplifying development processes and preparing for the upcoming deprecation of AWS SDK v2 in 2025.
51-51: LGTM: Service references updated consistently.All instances of
awsServicehave been correctly replaced withs3Servicethroughout the test cases. The method calls remain unchanged, indicating that the service interface is consistent.To ensure complete consistency, please run the following command to check for any remaining instances of 'awsService':
If this command returns any results, please review those lines for necessary updates.
Also applies to: 63-63, 77-80, 93-95, 105-105, 116-116, 129-129, 144-144, 161-161, 173-173
✅ Verification successful
Verified: All instances of
awsServicehave been successfully replaced withs3Service.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all instances of 'awsService' have been replaced grep -n 'awsService' libs/nest/aws/src/lib/s3.service.spec.tsLength of output: 61
HjorturJ
added
automerge
…th-dedicated-class
…th-dedicated-class
kodiakhq
bot
deleted the
chore/replacing-custom-s3-implementations-with-dedicated-class
branch
Replacing more cases of unique s3 implementations with the new shared service
Task link
What
Slowly replacing all references to s3 with references to a single shared service class
Why
Makes devs life easier and aws sdk v2 is being depricated in 2025
Checklist:
Summary by CodeRabbit
New Features
AwsServicetoS3Servicefor file handling, improving service efficiency.Bug Fixes
Refactor
S3Service.Tests
S3Serviceimplementation, ensuring continued test coverage and accuracy.