OBPIH-3614 Add invoice items modal #2315
Conversation
| @@ -152,6 +153,18 @@ class InvoiceService { | |||
| invoiceItem.delete() | |||
| } | |||
|
|
|||
| def addItems(Invoice invoice, List items) { | |||
There was a problem hiding this comment.
Let's find a way to do add/update in one service method so we don't need to add an RPC endpoint on the API. I want this to be a clean REST API that we use as the standard to refactor everything else.
Something like this should work.
def updateInvoiceItems(Invoice invoice, List items) {
items.each { item ->
InvoiceItem existingInvoiceItem = invoice.invoiceItems.find { it.id == item.id }
if (existingInvoiceItem) {
existingInvoiceItem.quantity = item.quantity
}
else {
InvoiceCandidate candidateItem = InvoiceCandidate.get(item.id)
if (candidateItem) {
InvoiceItem invoiceItem = invoiceItemService.createFromCandidate(candidateItem)
invoiceItem.quantity = item.quantityToInvoice
invoice.addToInvoiceItems(invoiceItem)
}
}
}
}
| @@ -152,6 +153,18 @@ class InvoiceService { | |||
| invoiceItem.delete() | |||
| } | |||
|
|
|||
| def addItems(Invoice invoice, List items) { | |||
| items.each { item -> | |||
| InvoiceCandidate candidateItem = InvoiceCandidate.get(item.id) | |||
There was a problem hiding this comment.
It feels like an additional query here seems unnecessary. Why are we doing this when we could be using the POST data?
There was a problem hiding this comment.
In post we send only candidate id and quantityToInvoice. I could send all data back, but then I will need to find BudgetCode by code, GlAccount by code and uom basing on the formatted string value ("${quantityUom?.code}/${quantityPerUom as Integer}" made in transient). I feel the current version is smoother and cleaner
There was a problem hiding this comment.
Ok sounds good. One issue we might need to deal with is when the candidate invoice item does not exist any longer (i.e. it was added to another invoice in the time between it was queried and saved in our invoice). Right now it seems like we're just silently ignoring that case but that could become confusing. It shouldn't happen very often but it'll be annoying to the user when it does.
There was a problem hiding this comment.
So let's throw an IllegalArgumentException or ValidationException for that case.
There was a problem hiding this comment.
Ok, I'll do it as a first thing tomorrow.
| invoiceItems: _.map(selectedInvoiceItems, (item, key) => ({ | ||
| id: key, | ||
| quantityToInvoice: _.toInteger(item.quantityToInvoice), | ||
| sortOrder: _.toInteger(item.sortOrder), |
There was a problem hiding this comment.
Is this sortOrder needed and saved anywhere? There is no such field in InvoiceItem.
| @@ -61,32 +62,32 @@ class InvoiceService { | |||
| return invoiceItems | |||
| } | |||
|
|
|||
| def getInvoiceCandidates(String id, String orderNumber, String shipmentNumber, String max, String offset) { | |||
| def getInvoiceCandidates(String id, String orderNumber, String shipmentNumber) { | |||
There was a problem hiding this comment.
@awalkowiak @pmuchowski Not sure where this comment should go, but next time we make changes to the invoice feature please change InvoiceCandidate to InvoiceItemCandidate (view, domain, etc). It's been bothering me since the initial PR, but I didn't want to delay the API any longer. But now that the API is working I'd like to refactor this.
Also, do we need an InvoiceService and InvoiceItemService? I'd prefer if we have one service per feature / component.
No description provided.