OBPIH-6037 Add bin location data to expiration reports by drodzewicz · Pull Request #4468 · openboxes/openboxes

drodzewicz · 2024-01-22T13:26:29Z

No description provided.

…ed stock csv reports

…and without binlocation included - refactor controller action for expiring stock to include binLocation if such parameter is passed - refactor controller action for expired stock to include binLocation if such parameter is passed - refactored listExpiredStock gsp to handle new data model - refactored listExpiringStock gsp to handle new data model

kchelstowski · 2024-01-23T12:14:50Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }
+        data = data.findAll { inventoryItems.contains(it.inventoryItem) }


Can't we do this filtering in the PA query?
I think we could just pass the inventoryItems (or inventoryItems.id) to getQuantityByBinLocation (or the second method as well) and just add the IN :inventoryItems clause?

This could additionally improve the performance.

And a suggestion (regarding what version you prefer):

List<Map> data = withBinLocation ? productAvailabilityService.getQuantityOnHandByBinLocation(location) : productAvailabilityService.getQuantityOnHandByInventoryItem(location) .collect{ key, val -> [ inventoryItem: key, quantity: val ] }

Sure I can do it.
I just didn't want to modify the old service methods for that but it probably is worth changing it.
I was following the old logic for getting the expired or expiring inventoryItems and filtering the PA by that but I agree, this should probably be done in the db.

One thing to mention is that we should probably have this type of logic in a more central service, like ProductAvailabilityService. The DashboardService was created before our product availability and should probably be phased out or refactored (i.e. it should delegate to product availability service and other reporting services).

kchelstowski · 2024-01-23T12:17:33Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }
+        data = data.findAll { inventoryItems.contains(it.inventoryItem) }


this seems to be a repetition of what is done in the listExpiredStock. Maybe we could create a private method in this controller (as you know me - I'd prefer to have that type of logic in the service, but no must, since this probably needs to be refactored anyway, anytime soon).

alannadolny · 2024-01-23T12:11:10Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        Location location = Location.get(session.warehouse.id)
+        Category categorySelected = Category.get(params.category)


Shouldn't we use data services since we are on Grails 3?

Let's come up with our conventions for data services before we go lock, stock, and barrel. But what exactly would be changing here?

Also, what is going to happen if params.category is null here?

And just for giggles, we could use Location, Category, Boolean as arguments to this action and be done with the data binding.

@jmiranda in general the difference is that data service's read operations are run in a readOnly transaction.
I once mentioned to the guys, that I needed to do a research about what happens if we are in a service (marked as @Transactional) and we'd run a method from data service that is readOnly - if the outer (service's) transaction would "swallow" that, hence the fetch would be done using our outer transaction, or if it would set our outer transaction as readOnly for a while.
In the end I think this readOnly doesn't matter at this point, because as soon as we run the:

categoryDataService.get(params.category)

the readOnly transaction would commit at this point (so in further lines we could still update a category).

Also, what is going to happen if params.category is null here?

The data service's get evaluates to the basic Domain.get anyway, hence it'd return null if an entity didn't exist.

I think the data services methods matter much, if we do not take care of a transaction lifecycle on our own, because in the end, they evaluate to basic domain methods like Domain.get(...), Domain.delete(...), Domain.save(...), so I don't have a strong opinion about forcing to use data services for such simple cases (if we do take care of transaction boundaries on our own).
I find the data services very helpful and powerful for stuff like fetch join query, etc. where it has helped us a lot already.

alannadolny · 2024-01-23T12:12:36Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        if (withBinLocation) {
+            data = productAvailabilityService.getQuantityOnHandByBinLocation(location)
+        } else {
+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }


Can't it be just written using a ternary operator?

sure, I can.
I see you guys really hate if/else statements... 😅
I feel like simple if/else most of the time is much more readable than a ternary.
Ternary is great for very short statements and this case kind of applies to that, although I was also considering the case where in the future we want to add another parameter which would call another service method and ternary in this case would be very bad.

But yeah, I can change it to ternary.

from my perspective it was only a suggestion for you to decide, which approach you prefer there. I want everyone to feel comfortable with their code style, since your version is not "breaking" any rules.

No @kchelstowski that is not how this works. It's ternary or nothing!!!!

Just kidding. I don't feel strongly either way. I do feel strongly that the code should be refactored and probably moved to the service layer and that the getExpiredStock should probably handle all or most of the work. But we can save that for another day in case it'll be too much work.

alannadolny · 2024-01-23T12:13:14Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        Location location = Location.get(session.warehouse.id)
+        Category category = Category.get(params.category)


The same as above

alannadolny · 2024-01-23T12:13:25Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        if (withBinLocation) {
+            data = productAvailabilityService.getQuantityOnHandByBinLocation(location)
+        } else {
+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }


the same as above

alannadolny · 2024-01-23T12:14:43Z

grails-app/views/inventory/listExpiredStock.gsp

+													<g:checkBox id="${dataEntry.inventoryItem?.id }"
+																name="inventoryItem.id"
+																class="checkbox"
+																checked="${false }"


Suggested change

checked="${false }"

checked="${false}"

alannadolny · 2024-01-23T12:16:34Z

grails-app/views/inventory/listExpiredStock.gsp

 							&nbsp; <g:message code="default.button.downloadAsCSV.label" default="Download as CSV"/>
 						</g:link>
+
+						<g:link params="[format:'csv',category: params.category, withBinLocation: true]" controller="inventory" action="listExpiredStock" class="button">


Suggested change

<g:link params="[format:'csv',category: params.category, withBinLocation: true]" controller="inventory" action="listExpiredStock" class="button">

<g:link params="[format: 'csv',category: params.category, withBinLocation: true]" controller="inventory" action="listExpiredStock" class="button">

+ split into new lines

alannadolny · 2024-01-23T12:17:16Z

grails-app/views/inventory/listExpiredStock.gsp

 						</div>

-						<g:link params="[format:'csv',category:params.category]" controller="${controllerName}" action="${actionName}" class="button">
+						<g:link params="[format:'csv',category: params.category]" controller="inventory" action="listExpiredStock" class="button">


there are a lot of params, maybe it would be more readable if we split those into new lines?

jmiranda · 2024-01-24T06:18:51Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        Location location = Location.get(session.warehouse.id)
+        Category categorySelected = Category.get(params.category)


Let's come up with our conventions for data services before we go lock, stock, and barrel. But what exactly would be changing here?

Also, what is going to happen if params.category is null here?

And just for giggles, we could use Location, Category, Boolean as arguments to this action and be done with the data binding.

jmiranda · 2024-01-24T06:20:06Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        if (withBinLocation) {
+            data = productAvailabilityService.getQuantityOnHandByBinLocation(location)
+        } else {
+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }


No @kchelstowski that is not how this works. It's ternary or nothing!!!!

jmiranda · 2024-01-24T06:22:23Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        if (withBinLocation) {
+            data = productAvailabilityService.getQuantityOnHandByBinLocation(location)
+        } else {
+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }


Just kidding. I don't feel strongly either way. I do feel strongly that the code should be refactored and probably moved to the service layer and that the getExpiredStock should probably handle all or most of the work. But we can save that for another day in case it'll be too much work.

jmiranda · 2024-01-24T06:25:07Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+            data = productAvailabilityService.getQuantityOnHandByInventoryItem(location)
+                    .collect{ key, val -> [ inventoryItem: key, quantity: val ] }
        }
+        data = data.findAll { inventoryItems.contains(it.inventoryItem) }


One thing to mention is that we should probably have this type of logic in a more central service, like ProductAvailabilityService. The DashboardService was created before our product availability and should probably be phased out or refactored (i.e. it should delegate to product availability service and other reporting services).

jmiranda · 2024-01-24T06:25:56Z

grails-app/controllers/org/pih/warehouse/inventory/InventoryController.groovy

+        List<Category> categories = inventoryItems?.collect { it.product.category }?.unique()
+
+        List<Map> data
+        if (withBinLocation) {


I haven't read the ticket, but is there still a case where we don't need or want the bin location data?

…ndByBinLocation and getQuantityOnHandByInventoryItem

kchelstowski · 2024-01-26T09:05:21Z

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

+                groupProperty("inventoryItem", "inventoryItem")
+                groupProperty("binLocation", "binLocation")
+                sum("quantityOnHand", "quantityOnHand")
+                sum("quantityAvailableToPromise", "quantityAvailableToPromise")


have you made sure this would eventually return 0? I'm asking because of this:

sum(case when pa.quantityAvailableToPromise > 0 then pa.quantityAvailableToPromise else 0 end)

a function that transforms this result collectQuantityOnHandByBinLocation handles the logic for setting a 0 for null values, but does not handle negative values...
Thanks for pointing this out, I think I am going to have to revert this to a simple SQL because it is quite hard to implement this logic with CASE WHEN in the criteria

kchelstowski · 2024-01-26T09:06:43Z

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

-                quantityMap[it[0]] = it[1]
+    Map<InventoryItem, Integer> getQuantityOnHandByInventoryItem(Location location, List<InventoryItem> inventoryItems = []) {
+        if (!location) {
+            throw new RuntimeException("Must specify location in order to get quantity on hand")


was there a reason you refactored this part to throw an exception? Maybe there is a case where we'd still want to proceed with e.g. refresh PA, and by doing so, you'd break that operation.

From what I have checked it seems we always require a location, but to be on the safe side I think I will revert it to returning an empty array instead of throwing an Exception.

kchelstowski · 2024-01-26T15:14:27Z

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

-                quantityMap[it[0]] = it[1]
+    Map<InventoryItem, Integer> getQuantityOnHandByInventoryItem(Location location, List<InventoryItem> inventoryItems = []) {
+        if (!location) {
+            return []


this method by default was returning an empty Map, not a List - even the returned type suggests it should be the Map.
I think this would not work as expected with the list.

thanks, there was a similar case that returns a list so I messed it up.

- return empty lists instead of throwing an Exception when location is not provided - bring back previous SQL query for QOHByBinLocation

drodzewicz added 2 commits January 18, 2024 15:33

OBPIH-6037 Remove bin Location column from regular expiring and expir…

d83202a

…ed stock csv reports

drodzewicz requested review from alannadolny, awalkowiak, jmiranda and kchelstowski January 22, 2024 13:26

drodzewicz self-assigned this Jan 22, 2024

drodzewicz changed the title ~~Obpih 6037 add bin location data to expiration reports~~ OBPIH-6037 Add bin location data to expiration reports Jan 22, 2024

kchelstowski reviewed Jan 23, 2024

View reviewed changes

alannadolny reviewed Jan 23, 2024

View reviewed changes

jmiranda reviewed Jan 24, 2024

View reviewed changes

drodzewicz force-pushed the OBPIH-6037-add-bin-location-data-to-expiration-reports branch from be54b3b to fbac56d Compare January 25, 2024 11:55

OBPIH-6037 invlude inventoryItem list as an option in getQuantityOnHa…

bd4767c

…ndByBinLocation and getQuantityOnHandByInventoryItem

drodzewicz force-pushed the OBPIH-6037-add-bin-location-data-to-expiration-reports branch from fbac56d to bd4767c Compare January 25, 2024 17:04

kchelstowski reviewed Jan 26, 2024

View reviewed changes

kchelstowski requested changes Jan 26, 2024

View reviewed changes

OBPIH-6037 Fixes after review

3474233

- return empty lists instead of throwing an Exception when location is not provided - bring back previous SQL query for QOHByBinLocation

drodzewicz force-pushed the OBPIH-6037-add-bin-location-data-to-expiration-reports branch from eaa29ba to 3474233 Compare January 26, 2024 15:20

kchelstowski approved these changes Jan 26, 2024

View reviewed changes

awalkowiak merged commit 9034da3 into feature/upgrade-to-grails-3.3.10 Feb 2, 2024

awalkowiak deleted the OBPIH-6037-add-bin-location-data-to-expiration-reports branch February 2, 2024 11:46

		Location location = Location.get(session.warehouse.id)
		Category categorySelected = Category.get(params.category)

		Location location = Location.get(session.warehouse.id)
		Category category = Category.get(params.category)

	<g:link params="[format:'csv',category: params.category, withBinLocation: true]" controller="inventory" action="listExpiredStock" class="button">
	<g:link params="[format: 'csv',category: params.category, withBinLocation: true]" controller="inventory" action="listExpiredStock" class="button">

Uh oh!

Conversation

drodzewicz commented Jan 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchelstowski Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kchelstowski Jan 24, 2024 •

edited

Loading