-
-
Notifications
You must be signed in to change notification settings - Fork 403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OBPIH-5847 Fix poor performance on PO show page #4266
Conversation
def orderComments = { | ||
Order order = Order.get(params.id) | ||
render(template: "orderComments", model: [orderInstance: order]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this is the result we wanted to have?
I imagine that now, when we go to the PO view page, and go between the tabs, we everytime fetch the same order instance (with all the fields etc).
I know that probably anyway it is faster now, but maybe we should return to the view only fields that we really need, so maybe to return some kind of command/dto objects, e.g.
class OrderCommentCommand {
String orderId
Comments comments
}
(pseudocode below, probably could be simplified/be more clean)
def orderComments = {
Order order = Order.get(params.id)
OrderCommentCommand orderCommentCommand =
new OrderCommentCommand(orderId: order?.id, comments: order?.comments)
render(template: "orderComments", model: [orderCommentCommand: orderCommentCommand])
}
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still fetch the same order in your example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but I don't return the whole order object to the view, but just the part, that we really need there (this is why I wrote "with all the fields etc")
If we wanted to improve it even more, we could create a query for that, so not to fetch the Order at the top, but only the part from the "command object" I presented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS. Data services could be a good candidate if we decided to create custom queries for those, and there to use the @Query
annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I wanted to pass the entire order instance object in params, but it was stringified, so I changed it to sending order's id only, and this additional get
did not really affect the performance in this case since the rendering is a problematic part now. Plus adding a command for each tab adds more work to be done for this "quick fix"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, those are only suggestions that came to my mind, if we wanted to improve the performance even more, but I agree with you that it adds some scope which we might not want to add now.
Still, even though I gave those suggestions, I think they wouldn't be a game changer, because we fetch only one order there, so the differences shouldn't be significant versus a situation where we'd have to loop over 100 orders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recorded a video to describe what I am seeing and what I think we need to do.
https://watch.screencastify.com/v/Wm2n3lW41EfiqMpAxT0M
I hadn't read @kchelstowski's comments before the video but we're basically saying the same thing
I know, but I don't return the whole order object to the view, but just the part, that we really need there (this is why I wrote "with all the fields etc")
So I agree with @kchelstowski on the problem but I'm not sure if we need separate command objects just yet.
The performance problems with the ajax requests are probably all bottled up in the template GSPs. Some of those are likely causing tons of N+1 queries. A more thoughtful approach would be to rewrite each of those to gather just the data needed for the page, hopefully in a single query.
I imagine there would be at least two methods
- Order getOrderSummary(String id)
- Order getOrderDetails(String id)
that queries the database (including joins) to gather all of the data needed for that particular tab.
If that's not enough we can add other methods as data gatherers for the other closures.
- Order getOrderItemStatuses(String id)
- Order getOrderItemDetails(String id)
- etc
Hopefully, only one of these methods will be the expensive one but I'm hoping we're able to limit it use to just the tabs that require the expensive view-backed data (i.e. invoice or status data that is taking a long time to load).
Assuming we can get everything we need packed into the Order object (or a map) then we can leave the command object for later.
For now, the important part is just trying to add some service methods that pull as much data as possible in one query. And honestly, I just need the summary tab to load in under 1s right now. Taking a little longer for subsequent tabs is fine until we have time to tackle the underlying slowness.
boolean getIsRegularInvoice() { | ||
return invoiceType?.code == InvoiceTypeCode.INVOICE | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you've just followed the convention, but I think this would be a nice tech debt topic for the future to discuss whether boolean names should start with the is
prefix, or it should be reserved for the getter.
Groovy is specific in this case, because we'll access the field without having to use get
prefix, but I know in Java the "best practices" are NOT to use is
for boolean field name and use it for the getter.
This is not the topic to discuss under this PR, but just wanted to point it out maybe for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you've just followed the convention, but I think this would be a nice tech debt topic for the future to discuss whether boolean names should start with the is prefix
I agree we should have a tech huddle about this to enumerate the conventions and document the one we want to standardize on.
I always forget what the correct convention for booleans is, but I prefer the Groovy/Grails convention of invoking the getter method through a property access so I think I like the way @awalkowiak did it above.
Boolean getIsApprovalRequired() {
}
if (requisition.origin.isApprovalRequired) {
...
}
vs
Boolean IsApprovalRequired() {
...
}
if (requisition.isApprovalRequired()) {
...
}
but I know in Java the "best practices" are NOT to use is for boolean field name and use it for the getter.
Unless I'm misunderstanding, Java does provide a design pattern for boolean properties that uses "is".
The slight difference is that Java is allowing for a is() method while get getIs() method allows us to use the Groovy getter convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm misunderstanding, Java does provide a design pattern for boolean properties that uses "is".
Yes, but they use it for the getter, not the for the property itself. Even the jackson tool when serializing an object would return a boolean without the is
.
Assuming you'd have a property:
private boolean isValid;
and if we returned this object from the API, it'd return it like:
{
"valid": true,
}
The problem with naming the boolean using the is
prefix is that naming the getter as getIs
feels weird.
Once again - in Groovy we don't have that much problem with that, because don't have to use getters, but to access the properties directly, so we don't have to do: something.getIsValid()
, but something.isValid
, so it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment below
def orderComments = { | ||
Order order = Order.get(params.id) | ||
render(template: "orderComments", model: [orderInstance: order]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recorded a video to describe what I am seeing and what I think we need to do.
https://watch.screencastify.com/v/Wm2n3lW41EfiqMpAxT0M
I hadn't read @kchelstowski's comments before the video but we're basically saying the same thing
I know, but I don't return the whole order object to the view, but just the part, that we really need there (this is why I wrote "with all the fields etc")
So I agree with @kchelstowski on the problem but I'm not sure if we need separate command objects just yet.
The performance problems with the ajax requests are probably all bottled up in the template GSPs. Some of those are likely causing tons of N+1 queries. A more thoughtful approach would be to rewrite each of those to gather just the data needed for the page, hopefully in a single query.
I imagine there would be at least two methods
- Order getOrderSummary(String id)
- Order getOrderDetails(String id)
that queries the database (including joins) to gather all of the data needed for that particular tab.
If that's not enough we can add other methods as data gatherers for the other closures.
- Order getOrderItemStatuses(String id)
- Order getOrderItemDetails(String id)
- etc
Hopefully, only one of these methods will be the expensive one but I'm hoping we're able to limit it use to just the tabs that require the expensive view-backed data (i.e. invoice or status data that is taking a long time to load).
Assuming we can get everything we need packed into the Order object (or a map) then we can leave the command object for later.
For now, the important part is just trying to add some service methods that pull as much data as possible in one query. And honestly, I just need the summary tab to load in under 1s right now. Taking a little longer for subsequent tabs is fine until we have time to tackle the underlying slowness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The domain changes are making me a little uncomfortable as I don't know what side effects those have. Not sure what to do about that, but I guess we could have a tech huddle to discuss in more detail.
boolean getIsRegularInvoice() { | ||
return invoiceType?.code == InvoiceTypeCode.INVOICE | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you've just followed the convention, but I think this would be a nice tech debt topic for the future to discuss whether boolean names should start with the is prefix
I agree we should have a tech huddle about this to enumerate the conventions and document the one we want to standardize on.
I always forget what the correct convention for booleans is, but I prefer the Groovy/Grails convention of invoking the getter method through a property access so I think I like the way @awalkowiak did it above.
Boolean getIsApprovalRequired() {
}
if (requisition.origin.isApprovalRequired) {
...
}
vs
Boolean IsApprovalRequired() {
...
}
if (requisition.isApprovalRequired()) {
...
}
but I know in Java the "best practices" are NOT to use is for boolean field name and use it for the getter.
Unless I'm misunderstanding, Java does provide a design pattern for boolean properties that uses "is".
The slight difference is that Java is allowing for a is() method while get getIs() method allows us to use the Groovy getter convention.
@@ -58,7 +59,7 @@ class InvoiceItem implements Serializable { | |||
User createdBy | |||
User updatedBy | |||
|
|||
static belongsTo = [invoice: Invoice] | |||
static belongsTo = [Invoice, ShipmentItem, OrderAdjustment, OrderItem] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worries me a tiny bit. Do we know what impact this is going to have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmiranda could you elaborate here on why this worries you? I had to put it here in order to be able to add it in hasMany relation on OrderItem, ShipmentItem, and OrderAdjustment (which is crucial for this performance and code overall improvements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could generate more SQL than we're expecting and could consequently have unintended consequences on performance.
I think you should be able to define invoice item with invoice as a parent and hasMany on other domains without the need to define cascade behavior (which is what belongsTo is doing).
https://stackoverflow.com/questions/23905638/grails-domain-class-hasone-hasmany-without-belongsto
I assume you're encountering a Grails error without this. So post that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To resolve this just let me know
- if it's possible to include back references on each of these. And if not, what is the error you're getting from Grails.
- Also does this add any unnecessary SQL queries when retrieving invoice items. Does it go through each of those associations (in either case, with or without back reference).
Order order = Order.get(params.id) | ||
def orderItems = order?.orderItems | ||
render(template: "orderSummary", model: [ | ||
isPurchaseOrder: order.isPurchaseOrder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmiranda I started moving things to the controller that are needed by the template and since there are used properties from order and order items I decided to leave it as is -> fetch order and pull order items (and other required data) from it instead of fetching only order items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think we can refactor this a bit more when we move to Grails 3. As @kchelstowski mentioned this might look better as queries defined in the data service.
This is the one change I would like to see so we can move this in that direction when the time comes
def orderSummary = {
Map orderSummary = orderService.getOrderSummary(String id)
render render(template: "orderSummary", model: orderSummary)
}
with the order summary method looking something like this. And since we know that we're just using this method for reading data we can add in some extra savings.
@Transactional(readOnly=true)
Map getOrderSummary(String id) {
Order order = Order.read(params.id)
return [
isPurchaseOrder: order.isPurchaseOrder
isPutawayOrder: order.isPutawayOrder,
orderItems: orderItems,
anySupplierCode: orderItems?.any { it.productSupplier?.supplierCode },
anyManufacturerName: orderItems?.any { it.productSupplier?.manufacturerName },
anyManufacturerCode: orderItems?.any { it.productSupplier?.manufacturerCode },
currencyCode: order.currencyCode ?: grailsApplication.config.openboxes.locale.defaultCurrencyCode,
subtotal: order.subtotal,
totalAdjustments: order.totalAdjustments,
total: order.total
]
}
This gives us a clean / DRY way of retrieving the data that can be refactored in the future. We'll also have an easier time unit testing and performance tuning in the future since all of the data retrieval is centralized in a single service method.
This is one step away from @kchelstowski's command object approach. And I don't see us needing that because the map works well enough as a DTO. The command object would be necessary if we then needed to add more methods and validation to the object.
Order order = Order.get(params.id) | ||
def orderItems = order?.orderItems | ||
render(template: "orderSummary", model: [ | ||
isPurchaseOrder: order.isPurchaseOrder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmiranda I started moving things to the controller that are needed by the template and since there are used properties from order and order items I decided to leave it as is -> fetch order and pull order items (and other required data) from it instead of fetching only order items.
@@ -306,6 +326,21 @@ | |||
$(currentRow).hide(); | |||
}); | |||
} | |||
|
|||
function fetchOrderItemsDerivedStatus() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmiranda Moving this to async improved the loading/rendering times by another ~2s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@kchelstowski @jmiranda I added another improvement, please take a look:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should go over this one in a tech huddle.
@@ -58,7 +59,7 @@ class InvoiceItem implements Serializable { | |||
User createdBy | |||
User updatedBy | |||
|
|||
static belongsTo = [invoice: Invoice] | |||
static belongsTo = [Invoice, ShipmentItem, OrderAdjustment, OrderItem] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could generate more SQL than we're expecting and could consequently have unintended consequences on performance.
I think you should be able to define invoice item with invoice as a parent and hasMany on other domains without the need to define cascade behavior (which is what belongsTo is doing).
https://stackoverflow.com/questions/23905638/grails-domain-class-hasone-hasmany-without-belongsto
I assume you're encountering a Grails error without this. So post that here.
@@ -306,6 +326,21 @@ | |||
$(currentRow).hide(); | |||
}); | |||
} | |||
|
|||
function fetchOrderItemsDerivedStatus() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Order order = Order.get(params.id) | ||
def orderItems = order?.orderItems | ||
render(template: "orderSummary", model: [ | ||
isPurchaseOrder: order.isPurchaseOrder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think we can refactor this a bit more when we move to Grails 3. As @kchelstowski mentioned this might look better as queries defined in the data service.
This is the one change I would like to see so we can move this in that direction when the time comes
def orderSummary = {
Map orderSummary = orderService.getOrderSummary(String id)
render render(template: "orderSummary", model: orderSummary)
}
with the order summary method looking something like this. And since we know that we're just using this method for reading data we can add in some extra savings.
@Transactional(readOnly=true)
Map getOrderSummary(String id) {
Order order = Order.read(params.id)
return [
isPurchaseOrder: order.isPurchaseOrder
isPutawayOrder: order.isPutawayOrder,
orderItems: orderItems,
anySupplierCode: orderItems?.any { it.productSupplier?.supplierCode },
anyManufacturerName: orderItems?.any { it.productSupplier?.manufacturerName },
anyManufacturerCode: orderItems?.any { it.productSupplier?.manufacturerCode },
currencyCode: order.currencyCode ?: grailsApplication.config.openboxes.locale.defaultCurrencyCode,
subtotal: order.subtotal,
totalAdjustments: order.totalAdjustments,
total: order.total
]
}
This gives us a clean / DRY way of retrieving the data that can be refactored in the future. We'll also have an easier time unit testing and performance tuning in the future since all of the data retrieval is centralized in a single service method.
This is one step away from @kchelstowski's command object approach. And I don't see us needing that because the map works well enough as a DTO. The command object would be necessary if we then needed to add more methods and validation to the object.
orderItems: orderItems, | ||
anySupplierCode: orderItems?.any { it.productSupplier?.supplierCode }, | ||
anyManufacturerName: orderItems?.any { it.productSupplier?.manufacturerName }, | ||
anyManufacturerCode: orderItems?.any { it.productSupplier?.manufacturerCode }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this "any*" thing. If we need to do it this way, let's just grab "any" product supplier once and use the supplierCode, manufacturerName, manufacturerCode from that supplier.
currencyCode: order.currencyCode ?: grailsApplication.config.openboxes.locale.defaultCurrencyCode, | ||
subtotal: order.subtotal, | ||
totalAdjustments: order.totalAdjustments, | ||
total: order.total, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be pushed off until later if we know that this is not a huge performance issue, but ...
... consider looking into some performance improvements for these three "properties" as well. I'm guessing this could at least be combined into a single method that pulls all of the data once and then returns the total, subtotal, and adjustments.
Map getOrderCosts() {
// retrieve data necessary to calculate subtotal and adjustments
return [
subtotal: subtotal,
adjustments: adjustments,
total: subtotal + adjustments
]
}
isPutawayOrder: order.isPutawayOrder, | ||
orderItems: orderItems, | ||
currencyCode: order.currencyCode ?: grailsApplication.config.openboxes.locale.defaultCurrencyCode, | ||
total: order.total, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to see a getOrderItemSummary method here same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model will inform decisions about what the API should look like.
@@ -59,9 +58,13 @@ class OrderAdjustment implements Serializable { | |||
|
|||
static belongsTo = [order: Order, orderItem: OrderItem] | |||
|
|||
static hasMany = [invoiceItems: InvoiceItem] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love adding new relationships that don't make sense. I don't understand why we want to do this. We're creating a new join table? How are we populating that join table?
I'm clearing missing something but here so I suggest we move this to a tech huddle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not new. It was already on the InvoiceItem
before as:
static hasMany = [shipmentItems: ShipmentItem, orderItems: OrderItem, orderAdjustments: OrderAdjustment]
and there are already join tables for these, these were just not used on the order items, order adjustments, and shipment items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have agreed on next steps.
@@ -58,7 +59,7 @@ class InvoiceItem implements Serializable { | |||
User createdBy | |||
User updatedBy | |||
|
|||
static belongsTo = [invoice: Invoice] | |||
static belongsTo = [Invoice, ShipmentItem, OrderAdjustment, OrderItem] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To resolve this just let me know
- if it's possible to include back references on each of these. And if not, what is the error you're getting from Grails.
- Also does this add any unnecessary SQL queries when retrieving invoice items. Does it go through each of those associations (in either case, with or without back reference).
isPutawayOrder: order.isPutawayOrder, | ||
orderItems: orderItems, | ||
currencyCode: order.currencyCode ?: grailsApplication.config.openboxes.locale.defaultCurrencyCode, | ||
total: order.total, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model will inform decisions about what the API should look like.
We also decided to push this to the develop branch so we get a full regression test cycle run on it for 0.8.23. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments for requested changes. Once these are done I think we'll be good to merge.
@@ -64,6 +67,11 @@ class InvoiceController { | |||
def invoiceInstance = Invoice.get(params.id) | |||
if (invoiceInstance) { | |||
try { | |||
invoiceInstance.invoiceItems?.each { InvoiceItem invoiceItem -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this again really worries me. For now can you at least move lines 70-75 to the InvoiceService and just call invoiceService.deleteInvoice(invoice) here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new deleteInvoiceItem(InvoiceItem invoiceItem) method suggested below ... this new method InvoiceService.deleteInvoice(Invoice) should look like this
void deleteInvoice(Invoice invoice) {
invoice.invoiceItems.each { InvoiceItem invoiceItem ->
deleteInvoiceItem(invoiceItem)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -186,6 +186,9 @@ class InvoiceService { | |||
InvoiceItem invoiceItem = InvoiceItem.get(id) | |||
Invoice invoice = invoiceItem.invoice | |||
invoice.removeFromInvoiceItems(invoiceItem) | |||
invoiceItem.orderItems?.each { OrderItem it -> it.removeFromInvoiceItems(invoiceItem) } | |||
invoiceItem.orderAdjustments?.each { OrderAdjustment it -> it.removeFromInvoiceItems(invoiceItem) } | |||
invoiceItem.shipmentItems?.each { ShipmentItem it -> it.removeFromInvoiceItems(invoiceItem) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to a new method. Add some error handling if it makes you feel better i.e. null check on invoiceItem
void deleteInvoiceItem(InvoiceItem invoiceItem) {
invoiceItem.orderItems?.each { OrderItem it -> it.removeFromInvoiceItems(invoiceItem) }
invoiceItem.orderAdjustments?.each { OrderAdjustment it -> it.removeFromInvoiceItems(invoiceItem) }
invoiceItem.shipmentItems?.each { ShipmentItem it -> it.removeFromInvoiceItems(invoiceItem) }
invoiceItem.delete()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
<td> | ||
<div class="tag ${orderItem?.canceled ? 'tag-danger' : ''}"> | ||
<format:metadata obj="${!orderItem?.canceled && orderItemsDerivedStatus[orderItem?.id] ? orderItemsDerivedStatus[orderItem?.id] : orderItem?.orderItemStatusCode?.name()}"/> | ||
<span class="${orderItem?.id}">${g.message(code: 'default.loading.label')}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels odd. Why not use id=${orderItem.id} and class could be orderItemStatus or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmiranda It's done on purpose because the derived status is used on the first and second tabs and item id is used to find elements and replace them. If I'd use an id property here instead of class, then after entering the second tab only ids on the first tab (hidden at the moment) would be replaced with statuses and the second tab would be hanging with "loading..." labels, because jquery would replace the first id it found and would be happy about it.
- add proper hasMany relations on OrderItem, OrderAdjustment and ShipmentItem - remove getInvoiceItems methods with poor performance
after recent domain changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* OBPIH-5847 Fix invoice items relations - add proper hasMany relations on OrderItem, OrderAdjustment and ShipmentItem - remove getInvoiceItems methods with poor performance * OBPIH-5847 Render tabs asynchronously on order show page * OBPIH-5847 Render order items derived status asynchronously * OBPIH-5847 Add methods in order service for getting summary and item status * OBPIH-5847 Fix deleting invoice items and invoices after recent domain changes
No description provided.