employee-dev-10-fix-user-profile-upload-photo #205
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @yennanliu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've reviewed this pull request. The primary goal of this PR is to implement the functionality for users to upload and display profile photos within the employee system application. This involves changes across both the backend (Spring Boot) and the frontend (Vue.js) to handle the storage, retrieval, and display of user photos. A new Markdown file is also added to provide clear testing instructions for this new feature.
Highlights
- Backend Photo Handling: New endpoints have been added to the
UserControllerto support updating a user with photo data (POST /users/update/{userId}) and retrieving a user's photo (GET /users/photo/{userId}). TheUserServicehas been updated with a new method (updateUserWithPhoto) to handle saving the photo bytes to the database and modifying the existingupdateUsermethod to correctly handle updates without overwriting the photo field. - Frontend Photo Upload & Preview: The
EditUser.vuecomponent has been enhanced to include a file input specifically for photo uploads. It now includes client-side validation for file type (JPEG, PNG, GIF) and size (max 5MB). A photo preview feature is implemented to show the selected image before submission, or display the existing user photo. - Frontend Photo Display: The
UserBox.vuecomponent (used in the user list) andShowUserDetails.vuecomponent (user profile page) have been modified to fetch and display the user's photo using the new backend photo retrieval endpoint. Fallback logic is included to display initials or a default image if a photo is not available or fails to load. - Testing Guide: A new Markdown file (
TEST_PHOTO_UPLOAD.md) has been added, providing detailed steps andcurlcommands for testing the photo upload and display functionality on both the backend and frontend, including expected behavior and error scenarios.
Changelog
Click here to see the changelog
- springEmployeeSystem/TEST_PHOTO_UPLOAD.md
- Added a new file containing a comprehensive guide for testing the user photo upload feature.
- Includes backend testing steps with curl commands for upload and retrieval.
- Includes frontend testing steps covering the UI flow, expected behavior, and error scenarios.
- Lists the backend and frontend files modified in this PR.
- springEmployeeSystem/backend/EmployeeSystem/src/main/java/EmployeeSystem/controller/UserController.java
- Added imports for
HttpHeaders,MediaType, andObjectMapper. - Introduced a new
POST /users/update/{userId}endpoint to handle multipart form data containing user JSON and an optional photo file. - Added a new
GET /users/photo/{userId}endpoint to retrieve user photo bytes. - Includes basic error handling and logging for the new endpoints.
- Added imports for
- springEmployeeSystem/backend/EmployeeSystem/src/main/java/EmployeeSystem/service/UserService.java
- Added imports for
IOException,Optional, andMultipartFile. - Modified the existing
updateUsermethod to useOptionaland update fields individually to prevent overwriting the photo field. - Added a new
updateUserWithPhotomethod to handle updating user details and saving the photo bytes from aMultipartFile. - Includes error handling for file processing and user not found scenarios.
- Added imports for
- springEmployeeSystem/frontend/employee-system-ui/src/components/User/UserBox.vue
- Replaced the initials display with an
<img>tag to show the user's photo. - Added a computed property
userPhotoUrlto dynamically generate the photo retrieval URL. - Implemented error handling (
@errorlistener andphotoErrorstate) to fallback to displaying initials if the photo fails to load. - Added CSS styles for the avatar image.
- Replaced the initials display with an
- springEmployeeSystem/frontend/employee-system-ui/src/views/User/EditUser.vue
- Updated the file input for photo upload with a specific class.
- Added state (
photoFile,photoPreview,photoError) and computed property (existingPhotoUrl) for handling photo selection and preview. - Implemented a photo preview area that shows either the selected file or the existing user photo.
- Modified the
handleFileUploadmethod to perform client-side validation for file type (image/*) and size (<= 5MB). - Added a
formatFileSizehelper method for displaying file size. - Updated the
editUsermethod to correctly useFormDatafor sending both user data (as JSON string) and the photo file to the backend. - Added CSS styles for the photo preview section and file input.
- springEmployeeSystem/frontend/employee-system-ui/src/views/User/ShowUserDetails.vue
- Replaced the static image display with an
<img>tag using a dynamic source. - Added state (
photoError) and a computed property (userPhotoUrl) to fetch the user's photo from the backend. - Implemented error handling (
@errorlistener andphotoErrorstate) to fallback to a default image if the photo fails to load. - Added CSS styles for the user photo container and image.
- Replaced the static image display with an
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature: user photo uploads. The changes span both backend and frontend, enabling users to upload, update, and display profile pictures. The inclusion of TEST_PHOTO_UPLOAD.md is excellent and provides clear instructions for testing this new functionality – great job on that!
The backend implementation introduces new endpoints for photo upload and retrieval, and refactors the user update logic. The frontend components are updated to handle photo display, preview, and upload with validation.
While the core functionality seems well-implemented, there are a few areas for improvement, primarily concerning code duplication, exception handling, and configuration, which I've detailed in the comments. Addressing these will enhance the robustness and maintainability of the new feature.
Summary of Findings
- Code Duplication in UserService: The
UserServicecontains duplicated logic for updating user entity fields in bothupdateUserandupdateUserWithPhotomethods. This should be refactored into a common helper method to improve maintainability. - Backend URL Hardcoding in Frontend: The backend API base URL (
http://localhost:9998) is hardcoded in multiple Vue components. This should be externalized to a configuration file for easier deployment across different environments. - ObjectMapper Instantiation: A new
ObjectMapperinstance is created per request inUserController, which can be inefficient. A shared, pre-configured instance should be used. - Generic Exception Handling: Generic
ExceptionandRuntimeExceptionare used inUserControllerandUserService. Using more specific, custom exceptions and avoiding direct exposure ofe.getMessage()in API responses would improve error handling and security. - Hardcoded Image Content-Type: The
getUserPhotoendpoint inUserControllerhardcodes theContent-TypetoIMAGE_JPEG. This should ideally be dynamic based on the actual uploaded image type. - Minor Typos/Spacing in
TEST_PHOTO_UPLOAD.md: TheTEST_PHOTO_UPLOAD.mdfile has minor typos/spacing issues (e.g., double space after '5MB', trailing space after 'initials'). These are low severity and were not commented on directly due to review settings. - Debug
console.logstatements: Severalconsole.logstatements are present in the Vue components, likely for debugging. These should be removed or handled by a proper logger before merging to production. These are low severity and were not commented on directly due to review settings.
Merge Readiness
This pull request adds valuable photo upload functionality. However, due to the high-severity code duplication issue in UserService.java and several medium-severity concerns regarding exception handling, configuration, and potential performance impacts, I recommend addressing these points before merging. The provided TEST_PHOTO_UPLOAD.md is a great asset for verifying the changes. As a reviewer, I am not authorized to approve the pull request, but I suggest these changes be made to ensure higher code quality and maintainability. Other reviewers should also assess the changes.
| // Update fields while preserving existing data | ||
| if (userCreateDto.getFirstName() != null) { | ||
| existingUser.setFirstName(userCreateDto.getFirstName()); | ||
| } | ||
| if (userCreateDto.getLastName() != null) { | ||
| existingUser.setLastName(userCreateDto.getLastName()); | ||
| } | ||
| if (userCreateDto.getEmail() != null) { | ||
| existingUser.setEmail(userCreateDto.getEmail()); | ||
| } | ||
| if (userCreateDto.getRole() != null) { | ||
| existingUser.setRole(userCreateDto.getRole()); | ||
| } | ||
| if (userCreateDto.getDepartementId() != null) { | ||
| existingUser.setDepartementId(userCreateDto.getDepartementId()); | ||
| } | ||
| if (userCreateDto.getManagerId() != null) { | ||
| existingUser.setManagerId(userCreateDto.getManagerId()); | ||
| } | ||
|
|
||
| userRepository.save(existingUser); | ||
| log.info("User updated successfully with ID: {}", userCreateDto.getId()); | ||
| } else { | ||
| log.error("User with ID {} not found", userCreateDto.getId()); | ||
| throw new RuntimeException("User not found with ID: " + userCreateDto.getId()); | ||
| } | ||
| } | ||
|
|
||
| public void updateUserWithPhoto(UserCreateDto userCreateDto, MultipartFile photo) { | ||
|
|
||
| Optional<User> optionalUser = userRepository.findById(userCreateDto.getId()); | ||
| if (optionalUser.isPresent()) { | ||
| User existingUser = optionalUser.get(); | ||
|
|
||
| // Update basic fields | ||
| if (userCreateDto.getFirstName() != null) { | ||
| existingUser.setFirstName(userCreateDto.getFirstName()); | ||
| } | ||
| if (userCreateDto.getLastName() != null) { | ||
| existingUser.setLastName(userCreateDto.getLastName()); | ||
| } | ||
| if (userCreateDto.getEmail() != null) { | ||
| existingUser.setEmail(userCreateDto.getEmail()); | ||
| } | ||
| if (userCreateDto.getRole() != null) { | ||
| existingUser.setRole(userCreateDto.getRole()); | ||
| } | ||
| if (userCreateDto.getDepartementId() != null) { | ||
| existingUser.setDepartementId(userCreateDto.getDepartementId()); | ||
| } | ||
| if (userCreateDto.getManagerId() != null) { | ||
| existingUser.setManagerId(userCreateDto.getManagerId()); | ||
| } |
There was a problem hiding this comment.
The logic for updating user fields (lines 171-191 in updateUser and lines 205-223 in updateUserWithPhoto) appears to be duplicated. This could lead to maintenance challenges, as any changes to this logic would need to be applied in two places. Could this common logic be extracted into a private helper method? For example:
private void mapUserDtoToEntity(UserCreateDto dto, User entity) {
if (dto.getFirstName() != null) {
entity.setFirstName(dto.getFirstName());
}
// ... other fields
}Then, both updateUser and updateUserWithPhoto could call this helper method:
In updateUser:
// ...
User existingUser = optionalUser.get();
mapUserDtoToEntity(userCreateDto, existingUser);
userRepository.save(existingUser);
// ...In updateUserWithPhoto:
// ...
User existingUser = optionalUser.get();
mapUserDtoToEntity(userCreateDto, existingUser);
// ... (photo handling logic)
userRepository.save(existingUser);
// ...This would improve maintainability and reduce redundancy.
|
|
||
| try { | ||
| // Parse the JSON string to UserCreateDto | ||
| ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
Is there a reason for creating a new ObjectMapper instance on each request? Instantiating ObjectMapper can be somewhat expensive. Consider injecting a pre-configured ObjectMapper bean (e.g., the one Spring Boot auto-configures) or defining it as a static final field if a shared instance is appropriate for your use case. This can improve performance by avoiding repeated object creation and configuration.
| } catch (Exception e) { | ||
| log.error("Error updating user: {}", e.getMessage()); | ||
| return new ResponseEntity<>( | ||
| new ApiResponse(false, "Failed to update user: " + e.getMessage()), | ||
| HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
Catching a generic Exception and then exposing e.getMessage() directly in the ApiResponse could potentially leak internal system details or stack trace information that might not be suitable for clients. Would it be better to catch more specific exceptions (e.g., IOException for parsing, custom application exceptions from the service layer) and return a more generic error message to the client, while logging the detailed e.getMessage() for debugging purposes?
| User user = userService.getUserById(userId); | ||
| if (user != null && user.getPhoto() != null) { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.setContentType(MediaType.IMAGE_JPEG); // Default to JPEG, could be enhanced to detect type |
There was a problem hiding this comment.
The comment // Default to JPEG, could be enhanced to detect type is a good observation. Hardcoding MediaType.IMAGE_JPEG means all retrieved photos will be declared as JPEGs, regardless of their actual type (e.g., PNG, GIF). For a future enhancement, have you considered storing the image's content type when it's uploaded, or perhaps attempting to detect it from the byte array (e.g., using java.nio.file.Files.probeContentType or a library like Apache Tika) before setting the Content-Type header? This would make the endpoint more robust.
| log.info("User updated successfully with ID: {}", userCreateDto.getId()); | ||
| } else { | ||
| log.error("User with ID {} not found", userCreateDto.getId()); | ||
| throw new RuntimeException("User not found with ID: " + userCreateDto.getId()); |
There was a problem hiding this comment.
Throwing a generic RuntimeException when a user is not found (and similarly on lines 233 and 241) results in a 500 Internal Server Error. Would it be more appropriate to throw a custom, more specific exception (e.g., UserNotFoundException extends RuntimeException)? This custom exception could then be handled by a @ControllerAdvice to return a 404 Not Found status, which is semantically more accurate for this scenario.
| if (!this.user || !this.user.id || this.photoError) { | ||
| return null; | ||
| } | ||
| return `http://localhost:9998/users/photo/${this.user.id}`; |
There was a problem hiding this comment.
The base URL http://localhost:9998 is hardcoded here and in other frontend files (e.g., EditUser.vue, ShowUserDetails.vue). This can make it difficult to deploy the frontend to different environments (staging, production) where the backend API might be at a different address. Have you considered moving this base URL to a configuration file (e.g., .env files) or an Axios global configuration? This would make the application more flexible.
For example, you could create a .env.local file:
VUE_APP_API_BASE_URL=http://localhost:9998
And then use it in your components:
return `${process.env.VUE_APP_API_BASE_URL}/users/photo/${this.user.id}`;
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the functionality to upload and display user profile photos, along with a helpful testing guide. The changes touch both the backend (Spring Boot) and frontend (Vue.js) to implement this feature. The frontend includes client-side validation and photo preview, which is a good user experience improvement. The backend provides endpoints for updating user data with a photo and retrieving the photo.
The addition of the TEST_PHOTO_UPLOAD.md file is a great step towards documenting how to verify this new feature. Overall, the implementation provides the core functionality requested.
Summary of Findings
- Server-side photo validation: The backend is missing server-side validation for uploaded photo file types and sizes. This is a security vulnerability and should be added.
- Code Duplication in UserService: The logic for updating user fields is duplicated in the
updateUserandupdateUserWithPhotomethods. This could be refactored into a shared helper method. - Broad Exception Handling: The
UserControlleruses broadcatch (Exception e)blocks. Catching more specific exceptions would improve error handling and debugging. - Hardcoded Image Type: The photo retrieval endpoint in
UserControllerhardcodes the responseContent-Typetoimage/jpeg. This should be dynamic based on the actual uploaded image type. - Manual JSON Parsing in Controller: The
updateUserWithPhotoendpoint manually parses the user JSON string. Using@RequestPartwould allow Spring to handle this automatically. - Generic Exceptions from Service: The
UserServicethrows genericRuntimeExceptionfor specific errors like user not found or photo processing issues. More specific custom exceptions could be beneficial. - Lack of Automated Tests: While a manual testing guide is provided, there are no automated unit or integration tests included for the new photo upload/retrieval functionality in either the backend or frontend. (Note: This was not included as a review comment due to review settings.)
Merge Readiness
The pull request implements the core photo upload and display feature and includes helpful client-side validation and a testing guide. However, there is a high-severity issue regarding the lack of server-side validation for photo uploads, which poses a security risk. There are also several medium-severity issues related to code maintainability and error handling in the backend.
I recommend addressing the high-severity issue and the medium-severity issues before merging this pull request. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.
| if (photo != null) { | ||
| log.info(">>> Photo size: {} bytes", photo.getSize()); | ||
| } |
There was a problem hiding this comment.
While the frontend performs validation, it's crucial to also implement server-side validation for file type and size. This prevents malicious uploads or denial-of-service attacks if the frontend validation is bypassed. You should check the photo.getContentType() and photo.getSize() here before processing the file.
| if (photo != null) { | |
| log.info(">>> Photo size: {} bytes", photo.getSize()); | |
| } | |
| if (photo != null && !photo.isEmpty()) { | |
| log.info(">>> Photo size: {} bytes", photo.getSize()); | |
| // TODO: Add server-side validation for file type and size | |
| } |
| @RequestParam("user") String userJson, | ||
| @RequestParam(value = "photo", required = false) MultipartFile photo) { |
There was a problem hiding this comment.
Using @RequestParam("user") String userJson requires manual JSON parsing (ObjectMapper). Spring can handle this automatically for multipart/form-data requests using @RequestPart. This would make the controller method signature cleaner and leverage Spring's built-in capabilities.
| @RequestParam("user") String userJson, | |
| @RequestParam(value = "photo", required = false) MultipartFile photo) { | |
| @RequestPart("user") UserCreateDto userCreateDto, | |
| @RequestPart(value = "photo", required = false) MultipartFile photo) { |
| try { | ||
| // Parse the JSON string to UserCreateDto | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| UserCreateDto userCreateDto = objectMapper.readValue(userJson, UserCreateDto.class); | ||
| userCreateDto.setId(userId); |
There was a problem hiding this comment.
If you switch to @RequestPart("user") UserCreateDto userCreateDto as suggested, you won't need this manual parsing step.
| try { | |
| // Parse the JSON string to UserCreateDto | |
| ObjectMapper objectMapper = new ObjectMapper(); | |
| UserCreateDto userCreateDto = objectMapper.readValue(userJson, UserCreateDto.class); | |
| userCreateDto.setId(userId); | |
| try { | |
| // userCreateDto is automatically parsed by Spring | |
| userCreateDto.setId(userId); |
| } catch (Exception e) { | ||
| log.error("Error updating user: {}", e.getMessage()); | ||
| return new ResponseEntity<>( | ||
| new ApiResponse(false, "Failed to update user: " + e.getMessage()), | ||
| HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception is very broad and can hide specific issues. Consider catching more specific exceptions (e.g., IOException from file processing, custom exceptions from the service layer for user not found or validation errors) to provide more targeted error responses and logging.
} catch (IOException e) {
log.error("Error processing photo for user {}: {}", userId, e.getMessage());
return new ResponseEntity<>(
new ApiResponse(false, "Failed to process photo: " + e.getMessage()),
HttpStatus.BAD_REQUEST); // Or appropriate status
} catch (RuntimeException e) { // Catch RuntimeException from service
log.error("Error updating user {}: {}", userId, e.getMessage());
return new ResponseEntity<>(
new ApiResponse(false, "Failed to update user: " + e.getMessage()),
HttpStatus.BAD_REQUEST); // Or appropriate status
} catch (Exception e) {
log.error("An unexpected error occurred updating user {}: {}", userId, e.getMessage());
return new ResponseEntity<>(
new ApiResponse(false, "An unexpected error occurred: " + e.getMessage()),
HttpStatus.INTERNAL_SERVER_ERROR);
}| User user = userService.getUserById(userId); | ||
| if (user != null && user.getPhoto() != null) { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.setContentType(MediaType.IMAGE_JPEG); // Default to JPEG, could be enhanced to detect type |
There was a problem hiding this comment.
The photo retrieval endpoint hardcodes the Content-Type header to MediaType.IMAGE_JPEG. If users can upload PNG or GIF images, this will result in the browser potentially misinterpreting the image type. It would be better to store the content type along with the photo bytes in the database and set the header dynamically based on the stored type.
headers.setContentType(MediaType.IMAGE_JPEG); // TODO: Determine actual image type from stored data or byte analysis
No description provided.