Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Task #212710 feat: [BE] Upload API - Changes for resizing profile images during upload, changes for retrival of images based on size #789

Open
wants to merge 4 commits into
base: release-2.7.0
Choose a base branch
from

Conversation

manojLondhe
Copy link
Collaborator

@manojLondhe manojLondhe commented Feb 16, 2024

Task #212710 feat: [BE] Upload API - Changes for resizing profile images during upload, changes for retrival of images based on size

Summary by CodeRabbit

  • New Features

    • Integrated new functionality for resizing and uploading images with support for multiple sizes.
    • Enhanced search capabilities in the facilitator service with improved filter conditions.
    • Added support for additional file types in file upload processes.
  • Enhancements

    • Improved readability and consistency of code through better formatting and whitespace adjustments across multiple services.
    • Updated query parameters and formatting for more efficient data retrieval and management.
  • Bug Fixes

    • Fixed query issues in beneficiary services to ensure accurate data fetching and display.
  • Documentation

    • Updated internal documentation to reflect changes in query and file handling methods.

Copy link

coderabbitai bot commented Feb 16, 2024

Walkthrough

The recent updates primarily involve code formatting improvements and the integration of the sharp library for image resizing. Changes include modifying query strings, enhancing readability with whitespace and indentation adjustments, and refining the file upload process to handle different types and sizes. These adjustments aim to improve maintainability and functionality without altering the core logic.

Changes

File Path Change Summary
src/package.json Added sharp and rxjs packages.
.../beneficiaries.service.ts Improved query formatting, added parameters for image document retrieval, and updated fields.
.../camp/camp.service.ts Minor query string adjustments for spacing and indentation.
.../facilitator.service.ts Enhanced code readability, updated query formatting, and added file upload functionality.
.../services/s3/s3.service.ts Modified uploadFile method to improve file handling and error messaging.
.../upload-file/... Enhanced file handling with sharp for image resizing, improved parameter readability, and added size handling in file retrieval methods.

Recent Review Details

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between a98a765 and a180e59.
Files ignored due to path filters (1)
  • src/package-lock.json is excluded by !**/package-lock.json
Files selected for processing (7)
  • src/package.json (1 hunks)
  • src/src/beneficiaries/beneficiaries.service.ts (21 hunks)
  • src/src/camp/camp.service.ts (33 hunks)
  • src/src/facilitator/facilitator.service.ts (13 hunks)
  • src/src/services/s3/s3.service.ts (2 hunks)
  • src/src/upload-file/upload-file.controller.ts (3 hunks)
  • src/src/upload-file/upload-file.service.ts (9 hunks)
Files skipped from review due to trivial changes (2)
  • src/package.json
  • src/src/camp/camp.service.ts
Additional Context Used
Path-based Instructions (5)
src/src/upload-file/upload-file.controller.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/services/s3/s3.service.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/upload-file/upload-file.service.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/facilitator/facilitator.service.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/src/beneficiaries/beneficiaries.service.ts (1)

Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:

  • The code adheres to best practices associated with nodejs.
  • The code adheres to best practices associated with nestjs framework.
  • The code adheres to best practices recommended for performance.
  • The code adheres to similar naming conventions for controllers, models, services, methods, variables."
Learnings (1)
src/src/beneficiaries/beneficiaries.service.ts (1)
User: manojLondhe"
PR: tekdi/eg-backend#656
File: src/src/beneficiaries/beneficiaries.service.ts:233-234
Timestamp: 2023-12-28T13:01:35.712Z
Learning: The `program_id` and `academic_year_id` variables are validated at the middleware level, so no further validation is needed within the service methods where they are used.
Additional comments not posted (17)
src/src/upload-file/upload-file.controller.ts (3)

34-40: Refactored parameters in addFile method improve readability.


53-60: Added size query parameter in getFileUrl method enhances functionality.

Verification successful

The verification process has successfully identified the getFile method within the UploadFileService class in the upload-file.service.ts file. The method signature includes parameters for id, size, and response, which aligns with the changes noted in the review comment regarding the addition of the size parameter in the getFileUrl method of the controller.

The getFile method in the service utilizes the size parameter to potentially fetch a resized image path, which is then used to retrieve the file URL from the S3 service. This confirms that the size parameter is indeed being used appropriately within the service method, as intended by the changes in the controller.

Based on this information, the review comment is accurate, and the implementation aligns with the described functionality.

Conclusion: The addition of the size query parameter in the getFileUrl method is correctly implemented and used in the corresponding service method.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the `size` parameter is correctly used in the corresponding service method.
ast-grep --lang typescript --pattern $'getFile($_, $_, $_)'

Length of output: 60



Script:

#!/bin/bash
# Search for any method named `getFile` in the TypeScript files to understand its signature and usage.
ast-grep --lang typescript --pattern $'class $_ { $$$ getFile($$$) { $$$ } $$$ }'

Length of output: 28591


63-69: Added size query parameter in getDocumentById method enhances functionality.

Verification successful

The verification process has confirmed that the getDocumentById method in the UploadFileService class correctly utilizes the size parameter. This parameter is used to determine if a resized image should be fetched, as seen in the method implementation where getResizedImagePath is called with the document name and size. This method checks if the file is an image and if the requested size is among the allowed resizing variants before constructing the path for the resized image.

Thus, the addition of the size query parameter in the getDocumentById method in the controller, which is passed down to the service method, is correctly implemented and enhances the functionality by allowing size-based document retrieval.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the `size` parameter is correctly used in the corresponding service method.
ast-grep --lang typescript --pattern $'getDocumentById($_, $_, $_)'

Length of output: 68



Script:

#!/bin/bash
# Attempt to locate the `getDocumentById` method in the service file and verify the usage of the `size` parameter.
# First, find the file that likely contains the service method.
fd "upload-file.service.ts" --exec cat {} \;
# Then, use a more specific pattern to locate the method and check the parameter usage.
ast-grep --lang typescript --pattern $'class $_ { $$$ getDocumentById($_, $_, $_) { $$$ } $$$ }'

Length of output: 42169

src/src/services/s3/s3.service.ts (1)

30-50: Refactored uploadFile method to accept fileBuffer and contentType enhances flexibility and robustness.

src/src/upload-file/upload-file.service.ts (3)

43-74: Implemented resizing logic in resizeImage method using sharp library enhances image handling capabilities.

Verification successful

The verification process has confirmed the presence and implementation of the resizeImage method within the UploadFileService class. The method is designed to handle image resizing and integrates with the S3 service for uploading the resized images. The method signature and its functionality, as described in the review comment, align with the code found in the repository.

This method effectively resizes images to multiple specified dimensions and uploads each variant to an S3 bucket using a structured key format. The use of the sharp library for image processing, including maintaining EXIF data and specifying the resizing strategy, is correctly implemented as per the requirements.

Conclusion:
The resizeImage method is implemented as described, and its integration with the S3 service is correctly handled. The method supports multiple image sizes and uses appropriate options for image resizing.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the resizing logic is correctly implemented and integrates well with the S3 upload.
ast-grep --lang typescript --pattern $'resizeImage($_, $_)'

Length of output: 60



Script:

#!/bin/bash
# Attempt to find the resizeImage method with a broader search pattern
ast-grep --lang typescript --pattern $'class $_ { $$$ resizeImage($_, $_) { $$$ } $$$ }'

Length of output: 28598


Script:

#!/bin/bash
# Use ripgrep to search for the resizeImage method in TypeScript files if ast-grep fails again
rg 'resizeImage' -t ts

Length of output: 207


88-188: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-217]

Enhanced addFile method with conditional logic for resizing based on file and document types.

Verification successful

The provided output from the shell script confirms the presence of the addFile method within the UploadFileService class in the upload-file.service.ts file. The method includes conditional logic for resizing images based on the document type, which matches the description in the review comment.

The conditional logic checks if the file is an image and if the document type is included in the list of types that should be resized. This is done using the isImage method and the documentTypesToBeResized array. If both conditions are met, the resizeImage method is called to handle the resizing of the image.

Given this information, the review comment stating that the addFile method has been enhanced with conditional logic for resizing based on file and document types is accurate. The implementation in the code aligns with the description provided in the review comment.

Conclusion: The review comment is verified as accurate.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the conditional logic for resizing is correctly implemented in the `addFile` method.
ast-grep --lang typescript --pattern $'addFile($_, $_, $_, $_, $_)'

Length of output: 68



Script:

#!/bin/bash
# Attempt to locate the `addFile` method with a broader pattern and verify the conditional logic for resizing
ast-grep --lang typescript --pattern $'class $_ { $$$ addFile($_, $_, $_, $_, $_) { $$$ } $$$ }'

Length of output: 28606


354-381: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [333-391]

Enhanced getDocumentById method to handle resized image retrieval based on size.

Verification successful

The verification process has successfully located the getDocumentById method within the UploadFileService class. The method indeed handles the retrieval of resized images based on the provided size parameter, as described in the review comment. This functionality is implemented through the getResizedImagePath method, which adjusts the image path based on the requested size before fetching the URL from the S3 service.

The method checks if the document exists in the database and handles responses appropriately, including sending different HTTP status codes based on the existence of the document and the successful retrieval of the file URL. This aligns with the enhanced functionality noted in the review comment.

Given this information, the implementation of the resized image retrieval logic in the getDocumentById method is confirmed to be as described in the review comment.

Enhanced getDocumentById method to handle resized image retrieval based on size is verified as correctly implemented.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the resized image retrieval logic is correctly implemented in the `getDocumentById` method.
ast-grep --lang typescript --pattern $'getDocumentById($_, $_, $_)'

Length of output: 68



Script:

#!/bin/bash
# Attempt to locate the `getDocumentById` method with a broader pattern to ensure it is captured.
ast-grep --lang typescript --pattern $'class $_ { $$$ getDocumentById($$$) { $$$ } $$$ }'

Length of output: 32472

src/src/facilitator/facilitator.service.ts (6)

455-461: Ensure consistent use of query formatting for better readability and maintainability.


464-464: Verify that the status filtering logic aligns with the application's requirements and that no statuses are unintentionally excluded.


1706-1816: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1709-1854]

Review the GraphQL query for potential performance issues with large datasets, especially with multiple nested subqueries.


1920-1920: Ensure that the document retrieval by ID handles errors gracefully and logs them appropriately.


1981-1982: Consider adding explicit checks for empty search terms to avoid unnecessary database queries.


2074-2079: Check for potential performance issues with the nested queries and ensure that they are optimized for large datasets.

src/src/beneficiaries/beneficiaries.service.ts (4)

863-863: Validate the use of hardcoded '64' as a parameter.

Ensure that the hardcoded value '64' used in the getDocumentById method call is intentional and correctly applies to the use case. Consider defining it as a constant if it's a standard value used across multiple places.


1177-1177: Verify the use of 'original' as a parameter in multiple places.

The string 'original' is used repeatedly as a parameter in getDocumentById. Confirm that this is the expected behavior and consider using a constant if this value does not change.


1313-1337: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1316-1360]

Review the structure and formatting of the GraphQL query in findOne.

The GraphQL query inside the findOne method is quite extensive. Consider breaking it down into smaller, reusable fragments or moving parts of it to separate functions to improve readability and maintainability.


1463-1469: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1456-1476]

Ensure error handling for external service calls.

The methods getDocumentById are called multiple times without handling potential errors from these calls. It's crucial to add error handling to prevent runtime exceptions if the external service fails.

try {
  const { success, data: fileData } = await this.uploadFileService.getDocumentById(mappedData?.profile_photo_1?.id, 'original');
  if (success && fileData?.fileUrl) {
    mappedData.profile_photo_1.fileUrl = fileData.fileUrl;
  }
} catch (error) {
  console.error('Failed to retrieve document:', error);
}

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

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

Copy link

sonarcloud bot commented Feb 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@manojLondhe
Copy link
Collaborator Author

@coderabbitai review

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

@@ -315,7 +315,7 @@ export class BeneficiariesService {
district
program_beneficiaries{
user_id
facilitator_id
facilitator_id
Copy link

Choose a reason for hiding this comment

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

Ensure consistent indentation for facilitator_id.

-							facilitator_id
+                           facilitator_id

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
facilitator_id
facilitator_id

Comment on lines 484 to 490
district
block
mobile
program_beneficiaries {
program_beneficiaries {
id
user_id,
facilitator_id,
Copy link

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [487-497]

Check for potential duplication of facilitator_id in the GraphQL query.

It appears that facilitator_id is listed twice in the selection set of the GraphQL query. This could be a mistake and may lead to unnecessary data retrieval or confusion.

Comment on lines +623 to +643
let qury = `query MyQuery {
${status.map(
(item) => `${
!isNaN(Number(item[0])) ? '_' + item : item
}:program_beneficiaries_aggregate(where:{
_and: [
{
_and: [
{
facilitator_id: { _eq: ${user?.data?.id} }
},{
status: {_eq: "${item}"}
},
},{
status: {_eq: "${item}"}
},
{ user: { id: { _is_null: false } } }

]
}) {
aggregate {
count
}
}`,

]
}) {
aggregate {
count
}
}`,
)}

}`;

Copy link

Choose a reason for hiding this comment

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

Optimize GraphQL query by using template literals for dynamic query construction.

Consider using template literals for constructing GraphQL queries dynamically. This approach enhances readability and maintainability of the code.

let qury = `query MyQuery {
  ${status.map(
    (item) => `${!isNaN(Number(item[0])) ? '_' + item : item}: program_beneficiaries_aggregate(where:{
      _and: [
        {facilitator_id: { _eq: ${user?.data?.id} }},
        {status: {_eq: "${item}"}},
        { user: { id: { _is_null: false } } }
      ]
    }) {
      aggregate {
        count
      }
    }`
  )}
}`;

@@ -108,7 +108,7 @@ export class FacilitatorService {
count
}
}

Copy link

Choose a reason for hiding this comment

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

Consider using parameterized queries to prevent SQL injection vulnerabilities.

Comment on lines +1192 to +1195
{ first_name: { _ilike: "%${body.search}%" } },
{ last_name: { _ilike: "%${body.search}%" } },
{ email_id: { _ilike: "%${body.search}%" } }
]} `);
Copy link

Choose a reason for hiding this comment

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

Use parameterized queries for search functionality to prevent SQL injection.

Comment on lines +1623 to +1626
{ first_name: { _ilike: "%${body.search}%" } },
{ last_name: { _ilike: "%${body.search}%" } },
{ email_id: { _ilike: "%${body.search}%" } }
]} `);
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The search for test cases related to the search functionality in the test files has revealed references to search-related tests in the lms.controller.ts and lms.service.ts files. However, these references are specific to the LMS (Learning Management System) module and do not directly relate to the search functionality in the facilitator.service.ts file, which was the focus of the original review comment.

Given this context, it appears that there are no specific test cases for the search functionality in the facilitator.service.ts file. This suggests that the original review comment's concern about thoroughly testing the search functionality is valid and needs attention.

Action Required:
Please ensure that the search functionality in the facilitator.service.ts file is thoroughly tested to handle various input scenarios gracefully. This includes creating and executing tests that cover different types of input, including edge cases, to ensure robustness and reliability of the search feature.

Analysis chain

Ensure that the search functionality is thoroughly tested to handle various input scenarios gracefully.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for test cases related to search functionality in the test files
rg --type ts "search functionality tests"

Length of output: 43



Script:

#!/bin/bash
# Search for test cases related to the search functionality using broader terms
rg --type ts -i "search" | grep -i "test"

Length of output: 757

Comment on lines +2202 to +2203
address_line_1
address_line_2
Copy link

Choose a reason for hiding this comment

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

Ensure that address lines are sanitized and validated to prevent XSS and other injection attacks.

@sagarkoshti1990 sagarkoshti1990 changed the base branch from release-2.0.0 to release-2.1.0 April 18, 2024 12:32
@sagarkoshti1990 sagarkoshti1990 changed the base branch from release-2.1.0 to release-2.2.0 May 22, 2024 07:09
@rushi-tekdi rushi-tekdi changed the base branch from release-2.2.0 to release-2.3.0 June 21, 2024 05:04
@sagarkoshti1990 sagarkoshti1990 changed the base branch from release-2.5.0 to release-2.6.0 August 30, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant