OBPIH-3799, OBPIH-3840 Generate Invoice for PO after prepayment #2412
Conversation
| @@ -256,9 +256,9 @@ class UrlMappings { | |||
| } | |||
|
|
|||
| // Invoice API | |||
| "/api/invoices/$id/invoiceItems"(parseRequest: true) { | |||
| "/api/invoices/$id/items"(parseRequest: true) { | |||
There was a problem hiding this comment.
Interesting chose. Can/should we apply this to other APIs in the near future?
There was a problem hiding this comment.
I think we should if we can specify the same endpoint for different request methods. In this case we had two separate endpoints for no reason.
| @@ -95,4 +95,15 @@ class InvoiceController { | |||
| Invoice invoice = invoiceService.generatePrepaymentInvoice(order) | |||
| redirect(action: "create", params: [id: invoice.id]) | |||
| } | |||
|
|
|||
| def generateInvoiceAfterPrepayment = { | |||
There was a problem hiding this comment.
I think this can just be generateInvoice().
| @@ -69,7 +69,7 @@ class Invoice implements Serializable { | |||
| invoiceItems cascade: "all-delete-orphan" | |||
| } | |||
|
|
|||
| static transients = ['vendorInvoiceNumber', 'totalValue', 'totalValueNormalized', 'documents', 'status'] | |||
| static transients = ['vendorInvoiceNumber', 'totalValue', 'totalValueNormalized', 'documents', 'status', 'hasPrepaymentAssociated'] | |||
There was a problem hiding this comment.
Change to hasPrepaymentInvoice to keep it consistent.
There was a problem hiding this comment.
But technically invoice does not have prepayment invoice (order does)
| ShipmentItem getShipmentItem() { | ||
| return shipmentItems ? shipmentItems?.iterator()?.next() : null | ||
| OrderItem getOrderItem() { | ||
| return orderItems ? orderItems?.find { it } : null |
There was a problem hiding this comment.
Why the switch? I can see why it might help (choose the first non-null item) but do we think it'll ever be null?
There was a problem hiding this comment.
This change was more for IDE to get rid of underscore warning
| OrderItem getOrderItem() { | ||
| if (orderAdjustments) { | ||
| return orderAdjustment?.orderItem | ||
| Order getOrder() { |
There was a problem hiding this comment.
This method scares me the most. I think I'd prefer to leave it up to the developer unless it causes us to write this branching logic whenever we need to access Order.
If we do need it, perhaps since a shipment might include multiple orders we always return a list of Orders here to make sure our Invoice model exposes the right method signature. It would be bad if the system supported adding multiple orders per shipment / invoice, but the model itself did not.
List<Order> getOrders() {
return orderItem ? [orderItem.order] : orderAdjustment ? [orderAdjustment.order] : shipmentItem.orderItems*.order
}
There was a problem hiding this comment.
I'll investigate it with other refactors, so this can be tested
| @@ -175,8 +174,8 @@ class InvoiceItem implements Serializable { | |||
| Map toJson() { | |||
| return [ | |||
| id: id, | |||
| orderNumber: orderNumber, | |||
| shipmentNumber: shipmentNumber, | |||
| orderNumber: order?.orderNumber ?: '', | |||
There was a problem hiding this comment.
Why empty string here and not null?
| left join shipment_invoice on shipment_invoice.shipment_item_id = shipment_item.id | ||
| where shipment_invoice.invoice_item_id is null and shipment.current_status = 'SHIPPED'); | ||
| where order_invoice.invoice_item_id is null and shipment_invoice.invoice_item_id is null and shipment.current_status = 'SHIPPED'); |
There was a problem hiding this comment.
Each condition of the where clause should be on a separate line for readability.
| invoice.invoiceType = InvoiceType.findByCode(InvoiceTypeCode.PREPAYMENT_INVOICE) | ||
| createOrUpdateVendorInvoiceNumber(invoice, order.orderNumber + Constants.PREPAYMENT_INVOICE_SUFFIX) | ||
|
|
||
| if (order.orderItems.size() + order.orderAdjustments.size() == 0) { |
There was a problem hiding this comment.
Careful here. I think Hibernate/Grails will initialize the orderItems and orderAdjustments with an empty list, but for unit tests it might not so you'll need to add null safety.
Plus I would suggest using something more readable like this.
if (!order?.orderItems?.empty && !order?.orderAdjustments?.empty)
There was a problem hiding this comment.
Thanks for the tip
| @@ -171,14 +171,22 @@ | |||
| ${warehouse.message(code: 'default.button.rollback.label')} | |||
| </g:link> | |||
| <g:if test="${!orderInstance?.hasInvoice && orderInstance?.paymentTerm?.prepaymentPercent && | |||
There was a problem hiding this comment.
We should encapsulate this logic in a domain method. In fact, I think we use each component in other places so let's add methods for each of them.
Boolean isPrepaymentInvoiceAllowed() {
return !this.hasInvoice && this.isPrepaymentRequired && !this.empty()
}
Boolean isPrepaymentRequired() {
return this.paymentTerm?.prepaymentPercent > 0
}
Boolean isEmpty() {
return this.orderItems?.empty && orderInstance?.orderAdjustments?.empty
}
I can't remember the convention for is/has methods in Grails, so you might have to play around with isSomeCondition() vs getIsSomeCondition().
And isEmpty is not descriptive enough so please suggest a more descriptive name if you can think of one. The following is a little long and awkward, but descriptive.
isOrderItemsAndOrderAdjustmentsEmpty
There was a problem hiding this comment.
I'll tackle it with refactors for Order.getInvoiceItems
No description provided.