OBPIH-5610 Significant performance issues with Export Orders #4013
Conversation
kchelstowski
added
the
warn: do not merge
|
Marked as "do not merge", since there are some differences visible in table vs csv |
| `order`.order_number as order_number, | ||
| order_item.id as order_item_id, |
There was a problem hiding this comment.
it was causing the problem of duplicates results, e.g. I was getting some weird order_id here when supposed to get only order_item_id:
There was a problem hiding this comment.
Thanks for fixing the column order issue.
| SQLQuery sqlQuery = sessionFactory.currentSession.createSQLQuery(query) | ||
| final count = sqlQuery.with { | ||
| setString("orderId", order.id) | ||
| setString("prepaymentTypeCode", InvoiceTypeCode.PREPAYMENT_INVOICE.toString()) |
There was a problem hiding this comment.
As I mentioned in the ticket, I'm not sure this is correct. If there are requirements for excluding prepayment invoices in the "invoiced" count, then we can move this into the query (we don't need to use query parameters).
There was a problem hiding this comment.
I explained in the ticket why they should be excluded and how prepayments are excluded now in this method:
openboxes/grails-app/domain/org/pih/warehouse/order/OrderItem.groovy
Lines 344 to 354 in c6bc349
I've added it as query parameters to follow the convention you did with orderId - I didn't wanna mix things up, to have one parameter via query parameters, and one inside the query directly.
There was a problem hiding this comment.
https://github.com/openboxes/openboxes/blob/develop/grails-app/migrations/views/order-summary-helper-views.sql#LL87C43-L87C46 This one here is doing that for OrderSummary (I know it's not intuitive and requires at least a comment), Katarzyna also linked tickets regarding the functionality in the OBPIH-5610.
There was a problem hiding this comment.
I left two comments in the code. I think we should split these two cases and we should not mix those two:
- Order.getInvoiceItems
- Order.getInvoicedOrderItems
because these are two different things. The first one should not be used for getting invoiced items for PO (but currently this one is used)
| @@ -155,7 +156,7 @@ class PurchaseOrderApiController { | |||
| order?.orderedOrderItems?.size() ?: 0, | |||
| order?.shippedOrderItems?.size() ?: 0, | |||
| order?.receivedOrderItems?.size() ?: 0, | |||
| order?.invoiceItems?.size() ?: 0, | |||
| invoiceService.countInvoicedOrderItems(order)?:0, | |||
There was a problem hiding this comment.
order?.invoiceItems?.size() should not be used here at all in the beginning, because it's taking all invoice items instead of invoiced order items, plus I even left a comment above order?.invoiceItems some time ago that it should be used for one order only but not for a list of orders like it was used in this case.
Perhaps it would be better to add all these counts to the OrderSummary? We would have then everything in one place without the need to have additional queries. All orderedOrderItems, shippedOrderItems, receivedOrderItems, and invoicedOrderItems could be additionally (easily) calculated in the helper views and stored in the OrderSummary.
Refactoring or improving orderItem?.invoiceItems and orderItem?.quantityInvoicedInStandardUom should also be done definitely but it probably doesn't have to be done for PO export if were getting the item counts from view or the query that you created.
| SQLQuery sqlQuery = sessionFactory.currentSession.createSQLQuery(query) | ||
| final count = sqlQuery.with { | ||
| setString("orderId", order.id) | ||
| setString("prepaymentTypeCode", InvoiceTypeCode.PREPAYMENT_INVOICE.toString()) |
There was a problem hiding this comment.
https://github.com/openboxes/openboxes/blob/develop/grails-app/migrations/views/order-summary-helper-views.sql#LL87C43-L87C46 This one here is doing that for OrderSummary (I know it's not intuitive and requires at least a comment), Katarzyna also linked tickets regarding the functionality in the OBPIH-5610.
|
I am closing this one since it is fixed in another way in this PR #4027 |
No description provided.