[
https://issues.apache.org/jira/browse/OFBIZ-5761?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14126966#comment-14126966
]
Jacques Le Roux edited comment on OFBIZ-5761 at 9/9/14 8:35 PM:
----------------------------------------------------------------
Thanks for your review. This is indeed not orthodox and there are more cases.
I was expecting few issues. Though I spent a number of hours (I did not count
them) to create this patch from the R12.04 addon, I did not review the Java
code in detail yet. Being 1st focused on having things working and creating
this patch which was not straightforward.
Looking at this pattern I found more of it. We could see it as a performance
improvement (no service calls overhead) but I agree it's not the right way and
not a best practice. Async calls should be used if possible.
addOrderItemShipGroup,
-editOrderItemShipGroup-,
-updateOrderItemShipGroupFromContext-,
deleteOrderItemShipGroupAssoc and
-validateOrderItemShipGroupAssoc-
are also concerned.
Though
editOrderItemShipGroup
updateOrderItemShipGroupFromContext and
validateOrderItemShipGroupAssoc
are only local to the OrderServices class. They should be private, and their
signatures should not be like service ones. Maybe the reason is the addon was
no intrinsically autonomous (I found other aspects like that, which have been
fixed since) and there are other snippets in (an)other addon/s which contains
the definitions of the corresponding services. I doubt it but to be clarified
by the Neogia team before we change the 3 methods signatures. Better to include
the services defintion here if it's the case.
was (Author: jacques.le.roux):
Thanks for your review. This is indeed not orthodox and there are more cases.
I was expecting few issues. Though I spent a number of hours (I did not count
them) to create this patch from the R12.04 addon, I did not review the Java
code in detail yet. Being 1st focused on having things working and creating
this patch which was not straightforward.
Looking at this pattern I found more of it. We could see it as a performance
improvement (no service calls overhead) but I agree it's not the right way and
not a best practice. Async calls should be used if possible.
addOrderItemShipGroup,
-editOrderItemShipGroup-,
-updateOrderItemShipGroupFromContext-,
deleteOrderItemShipGroup and
-validateOrderItemShipGroupAssoc-
are also concerned.
Though
editOrderItemShipGroup
updateOrderItemShipGroupFromContext and
validateOrderItemShipGroupAssoc
are only local to the OrderServices class. They should be private, and their
signatures should not be like service ones. Maybe the reason is the addon was
no intrinsically autonomous (I found other aspects like that, which have been
fixed since) and there are other snippets in (an)other addon/s which contains
the definitions of the corresponding services. I doubt it but to be clarified
by the Neogia team before we change the 3 methods signatures. Better to include
the services defintion here if it's the case.
> Allow to edit ship groups contents after and order has been created
> -------------------------------------------------------------------
>
> Key: OFBIZ-5761
> URL: https://issues.apache.org/jira/browse/OFBIZ-5761
> Project: OFBiz
> Issue Type: Improvement
> Components: order
> Affects Versions: Trunk
> Reporter: Jacques Le Roux
> Assignee: Jacques Le Roux
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-5761 - OISG Management.patch
>
>
> Currently you can only move order items between ship groups while you create
> an order. I needed to do it after order creation. When I met Olivier (Heintz)
> at the RMLL 2014 in July, I found the Neogia team has developed a such
> feature and had it as an addon (named oisg-management) for R12.04. Then
> exchanging with Nicolas (Malin), and Pierre (Gaudin) I decided to give it a
> go. I will quickly explain the following history, for the Neogia team to know
> the current situation and what has changed.
> After updating the code to work with current trunk (instead of R12.04) I
> found it was working well but some minor issues. I then exchanged with Leila
> (Mekika) from the Neogia team and we could quickly fix the minor issues:
> * text harcoded, no labels. I began to fix them, thanks to Leila who
> completed the major part and explained me some tricks about the
> oisg-management addon.
> * A redundant button associated with the new addOrderItemShipGroup service.
> I removed it because the current button calls createOrderItemShipGroup which
> is enough. We could BTW consider using addOrderItemShipGroup instead. It's
> more complete (see below for instance) but that"s rather a matter of taste.
> There was a mechanism to merge sales taxes to get them grouped by ship groups
> in order adjustments. I removed it because this can be done dynamically (see
> invoice.pdf) and it was removing the shipGroupSeqId from the order
> adjustments.
> I sorted (DESC) the OrderItemShipGroup in addOrderItemShipGroup in order to
> use the 1st ship group when copying shipmentMethodTypeId, carrierPartyId,
> carrierRoleTypeId, contactMechId when shipmentMethodTypeId and carrierPartyId
> are not passed to the service.
> I later fixed a bug I found in loadCartForUpdate service when removing the
> adjustments.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
