OBGM-508 Improve product availability refresh performance by kchelstowski · Pull Request #4395 · openboxes/openboxes

kchelstowski · 2023-11-29T13:28:47Z

No description provided.

alannadolny · 2023-11-29T14:45:55Z

src/main/groovy/org/pih/warehouse/inventory/QuantityByBinLocationDto.groovy

+    int compareTo(QuantityByBinLocationDto obj) {
+        inventoryItem?.expirationDate <=> obj?.inventoryItem?.expirationDate
+                ?: binLocation?.name <=> obj?.binLocation?.name
+    }


I know that we had the same way of comparison previously, but I think we can add the id as a fallback, just to be sure that all of the objects are present. I remember Justin explaining what happening if we will have the same values in the compareTo methods: #4387 (comment)
I wrote the custom validator for the name method of location domain, and the names are unique only under the same parents, so they can be duplicated if they are under different parents, so there is a chance for disappearing.

Suggested change

int compareTo(QuantityByBinLocationDto obj) {

inventoryItem?.expirationDate <=> obj?.inventoryItem?.expirationDate

?: binLocation?.name <=> obj?.binLocation?.name

}

int compareTo(QuantityByBinLocationDto obj) {

inventoryItem?.expirationDate <=> obj?.inventoryItem?.expirationDate

?: binLocation?.name <=> obj?.binLocation?.name

?: id <=> obj?.id

}

I don't mind adding it, but it was only a copy paste from already existing (and working) code and I did not pay much attention to that.

Yeah it's a good point @drodzewicz . Whether it should be included in the PR is negotiable since (as @kchelstowski mentioned) this is just a copy-paste from the previous sort implementation.

If it's not included, then we should make sure we prioritize the following ticket and maybe a spike to test out the different implementations.
https://pihemr.atlassian.net/browse/OBGM-341

alannadolny · 2023-11-29T14:48:48Z

src/main/groovy/org/pih/warehouse/inventory/QuantityPickedByLocationAndProductDto.groovy

+class QuantityPickedByLocationAndProductDto {
+    Location binLocation
+    InventoryItem inventoryItem
+    Integer quantityAllocated


I know we were discussing this recently, but I am a little bit confused, because in the QuantityByBinLocationDto.groovy you made an empty line above each of the properties, but in this file, there are no empty lines, maybe we should decide what is the proper way of formatting it to be more consistent?

I made no empty lines here, to provoke the reaction 😆
Joking, just for the another dto, without empty lines it looked weird to me, and since there were only 3 properties and let it go, but I would also love to decide on the convention.

awalkowiak · 2024-01-25T18:43:03Z

@jmiranda I think this would be a time to revisit this one, and potentially merge to "let it sink in". I am going to re-review it too.

@kchelstowski in a spare time could you take a look at the conflict that is there?

kchelstowski · 2024-02-01T12:43:48Z

@awalkowiak conflicts resolved

awalkowiak

Overall LGTM, only the joins look kinda weird, but I think @kchelstowski explained it on the tech huddle.

awalkowiak · 2024-02-07T16:34:04Z

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

+        join fetch ${Transaction transaction = transactionEntry.transaction}
+        join fetch ${InventoryItem inventoryItem = transactionEntry.inventoryItem}
+        join fetch ${TransactionType transactionType = transaction.transactionType}
+        join fetch ${Product product = inventoryItem.product}
+        join fetch ${Category category = product.category}
+        left join fetch ${Product basicProduct = transactionEntry.product}
+        left join fetch ${Location binLocation = transactionEntry.binLocation}


@jmiranda I think that everything looks good to me, but this part is where I am not entirely sure about, so it would be good to get your review.

Yeah the syntax for the @query is new to me.

@kchelstowski Can you provide documentation where you found this syntax? I see one reference in the docs for the "join" (https://gorm.grails.org/latest/hibernate/manual/#_jpa_ql_queries) but nothing regarding the "fetch".

@jmiranda yeah, Grails' docs regarding the JPA-QL queries are not very extensive, hence I just used my knowledge from Spring to write this query and was just checking what SQL query is generated. But if you wanted to have some docs reference, this could be the one: https://docs.oracle.com/cd/E11035_01/kodo41/full/html/ejb3_langref.html#ejb3_langref_fetch_joins

I pasted the generated SQL in the confluence doc, but as a reminder, I'm pasting it here:

Hibernate: select transactio0_.id as id1_122_0_, transactio1_.id as id1_121_1_, transactio7_.id as id1_124_2_, inventoryi2_.id as id1_22_3_, product4_.id as id1_67_4_, category5_.id as id1_5_5_, location3_.id as id1_35_6_, product6_.id as id1_67_7_, transactio0_.version as version2_122_0_, transactio0_.product_id as product_3_122_0_, transactio0_.comments as comments4_122_0_, transactio0_.transaction_id as transact5_122_0_, transactio0_.quantity as quantity6_122_0_, transactio0_.bin_location_id as bin_loca7_122_0_, transactio0_.reason_code as reason_c8_122_0_, transactio0_.inventory_item_id as inventor9_122_0_, transactio1_.version as version2_121_1_, transactio1_.date_created as date_cre3_121_1_, transactio1_.transaction_number as transact4_121_1_, transactio1_.last_updated as last_upd5_121_1_, transactio1_.incoming_shipment_id as incoming6_121_1_, transactio1_.updated_by_id as updated_7_121_1_, transactio1_.order_id as order_id8_121_1_, transactio1_.receipt_id as receipt_9_121_1_, transactio1_.date_confirmed as date_co10_121_1_, transactio1_.confirmed_by_id as confirm11_121_1_, transactio1_.outgoing_shipment_id as outgoin12_121_1_, transactio1_.source_id as source_13_121_1_, transactio1_.requisition_id as requisi14_121_1_, transactio1_.transaction_date as transac15_121_1_, transactio1_.comment as comment16_121_1_, transactio1_.transaction_type_id as transac17_121_1_, transactio1_.inventory_id as invento18_121_1_, transactio1_.destination_id as destina19_121_1_, transactio1_.created_by_id as created20_121_1_, transactio1_.confirmed as confirm21_121_1_, transactio7_.version as version2_124_2_, transactio7_.sort_order as sort_ord3_124_2_, transactio7_.date_created as date_cre4_124_2_, transactio7_.last_updated as last_upd5_124_2_, transactio7_.name as name6_124_2_, transactio7_.description as descript7_124_2_, transactio7_.transaction_code as transact8_124_2_, inventoryi2_.version as version2_22_3_, inventoryi2_.product_id as product_3_22_3_, inventoryi2_.date_created as date_cre4_22_3_, inventoryi2_.last_updated as last_upd5_22_3_, inventoryi2_.lot_number as lot_numb6_22_3_, inventoryi2_.comments as comments7_22_3_, inventoryi2_.lot_status as lot_stat8_22_3_, inventoryi2_.expiration_date as expirati9_22_3_, product4_.version as version2_67_4_, product4_.manufacturer as manufact3_67_4_, product4_.cold_chain as cold_cha4_67_4_, product4_.hazardous_material as hazardou5_67_4_, product4_.date_created as date_cre6_67_4_, product4_.essential as essentia7_67_4_, product4_.package_size as package_8_67_4_, product4_.product_family_id as product_9_67_4_, product4_.active as active10_67_4_, product4_.updated_by_id as updated11_67_4_, product4_.abc_class as abc_cla12_67_4_, product4_.manufacturer_code as manufac13_67_4_, product4_.upc as upc14_67_4_, product4_.brand_name as brand_n15_67_4_, product4_.gl_account_id as gl_acco16_67_4_, product4_.manufacturer_name as manufac17_67_4_, product4_.name as name18_67_4_, product4_.default_uom_id as default19_67_4_, product4_.price_per_unit as price_p20_67_4_, product4_.reconditioned as recondi21_67_4_, product4_.vendor_name as vendor_22_67_4_, product4_.unit_of_measure as unit_of23_67_4_, product4_.serialized as seriali24_67_4_, product4_.lot_control as lot_con25_67_4_, product4_.model_number as model_n26_67_4_, product4_.last_updated as last_up27_67_4_, product4_.vendor as vendor28_67_4_, product4_.lot_and_expiry_control as lot_and29_67_4_, product4_.cost_per_unit as cost_pe30_67_4_, product4_.controlled_substance as control31_67_4_, product4_.product_type_id as product32_67_4_, product4_.product_code as product33_67_4_, product4_.vendor_code as vendor_34_67_4_, product4_.category_id as categor35_67_4_, product4_.created_by_id as created36_67_4_, product4_.description as descrip37_67_4_, product4_.ndc as ndc38_67_4_, (select max(pc.color) from product_catalog_item pci left outer join product_catalog pc on pci.product_catalog_id = pc.id where pci.product_id = product4_.id group by pci.product_id) as formula2_4_, category5_.version as version2_5_5_, category5_.parent_category_id as parent_c3_5_5_, category5_.date_created as date_cre4_5_5_, category5_.last_updated as last_upd5_5_5_, category5_.is_root as is_root6_5_5_, category5_.sort_order as sort_ord7_5_5_, category5_.gl_account_id as gl_accou8_5_5_, category5_.name as name9_5_5_, category5_.description as descrip10_5_5_, location3_.version as version2_35_6_, location3_.date_created as date_cre3_35_6_, location3_.logo as logo4_35_6_, location3_.last_updated as last_upd5_35_6_, location3_.organization_id as organiza6_35_6_, location3_.active as active7_35_6_, location3_.zone_id as zone_id8_35_6_, location3_.location_number as location9_35_6_, location3_.address_id as address10_35_6_, location3_.sort_order as sort_or11_35_6_, location3_.manager_id as manager12_35_6_, location3_.location_group_id as locatio13_35_6_, location3_.bg_color as bg_colo14_35_6_, location3_.name as name15_35_6_, location3_.inventory_id as invento16_35_6_, location3_.location_type_id as locatio17_35_6_, location3_.parent_location_id as parent_18_35_6_, location3_.fg_color as fg_colo19_35_6_, location3_.description as descrip20_35_6_, product6_.version as version2_67_7_, product6_.manufacturer as manufact3_67_7_, product6_.cold_chain as cold_cha4_67_7_, product6_.hazardous_material as hazardou5_67_7_, product6_.date_created as date_cre6_67_7_, product6_.essential as essentia7_67_7_, product6_.package_size as package_8_67_7_, product6_.product_family_id as product_9_67_7_, product6_.active as active10_67_7_, product6_.updated_by_id as updated11_67_7_, product6_.abc_class as abc_cla12_67_7_, product6_.manufacturer_code as manufac13_67_7_, product6_.upc as upc14_67_7_, product6_.brand_name as brand_n15_67_7_, product6_.gl_account_id as gl_acco16_67_7_, product6_.manufacturer_name as manufac17_67_7_, product6_.name as name18_67_7_, product6_.default_uom_id as default19_67_7_, product6_.price_per_unit as price_p20_67_7_, product6_.reconditioned as recondi21_67_7_, product6_.vendor_name as vendor_22_67_7_, product6_.unit_of_measure as unit_of23_67_7_, product6_.serialized as seriali24_67_7_, product6_.lot_control as lot_con25_67_7_, product6_.model_number as model_n26_67_7_, product6_.last_updated as last_up27_67_7_, product6_.vendor as vendor28_67_7_, product6_.lot_and_expiry_control as lot_and29_67_7_, product6_.cost_per_unit as cost_pe30_67_7_, product6_.controlled_substance as control31_67_7_, product6_.product_type_id as product32_67_7_, product6_.product_code as product33_67_7_, product6_.vendor_code as vendor_34_67_7_, product6_.category_id as categor35_67_7_, product6_.created_by_id as created36_67_7_, product6_.description as descrip37_67_7_, product6_.ndc as ndc38_67_7_, (select max(pc.color) from product_catalog_item pci left outer join product_catalog pc on pci.product_catalog_id = pc.id where pci.product_id = product6_.id group by pci.product_id) as formula2_7_ from transaction_entry transactio0_ inner join transaction transactio1_ on transactio0_.transaction_id=transactio1_.id inner join transaction_type transactio7_ on transactio1_.transaction_type_id=transactio7_.id inner join inventory_item inventoryi2_ on transactio0_.inventory_item_id=inventoryi2_.id inner join product product4_ on inventoryi2_.product_id=product4_.id inner join category category5_ on product4_.category_id=category5_.id left outer join location location3_ on transactio0_.bin_location_id=location3_.id left outer join product product6_ on transactio0_.product_id=product6_.id where transactio1_.inventory_id=? order by transactio1_.transaction_date asc, transactio1_.date_created asc

jmiranda · 2024-02-09T06:38:06Z

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

     * @return
     */
-    List getQuantityByBinLocation(List<TransactionEntry> entries, boolean includeOutOfStock) {
+    List<QuantityByBinLocationDto> getQuantityByBinLocation(List<TransactionEntry> entries, boolean includeOutOfStock) {


I don't like the Dto suffix. It's adds little value and is distracting. I'm also not a fan of the class name, but it is understandable (you're just following the convention of the method name which was poorly named by me). But replicating the method name makes me think that we're going to be creating separate DTOs for every method we refactor which would be a problem.

So let's drop the Dto and come up with a decent name that conveys the data that is being transferred. From what I gather, the granularity is basically the same as the AvailableItem class with a few more properties. The issue there is that we'd need to deal with the "status".

So we can either make this work with AvailableItem, a similarly named class, or just continue to use the map until we resolve the naming issue.

Sure, I have not felt confident naming those, because I have not been entirely sure what they refer to actually.
As I explained at the beginning - it's been difficult to debug those without having the types.
Btw. we already have a dto named AvailabileItem, so do you maybe have any other idea, how to name this one?

To be honest, I feel like we should somehow distinguish classes that are DTOs. When I joined this project and saw OutboundStockMovement I thought that this was a normal database entity, and it took me a while to understand that this exact class is a DTO. Maybe we should align on some exact naming regarding DTOs? Or maybe a good idea would be to place them in a directory called DTO and still use the same class names as we had before?

cc @awalkowiak @drodzewicz

Decided to use org.pih.warehouse.inventory.BinLocationItem for now.

jmiranda · 2024-02-09T06:41:15Z

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

     *     (IMPORTANT: Outbound returns can have picked items with RECALLED lots and bins with HOLD_STOCK activity)
     * */
-    def getQuantityPickedByProductAndLocation(Location location, Product product) {
+    List<QuantityPickedByLocationAndProductDto> getQuantityPickedByProductAndLocation(Location location, Product product) {


Same with this one. Lose the Dto and call it something like AllocatedItem or AllocationItem. Or StockAllocationItem could also work since it follows the same pattern as descriptive pattenr as StockMovementItem. But I'm open to other suggestions.

Decided to use AllocatedItem. Should live in the same package as AvailableItem although that is an inner class in StockMovementItem.

org.pih.warehouse.api.AllocatedItem

jmiranda · 2024-02-09T06:53:46Z

src/main/groovy/org/pih/warehouse/inventory/QuantityByBinLocationDto.groovy

+    int compareTo(QuantityByBinLocationDto obj) {
+        inventoryItem?.expirationDate <=> obj?.inventoryItem?.expirationDate
+                ?: binLocation?.name <=> obj?.binLocation?.name
+    }


Yeah it's a good point @drodzewicz . Whether it should be included in the PR is negotiable since (as @kchelstowski mentioned) this is just a copy-paste from the previous sort implementation.

If it's not included, then we should make sure we prioritize the following ticket and maybe a spike to test out the different implementations.
https://pihemr.atlassian.net/browse/OBGM-341

jmiranda

LGTM. I'd like @awalkowiak to take one last look and then merge if he's satisfied.

kchelstowski requested review from alannadolny, awalkowiak, drodzewicz and jmiranda November 29, 2023 13:28

kchelstowski self-assigned this Nov 29, 2023

alannadolny reviewed Nov 29, 2023

View reviewed changes

kchelstowski added warn: do not merge Marks a pull request that is not yet ready to be merged 0.9.1 labels Nov 29, 2023

jmiranda added this to the 0.9.1 milestone Dec 4, 2023

jmiranda removed the 0.9.1 label Dec 4, 2023

kchelstowski force-pushed the OBGM-508 branch from 6e9d57c to 5700994 Compare February 1, 2024 11:45

kchelstowski added 2 commits February 1, 2024 12:53

OBGM-508 Improve product availability refresh performance

76b367a

Revert rebase inconsistency

10137d6

kchelstowski force-pushed the OBGM-508 branch from 5700994 to 10137d6 Compare February 1, 2024 11:56

awalkowiak removed the warn: do not merge Marks a pull request that is not yet ready to be merged label Feb 1, 2024

awalkowiak reviewed Feb 7, 2024

View reviewed changes

jmiranda requested changes Feb 9, 2024

View reviewed changes

OBGM-508 Rename dto classes

7aeae37

jmiranda approved these changes Feb 13, 2024

View reviewed changes

awalkowiak approved these changes Feb 13, 2024

View reviewed changes

awalkowiak merged commit 0c3bd10 into feature/upgrade-to-grails-3.3.10 Feb 13, 2024

awalkowiak deleted the OBGM-508 branch February 13, 2024 11:34

ewaterman mentioned this pull request Jul 6, 2024

[OBPIH-6479, OBPIH-6483] PR labeler, PR template description #4687

Merged

Uh oh!

Conversation

kchelstowski commented Nov 29, 2023

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!

awalkowiak commented Jan 25, 2024

Uh oh!

kchelstowski commented Feb 1, 2024

Uh oh!

awalkowiak left a comment

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!

alannadolny Feb 9, 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!

jmiranda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alannadolny Feb 9, 2024 •

edited

Loading