OBPIH-5616 Improve calculating product availability for picklist items #4005

awalkowiak · 2023-04-25T14:07:37Z

No description provided.

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

grails-app/services/org/pih/warehouse/inventory/InventoryService.groovy

drodzewicz · 2023-04-26T13:23:47Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -1100,7 +1102,7 @@ class StockMovementService {
 // Save requisition item before PA refresh
 requisitionItem.save(flush: true)

- productAvailabilityService.refreshProductsAvailability(requisitionItem?.requisition?.origin?.id, [requisitionItem?.product?.id], false)
+ productAvailabilityService.refreshProductsAvailability(requisitionItem?.requisition?.origin?.id, [requisitionItem?.product?.id], binLocations?.unique(), false)


I am wondering if the binLocations List shouldn't be a Set instead. That way it would eliminate the need for performing a binLocations?.unique(), but I am unsure about the performance implications of the Set vs List. I see some people mentioning on stack overflow that HashSets are not as well optimised while other say that it is preferable in this kind of a situation

I think that's a fair assessment. The performance implications between set / list.unique will be negligible compared to the time spent doing anything with the database so it would probably be better to handle this properly with a set.

awalkowiak · 2023-04-27T16:18:14Z

(converted back to draft, because there will be one more improvement included)

…ility refresh

jmiranda · 2023-04-30T03:55:50Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ }
+ }
+
+ productIds.each { String productId ->


You can use a Product.getAll(productIds) here.

jmiranda · 2023-04-30T03:59:50Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ log.info "Refreshing product availability location ${location}, product ${product}, binLocations ${binLocations}, forceRefresh ${forceRefresh}..."
+ def startTime = System.currentTimeMillis()
+ List calculatedBinLocations = calculateBinLocations(location, product, binLocations)
+ // FIXME: if this deadlocks, it safe to auto-retry here?


Probably not. Are you seeing a lot of deadlocks here? If so, we should investigate.

@jmiranda this was just copy paste from similar method above

jmiranda · 2023-04-30T04:00:42Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ FROM location_supported_activities lsa_select
+ GROUP BY lsa_select.location_id
+ ) lsa ON lsa.location_id = l.id
+ WHERE (r.origin_id = :locationId AND r.status IN (${RequisitionStatus.listPending().collect { "'$it'" }.join(',')}))


Why can't you pass this in as a query parameter?

@jmiranda Because it was not handled (parsed to string) properly for sql to work as expected

jmiranda · 2023-04-30T04:10:01Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

-
- return results.collect { [binLocation: it[0]?.id, inventoryItem: it[1]?.id, quantityAllocated: it[2]] }
+ def query = """
+ SELECT bin_location_id, inventory_item_id, SUM(quantity) FROM (


As I mentioned on the call the other day, we should include product and location in the select even though they are passed in as query parameters. There's a small chance that might lead to slower performance if those columns aren't in a covering index but I don't suspect that's the case. Also, the inner (raw query) should not include the sum / group by if we can help it. It's usually better to leave the group by until the end i.e. outer query. I could be wrong about this, but it feels wrong to apply aggregation inside a nested query if you're going to do the same on the outside.

Also I see we're getting rid of the weird count + count / count(*). I think that's safe since we're simplifying the query by breaking into two separate queries and unioning the results. But we should try to understand and/or compare results from the previous query so we know we're getting the same results.

jmiranda · 2023-04-30T04:14:42Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ sum(pli.quantity) as quantity
+ FROM picklist_item pli
+ INNER JOIN picklist p ON pli.picklist_id = p.id
+ LEFT JOIN requisition_item ri ON pli.requisition_item_id = ri.id


In my opinion, we don't need to use LEFT JOIN any longer. That was one of the reasons why I wanted to do a UNION here. I think location (bin location) is the only table that needs to be LEFT JOIN'd.

jmiranda · 2023-04-30T04:15:36Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ SELECT
+ GROUP_CONCAT(lsa_select.supported_activities_string) as activities,
+ lsa_select.location_id as location_id
+ FROM location_supported_activities lsa_select


Remember we want to check both location and location type. Location supported overrides location type supported activities. If that's too difficult, then we can save that for another time.

jmiranda · 2023-04-30T04:18:08Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ GROUP BY bin_location_id, inventory_item_id;
+ """
+
+ def results = dataService.executeQuery(


If you can't figure out the issue where, try using Hibernate to execute the query instead of the Groovy SQL client we're using.

https://dzone.com/articles/grails-goodness-using

jmiranda · 2023-04-30T04:22:54Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -1100,7 +1102,7 @@ class StockMovementService {
 // Save requisition item before PA refresh
 requisitionItem.save(flush: true)

- productAvailabilityService.refreshProductsAvailability(requisitionItem?.requisition?.origin?.id, [requisitionItem?.product?.id], false)
+ productAvailabilityService.refreshProductsAvailability(requisitionItem?.requisition?.origin?.id, [requisitionItem?.product?.id], binLocations?.unique(), false)


I think that's a fair assessment. The performance implications between set / list.unique will be negligible compared to the time spent doing anything with the database so it would probably be better to handle this properly with a set.

jmiranda · 2023-04-30T04:23:56Z

grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy

@@ -2117,13 +2120,14 @@ class StockMovementService {
 picklistItem.disableRefresh = Boolean.TRUE
 picklistItem.picklist?.removeFromPicklistItems(picklistItem)
 picklistItem.requisitionItem?.removeFromPicklistItems(picklistItem)
+ binLocations.add(picklistItems?.binLocation?.id)


Should this be picklistItem?.binLocation?.id

This was causing the following stacktrace

2023-05-11 15:03:36,185 [http-8080-4] INFO filters.AccessLogFilters - stockMovementApi.reviseItems: [user:jmiranda, location:Distribution Center] 2023-05-11 15:03:36,193 [http-8080-4] INFO filters.AccessLogFilters - No rule for stockMovementApi:reviseItems -> allow anonymous 2023-05-11 15:03:36,198 [http-8080-4] INFO core.UserService - Is role ROLE_FINANCE in [ROLE_MANAGER, ROLE_AUTHENTICATED, ROLE_BROWSER, ROLE_ADMIN, ROLE_SUPERUSER, ROLE_ASSISTANT] = false 2023-05-11 15:03:36,198 [http-8080-4] INFO core.UserService - Is role ROLE_SUPERUSER in [ROLE_MANAGER, ROLE_AUTHENTICATED, ROLE_BROWSER, ROLE_ADMIN, ROLE_SUPERUSER, ROLE_ASSISTANT] = true 2023-05-11 15:03:36,266 [http-8080-4] INFO inventory.StockMovementService - Shipment workflow Stock movement shipment workflow 2023-05-11 15:03:36,385 [http-8080-4] INFO api.StockMovementApiController - binding lineItems: [[id:ff808181880c4df001880c53118d0003, reasonCode:LOW_STOCK, quantityRevised:1]] 2023-05-11 15:03:36,399 [http-8080-4] INFO inventory.StockMovementService - Removing previous changes, picklists and shipments, if present 2023-05-11 15:03:36,527 [http-8080-4] INFO inventory.ProductAvailabilityService - refreshProductsAvailability: 8a8a9e9665c4f59d0165c54ec6b10027, [10001], [[8a8a9e96687c94ce0168bab1c5a8285c]], false 2023-05-11 15:03:36,532 [pool-2-thread-1] INFO inventory.RefreshProductAvailabilityEventService - Application event been disabled by event publisher 2023-05-11 15:03:36,532 [pool-2-thread-1] INFO inventory.RefreshProductAvailabilityEventService - Application event [disableRefresh:true, forceRefresh:false, class:class org.pih.warehouse.inventory.RefreshProductAvailabilityEvent, locationId:8a8a9e9665c4f59d0165c54ec6b10027, productIds:[10001], synchronousRequired:false, timestamp:1683835416503, metaClass:org.codehaus.groovy.runtime.HandleMetaClass@4ba20442[groovy.lang.ExpandoMetaClass@4ba20442[class org.pih.warehouse.inventory.RefreshProductAvailabilityEvent]], source:org.pih.warehouse.picklist.PicklistItem : ff808181880c4df001880c59fde2000d] 2023-05-11 15:03:36,549 [http-8080-4] ERROR errors.GrailsExceptionResolver - Exception occurred when processing request: [POST] /openboxes/api/stockMovements/ff808181880c4df001880c528d670001/reviseItems Stacktrace follows: org.hibernate.TypeMismatchException: Provided id of the wrong type for class org.pih.warehouse.core.Location. Expected: class java.lang.String, got class java.util.ArrayList at org.pih.warehouse.inventory.ProductAvailabilityService$_refreshProductsAvailability_closure3.doCall(ProductAvailabilityService.groovy:106) at org.pih.warehouse.inventory.ProductAvailabilityService.refreshProductsAvailability(ProductAvailabilityService.groovy:104) at org.pih.warehouse.inventory.ProductAvailabilityService$$FastClassByCGLIB$$7ee8fd31.invoke(<generated>) at net.sf.cglib.proxy.MethodProxy.invoke(MethodProxy.java:149) at org.pih.warehouse.inventory.ProductAvailabilityService$$EnhancerByCGLIB$$80269ed0.refreshProductsAvailability(<generated>) at org.pih.warehouse.inventory.ProductAvailabilityService$refreshProductsAvailability.call(Unknown Source) at org.pih.warehouse.inventory.StockMovementService.removeShipmentAndPicklistItemsForModifiedRequisitionItem(StockMovementService.groovy:2130) at org.pih.warehouse.inventory.StockMovementService$_reviseItems_closure65.doCall(StockMovementService.groovy:1944) at org.pih.warehouse.inventory.StockMovementService.reviseItems(StockMovementService.groovy:1934) at org.pih.warehouse.inventory.StockMovementService$$FastClassByCGLIB$$b65c0b56.invoke(<generated>) at net.sf.cglib.proxy.MethodProxy.invoke(MethodProxy.java:149) at org.pih.warehouse.inventory.StockMovementService$$EnhancerByCGLIB$$456057a7.reviseItems(<generated>) at org.pih.warehouse.inventory.StockMovementService$reviseItems.call(Unknown Source) at org.pih.warehouse.api.StockMovementApiController$_closure12.doCall(StockMovementApiController.groovy:265) at org.pih.warehouse.api.StockMovementApiController$_closure12.doCall(StockMovementApiController.groovy) at grails.plugin.springcache.web.GrailsFragmentCachingFilter.doFilter(GrailsFragmentCachingFilter.groovy:66) at net.sf.ehcache.constructs.web.filter.Filter.doFilter(Filter.java:86) at org.grails.plugin.resource.DevModeSanityFilter.doFilter(DevModeSanityFilter.groovy:44) at java.lang.Thread.run(Thread.java:748)

@jmiranda yes this should be picklistItem?.binLocation?.id (singular item)

jmiranda · 2023-04-30T04:25:35Z

grails-app/services/org/pih/warehouse/picklist/PicklistService.groovy

@@ -74,12 +75,13 @@ class PicklistService {
 it.disableRefresh = Boolean.TRUE
 picklist.removeFromPicklistItems(it)
 orderItem.removeFromPicklistItems(it)
+ binLocations.add(it.binLocation?.id)


Since we're not doing any filtering (like in cases above) we could probably just collect all of the bin locations before the findAll.

def binLocationIds = picklist?.picklistItems?.collect { it?.binLocation?.id }

…vailability refresh

…native SQL query API

jmiranda · 2023-05-15T20:27:07Z

@awalkowiak Please review when you get a chance.

kchelstowski · 2023-05-16T07:11:12Z

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy

+ .setString("locationId", location?.id)
+ .setString("productId", product?.id ?: '')
+ .setParameterList("pendingRequisitionStatuses", RequisitionStatus.listPending().collect { it.name() })
+ .setParameterList("pendingOrderStatuses", OrderStatus.listPending().collect { it.name() })


LGTM, just wondering if it would work without .collect here, since toString() returns name(), if Hibernate would convert that properly

awalkowiak · 2023-05-16T15:27:56Z

@jmiranda the code looks good, but I still want to test it out locally. Did you omit the location type activity codes check on purpose or you forgot about this? (I am not 100% sure if this is required)

awalkowiak · 2023-05-17T12:46:25Z

@jmiranda LGTM. I checked the "problematic" (refreshing product availability after generating picklist) part locally. It looks good. I cannot give the approval, since I created the PR, but I think it can be merged.

awalkowiak requested a review from jmiranda April 25, 2023 14:07

awalkowiak force-pushed the OBPIH-5616 branch from 3c10365 to 565a1fe Compare April 26, 2023 12:25

awalkowiak marked this pull request as ready for review April 26, 2023 12:30

awalkowiak force-pushed the OBPIH-5616 branch from 565a1fe to b02b6a2 Compare April 26, 2023 12:37

awalkowiak requested review from drodzewicz, alannadolny and kchelstowski April 26, 2023 12:39

alannadolny requested changes Apr 26, 2023

View reviewed changes

grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy Outdated Show resolved Hide resolved

kchelstowski reviewed Apr 26, 2023

View reviewed changes

grails-app/services/org/pih/warehouse/inventory/InventoryService.groovy Show resolved Hide resolved

awalkowiak force-pushed the OBPIH-5616 branch from b02b6a2 to c227ef3 Compare April 26, 2023 13:14

kchelstowski approved these changes Apr 26, 2023

View reviewed changes

drodzewicz reviewed Apr 26, 2023

View reviewed changes

alannadolny approved these changes Apr 26, 2023

View reviewed changes

awalkowiak marked this pull request as draft April 27, 2023 16:17

awalkowiak added 2 commits April 28, 2023 17:54

OBPIH-5616 Improve calculating product availability for picklist items

f7c4b7b

OBPIH-5616 Improve calculating quantity allocated for product availab…

585221a

…ility refresh

awalkowiak force-pushed the OBPIH-5616 branch from c227ef3 to 585221a Compare April 28, 2023 16:55

jmiranda requested changes Apr 30, 2023

View reviewed changes

drodzewicz closed this May 4, 2023

drodzewicz reopened this May 4, 2023

awalkowiak changed the base branch from release/0.8.22 to develop May 11, 2023 10:54

awalkowiak changed the base branch from develop to release/0.8.22-hotfix1 May 15, 2023 16:34

jmiranda added 2 commits May 15, 2023 15:04

OBPIH-5616 Fixed bug re: tracking bin locations requiring a product a…

8018193

…vailability refresh

OBPIH-5616 Refactored quantity picked calculation to use Hibernate's …

19da43e

…native SQL query API

jmiranda requested review from alannadolny, kchelstowski, drodzewicz and jmiranda May 15, 2023 20:27

jmiranda marked this pull request as ready for review May 15, 2023 20:27

kchelstowski approved these changes May 16, 2023

View reviewed changes

alannadolny approved these changes May 16, 2023

View reviewed changes

drodzewicz approved these changes May 16, 2023

View reviewed changes

awalkowiak merged commit 65b4bc7 into release/0.8.22-hotfix1 May 18, 2023
2 checks passed

awalkowiak deleted the OBPIH-5616 branch May 18, 2023 09:57

awalkowiak restored the OBPIH-5616 branch May 18, 2023 14:35

awalkowiak deleted the OBPIH-5616 branch May 18, 2023 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5616 Improve calculating product availability for picklist items #4005

OBPIH-5616 Improve calculating product availability for picklist items #4005

awalkowiak commented Apr 25, 2023

drodzewicz Apr 26, 2023

jmiranda Apr 30, 2023

awalkowiak commented Apr 27, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

awalkowiak May 12, 2023

jmiranda Apr 30, 2023

awalkowiak May 12, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

jmiranda Apr 30, 2023

jmiranda May 11, 2023

awalkowiak May 12, 2023

jmiranda Apr 30, 2023

jmiranda commented May 15, 2023

kchelstowski May 16, 2023

awalkowiak commented May 16, 2023

awalkowiak commented May 17, 2023

OBPIH-5616 Improve calculating product availability for picklist items #4005

OBPIH-5616 Improve calculating product availability for picklist items #4005

Conversation

awalkowiak commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalkowiak commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda commented May 15, 2023

Choose a reason for hiding this comment

awalkowiak commented May 16, 2023

awalkowiak commented May 17, 2023