-
Notifications
You must be signed in to change notification settings - Fork 416
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
Update "create checkout" follows new design #1224
base: main
Are you sure you want to change the base?
Conversation
Order Coverage Report
|
Customer Coverage Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong mapping for taxAmount
Quality Gate passed for 'order'Issues Measures |
Quality Gate passed for 'customer'Issues Measures |
} | ||
|
||
public void addAmount(double a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there are 2 addAmount
methods? Please remove the unused one
} | ||
|
||
public void subtractAmount(BigDecimal a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not used anywhere
} else { | ||
t.setProductName(product.getName()); | ||
t.setProductPrice(BigDecimal.valueOf(product.getPrice())); | ||
checkout.addAmount(product.getPrice() * t.getQuantity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace product.getPrice() * t.getQuantity()
with BigDecimal operations to prevent precision errors
} | ||
|
||
public CheckoutVm getCheckoutPendingStateWithItemsById(String id) { | ||
|
||
Checkout checkout = checkoutRepository.findByIdAndCheckoutState(id, CheckoutState.PENDING).orElseThrow(() | ||
-> new NotFoundException(CHECKOUT_NOT_FOUND, id)); | ||
-> new NotFoundException(CHECKOUT_NOT_FOUND, id)); | ||
|
||
if (!checkout.getCreatedBy().equals(AuthenticationUtils.getCurrentUserId())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an ADMIN user, if I am not the user who created the checkout, would I be able to call this method?
Please extract this validation to a separate method
checkoutItems.forEach(t -> { | ||
ProductCheckoutListVm product = products.get(t.getProductId()); | ||
if (product == null) { | ||
throw new NotFoundException("PRODUCT_NOT_FOUND", t.getProductId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please define a constant for "PRODUCT_NOT_FOUND"
|
||
return checkoutVm.toBuilder().checkoutItemVms(checkoutItemVms).build(); | ||
} | ||
|
||
public Long updateCheckoutStatus(CheckoutStatusPutVm checkoutStatusPutVm) { | ||
Checkout checkout = checkoutRepository.findById(checkoutStatusPutVm.checkoutId()) | ||
.orElseThrow(() -> new NotFoundException(CHECKOUT_NOT_FOUND, checkoutStatusPutVm.checkoutId())); | ||
.orElseThrow(() -> new NotFoundException(CHECKOUT_NOT_FOUND, checkoutStatusPutVm.checkoutId())); | ||
checkout.setCheckoutState(CheckoutState.valueOf(checkoutStatusPutVm.checkoutStatus())); | ||
checkoutRepository.save(checkout); | ||
Order order = orderService.findOrderByCheckoutId(checkoutStatusPutVm.checkoutId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use logging to track key operations like checkout creation or status update, etc.
import com.yas.order.config.ServiceUrlConfig; | ||
import com.yas.order.utils.AuthenticationUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use com.yas.commonlibrary.utils.AuthenticationUtils
.toUri(); | ||
.fromHttpUrl(serviceUrlConfig.product()) | ||
.path("/backoffice/product-variations/" + productId) | ||
.buildAndExpand() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this to append path variables instead of .path("/backoffice/product-variations/" + productId)
final URI url = UriComponentsBuilder
.fromHttpUrl(serviceUrlConfig.product())
.path("/backoffice/product-variations/${productId}")
.buildAndExpand(productId)
.toUri();
private final OrderService orderService; | ||
private final ProductService productService; | ||
private final CheckoutMapper checkoutMapper; | ||
|
||
public CheckoutVm createCheckout(CheckoutPostVm checkoutPostVm) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi a @minhtridn2001, @NhatTranMinh15,
Do you think this method should be named initCheckout(...)
? It seems to handle the case when the user initiates the checkout.
If this method is meant to create a checkout
, it should handle more cases than just the PENDING
status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, should we check if the checkout
already exists (by verifying the checkout status
and checkout items
) instead of creating a new one each time the user clicks Proceed to checkout, especially when they might click it multiple times with the same items?
Ref: #1105