OBPIH-6044 Export invoice lines details on invoices list page by drodzewicz · Pull Request #4484 · openboxes/openboxes

drodzewicz · 2024-01-29T14:24:13Z

No description provided.

kchelstowski · 2024-01-29T14:51:03Z

grails-app/controllers/org/pih/warehouse/UrlMappings.groovy

+            controller = "invoiceApi"
+            action = [GET: "getAllInvoiceItems"]
+        }
+


I don't mind it, but I think we used to do it via the export mapping, with a proper param (e.g. "orderItems").

yes this is probably the way to go, this should be on /api/invoiceItems/

kchelstowski · 2024-01-29T14:56:39Z

grails-app/controllers/org/pih/warehouse/api/InvoiceApiController.groovy

+    def getAllInvoiceItems() {
+        Location location = Location.get(session.warehouse.id)
+        params.partyFromId = location?.organization?.id
+        List<InvoiceList> invoices = invoiceService.getInvoices(params)


I spoke about that with @drodzewicz, but I want to make my point here, so that everyone can give their opinion - in my opinion it should be dealt directly via InvoiceItem.createCriteria() and

invoice { 'in'("id" ,params.invoicesIds) }

clause.
Plus I would monitor the performance here, because we might run into many N+1 - if we do, we could play with the data services and joins, but for now, I'd like to get others opinion about the approach of first getting the invoices and then getting the invoice items.

I don't think we are at risk of N+1 queries, if I am not missing anything this should be max 2 queries one for invoices and one for invoiceItems.

My goal was to reuse the query filter logic on invoiceService.getInvoices(params) since we are expecting to get only those InvoiceItems that are a part of filtered invoices from invoice/list page, basically aggregate these queries.
First get invoices (filtered if necessary) and then based on these invoices get all of the invoiceItems

@drodzewicz On the other hand if this is for "import all" case you could also just do: get invoices -> return invoices.invoiceItems. Is the pagination required here? However if this is going to land in its own api controller, or the pagination params are required, then:

I agree with @kchelstowski on that one in the long term. If you need to do some further invoice filtering you still can do it in the suggested by Kacper way as a:

invoice { 'in'('id', params.invoiceIds) // if there are invoice ids passed whatever other invoice filter you have there }

Instead of doing get invoices -> get invoice items by these invoices.

kchelstowski · 2024-01-29T14:57:18Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+                    invoiceItem.invoice?.currencyUom.name,
+                    invoiceItem.invoice?.status?.name(),
+                    "${invoiceItem.invoice?.partyFrom?.code} ${invoiceItem.invoice?.partyFrom?.name}",
+                    invoiceItem.invoice?.invoiceType?.code?.name() ?: "INVOICE",


let's use the enum from InvoiceTypeCode in the fallback

kchelstowski · 2024-01-29T14:57:31Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+                    invoiceItem.invoice?.status?.name(),
+                    "${invoiceItem.invoice?.partyFrom?.code} ${invoiceItem.invoice?.partyFrom?.name}",
+                    invoiceItem.invoice?.invoiceType?.code?.name() ?: "INVOICE",
+                    invoiceItem.product?.productCode ?: "all",


what does "all" mean here?

on invoice show page we have a similar case where we label adjustments as all since adjustments don't have a product code

openboxes/grails-app/views/invoice/_invoiceItems.gsp

Lines 23 to 25 in 3627a1d

<td>

${invoiceItem?.product?.productCode?:g.message(code:'default.all.label')}

</td>

kchelstowski · 2024-01-29T14:58:27Z

src/js/api/urls.js


 // INVOICE
 export const INVOICE_API = `${API}/invoices`;
+export const INVOICE_ITEMS_API = `${API}/invoices/items`;


if we agree this should be the url, then I think this could be shortened to:

export const INVOICE_ITEMS_API = `${INVOICE_API}/items`;

kchelstowski · 2024-01-29T14:59:05Z

src/js/components/invoice/InvoiceListTable.jsx

              type="button"
              className="dropdown-item"
-              onClick={() => downloadInvoices()}
+              onClick={downloadInvoices}


I said it once to @alannadolny , but we need to be aware, that in this case, if we added any argument to the downloadInvoices method, this would be run with the event from the button.

kchelstowski · 2024-01-29T15:14:12Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+                    invoiceItem?.quantity,
+                    invoiceItem?.quantityPerUom,
+                    invoiceItem?.unitPrice ?: 0,
+                    invoiceItem?.totalAmount ?: 0,


@drodzewicz I meant you might run into N+1 in this place, where you refer to many different entities and this would run N+1 for every invoice item. So my N+1 comment from above was not directly refering to the "two queries" you made, but overall.

@kchelstowski Yup, you're right, I also prefer to use our data services here (or just measure the performance and make a decision based on that)

- change endpoint for exporting invoiceItems - fixes for const values

drodzewicz · 2024-01-31T09:56:38Z

Pushed some requested changes suggetsed by @kchelstowski and waiting for other developers (@alannadolny @awalkowiak), to review this PR and give an opinion on Kacpers comments

alannadolny · 2024-01-31T11:21:44Z

grails-app/controllers/org/pih/warehouse/api/InvoiceApiController.groovy

+    def getAllInvoiceItems() {
+        Location location = Location.get(session.warehouse.id)
+        params.partyFromId = location?.organization?.id
+        List<InvoiceList> invoices = invoiceService.getInvoices(params)


One more point here - should we pass the whole params object as an argument as it is done here?

List<InvoiceList> invoices = invoiceService.getInvoices(params)

alannadolny · 2024-01-31T11:24:29Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

    }
+
+    CSVPrinter getInvoiceItemsCsv(List<InvoiceItem> invoiceItems) {
+        def g = Holders.grailsApplication.mainContext.getBean('org.grails.plugins.web.taglib.ApplicationTagLib')


Maybe it is worth to use move this to a function (like it is already done in some places)

def getApplicationTagLib() { return Holders.grailsApplication.mainContext.getBean(ApplicationTagLib) }

alannadolny · 2024-01-31T11:25:53Z

grails-app/services/org/pih/warehouse/invoice/InvoiceService.groovy

+                    invoiceItem?.quantity,
+                    invoiceItem?.quantityPerUom,
+                    invoiceItem?.unitPrice ?: 0,
+                    invoiceItem?.totalAmount ?: 0,


@kchelstowski Yup, you're right, I also prefer to use our data services here (or just measure the performance and make a decision based on that)

awalkowiak · 2024-01-31T11:29:57Z

grails-app/controllers/org/pih/warehouse/UrlMappings.groovy


+        "/api/invoiceItems"(parseRequest: true) {
+            controller = "invoiceApi"
+            action = [GET: "getAllInvoiceItems"]


I think this should be in a separate InvoiceItemApiController, because what I see the getInvoiceItems in the InvoiceAPIController is meant to get the invoice items from a specific invoice item, so if this one is meant to pull all invoice items from different invoices, then this should land in a separate controller.

we also have

"/api/invoiceItems/$id/validation" { controller = "invoiceApi" action = [POST: "validateInvoiceItem"] }

so this will also have to end up in invoiceItemApicontroller

Not really, I think this one can stay where it is, at least for now

drodzewicz · 2024-02-05T09:26:06Z

So after looking through the problem again, I came to the conclusion that my initial approach was wrong and this exact export should not have bee handled by a different endpoint targeting a list of invoiceItems but instead I should have treated it as an expansion to invoice/list since we are interested in all of the filter parameters to persist from the invoice list to invoiceItem.

An exact case is handled in Purchase Order list

openboxes/grails-app/controllers/org/pih/warehouse/api/PurchaseOrderApiController.groovy

Lines 28 to 44 in 3627a1d

    
           def list = { 
        
               List<OrderSummary> purchaseOrders = orderService.getPurchaseOrders(params) 
        
               if (params.format == 'csv') { 
        
                   def csv = params.boolean("orderItems") ? getOrderItemsCsv(purchaseOrders) : getOrdersCsv(purchaseOrders) 
        
                   def name = params.boolean("orderItems") ? "OrderLineItems" : "Orders" 
        
                   response.setHeader("Content-disposition", "attachment; filename=\"${name}-${new Date().format("MM/dd/yyyy")}.csv\"") 
        
                   render(contentType: "text/csv", text: csv.out.toString()) 
        
                   return 
        
               } 
        
               render([ 
        
                   data: purchaseOrders, 
        
                   totalPrice: purchaseOrders?.sum { it.order.totalNormalized?:0.0 } ?:0.0, 
        
                   totalCount: purchaseOrders?.totalCount 
        
               ] as JSON) 
        
            }

So following the example of PO list I did the same thing, which is exporting a different document based on passed parameter invoiceItems=true.

Additionally as Kacper mentioned in previous comments, it would be nice to fetch invoice items eagerly for this export to avoid unnecessary N+1 queries to the database, so I added a parameter fetchItemsEagerly to specify the FetchMode eager for invoiceItems on invoicesList.
After comparing the export time of both cases here are some results.
Exporting 780 invoice items from 350 Invoices:

with eager fetch: 1309 ms (avg)
without eager fetch: 1347 ms (avg)

kchelstowski · 2024-02-05T10:09:53Z

src/js/hooks/list-pages/invoice/useInvoiceListTableData.jsx


 import { hideSpinner, showSpinner } from 'actions';
 import invoiceApi from 'api/services/InvoiceApi';
+import invoiceItemApi from 'api/services/InvoiceItemApi';


I think it is not used?

it's not, thanks

- add invoiceItems parameter to specify invoiceItem export - rollback previous changes - add fetchItemsEagerly parameter to fetch invoiceitems eagerly

kchelstowski

thanks for the times @drodzewicz . Even though the difference is not that huge right now, we will get the benefit from it when the invoice table becomes bigger and bigger.

drodzewicz added 3 commits January 29, 2024 15:19

OBPIH-6044 Added endpoint to export Invoice items csv

aa92afa

OBPIH-6044 Export invoice items on client

fe46e6c

OBPIH-6044 rename exported file

e4ec36a

drodzewicz requested review from alannadolny, awalkowiak and kchelstowski January 29, 2024 14:24

drodzewicz self-assigned this Jan 29, 2024

OBPIH-6044 Fixed return types for invoice items methods

1a1c187

drodzewicz force-pushed the OBPIH-6044-export-invoice-lines-details-on-invoices-list-page branch from b927232 to 1a1c187 Compare January 29, 2024 14:54

kchelstowski reviewed Jan 29, 2024

View reviewed changes

OBPIH-6044 Fixes after review

d7050f7

- change endpoint for exporting invoiceItems - fixes for const values

alannadolny reviewed Jan 31, 2024

View reviewed changes

awalkowiak requested changes Jan 31, 2024

View reviewed changes

drodzewicz force-pushed the OBPIH-6044-export-invoice-lines-details-on-invoices-list-page branch from 2ada6eb to 2fee9b2 Compare February 5, 2024 09:15

drodzewicz requested review from alannadolny, awalkowiak and kchelstowski February 5, 2024 09:26

kchelstowski reviewed Feb 5, 2024

View reviewed changes

OBPIH-6044 Export invoiceItems on invoice list endpoint

c86f6c3

- add invoiceItems parameter to specify invoiceItem export - rollback previous changes - add fetchItemsEagerly parameter to fetch invoiceitems eagerly

drodzewicz force-pushed the OBPIH-6044-export-invoice-lines-details-on-invoices-list-page branch from 2fee9b2 to c86f6c3 Compare February 5, 2024 10:12

kchelstowski approved these changes Feb 5, 2024

View reviewed changes

awalkowiak merged commit 1a8a1c2 into feature/upgrade-to-grails-3.3.10 Feb 5, 2024

awalkowiak deleted the OBPIH-6044-export-invoice-lines-details-on-invoices-list-page branch February 5, 2024 17:19

	<td>
	${invoiceItem?.product?.productCode?:g.message(code:'default.all.label')}
	</td>

Uh oh!

Conversation

drodzewicz commented Jan 29, 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!

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!

drodzewicz commented Jan 31, 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!

drodzewicz commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kchelstowski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants