-
Notifications
You must be signed in to change notification settings - Fork 417
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
Issue #1168 storefront only get product added to warehouse #1246
base: main
Are you sure you want to change the base?
Issue #1168 storefront only get product added to warehouse #1246
Conversation
Inventory Coverage Report
|
Quality Gate passed for 'storefront'Issues Measures |
Quality Gate passed for 'inventory'Issues Measures |
Quality Gate passed for 'product'Issues Measures |
Product 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 @DinhThaiVV,
Kindly check my comments. There are a few points that may need discussion. Let me know if you have any inquiries.
Thanks!
Page<Product> getFeaturedProduct(Pageable pageable); | ||
+ "AND p.isVisibleIndividually = TRUE " | ||
+ "AND p.isPublished = TRUE " | ||
+ "AND p.id IN (:productIds) " |
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 move p.id IN (:productIds)
to the top condition to utilize the index on the id
column.
Passing a large list of productIds
in the IN clause may result in a query limit error.
Consider applying batching here.
|
||
@Query(value = "SELECT p FROM Product p LEFT JOIN p.productCategories pc LEFT JOIN pc.category c " | ||
@Query(value = "SELECT distinct p FROM Product p LEFT JOIN p.productCategories pc LEFT JOIN pc.category c " | ||
+ "WHERE LOWER(p.name) LIKE %:productName% " |
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 move the indexed condition id
to the top.
Do we have an index on the product name? If so, using LOWER(p.name) may prevent the index use.
List<ProductThumbnailGetVm> productThumbnailVms = new ArrayList<>(); | ||
Page<Product> productPage = productRepository.getFeaturedProduct(pageable); | ||
List<Long> productIds = inventoryService.getProductIdsAddedWarehouse(); |
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 current approach of fetching all available products in real-time should work. However, I don't like this approach so much as it impacts the responsibility if the number of products or requests is high. Please consider caching or an event-driven approach.
@@ -875,11 +862,14 @@ public ProductsGetVm getProductsByMultiQuery( | |||
Double endPrice | |||
) { | |||
Pageable pageable = PageRequest.of(pageNo, pageSize); | |||
List<Long> productIds = inventoryService.getProductIdsAddedWarehouse(); | |||
if (productIds.isEmpty()) |
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 add the block {} for this if.
@@ -16,4 +16,7 @@ List<Stock> findByWarehouseIdAndProductIdIn(Long warehouseId, | |||
List<Long> productIds); | |||
|
|||
boolean existsByWarehouseIdAndProductId(Long warehouseId, Long productId); | |||
|
|||
@Query("SELECT distinct s.productId FROM Stock s") | |||
List<Long> findProductIdsAddedWarehouse(); |
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.
Do we have cases where quantity = 0? Those records should be excluded, right?
findProductIdsInStock
may be a better name.
Issue #1168