From f3ed5e0b9abdf97177a5ddcb9e4b4b189c99d2bd Mon Sep 17 00:00:00 2001 From: Harkamal Randhawa Date: Fri, 17 Apr 2026 15:12:00 -0600 Subject: [PATCH] fix review findings --- .../backend/controller/AuthController.java | 14 +---- .../controller/UserAvatarController.java | 58 +++++++------------ .../backend/service/CouponService.java | 4 +- .../backend/util/ImageValidationUtil.java | 8 ++- 4 files changed, 30 insertions(+), 54 deletions(-) diff --git a/backend/src/main/java/com/petshop/backend/controller/AuthController.java b/backend/src/main/java/com/petshop/backend/controller/AuthController.java index ad1e9be4..7bdb53a1 100644 --- a/backend/src/main/java/com/petshop/backend/controller/AuthController.java +++ b/backend/src/main/java/com/petshop/backend/controller/AuthController.java @@ -24,6 +24,7 @@ import com.petshop.backend.service.AvatarStorageService; import com.petshop.backend.service.EmailService; import com.petshop.backend.service.PasswordResetService; import com.petshop.backend.util.AuthenticationHelper; +import com.petshop.backend.util.ImageValidationUtil; import com.petshop.backend.util.PhoneUtils; import com.petshop.backend.util.StringUtils; import jakarta.validation.Valid; @@ -296,18 +297,7 @@ public class AuthController { public ResponseEntity uploadAvatar(@RequestParam("avatar") MultipartFile file) { User user = getAuthenticatedUser(); - if (file.isEmpty()) { - throw new BusinessException("Please select a file to upload"); - } - - if (file.getSize() > 5 * 1024 * 1024) { - throw new BusinessException("File size must not exceed 5MB"); - } - - String contentType = file.getContentType(); - if (contentType == null || (!contentType.equals("image/jpeg") && !contentType.equals("image/png") && !contentType.equals("image/gif"))) { - throw new BusinessException("Only JPG, PNG, and GIF images are allowed"); - } + ImageValidationUtil.validate(file); try { avatarStorageService.deleteAvatar(user); diff --git a/backend/src/main/java/com/petshop/backend/controller/UserAvatarController.java b/backend/src/main/java/com/petshop/backend/controller/UserAvatarController.java index 68ef41ef..602cfe54 100644 --- a/backend/src/main/java/com/petshop/backend/controller/UserAvatarController.java +++ b/backend/src/main/java/com/petshop/backend/controller/UserAvatarController.java @@ -1,10 +1,12 @@ package com.petshop.backend.controller; import com.petshop.backend.entity.User; +import com.petshop.backend.exception.BusinessException; +import com.petshop.backend.exception.ResourceNotFoundException; import com.petshop.backend.repository.UserRepository; import com.petshop.backend.service.AvatarStorageService; +import com.petshop.backend.util.ImageValidationUtil; import org.springframework.core.io.Resource; -import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.DeleteMapping; @@ -15,6 +17,8 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.multipart.MultipartFile; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.HashMap; @@ -24,6 +28,8 @@ import java.util.Map; @RequestMapping("/api/v1/users") public class UserAvatarController { + private static final Logger log = LoggerFactory.getLogger(UserAvatarController.class); + private final UserRepository userRepository; private final AvatarStorageService avatarStorageService; @@ -53,29 +59,10 @@ public class UserAvatarController { @PostMapping("/{userId}/avatar") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity uploadUserAvatar(@PathVariable Long userId, @RequestParam("avatar") MultipartFile file) { - User user = userRepository.findById(userId).orElse(null); - if (user == null) { - return ResponseEntity.notFound().build(); - } + User user = userRepository.findById(userId) + .orElseThrow(() -> new ResourceNotFoundException("User not found with id: " + userId)); - if (file.isEmpty()) { - Map error = new HashMap<>(); - error.put("message", "Please select a file to upload"); - return ResponseEntity.badRequest().body(error); - } - - if (file.getSize() > 5 * 1024 * 1024) { - Map error = new HashMap<>(); - error.put("message", "File size must not exceed 5MB"); - return ResponseEntity.badRequest().body(error); - } - - String contentType = file.getContentType(); - if (contentType == null || (!contentType.equals("image/jpeg") && !contentType.equals("image/png") && !contentType.equals("image/gif"))) { - Map error = new HashMap<>(); - error.put("message", "Only JPG, PNG, and GIF images are allowed"); - return ResponseEntity.badRequest().body(error); - } + ImageValidationUtil.validate(file); try { avatarStorageService.deleteAvatar(user); @@ -88,31 +75,26 @@ public class UserAvatarController { result.put("message", "Avatar uploaded successfully"); return ResponseEntity.ok(result); } catch (IOException e) { - Map error = new HashMap<>(); - error.put("message", "Failed to upload avatar: " + e.getMessage()); - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(error); + throw new BusinessException("Failed to upload avatar: " + e.getMessage()); } } @DeleteMapping("/{userId}/avatar") @PreAuthorize("hasRole('ADMIN')") public ResponseEntity deleteUserAvatar(@PathVariable Long userId) { - User user = userRepository.findById(userId).orElse(null); - if (user == null) { - return ResponseEntity.notFound().build(); - } + User user = userRepository.findById(userId) + .orElseThrow(() -> new ResourceNotFoundException("User not found with id: " + userId)); try { avatarStorageService.deleteAvatar(user); - user.setAvatarUrl(null); - userRepository.save(user); - Map result = new HashMap<>(); - result.put("message", "Avatar removed successfully"); - return ResponseEntity.ok(result); } catch (IOException e) { - Map error = new HashMap<>(); - error.put("message", "Failed to remove avatar: " + e.getMessage()); - return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(error); + log.warn("Failed to delete avatar for user {}: {}", userId, e.getMessage()); } + user.setAvatarUrl(null); + userRepository.save(user); + + Map result = new HashMap<>(); + result.put("message", "Avatar removed successfully"); + return ResponseEntity.ok(result); } } diff --git a/backend/src/main/java/com/petshop/backend/service/CouponService.java b/backend/src/main/java/com/petshop/backend/service/CouponService.java index eb3b605d..4cb9f295 100644 --- a/backend/src/main/java/com/petshop/backend/service/CouponService.java +++ b/backend/src/main/java/com/petshop/backend/service/CouponService.java @@ -126,8 +126,8 @@ public class CouponService { BigDecimal discount = BigDecimal.ZERO; String type = coupon.getDiscountType(); if (isPercentageType(type)) { - discount = subtotal.multiply(coupon.getDiscountValue()) - .divide(BigDecimal.valueOf(100), 2, RoundingMode.HALF_UP); + discount = subtotal.multiply(coupon.getDiscountValue() + .divide(new BigDecimal("100"), 4, RoundingMode.HALF_UP)); } else if (isFixedType(type)) { discount = coupon.getDiscountValue(); } diff --git a/backend/src/main/java/com/petshop/backend/util/ImageValidationUtil.java b/backend/src/main/java/com/petshop/backend/util/ImageValidationUtil.java index 30214fb4..4ab26d2b 100644 --- a/backend/src/main/java/com/petshop/backend/util/ImageValidationUtil.java +++ b/backend/src/main/java/com/petshop/backend/util/ImageValidationUtil.java @@ -7,15 +7,19 @@ import java.util.Locale; public final class ImageValidationUtil { - public static final long MAX_IMAGE_SIZE = 5 * 1024 * 1024; + public static final long DEFAULT_MAX_IMAGE_SIZE = 5 * 1024 * 1024; private ImageValidationUtil() {} public static void validate(MultipartFile file) { + validate(file, DEFAULT_MAX_IMAGE_SIZE); + } + + public static void validate(MultipartFile file, long maxSizeBytes) { if (file.isEmpty()) { throw new BusinessException("Please select an image to upload"); } - if (file.getSize() > MAX_IMAGE_SIZE) { + if (file.getSize() > maxSizeBytes) { throw new BusinessException("Image file size must be less than 5MB"); } String contentType = file.getContentType();