Update "create checkout" follows new design #1224
Conversation
Order Coverage Report
|
Customer Coverage Report
|
There was a problem hiding this comment.
Hi @NhatTranMinh15,
Thanks for your effort 👍. Please check my comments, and let’s discuss if you have any inquiries.
Thanks.
| @Mapping(target = "totalAmount", source = "totalAmount") // Ánh xạ tường minh cho totalAmount | ||
| @Mapping(target = "totalDiscountAmount", source = "totalDiscountAmount") | ||
| // @Mapping(target = "totalAmount", source = "totalAmount") // Ánh xạ tường minh cho totalAmount | ||
| // @Mapping(target = "totalDiscountAmount", source = "totalDiscountAmount") |
There was a problem hiding this comment.
Remove these comments
| @@ -65,7 +73,8 @@ public class Checkout extends AbstractAuditEntity { | |||
| private String attributes; | |||
|
|
|||
| @SuppressWarnings("unused") | |||
| private BigDecimal totalAmount; | |||
| @Builder.Default | |||
| private long totalAmount = 0; | |||
There was a problem hiding this comment.
Why is BigDecimal not used for totalAmount?
@Builder.Default is unnecessary here because primitive types like long already have default values
|
|
||
| CheckoutVm checkoutVm = checkoutMapper.toVm(savedCheckout); | ||
| if (CollectionUtils.isEmpty(checkoutPostVm.checkoutItemPostVms())) { |
There was a problem hiding this comment.
Isn't checkoutItems required? It should result in a bad request if checkoutItems is empty
|
|
||
| ProductGetCheckoutListVm response = productService.getProductInfomation(productIds, 0, productIds.size()); | ||
|
|
||
| if (response != null) { |
There was a problem hiding this comment.
Please log an error message and throw an exception in case response is null.
|
|
||
| if (response != null) { | ||
| Map<Long, ProductCheckoutListVm> products | ||
| = response.productCheckoutListVms() |
There was a problem hiding this comment.
Please separate this part into a method.
| .stream() | ||
| .map(checkoutItemPostVm -> { | ||
| CheckoutItem item = checkoutMapper.toModel(checkoutItemPostVm); | ||
| checkout.addAmount(item.getQuantity()); |
There was a problem hiding this comment.
Wrong mapping?
amount = price * quantity
| @@ -76,20 +100,20 @@ public CheckoutVm getCheckoutPendingStateWithItemsById(String id) { | |||
| -> new NotFoundException(CHECKOUT_NOT_FOUND, id)); | |||
|
|
|||
| if (!checkout.getCreatedBy().equals(AuthenticationUtils.getCurrentUserId())) { | |||
| throw new Forbidden(Constants.ErrorCode.FORBIDDEN); | |||
There was a problem hiding this comment.
The ADMIN role should be permitted also. Let's create a helper method for this since it’s reusable
Please log a message for additional error insights.
| @Retry(name = "restApi") | ||
| @CircuitBreaker(name = "restCircuitBreaker", fallbackMethod = "handleProductInfomationFallback") | ||
| public ProductGetCheckoutListVm getProductInfomation(Set<Long> ids, int pageNo, int pageSize) { | ||
| final String jwt = ((Jwt) SecurityContextHolder.getContext().getAuthentication().getPrincipal()) |
There was a problem hiding this comment.
Please use the helper method to get the jwt token. If the method doesn’t exist, create one in the common library
| @Column(name = "tax") | ||
| private BigDecimal taxAmount; | ||
| @SuppressWarnings("unused") | ||
| private Long tax; |
There was a problem hiding this comment.
Tax amount should be of type BigDecimal.
Please keep using BigDecimal taxAmount;
| if (product != null) { | ||
| t.setProductName(product.getName()); | ||
| t.setProductPrice(BigDecimal.valueOf(product.getPrice())); | ||
| t.setTax(product.getTaxClassId()); |
There was a problem hiding this comment.
Wrong mapping for taxAmount
|
|
Ref: #1105